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: make hide from TOC a visibility section setting #33952

Merged
merged 8 commits into from
Feb 29, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,4 @@ class XblockSerializer(StrictSerializer):
target_index = serializers.IntegerField(required=False, allow_null=True)
boilerplate = serializers.JSONField(required=False, allow_null=True)
staged_content = serializers.CharField(required=False, allow_null=True)
hide_from_toc = serializers.BooleanField(required=False, allow_null=True)
17 changes: 17 additions & 0 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
Original file line number Diff line number Diff line change
Expand Up @@ -1445,6 +1445,23 @@ def test_create_block(self):
).replace('REPLACE', r'([0-9]|[a-f]){3,}')
self.assertRegex(data['locator'], retarget)

@ddt.data(True, False)
def test_hide_xblock_from_toc_via_handler(self, hide_from_toc):
"""Test that the hide_from_toc field can be set via the xblock_handler."""
course = CourseFactory.create()
sequential = BlockFactory.create(parent_location=course.location)
data = {
"metadata": {
"hide_from_toc": hide_from_toc
}
}

response = self.client.ajax_post(get_url("xblock_handler", sequential.location), data)
sequential = self.store.get_item(sequential.location)

self.assertEqual(response.status_code, 200)
self.assertEqual(hide_from_toc, sequential.hide_from_toc)

def test_capa_block(self):
"""Test that a problem treats markdown specially."""
course = CourseFactory.create()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,8 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements
"group_access": xblock.group_access,
"user_partitions": user_partitions,
"show_correctness": xblock.show_correctness,
"hide_from_toc": xblock.hide_from_toc,
"enable_hide_from_toc_ui": settings.FEATURES.get("ENABLE_HIDE_FROM_TOC_UI", False),
}
)

Expand Down Expand Up @@ -1217,6 +1219,15 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements
else:
xblock_info["staff_only_message"] = False

if xblock_info["hide_from_toc"]:
xblock_info["hide_from_toc_message"] = True
elif child_info and child_info["children"]:
xblock_info["hide_from_toc_message"] = all(
child["hide_from_toc_message"] for child in child_info["children"]
)
else:
xblock_info["hide_from_toc_message"] = False

# If the ENABLE_TAGGING_TAXONOMY_LIST_PAGE feature flag is enabled, we show the "Manage Tags" options
if use_tagging_taxonomy_list_page():
xblock_info["use_tagging_taxonomy_list_page"] = True
Expand Down Expand Up @@ -1345,6 +1356,7 @@ class VisibilityState:
needs_attention = "needs_attention"
staff_only = "staff_only"
gated = "gated"
hide_from_toc = "hide_from_toc"


