Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: optional xblocks #638

Open
wants to merge 10 commits into
base: opencraft-release/palm.1
Choose a base branch
from

Conversation

DanielVZ96
Copy link
Member

@DanielVZ96 DanielVZ96 commented Feb 29, 2024

Description

This PR implements separating optional progress from normal progress, and also displaying optional chapters and sequences in the outline.

  • Impacts: Learners and authors

Supporting information

Testing instructions

  1. Run the MFE from this branch: feat: optional xblocks openedx/frontend-app-learning#1296
  2. Add "done" to the advanced module list
  3. As a non-staff user go to the progress tab and assert only the typical progress donut appears
  4. As a staff user, on the cms open the settings of any sequence or chapter and mark as optional
  5. As a non-staff user, complete some units, and also the optional unit
  6. Navigate to the progress tab again and assert a new optional donut appears with your progress

Screenshots

optional-outline

cms-optional

progress

course-settings

Private-ref: https://tasks.opencraft.com/browse/BB-8586

cms/djangoapps/contentstore/utils.py Outdated Show resolved Hide resolved
Comment on lines 1241 to 1242
optional_completion: this.model.get('optional_completion'),
optionalAncestor: this.model.get('ancestor_has_optional_completion')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we should not mix the snake_case with camelCase.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -14,7 +14,7 @@ class BlockCompletionTransformer(BlockStructureTransformer):
Keep track of the completion of each block within the block structure.
"""
READ_VERSION = 1
WRITE_VERSION = 1
WRITE_VERSION = 2
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this seems to be a backward-compatible change, so we can revert bumping this version.

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

@@ -437,3 +437,18 @@ def test_cannot_enroll_if_full(self):
self.update_course_and_overview()
CourseEnrollment.enroll(UserFactory(), self.course.id) # grr, some rando took our spot!
self.assert_can_enroll(False)

def test_optional_completion(self):
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we use a more verbose name for this test, like "test_optional_completion_off_by_default"?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

lms/djangoapps/courseware/courses.py Show resolved Hide resolved
Comment on lines 1410 to 1411
if xblock_info['optional_completion']:
xblock_info['ancestor_has_optional_completion'] = ancestor_has_optional_completion(xblock, parent_xblock)
Copy link
Member

Choose a reason for hiding this comment

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

What if the optional_completion is set to False (default), but the parent XBlock has it set to True? In this use case, the field is still active. We should disable this checkbox when ancestor_has_optional_completion returns True, regardless of the xblock_info['optional_completion'] value.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

tabs[1].editors = [StaffLockEditor];
} else if (xblockInfo.isSequential()) {
tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor];
tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor, OptionalCompletionEditor];
Copy link
Member

Choose a reason for hiding this comment

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

Regarding this comment about hiding the editor when the completion tracking is not enabled, please see the following parts of code that handle a similar scenario for another editor:

is_custom_relative_dates_active: ${CUSTOM_RELATIVE_DATES.is_enabled(context_course.id) | n, dump_js_escaped_json},

https://github.com/open-craft/edx-platform/blob/42451caa2612a63b42bf5686e6217817e0dd2098/cms/static/js/views/modals/course_outline_modals.js#L1290-L1292

I think we shouldn't move this to a follow-up ticket because this feature is not enabled by default in edx-platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

wow i didn't realize it'd be so straight forward. implementing this

Copy link
Member Author

Choose a reason for hiding this comment

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

done. I wasn't able to test it manually without hardcoding the value in the base.html. it seems waffle flags are cached somewhere and take quite some time to invalidate the cache.

In case you want to test it and have my same problem you can hard code completion_tracking_enabled to False in cms/templates/base.html line 162.

<%- gettext('Optional') %>
<% } %>
<p class="field-message">
<% var message = gettext('Optional %(xblockType)ss won\'t count towards course or parent completion. Only has an effect if completion module is enabled.') %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<% var message = gettext('Optional %(xblockType)ss won\'t count towards course or parent completion. Only has an effect if completion module is enabled.') %>
<% var message = gettext('Optional %(xblockType)ss won\'t count towards course or parent completion.') %>

Once we hide the editor when the completion tracking is not enabled, we can remove this part.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@DanielVZ96 DanielVZ96 force-pushed the dvz/optional-lesson branch 2 times, most recently from 487ae2d to a2968d0 Compare March 23, 2024 06:13
]);
appendSetFixtures(mockOutlinePage);
mockCourseJSON = createMockCourseJSON({}, [
createMockSectionJSON({}, [
createMockSectionJSON({completion_tracking_enabled: true}, [
Copy link
Member

Choose a reason for hiding this comment

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

@DanielVZ96, the completion tracking should be enabled for the course instead of the section.

Also, you should be able to use something like console.warn(window. course) to verify that it's applied correctly.

@DanielVZ96 DanielVZ96 force-pushed the dvz/optional-lesson branch 18 times, most recently from 3f0b03c to f7745fd Compare March 31, 2024 05:41
@DanielVZ96 DanielVZ96 force-pushed the dvz/optional-lesson branch 18 times, most recently from 3046507 to 84ad5ee Compare April 2, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants