fix: hide sequences & sections when access is restricted in units #33191

CefBoud · 2023-09-06T15:46:45Z

ISSUE #32892

Description

This PR addresses Issue#32892. The problem is that users who don't have access to any units in a sequence, because their cohort lacks the permission, can still see the sequence in their outline in the frontend-app-learning. The same applies to sections. If a user doesn't have access to all the section's sequences, the section should not be part of the outline.

As the issue mentions, there is a function that bubbles up access groups from Units to the parent Sequence in case all Units have the same access. This PR generalizes the function and applies it to bubble up access groups from Sequences to the parent Section in case all Sequences have the same access. => No bubbling to avoid performance issues

To get the outline, we rely on a list of processors. From what I can tell, only EnrollmentTrackPartitionGroupsOutlineProcessor removes inaccessible Sections/Sequences by relying on User Partitions, but as the name implies, only in the case of Enrollment Track restrictions, not cohorts. That's why this PR introduces a new CohortPartitionGroupsOutlineProcessor.

Supporting information

I created get_cohorted_user_partition_id in openedx/core/djangoapps/course_groups/cohorts.py because there is a need to retrieve the course's cohorted partition id, to check cohorted access. The function must solely rely on a Django Model (MySql) unlike other functions in the same file which in multiple cases lead to an interaction with the modulestore, and this is prohibited in all OutlineProcessor.

Testing instructions

Launch DevStack
Login/Sign up as a non-staff user (user2 for example) and enroll in the Demo Course.
The user can access all Sections/Sequences/Units. (e.g. Introduction > Demo Course Overview > Introduction: Video and Sequences)
In another session (Incognito/ different browser), log in as admin
Head to the course studio group settings
and create a Content Group group1.
create a cohort cohort1 in Instructor Tab > Cohorts Tab in the LMS and link it to group1.
We create another cohort cohort2 without assigning it to any group. Assign user2 to cohort2 in the same tab.
In the studio course outline, we choose a Section with a single Sequence that has a Single Unit and restrict its access to group2 for instance, Introduction: Video and Sequences
Upon accessing the frontend-app outline, user2 will be able to see the Section & Sequence even if he doesn't have access to the underlying Unit.

The video below recreates the problem. At the end, user2 should not be able to see the Introduction Section, nor the Demo Course Overview Sequence.

demo_issue_edx-platform_32892.mp4

After applying this PR, the Sequences whose Units are inaccessible will not show in the outline. Neither will the Sections whose Sequences are inaccessible.

openedx-webhooks · 2023-09-06T15:46:49Z

Thanks for the pull request, @CefBoud! 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.

ormsbee · 2023-09-07T19:01:23Z

@CefBoud: Just a reminder to please fill out the CLA if you haven't already. Thank you!

CefBoud · 2023-09-08T06:59:23Z

@ormsbee I filled out the form and I am waiting to receive the CLA.

0x29a

I left a few nits and one question related to tests, but 👍 from me. Good job!

I tested this: sections without accessible content aren't visible when cohorts are enabled.
I read through the code.
Includes documentation

CefBoud · 2023-09-13T13:09:33Z

openedx/core/djangoapps/content/learning_sequences/api/processors/cohort_partition_groups.py

+    def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime):
+        super().__init__(course_key, user, at_time)


I see what you mean but I tried to follow what was done in other processors : EnrollmentTrackPartitionGroupsOutlineProcessor, ContentGatingOutlineProcessor, ScheduleOutlineProcessor.

I think either way is fine, FWIW. Unless it's causing some kind of error in the linting?

It is not causing any errors.

0x29a · 2023-09-13T11:17:46Z

openedx/core/djangoapps/content/learning_sequences/api/processors/cohort_partition_groups.py

+            if user_cohort:
+                self.user_cohort_group_id, _ = get_group_info_for_cohort(user_cohort)
+
+    def _is_user_excluded_by_partition_group(self, user_partition_groups):


Shouldn't this be typed too?

Definitily!

0x29a · 2023-09-13T11:18:27Z

openedx/core/djangoapps/content/learning_sequences/api/tests/test_outlines.py