def _compute_visibility_state(
Expand All @@ -1355,29 +1367,36 @@ def _compute_visibility_state(
"""
if xblock.visible_to_staff_only:
return VisibilityState.staff_only
elif xblock.hide_from_toc:
return VisibilityState.hide_from_toc
elif is_unit_with_changes:
# Note that a unit that has never been published will fall into this category,
# as well as previously published units with draft content.
return VisibilityState.needs_attention

is_unscheduled = xblock.start == DEFAULT_START_DATE
is_live = is_course_self_paced or datetime.now(UTC) > xblock.start
if child_info and child_info.get("children", []):
if child_info and child_info.get("children", []): # pylint: disable=too-many-nested-blocks
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: there are better ways to resolve this warning than this, but this PR is complex enough. So, I'll open a new PR proposing a refactoring change so this quality failure is not raised anymore. I'll attach the PR in this thread when it's ready.

Copy link
Member

Choose a reason for hiding this comment

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

I think this function and the file are already plenty complex. This feature is not making it better. However having a different PR that is a requirement for this I think is the wrong approach.

If you are planning to extract this for in a "private" helper function at the top of the file or something with a similar complexity I suggest to do it already in this PR as a new commit and leave it at that.

Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Feb 27, 2024

Choose a reason for hiding this comment

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

Thanks for the suggestion, but I can't entirely agree. I don't think it is a good idea to introduce more changes to this PR, adding more tests and points of failure, so I'd prefer disabling the pylint rule. Maybe we should consider the refactor an enhancement instead of a requirement.

all_staff_only = True
all_unscheduled = True
all_live = True
all_hide_from_toc = True
for child in child_info["children"]:
child_state = child["visibility_state"]
if child_state == VisibilityState.needs_attention:
return child_state
elif not child_state == VisibilityState.staff_only:
all_staff_only = False
if not child_state == VisibilityState.unscheduled:
all_unscheduled = False
if not child_state == VisibilityState.live:
all_live = False
if not child_state == VisibilityState.hide_from_toc:
all_hide_from_toc = False
if not child_state == VisibilityState.unscheduled:
all_unscheduled = False
if not child_state == VisibilityState.live:
all_live = False
if all_staff_only:
return VisibilityState.staff_only
elif all_hide_from_toc:
return VisibilityState.hide_from_toc
elif all_unscheduled:
return (
VisibilityState.unscheduled
Expand Down
8 changes: 8 additions & 0 deletions cms/static/js/models/xblock_info.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ define(
* List of tags of the unit. This list is managed by the content_tagging module.
*/
tags: null,
/**
* True if the xblock is not visible to students only via links.
*/
hide_from_toc: null,
/**
* True iff this xblock should display a "Contains staff only content" message.
*/
hide_from_toc_message: null,
},

initialize: function() {
Expand Down
26 changes: 21 additions & 5 deletions cms/static/js/spec/views/pages/course_outline_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ describe('CourseOutlinePage', function() {
user_partition_info: {},
highlights_enabled: true,
highlights_enabled_for_messaging: false,
hide_from_toc: false,
felipemontoya marked this conversation as resolved.
Show resolved Hide resolved
enable_hide_from_toc_ui: true
}, options, {child_info: {children: children}});
};

Expand All @@ -68,7 +70,9 @@ describe('CourseOutlinePage', function() {
show_review_rules: true,
user_partition_info: {},
highlights_enabled: true,
highlights_enabled_for_messaging: false
highlights_enabled_for_messaging: false,
hide_from_toc: false,
enable_hide_from_toc_ui: true
}, options, {child_info: {children: children}});
};

Expand All @@ -93,7 +97,9 @@ describe('CourseOutlinePage', function() {
group_access: {},
user_partition_info: {},
highlights: [],
highlights_enabled: true
highlights_enabled: true,
hide_from_toc: false,
enable_hide_from_toc_ui: true
}, options, {child_info: {children: children}});
};

Expand Down Expand Up @@ -123,7 +129,9 @@ describe('CourseOutlinePage', function() {
},
user_partitions: [],
group_access: {},
user_partition_info: {}
user_partition_info: {},
hide_from_toc: false,
enable_hide_from_toc_ui: true
}, options, {child_info: {children: children}});
};

Expand All @@ -141,7 +149,9 @@ describe('CourseOutlinePage', function() {
edited_by: 'MockUser',
user_partitions: [],
group_access: {},
user_partition_info: {}
user_partition_info: {},
hide_from_toc: false,
enable_hide_from_toc_ui: true
}, options);
};

Expand Down Expand Up @@ -1214,7 +1224,9 @@ describe('CourseOutlinePage', function() {
is_practice_exam: false,
is_proctored_exam: false,
default_time_limit_minutes: 150,
hide_after_due: true
hide_after_due: true,
hide_from_toc: false,
enable_hide_from_toc_ui: true,
}, [
createMockVerticalJSON({
has_changes: true,
Expand Down Expand Up @@ -1397,6 +1409,7 @@ describe('CourseOutlinePage', function() {
default_time_limit_minutes: 150,
hide_after_due: true,
is_onboarding_exam: false,
hide_from_toc: null,
}
});
expect(requests[0].requestHeaders['X-HTTP-Method-Override']).toBe('PATCH');
Expand Down Expand Up @@ -2240,6 +2253,8 @@ describe('CourseOutlinePage', function() {
is_practice_exam: false,
is_proctored_exam: false,
default_time_limit_minutes: null,
hide_from_toc: false,
enable_hide_from_toc_ui: true,
}, [
createMockVerticalJSON({
has_changes: true,
Expand Down Expand Up @@ -2521,6 +2536,7 @@ describe('CourseOutlinePage', function() {
publish: 'republish',
metadata: {
visible_to_staff_only: null,
hide_from_toc: null
}
});
})
Expand Down
41 changes: 38 additions & 3 deletions cms/static/js/views/modals/course_outline_modals.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,8 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
var isProctoredExam = xblockInfo.get('is_proctored_exam');
var isPracticeExam = xblockInfo.get('is_practice_exam');
var isOnboardingExam = xblockInfo.get('is_onboarding_exam');
var enableHideFromTOCUI = xblockInfo.get('enable_hide_from_toc_ui');
var hideFromTOC = xblockInfo.get('hide_from_toc');
var html = this.template($.extend({}, {
xblockInfo: xblockInfo,
xblockType: this.options.xblockType,
Expand All @@ -323,6 +325,8 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
isProctoredExam: isProctoredExam,
isPracticeExam: isPracticeExam,
isOnboardingExam: isOnboardingExam,
enableHideFromTOCUI: enableHideFromTOCUI,
hideFromTOC: hideFromTOC,
isTimedExam: isTimeLimited && !(
isProctoredExam || isPracticeExam || isOnboardingExam
),
Expand Down Expand Up @@ -798,6 +802,10 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
return this.model.get('ancestor_has_staff_lock');
},

isModelHiddenFromTOC: function() {
return this.model.get('hide_from_toc');
},

getContext: function() {
return {
hasExplicitStaffLock: this.isModelLocked(),
Expand All @@ -812,6 +820,8 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
afterRender: function() {
AbstractVisibilityEditor.prototype.afterRender.call(this);
this.setLock(this.isModelLocked());
this.setHideFromTOC(this.isModelHiddenFromTOC());
this.setVisibleToLearners(this.isVisibleToLearners());
},

setLock: function(value) {
Expand All @@ -822,16 +832,33 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
return this.$('#staff_lock').is(':checked');
},

setHideFromTOC: function(value) {
this.$('#hide_from_toc').prop('checked', value);
},

setVisibleToLearners: function(value) {
this.$('#visible_to_learners').prop('checked', value);
},

isVisibleToLearners: function() {
return this.$('#staff_lock').is(':not(:checked)') && this.$('#hide_from_toc').is(':not(:checked)');
},

isHiddenFromTOC: function() {
return this.$('#hide_from_toc').is(':checked');
},

hasChanges: function() {
return this.isModelLocked() !== this.isLocked();
return this.isModelLocked() !== this.isLocked() || this.isModelHiddenFromTOC() !== this.isHiddenFromTOC();
},

getRequestData: function() {
if (this.hasChanges()) {
return {
publish: 'republish',
metadata: {
visible_to_staff_only: this.isLocked() ? true : null
visible_to_staff_only: this.isLocked() || null,
hide_from_toc: this.isHiddenFromTOC() || null
}
};
} else {
Expand Down Expand Up @@ -1055,12 +1082,20 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
if (this.currentVisibility() === 'staff_only') {
metadata.visible_to_staff_only = true;
metadata.hide_after_due = null;
metadata.hide_from_toc = null;
} else if (this.currentVisibility() === 'hide_after_due') {
metadata.visible_to_staff_only = null;
metadata.hide_after_due = true;
} else {
metadata.hide_from_toc = null;
} else if (this.currentVisibility() === 'hide_from_toc'){
metadata.visible_to_staff_only = null;
metadata.hide_after_due = null;
metadata.hide_from_toc = true;
}
else {
metadata.visible_to_staff_only = null;
metadata.hide_after_due = null;
metadata.hide_from_toc = null;
}

return {
Expand Down
1 change: 1 addition & 0 deletions cms/static/js/views/pages/container_subviews.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils, MoveXBlockUtils, H
releaseDate: this.model.get('release_date'),
releaseDateFrom: this.model.get('release_date_from'),
hasExplicitStaffLock: this.model.get('has_explicit_staff_lock'),
hideFromTOC: this.model.get('hide_from_toc'),
staffLockFrom: this.model.get('staff_lock_from'),
enableCopyUnit: this.model.get('enable_copy_paste_units'),
course: window.course,
Expand Down
8 changes: 7 additions & 1 deletion cms/static/js/views/utils/xblock_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@ function($, _, gettext, ViewUtils, ModuleUtils, XBlockInfo, StringUtils) {
*
* staffOnly - all of the block's content is to be shown to staff only
* Note: staff only items do not affect their parent's state.
*
* hideFromTOC - all of the block's content is to be hidden from the table of contents.
*/
VisibilityState = {
live: 'live',
ready: 'ready',
unscheduled: 'unscheduled',
needsAttention: 'needs_attention',
staffOnly: 'staff_only',
gated: 'gated'
gated: 'gated',
hideFromTOC: 'hide_from_toc'
};

/**
Expand Down Expand Up @@ -310,6 +313,9 @@ function($, _, gettext, ViewUtils, ModuleUtils, XBlockInfo, StringUtils) {
if (visibilityState === VisibilityState.staffOnly) {
return 'is-staff-only';
}
if (visibilityState === VisibilityState.hideFromTOC) {
mariajgrimaldi marked this conversation as resolved.
Show resolved Hide resolved
return 'is-hidden-from-toc';
}
if (visibilityState === VisibilityState.gated) {
return 'is-gated';
}
Expand Down
1 change: 1 addition & 0 deletions cms/static/js/views/xblock_outline.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils, XBlockStringFieldE
includesChildren: this.shouldRenderChildren(),
hasExplicitStaffLock: this.model.get('has_explicit_staff_lock'),
staffOnlyMessage: this.model.get('staff_only_message'),
hideFromTOCMessage: this.model.get('hide_from_toc_message'),
course: course,
enableCopyPasteUnits: this.model.get("enable_copy_paste_units"), // ENABLE_COPY_PASTE_UNITS waffle flag
useTaggingTaxonomyListPage: this.model.get("use_tagging_taxonomy_list_page"), // ENABLE_TAGGING_TAXONOMY_LIST_PAGE waffle flag
Expand Down
22 changes: 22 additions & 0 deletions cms/static/sass/elements/_modules.scss
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,18 @@ $outline-indent-width: $baseline;
}
}

// CASE: is hidden from TOC
&.is-hidden-from-toc {
// needed to make sure direct children only
> .section-status,
> .subsection-status,
> .unit-status {
.status-message .icon {
color: $color-hide-from-toc;
}
}
}

// CASE: has gated content
&.is-gated {

Expand Down Expand Up @@ -603,6 +615,11 @@ $outline-indent-width: $baseline;
border-left-color: $color-staff-only;
}

// CASE: is hidden from TOC
&.is-hidden-from-toc {
border-left-color: $color-hide-from-toc;
}

// CASE: has gated content
&.is-gated {
border-left-color: $color-gated;
Expand Down Expand Up @@ -698,6 +715,11 @@ $outline-indent-width: $baseline;
border-left-color: $color-staff-only;
}

// CASE: is hidden from TOC
&.is-hidden-from-toc {
border-left-color: $color-hide-from-toc;
}

// CASE: is presented for gated
&.is-gated {
border-left-color: $color-gated;
Expand Down
1 change: 1 addition & 0 deletions cms/static/sass/partials/cms/theme/_variables-v1.scss
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ $color-ready: $green !default;
$color-warning: $orange-l2 !default;
$color-error: $red-l2 !default;
$color-staff-only: $black !default;
$color-hide-from-toc: $black !default;
$color-gated: $black !default;

$color-heading-base: $gray-d2 !default;
Expand Down
Loading
Loading