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

[e0d]

@MHaton mentioned that Sections are displayed for learners even when the learners cohort and the sections content group specify it's not for them.

Also annoyingly, learners always have access to sections, even when there are no units assigned to them in those sections. This means you'd have to put something in there saying "This isn't for you, move along". It's not ideal.

@ormsbee thought that the same logic that is used to hide subsections could be extend to hide sections. That logic is here.

[ormsbee]

FWIW, you can already add content group settings directly to both the section and subsection in OLX-there is just no UI exposed for it in Studio.

[CefBoud]

Hello,

FYI, I am currently working on this issue. I will create a PR as soon as I have something.

writing this to avoid duplicate work.

Cheers

[ormsbee]

@CefBoud: FYI that @Agrendalath is also going to be working with Section level settings soon. I don't think there's a conflict here, particularly if you are not adding new UI in Studio. But I wanted you folks to be aware of each other.

FYI @jmakowski1123

Good luck!

[ormsbee]

Relevant ticket: openedx/platform-roadmap#278

[ormsbee]

@CefBoud: I just saw in the #wg-product-core Slack yesterday that there is also this effort to make hide_from_toc (hide from table of contents) better supported: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3853975595/Draft+Hide+from+table+of+contents

One thing that belatedly occurred to me is that we might want to exclude a Section because the user cannot see any of its Subsections through some combination of different rules–e.g. say one subsection has the wrong cohort, and another is marked as hide_from_toc.

In light of this, maybe we should reconsider bubbling up cohort information all the way to the section level. We can make cohorts work exactly like enrollment tracks work today and bubble up only to the subsection. Then we can have a separate step to remove entirely empty sections–so we could add a remove_empty_sections method to CourseOutlineData. Something that looks kinda like:

def remove_empty_sections(self):
    empty_section_keys = [
        section.usage_key for section in self.sections if not section.sequences
    ]
    return self.remove(empty_section_keys)

Then we can add a call to that in the place where we filter results from the course outline.

Does that sound good to you?

FYI @jmakowski1123

[CefBoud]

@ormsbee Thanks for sharing and taking the time!

I started working on the issue, here is the draft PR.

One thing that belatedly occurred to me is that we might want to exclude a Section because the user cannot see any of its Subsections through some combination of different rules–e.g. say one subsection has the wrong cohort, and another is marked as hide_from_toc.

Exactly the conclusion I came to. I changed _bubbled_up_groups_from_units function to make it more general i.e. bubble up access groups from children (Units/Sequences) to parents (Sequence/Section) and then use it also to bubble up access groups from Sequences to Sections.

Currently, bubbled up access groups from Units to parent Sequences are effective and taken into account in the case of EnrollementTrack Partition Groups and that's thanks to EnrollmentTrackPartitionGroupsOutlineProcessor. For cohorts, there is no such Processor to enforce those bubbled up access groups. In my PR, I tried adding a CohortPartitionGroupsOutlineProcessor which does what the EnrollmentTrackPartitionGroupsOutlineProcessor does but for cohorted partition groups.

Then we can have a separate step to remove entirely empty sections–so we could add a remove_empty_sections method to CourseOutlineData. Something that looks kinda like:

Yes, but that view_handler only uses the CourseOutlineData to filter sequences and sends all sections regardless if they are present or not in the Outline. So even if an empty section in no longer present in the CourseOutlineData, it would still be sent. The same applies for supposedly inaccessible sections through bubbled up access groups.

That's why I also tried adding a filter section step much like there is a filter sequence one .

Instead of calling the new remove_empty_sections in the view handler, how about calling it inside the CourseOutlineData remove function? The empty section will no longer be part of the Outline and will get filtered out with the inaccessible ones after the Processors do their job.

If other OutlineProcessors are tweaked to also handle Sections in usage_keys_to_remove (this is already the case for EnrollmentTrackPartitionGroupsOutlineProcessor ), it would be taken into account with the new Section filtering.

What do you think?

[ormsbee]

Then we can have a separate step to remove entirely empty sections–so we could add a remove_empty_sections method to CourseOutlineData. Something that looks kinda like:

Yes, but that view_handler only uses the CourseOutlineData to filter sequences and sends all sections regardless if they are present or not in the Outline. So even if an empty section in no longer present in the CourseOutlineData, it would still be sent. The same applies for supposedly inaccessible sections through bubbled up access groups.