@@ -1716,6 +1718,216 @@ def test_processor_only(
            removed_usage_keys = processor.usage_keys_to_remove(full_outline)
            assert len(removed_usage_keys) == expected_values_dict[learner_to_verify.username]

+@ddt.ddt
+class CohortPartitionGroupsTestCase(OutlineProcessorTestCase):  # lint-amnesty, pylint: disable=missing-class-docstring


Do we need these linter directives here?

We do not! Removed.

0x29a · 2023-09-13T11:23:01Z

openedx/core/djangoapps/content/learning_sequences/api/tests/test_outlines.py

+    def _add_course_mode(
+        self,
+        course_key,
+        mode_slug=CourseMode.VERIFIED,
+        mode_display_name='Verified Certificate'
+    ):
+        """
+        Add a course mode to the test course_key.
+        Args:
+            course_key
+            mode_slug (str): the slug of the mode to add
+            mode_display_name (str): the display name of the mode to add
+            upgrade_deadline_expired (bool): whether the upgrade deadline has passed
+        """
+        signals.post_save.disconnect(update_masters_access_course, sender=CourseMode)
+        try:
+            CourseMode.objects.create(
+                course_id=course_key,
+                mode_slug=mode_slug,
+                mode_display_name=mode_display_name,
+                min_price=50
+            )
+        finally:
+            signals.post_save.connect(update_masters_access_course, sender=CourseMode)


Do we need to set a course mode to test this? I succeeded to test this using just audit mode.

Indeed, well spotted! Corrected.

0x29a · 2023-09-13T11:30:43Z

@ormsbee, could you help us with triggering the pipeline?

CefBoud · 2023-09-13T13:09:33Z

openedx/core/djangoapps/content/learning_sequences/api/processors/cohort_partition_groups.py

+    def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime):
+        super().__init__(course_key, user, at_time)


I see what you mean but I tried to follow what was done in other processors : EnrollmentTrackPartitionGroupsOutlineProcessor, ContentGatingOutlineProcessor, ScheduleOutlineProcessor.

ormsbee · 2023-09-13T13:21:20Z

lms/djangoapps/course_home_api/outline/views.py

+            course_blocks['children'] = [chapter_data
+                                         for chapter_data in course_blocks['children']
+                                         if chapter_data['id'] in available_section_ids
+                                         ] if 'children' in course_blocks else []


Nit: Please keep the old .get('children', []) piece instead of adding the if 'children' in... bit at the tail end of the list comprehension. Try to limit the use of the foo = bar if something else bar2 syntax to simple one-liners.

So something like:

course_blocks['children'] = [ chapter_data for chapter_data in course_blocks.get('children', []) if chapter_data['id'] in available_section_ids ]

FWIW, the linter might complain to you about the indentation here. I'm not sure if we have it in the rules for list comprehensions, but I tend to follow our long function call conventions when doing them.

Sure! Fixed.

I also fixed the CI pipeline errors.

ormsbee · 2023-09-13T21:17:27Z

@CefBoud: We do have your CLA back, so hopefully we can get this cleared soon. I'll let you know as soon as that part is all set. Thank you.

mphilbrick211 · 2023-09-15T13:51:35Z

@ormsbee @CefBoud - looks like the CLA check is still failing.

ormsbee · 2023-09-15T14:50:09Z

@mphilbrick211: Yeah, this is still in our court I think. I'll ping you with more details.

ormsbee · 2023-09-18T17:37:34Z

@CefBoud: Thank you for your patience during this process. I'm sorry that you had to go through it right as we were messing with the DocuSign integration. I see your CLA all set in our system now, so please rebase and I'll merge this later today (or first thing tomorrow).

CefBoud · 2023-09-18T18:13:27Z

@ormsbee Thank you! I am glad to hear it. I rebased and I see that the CLA is all set up. 😃

…ough cohorts

ormsbee · 2023-09-19T13:10:23Z

Re-running the tests now. I'll merge this as soon as tests pass.

openedx-webhooks · 2023-09-19T14:05:06Z

@CefBoud 🎉 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 · 2023-09-19T15:07:57Z

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

edx-pipeline-bot · 2023-09-19T15:30:18Z

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

edx-pipeline-bot · 2023-09-19T15:30:18Z

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 Sep 6, 2023

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 6, 2023

