-
Notifications
You must be signed in to change notification settings - Fork 4k
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 #34275
feat: optional xblocks #34275
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', | |
ReleaseDateEditor, DueDateEditor, SelfPacedDueDateEditor, GradingEditor, PublishEditor, AbstractVisibilityEditor, | ||
StaffLockEditor, UnitAccessEditor, ContentVisibilityEditor, TimedExaminationPreferenceEditor, | ||
AccessEditor, ShowCorrectnessEditor, HighlightsEditor, HighlightsEnableXBlockModal, HighlightsEnableEditor, | ||
DiscussionEditor; | ||
DiscussionEditor, OptionalContentEditor; | ||
|
||
CourseOutlineXBlockModal = BaseModal.extend({ | ||
events: _.extend({}, BaseModal.prototype.events, { | ||
|
@@ -1206,6 +1206,46 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', | |
} | ||
}); | ||
|
||
OptionalContentEditor = AbstractEditor.extend( | ||
{ | ||
templateName: 'optional-content-editor', | ||
className: 'edit-optional-content', | ||
|
||
afterRender: function() { | ||
AbstractEditor.prototype.afterRender.call(this); | ||
this.setValue(this.model.get("optional_content")); | ||
}, | ||
|
||
setValue: function(value) { | ||
this.$('input[name=optional_content]').prop('checked', value); | ||
}, | ||
|
||
currentValue: function() { | ||
return this.$('input[name=optional_content]').is(':checked'); | ||
}, | ||
|
||
hasChanges: function() { | ||
return this.model.get('optional_content') !== this.currentValue(); | ||
}, | ||
|
||
getRequestData: function() { | ||
if (this.hasChanges()) { | ||
return { | ||
publish: 'republish', | ||
metadata: { | ||
optional_content: this.currentValue() | ||
} | ||
}; | ||
} else { | ||
return {}; | ||
} | ||
}, | ||
getContext: function() { | ||
return { | ||
optional_content: this.model.get('optional_content') | ||
}; | ||
}, | ||
}) | ||
return { | ||
getModal: function(type, xblockInfo, options) { | ||
if (type === 'edit') { | ||
|
@@ -1245,10 +1285,10 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', | |
} | ||
]; | ||
if (xblockInfo.isChapter()) { | ||
tabs[0].editors = [ReleaseDateEditor]; | ||
tabs[0].editors = [ReleaseDateEditor, OptionalContentEditor]; | ||
tabs[1].editors = [StaffLockEditor]; | ||
} else if (xblockInfo.isSequential()) { | ||
tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor]; | ||
tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor, OptionalContentEditor]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should allow overriding this in Units as well. |
||
tabs[1].editors = [ContentVisibilityEditor, ShowCorrectnessEditor]; | ||
if (course.get('self_paced') && course.get('is_custom_relative_dates_active')) { | ||
tabs[0].editors.push(SelfPacedDueDateEditor); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -747,6 +747,7 @@ | |
|
||
.edit-discussion, | ||
.edit-staff-lock, | ||
.edit-optional-content, | ||
.edit-content-visibility, | ||
.edit-unit-access { | ||
margin-bottom: $baseline; | ||
|
@@ -760,19 +761,20 @@ | |
// UI: staff lock and discussion | ||
.edit-discussion, | ||
.edit-staff-lock, | ||
.edit-optional-content, | ||
.edit-settings-timed-examination, | ||
.edit-unit-access { | ||
.checkbox-cosmetic .input-checkbox { | ||
@extend %cont-text-sr; | ||
|
||
// CASE: unchecked | ||
~ .tip-warning { | ||
~.tip-warning { | ||
display: block; | ||
} | ||
|
||
// CASE: checked | ||
&:checked { | ||
~ .tip-warning { | ||
~.tip-warning { | ||
Comment on lines
-769
to
+777
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not seem to be right. Why did we change it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, autoformatting strikes again, will revert |
||
display: none; | ||
} | ||
} | ||
|
@@ -832,6 +834,7 @@ | |
|
||
.edit-discussion, | ||
.edit-unit-access, | ||
.edit-optional-content, | ||
.edit-staff-lock { | ||
.modal-section-content { | ||
@include font-size(16); | ||
|
@@ -874,6 +877,7 @@ | |
|
||
.edit-discussion, | ||
.edit-unit-access, | ||
.edit-optional-content, | ||
.edit-staff-lock { | ||
.modal-section-content { | ||
@include font-size(16); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<h3 class="modal-section-title"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole editor is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
<input type="checkbox" id="optional_content" name="optional_content" | ||
class="input input-checkbox" /> | ||
<%- gettext('Mark as optional') %> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add an explanation (perhaps with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
</h3> |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -54,6 +54,7 @@ def get_blocks(self, block): # pylint: disable=missing-function-docstring | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'resume_block': block.get('resume_block', False), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'type': block_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'has_scheduled_content': block.get('has_scheduled_content'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'optional_content': block.get('optional_content'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please read this: edx-platform/openedx/core/djangoapps/content/block_structure/transformer.py Lines 16 to 57 in b1f6a46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Agrendalath so based on that I should bump the block_completion transformer write version, and after we're sure all blocks have the new version, we bump the read version? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for child in children: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -552,7 +552,7 @@ def get_course_assignment_date_blocks(course, user, request, num_return=None, | |
|
||
|
||
@request_cached() | ||
def get_course_blocks_completion_summary(course_key, user): | ||
def get_course_blocks_completion_summary(course_key, user, optional=False): | ||
""" | ||
Returns an object with the number of complete units, incomplete units, and units that contain gated content | ||
for the given course. The complete and incomplete counts only reflect units that are able to be completed by | ||
|
@@ -566,10 +566,18 @@ def get_course_blocks_completion_summary(course_key, user): | |
course_usage_key = store.make_course_usage_key(course_key) | ||
block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True) | ||
|
||
def _is_optional(*keys): | ||
for key in keys: | ||
if block_data.get_xblock_field(key, 'optional_content', False): | ||
return True | ||
return False | ||
|
||
complete_count, incomplete_count, locked_count = 0, 0, 0 | ||
for section_key in block_data.get_children(course_usage_key): # pylint: disable=too-many-nested-blocks | ||
for subsection_key in block_data.get_children(section_key): | ||
for unit_key in block_data.get_children(subsection_key): | ||
if optional != _is_optional(section_key, subsection_key, unit_key): | ||
continue | ||
Comment on lines
+579
to
+580
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was handling it as if a parent was optional all children would behave as optional regardless of their actual value, but now that you mention it, in order to reduce confusion it'd be better to disable the checkbox when a parent is optional. |
||
complete = block_data.get_xblock_field(unit_key, 'complete', False) | ||
contains_gated_content = block_data.get_xblock_field(unit_key, 'contains_gated_content', False) | ||
if contains_gated_content: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,6 +238,17 @@ class InheritanceMixin(XBlockMixin): | |
scope=Scope.settings | ||
) | ||
|
||
optional_content = Boolean( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something similar to Same with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea. will rename it. |
||
display_name=_('Optional'), | ||
help=_( | ||
'Set this to true to mark this block as optional.' | ||
'Progress in this block won\'t count towards course completion progress' | ||
'and will count as optional progress instead.' | ||
), | ||
default=False, | ||
scope=Scope.settings, | ||
) | ||
|
||
@property | ||
def close_date(self): | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we show the editor only when the completion is enabled (
ENABLE_COMPLETION_TRACKING_SWITCH.is_enabled()
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Agrendalath I'm not sure how to implement that. can we do that in a followup ticket?