Skip to content

[BD-27][TNL-7891][BB-3539] Remove Empty Sections from cc2olx Conversions - ContextModuleSubHeader#44

Merged
alangsto merged 3 commits into
openedx:masterfrom
open-craft:shimulch/bb-3539-remove-empty-sections
Feb 22, 2021
Merged

[BD-27][TNL-7891][BB-3539] Remove Empty Sections from cc2olx Conversions - ContextModuleSubHeader#44
alangsto merged 3 commits into
openedx:masterfrom
open-craft:shimulch/bb-3539-remove-empty-sections

Conversation

@shimulch

@shimulch shimulch commented Feb 7, 2021

Copy link
Copy Markdown

This PR fixes an issue related to empty sections while there are canvas items with content type ContextModuleSubHeader. With this PR, cc2olx will treat these types of items as subsections and collapse related (consecutive) items under it as verticals.

JIRA tickets:

Discussions:

Dependencies: None

Screenshots:

Sandbox URL:

Merge deadline: None

Testing instructions:

  1. Pull this PR.
  2. Find a canvas imscc file that contains ContextModuleSubHeader, you can use the updated fixture with this PR.
  3. Convert using cc2olx.
  4. Check that item after ContextModuleSubHeader collapsed into it as verticals.

Author notes and concerns:
N/A

Reviewers

@openedx-webhooks

openedx-webhooks commented Feb 7, 2021

Copy link
Copy Markdown

Thanks for the pull request, @shimulch! I've created BLENDED-765 to keep track of it in Jira. More details are on the BD-27 project page.

When this pull request is ready, tag your edX technical lead.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 7, 2021
@shimulch shimulch changed the title [BB-3539] Remove Empty Sections from cc2olx Conversions - ContextModuleSubHeader [BD-27][TNL-7891][BB-3539] Remove Empty Sections from cc2olx Conversions - ContextModuleSubHeader Feb 7, 2021
@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Feb 7, 2021
@shimulch shimulch marked this pull request as ready for review February 10, 2021 12:26
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 10, 2021
Comment thread src/cc2olx/models.py Outdated
if not course_root:
return
sections = course_root.get("children", [])
sections = self.process_canvas_cc(course_root.get("children", []))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we run this in all cases, or only run this transformation for canvas flavored common cartridges?
We can check if course_settings/canvas_export.txt exists to determine if this is a canvas flavored CC.
Canvas writes this file to determine if it's from a canvas export.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaizoku, Thanks for the suggestion. Yeah this approach would be better. I've updated my PR.

@kaizoku

kaizoku commented Feb 18, 2021

Copy link
Copy Markdown
Contributor

Thanks for the PR @shimulch.
This works well for me on the affected courses and isn't causing issues with other courses. 👍

  • [✓] I tested this: (converted courses affected and not affected by issues, examined outlines and course structure in studio/lms)
  • [✓] I read through the code
  • [n/a] I checked for accessibility issues
  • [n/a] Includes documentation

@alangsto alangsto merged commit 430bf3d into openedx:master Feb 22, 2021
@shimulch shimulch deleted the shimulch/bb-3539-remove-empty-sections branch February 23, 2021 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants