feat: make hide from TOC a visibility section setting #33952

mariajgrimaldi · 2023-12-19T14:39:40Z

Description

This PR exposes the hide_from_toc xblock attribute so course authors can configure it as a section visibility option in Studio. Before this change, the Hide from TOC functionality was mainly used by OLX components. Hence, it wasn't available for configuration through the Studio UI. Still, its implementation existed in the platform and could be used by setting the attribute: hide_from_toc=true as part of the OLX definition.

So we could use the hide_from_toc attribute in the Studio UI making it configurable for sections and their children, we did the following:

Add hide_from_toc to the CMS Xblock handlers, serializers, and data classes so it can be recognized when setting the new visibility state option
Add it to the inheritance mixin so the sections' children recognize the configuration option
Make it available in the backbone frontend for course authors to configure as in visible_to_staff_only

Supporting information

These changes are part of the effort made to implement feature enhancement proposal: Hide sections from course outline

Testing instructions

Move into this branch
Collect statics by running: tutor dev exec lms openedx-assets build --env=dev
Enable TOC UI by setting: FEATURES["ENABLE_HIDE_FROM_TOC_UI"] = True in your CMS environment, this will allow course staff to configure the hide from toc option from the Studio UI.
Given the error explained in the Other information section, please use: CELERY_ALWAYS_EAGER = True in both of your environments (LMS/CMS).
Go to a sections' course, click the configuration gear > Visibility > Hide in course outline
As an instructor, you should be able to see the hidden section with its children
As a student, you should be able to see the hidden subsections/units with the URL from the blocks' View Live option

Screencast.from.15-02-24.15.27.32.mp4

Deadline

This effort is part of the Spanish consortium project, so it'd be ideal to merge this before the end of the project.

Other information

After deploying these changes to a tutor nightly remote installation, we found that the changes weren't reflected on the Course Outline after saving the new visibility changes for the 1st time. Now, the second time we save the changes are triggered. We don't have an explanation for it yet, but we're actively working on it. Here's a video of the behavior:

Screencast.from.19-12-23.12.08.56.mp4

Update: This was solved by #34020, which is already under review.

openedx-webhooks · 2023-12-19T14:39:45Z

Thanks for the pull request, @mariajgrimaldi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

cms/djangoapps/contentstore/signals/handlers.py

cms/templates/js/staff-lock-editor.underscore

cms/static/js/views/utils/xblock_utils.js

mariajgrimaldi · 2024-01-15T19:31:59Z

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

    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


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.

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.

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.

cms/templates/js/staff-lock-editor.underscore

cms/static/js/views/modals/course_outline_modals.js

cms/templates/js/staff-lock-editor.underscore

mariajgrimaldi · 2024-02-12T20:23:19Z

Hi there, @MaferMazu @felipemontoya @BryanttV: could you help me with a review here? Please feel free to add yourselves as reviewers if you have the time. Thanks!

mariajgrimaldi · 2024-02-12T20:24:07Z

@johnvente, all comments addressed! Could you check again? Thanks!

MaferMazu

Thanks for this, @mariajgrimaldi. The functionality works as expected, the code looks good to me, and I think it is sustained well by the proposal and all the discussion before the implementation.

@itsjeyd, don't the proposal and the previous discussion apply as a product review? If this needs a product review, can you help us tag the person who could help us with the product review? Thanks.

itsjeyd · 2024-02-21T09:26:01Z

@MaferMazu I suppose you're asking because I recently applied the product review label to this PR? :)

That's a permanent label for PRs whose review process includes getting approval from members of one or more product teams. (I.e., we don't remove it after product review is complete). The fact that it hadn't been added to this PR yet it was simply an oversight.

In the context of managing OSPRs, we've been using feature tickets on the platform roadmap to track status of product discussions. The feature ticket for this PR (as well as all other PRs related to Feature Enhancement Proposal: Hide Sections from course outline) is openedx/platform-roadmap#301.

Santiago confirmed product approval for the proposal there last week.

CC @mphilbrick211

MaferMazu · 2024-02-22T03:00:09Z

Oh, I wasn't aware of that. Thank you for the explanation, @itsjeyd.

itsjeyd · 2024-02-23T08:04:03Z

@MaferMazu No problem!

For future reference, the Pull Request Status Guide provides info on statuses and labels that you might see on OSPRs.

--

@mphilbrick211 It might make sense to update the definition of the product review label to explicitly mention that it's a permanent label.

Also, we're no longer using the Scheduled for Eng Review status so it would be worth removing it from the list at some point.

CC @sarina

cms/static/js/spec/views/pages/course_outline_spec.js

felipemontoya · 2024-02-27T21:04:27Z

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

    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


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.

cms/templates/js/staff-lock-editor.underscore

felipemontoya · 2024-02-27T21:20:38Z

I should have added with my review that I tested the code and it works just as the description explains. It is in line with the product definition in confluence so the feature is good to continue. The inline comments are more an engineering review.

Exposes the hide_from_toc xblock attribute so course authors can configure it as a section visibility option in Studio. Before this change, the Hide from TOC functionality was mainly used by OLX components. Hence, it wasn't available for configuration through the Studio UI. Still, its implementation existed in the platform and could be used by setting the attribute: hide_from_toc=true as part of the OLX definition. Ref: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3853975595/Feature+Enhancement+Proposal+Hide+Sections+from+course+outline

This reverts commit 0d402b714e085ef058672b8e57ed67418f29a684.

felipemontoya

Thanks for the fixes @mariajgrimaldi no more comments. I tested this again and all is working as it should.

openedx-webhooks · 2024-02-29T15:13:36Z

@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-02-29T16:28:48Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-02-29T16:46:03Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-02-29T16:46:03Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 19, 2023

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from bc00ad1 to 243c75c Compare December 19, 2023 16:42

BryanttV mentioned this pull request Dec 19, 2023

feat: expose hide_from_toc field in course blocks #33955

Merged

This was referenced Dec 29, 2023

feat: show message in course outline when hide from toc is enabled openedx/frontend-app-learning#1262

Merged

feat: show message in visibility tab in subsections when hide from toc is enabled from ui #33981

Merged

mariajgrimaldi commented Jan 8, 2024

View reviewed changes

cms/djangoapps/contentstore/signals/handlers.py Outdated Show resolved Hide resolved

mariajgrimaldi mentioned this pull request Jan 8, 2024

fix: use non_atomic_requests decorator in handle_block view #34020

Merged

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 606d74b to 664d959 Compare January 11, 2024 14:27

BryanttV mentioned this pull request Jan 12, 2024

feat: add share link button when hide from toc is enabled in sections #34043

Merged

itsjeyd mentioned this pull request Jan 12, 2024

Feature Enhancement Proposal - Hide Sections from course outline openedx/platform-roadmap#301

Open

This was referenced Jan 12, 2024

feat: return navigation disabled as sequence metadata #34049

Merged

feat: use navigation sequence metadata to disable navigation components openedx/frontend-app-learning#1273

Merged

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 664d959 to 8997e61 Compare January 12, 2024 19:08

johnvente reviewed Jan 12, 2024

View reviewed changes

cms/templates/js/staff-lock-editor.underscore Outdated Show resolved Hide resolved

johnvente reviewed Jan 12, 2024

View reviewed changes

cms/static/js/views/utils/xblock_utils.js Show resolved Hide resolved

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 0f9367e to 29c0612 Compare January 15, 2024 19:31

mariajgrimaldi commented Jan 15, 2024

View reviewed changes

mariajgrimaldi commented Jan 16, 2024

View reviewed changes

cms/templates/js/staff-lock-editor.underscore Show resolved Hide resolved

mariajgrimaldi marked this pull request as ready for review January 16, 2024 15:35

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 29c0612 to 3cc4cc9 Compare January 16, 2024 15:36

johnvente reviewed Jan 23, 2024

View reviewed changes

cms/static/js/views/modals/course_outline_modals.js Outdated Show resolved Hide resolved

johnvente reviewed Jan 23, 2024

View reviewed changes