CefBoud force-pushed the hide-section-sequence-when-no-access branch 4 times, most recently from 98ebc97 to 9d64067 Compare September 7, 2023 15:16

CefBoud mentioned this pull request Sep 7, 2023

Hide sections when a learners cohort doesn't have access to the associated content group. #32892

Open

CefBoud force-pushed the hide-section-sequence-when-no-access branch 4 times, most recently from 341a791 to 9c53194 Compare September 11, 2023 14:19

CefBoud marked this pull request as ready for review September 11, 2023 14:41

CefBoud force-pushed the hide-section-sequence-when-no-access branch 2 times, most recently from 3120317 to e4912d9 Compare September 13, 2023 05:46

0x29a approved these changes Sep 13, 2023

View reviewed changes

CefBoud force-pushed the hide-section-sequence-when-no-access branch from e4912d9 to 475a313 Compare September 13, 2023 12:54

CefBoud commented Sep 13, 2023

View reviewed changes

ormsbee reviewed Sep 13, 2023

View reviewed changes

CefBoud force-pushed the hide-section-sequence-when-no-access branch 4 times, most recently from 16e9d3b to b1e377a Compare September 13, 2023 16:00

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 15, 2023

CefBoud force-pushed the hide-section-sequence-when-no-access branch from d2ed435 to 3134903 Compare September 15, 2023 15:37

CefBoud force-pushed the hide-section-sequence-when-no-access branch from 3134903 to b4a922a Compare September 18, 2023 18:09

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 18, 2023

fix: hide sequences & sections when access is restricted in units thr…

32bb24b

…ough cohorts

ormsbee force-pushed the hide-section-sequence-when-no-access branch from b4a922a to 32bb24b Compare September 19, 2023 13:24

ormsbee merged commit 9b44065 into openedx:master Sep 19, 2023
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: hide sequences & sections when access is restricted in units #33191

fix: hide sequences & sections when access is restricted in units #33191

CefBoud commented Sep 6, 2023 •

edited

Loading

openedx-webhooks commented Sep 6, 2023 •

edited

Loading

ormsbee commented Sep 7, 2023

CefBoud commented Sep 8, 2023

0x29a left a comment

CefBoud Sep 13, 2023

ormsbee Sep 13, 2023

CefBoud Sep 13, 2023

0x29a Sep 13, 2023

CefBoud Sep 13, 2023

0x29a Sep 13, 2023

CefBoud Sep 13, 2023

0x29a Sep 13, 2023

CefBoud Sep 13, 2023

0x29a commented Sep 13, 2023 •

edited

Loading

CefBoud Sep 13, 2023

ormsbee Sep 13, 2023 •

edited

Loading

CefBoud Sep 13, 2023

CefBoud Sep 13, 2023

ormsbee commented Sep 13, 2023

mphilbrick211 commented Sep 15, 2023

ormsbee commented Sep 15, 2023

ormsbee commented Sep 18, 2023 •

edited

Loading

CefBoud commented Sep 18, 2023

ormsbee commented Sep 19, 2023

openedx-webhooks commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

		def __init__(self, course_key: CourseKey, user: types.User, at_time: datetime):
		super().__init__(course_key, user, at_time)

fix: hide sequences & sections when access is restricted in units #33191

fix: hide sequences & sections when access is restricted in units #33191

Conversation

CefBoud commented Sep 6, 2023 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Sep 6, 2023 • edited Loading

ormsbee commented Sep 7, 2023

CefBoud commented Sep 8, 2023

0x29a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x29a commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

ormsbee Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee commented Sep 13, 2023

mphilbrick211 commented Sep 15, 2023

ormsbee commented Sep 15, 2023

ormsbee commented Sep 18, 2023 • edited Loading

CefBoud commented Sep 18, 2023

ormsbee commented Sep 19, 2023

openedx-webhooks commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

edx-pipeline-bot commented Sep 19, 2023

CefBoud commented Sep 6, 2023 •

edited

Loading

openedx-webhooks commented Sep 6, 2023 •

edited

Loading

0x29a commented Sep 13, 2023 •

edited

Loading

ormsbee Sep 13, 2023 •

edited

Loading

ormsbee commented Sep 18, 2023 •

edited

Loading