Yup, I'm in total agreement–no matter how we decide to implement this on the learning_sequences API side, we'll have to modify the view handler to ignore sections unless they're in the get_user_course_outline call.

Instead of calling the new remove_empty_sections in the view handler, how about calling it inside the CourseOutlineData remove function? The empty section will no longer be part of the Outline and will get filtered out with the inaccessible ones after the Processors do their job.

Okay, that sounds reasonable. The main reason I had suggested making a new function is because of my backwards compatibility concerns, but I suspect this behavior change will be desired by most folks.

If other OutlineProcessors are tweaked to also handle Sections in usage_keys_to_remove (this is already the case for EnrollmentTrackPartitionGroupsOutlineProcessor ), it would be taken into account with the new Section filtering.

I don't quite understand this part.

---

So my understanding of the implementation would be:

1. Modify the CourseOutlineData.remove method by filtering out empty sections (adding something like and section.sequences here).
2. Filter out the sections in the view handler similar to how subsections are filtered today (which you've already implemented).
3. Implement a CohortPartitionGroupsOutlineProcessor that works similarly to the one for enrollment tracks.

If we do (1), do we still need to do bubble-up logic from sequences to sections in cms/djangoapps/contentstore/outlines.py?

[CefBoud]

I don't quite understand this part.

What I meant is that if there is a need to hide sections according to (new?) rules from other Processors, it would be taken into account. VisibilityOutlineProcessor and EnrollmentTrackPartitionGroupsOutlineProcessor already filter out sections.

So my understanding of the implementation would be:

1. Modify the CourseOutlineData.remove method by filtering out empty sections (adding something like and section.sequences here).
2. Filter out the sections in the view handler similar to how subsections are filtered today (which you've already implemented).
3. Implement a CohortPartitionGroupsOutlineProcessor that works similarly to the one for enrollment tracks.

Yes, exactly!

If we do (1), do we still need to do bubble-up logic from sequences to sections in cms/djangoapps/contentstore/outlines.py?

I understand what you mean. if a section's sequences are all inaccessible and removed, it, itself, will become empty and thus removed. But I would still advise for the bubbling up since it somewhat decouples a section from its sequences (maybe there is a future use-case where we'd only need a section's user partitions?). Furthermore, since the CourseSection Model has a new_user_partition_groups , why not make use of it? On the other hand, it is simpler to leave things as-is. Could go either way. 😄

[ormsbee]

I understand what you mean. if a section's sequences are all inaccessible and removed, it, itself, will become empty and thus removed. But I would still advise for the bubbling up since it somewhat decouples a section from its sequences (maybe there is a future use-case where we'd only need a section's user partitions?). Furthermore, since the CourseSection Model has a new_user_partition_groups , why not make use of it? On the other hand, it is simpler to leave things as-is. Could go either way. 😄

Yeah, I'm a little torn about this. On the one hand, it's appealing from the point of view of consistency. But on the other hand, we've had frustrating performance issues in the past around things like this–where you couldn't determine the value for something on an XBlock without checking on all of its descendants. Bubbling up content group settings from Unit to Subsection was necessary because we have a lot of production data like that–since as you know, there's no way to specify those settings on the subsection or section directly in Studio. But I don't want to set the precedent that things can bubble up the whole course tree like this unless we absolutely have to, and it sounds like we can get away with not doing so.

So for the scope of your current PR, please keep the existing behavior for bubbling up from units to subsections, and don't generalize it to sections.

That being said, if you want to make follow-on PRs to create a UI for subsection-level and section-level access group settings in Studio, you are more than welcome to. Someone was looking into doing this six months back, but didn't have time to implement. There's a short Slack thread about it.

[CefBoud]

@ormsbee
I removed the bubbling up from subsections to sections, keeping the existing behavior intact, and implemented what was mentioned above. The PR status is no longer draft.

I am still however waiting to receive the CLA and sign it so that the pipeline can run.

Thank you for taking the time.

[ormsbee]

@CefBoud: It sounds like you might have gotten caught in a window when we were trying to do some updates to our DocuSign integration. You should receive it soon.

[CefBoud]

@ormsbee I received and signed the CLA.

[tecoholic]

@ormsbee With #33191 merged, would be safe to mark this closed?