cms/templates/js/staff-lock-editor.underscore Outdated Show resolved Hide resolved

johnvente reviewed Jan 23, 2024

View reviewed changes

cms/templates/js/staff-lock-editor.underscore Outdated Show resolved Hide resolved

johnvente reviewed Jan 23, 2024

View reviewed changes

cms/templates/js/staff-lock-editor.underscore Outdated Show resolved Hide resolved

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch 3 times, most recently from 22ac48e to 0c68ab7 Compare February 12, 2024 20:13

MaferMazu approved these changes Feb 20, 2024

View reviewed changes

itsjeyd added the product review complete PR has gone through product review label Feb 23, 2024

felipemontoya requested changes Feb 27, 2024

View reviewed changes

itsjeyd mentioned this pull request Feb 28, 2024

feat!: footer legal links openedx/frontend-component-footer#403

Open

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch 3 times, most recently from 1ec6b5c to 697b58f Compare February 28, 2024 17:34

mariajgrimaldi added 5 commits February 28, 2024 13:36

refactor: address PR reviews

9bd092d

refactor: use hide_from_toc only for subsection definition

a0bb146

Revert "refactor: use hide_from_toc only for subsection definition"

7168594

This reverts commit 0d402b714e085ef058672b8e57ed67418f29a684.

refactor: address PR reviews

5a2199f

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 697b58f to 5a2199f Compare February 28, 2024 17:36

refactor: address PR reviews for less changes

e552d66

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from ed99841 to e552d66 Compare February 28, 2024 17:46

mariajgrimaldi added 2 commits February 28, 2024 14:02

refactor: move staff lock inside list fields group

96b5edb

refactor: reorder content visibility settings

b97b2ba

mariajgrimaldi force-pushed the MJG/configurable-hide-from-toc branch from 155272f to b97b2ba Compare February 28, 2024 19:12

felipemontoya approved these changes Feb 28, 2024

View reviewed changes

mariajgrimaldi merged commit f544a48 into openedx:master Feb 29, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make hide from TOC a visibility section setting #33952

feat: make hide from TOC a visibility section setting #33952

mariajgrimaldi commented Dec 19, 2023 •

edited

Loading

openedx-webhooks commented Dec 19, 2023 •

edited

Loading

mariajgrimaldi Jan 15, 2024

felipemontoya Feb 27, 2024

mariajgrimaldi Feb 27, 2024 •

edited

Loading

mariajgrimaldi commented Feb 12, 2024 •

edited

Loading

mariajgrimaldi commented Feb 12, 2024

MaferMazu left a comment

itsjeyd commented Feb 21, 2024

MaferMazu commented Feb 22, 2024

itsjeyd commented Feb 23, 2024

felipemontoya Feb 27, 2024

felipemontoya commented Feb 27, 2024

felipemontoya left a comment

openedx-webhooks commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

feat: make hide from TOC a visibility section setting #33952

feat: make hide from TOC a visibility section setting #33952

Conversation

mariajgrimaldi commented Dec 19, 2023 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Dec 19, 2023 • edited Loading

mariajgrimaldi Jan 15, 2024

Choose a reason for hiding this comment

felipemontoya Feb 27, 2024

Choose a reason for hiding this comment

mariajgrimaldi Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi commented Feb 12, 2024 • edited Loading

mariajgrimaldi commented Feb 12, 2024

MaferMazu left a comment

Choose a reason for hiding this comment

itsjeyd commented Feb 21, 2024

MaferMazu commented Feb 22, 2024

itsjeyd commented Feb 23, 2024

felipemontoya Feb 27, 2024

Choose a reason for hiding this comment

felipemontoya commented Feb 27, 2024

felipemontoya left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

edx-pipeline-bot commented Feb 29, 2024

mariajgrimaldi commented Dec 19, 2023 •

edited

Loading

openedx-webhooks commented Dec 19, 2023 •

edited

Loading

mariajgrimaldi Feb 27, 2024 •

edited

Loading

mariajgrimaldi commented Feb 12, 2024 •

edited

Loading