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: connect teams with content groups using dynamic partition generator #33788

Merged
merged 53 commits into from
Apr 25, 2024

Conversation

mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Nov 23, 2023

Description

This PR implements the connection from the teams feature to the content groups feature discussed and approved by product. This implementation uses the dynamic partition generator extension point to associate content groups with the users that belong to a Team.

This implementation was heavily inspired by the enrollment tracks dynamic partitions.

Rejected alternatives:

  • Using a fixed bridge as it is done for cohorts (pull/33105)

Supporting information

Product roadmap issue for this feature:
Enhancements to teams to make learner grouping more flexible
Connecting teams to content groups

As of today, course authors can create topics or team-sets within a course. Then, associate teams to each team-set so students can join later. When connecting teams to content groups, learners can work on group assignments and access content designed by the course author specifically for that course team, as with cohorts or enrollment tracks.

This is how this feature works behind the scenes (roughly):

  • We plugged in a new dynamic_partition_generator function called create_team_set_partition that creates a TeamUserPartition object in memory for each course's team-set created via Pages & Resources > Teams UI in Studio.
  • Each (content) group associated with each partition is dynamically generated based on the teams within the team-set when loading course content in the LMS.
  • So, each time a Student loads the course blocks, the course UserPartitionTransformer transformer collects the blocks the current user can access. When doing so, the registered course outline processors are called. Among them is the TeamPartitionGroupsOutlineProcessor, which gets the information in the blocks group_access field and then matches it with the student's group filtering the course tree for the current user.

Testing instructions

Warning: You must create a new course to avoid data inconsistencies after the test.
Since we're adding a new entrypoint to the setup.py file, you should connect to your LMS/CMS containers and run: pip install -e .
Then, restart them.

  1. After creating your course, you'll need to turn on a course-level flag that enables content groups for teams:
    Go to /admin/waffle_utils/waffleflagcourseoverridemodel
    Add new flag using your new course ID: teams.content_groups_for_teams

  2. Enable teams for the course: Studio > Select course > Content > Pages & resources > Teams > Teams toggle
    image

  3. Create a few team sets (or groups) from the Course Authoring MFE for your course:
    image

  4. Then, go to the LMS and create a few teams within those team-set (or topics, or groups):
    image
    image

  5. As a student, try loading the sections of the course, you'll be able to load each course section. You'll be able to see them as usual.

  6. Now, restrict topics for each team as you would with cohorts or enrollment tracks:
    image
    image
    image

  7. As a student, try loading the course sections again. You won't be able to see any content.

  8. As a student, join one of the teams and try again: you'll be able to see the content within your team

  9. As a student, join the other team: you'll be able to see the content within both teams

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

More technical info on teams: https://openedx.atlassian.net/wiki/spaces/AC/pages/160611195
Course author docs on teams: https://edx.readthedocs.io/projects/edx-partner-course-staff/en/latest/course_features/teams/teams_setup.html#teams-overview

PR Sandbox

Settings

TUTOR_GROVE_WAFFLE_FLAGS:
- name: teams.content_groups_for_teams
  everyone: true
- name: teams.enable_teams_app
  everyone: true

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 23, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Nov 23, 2023

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.

@mariajgrimaldi mariajgrimaldi changed the title [WIP] feat: connect teams with content groups using dynamic partition generator feat: connect teams with content groups using dynamic partition generator Nov 23, 2023
@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review November 24, 2023 14:37
@mariajgrimaldi
Copy link
Member Author

Hi folks. I'm tagging you in our official technical proposal for connecting teams to content groups since you were tagged in the 1st rough implementation of the feature: #32806 (comment)
FYI @ormsbee, @feanil, @bmtcril, @kdmccormick, @jmakowski1123

@mariajgrimaldi
Copy link
Member Author

Just so you know, the tests are failing because of the query count, so I'll leave them be until we have a review and I hear what you think about the extra queries caused by the course waffle flag check.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

This looks like a great feature @mariajgrimaldi . The approach of using a partition generator seems solid to me.

I'll finish reviewing it over the next few days. Here are my comments so far.

openedx/core/djangoapps/course_groups/flags.py Outdated Show resolved Hide resolved
# .. toggle_implementation: CourseWaffleFlag
# .. toggle_default: False
# .. toggle_description: This flag enables the use of content groups for teams.
# .. toggle_use_cases: open_edx
Copy link
Member

Choose a reason for hiding this comment

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

Is this off-by-default because it is new, or because some courses with teams wouldn't want the team-sets showing up in Studio?

To ask another way: do you think we should eventually remove the flag, or should it stay in the platform forever?

Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Feb 20, 2024

Choose a reason for hiding this comment

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

We actually added the flag because of this comment: https://openedx.atlassian.net/wiki/spaces/COMM/pages/3885334531/Enhancements+to+Teams+to+make+learner+grouping+more+flexible?focusedCommentId=3901063179. So from the product side, this feature will be optional for the foreseeable future.
I wouldn't say some courses with teams wouldn't want the team-sets showing up in Studio but:
Some courses with teams wouldn't want the team-sets content groups association in their Studio configuration.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion is little bit of both. Yes, Daniel during the product review asked for this to be optional but I also think that reducing the friction for deployment is important for the time being. Teams is not a feature that received much work in the last few years and this feature is reversing that. Better to have the capacity to turn off in case a course finds it conflicting with their usage of teams.

I like the question if this should be an option forever. My broad take is that once the feature has had real world usage we should remove the feature. Based primarily on the fact that it can always be ignored by course authors. But that is something that I would not want to block this PR on.

Copy link
Member

Choose a reason for hiding this comment

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

I like the question if this should be an option forever. My broad take is that once the feature has had real world usage we should remove the feature. Based primarily on the fact that it can always be ignored by course authors. But that is something that I would not want to block this PR on.

This resonates with me @felipemontoya . This waffle flag will be a configuration option that controls a configuration option 😄

Would you mind changing the use cases to opt_in, temporary, and set a target removal date for "Teak"? That would be a good point to decide whether to remove the flag, or leave as an opt-in feature forever.

openedx/core/lib/teams_config.py Outdated Show resolved Hide resolved
@kdmccormick kdmccormick self-assigned this Jan 29, 2024
@mariajgrimaldi
Copy link
Member Author

Hi @kdmccormick! Thank you for the feedback and the patience; I'll address your comments ASAP. :)

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 21, 2024
@mariajgrimaldi
Copy link
Member Author

We understand how difficult it can be to set up a testing environment to review specific pull requests, so we'll add access information to the installation where we've been testing these particular changes.

You can follow the testing instructions in the PR cover letter with these credentials:

Username Password User role
instructor-openedx instructor-openedx Instructor
student-openedx student-openedx Student

LMS
CMS

Test cases

With this setup:

  • A course with two team-sets A and B, each team-set with two teams: Team A.1 and team A.2 for team-set A, and team B.1 and team B.2 for team-set B.
  • There are two subsections, each with two units.
  • Unit 1 in the 1st subsection is restricted to team A.1, and unit 2 is restricted to team A.2.
  • Unit 1 in the 2nd subsection is restricted to team B.1, and unit 2 is restricted to team B.2.

Here's the list of test cases we've considered:

  • If the flag is off, then there's no way of configuring the teams + content groups integration.
  • When the flag is on, an instructor can configure content groups for the course using the unit content restrictions view.
  • When an instructor configures a unit as restricted for a team, a message is shown under the unit in the course outline.
  • When a non-staff user with no team loads subsections in the LMS with restricted children units, the user cannot see the restricted blocks in the course outline.
  • When a staff user with no team loads subsections in the LMS with restricted children units, the user can see the restricted blocks in the course outline.
  • A non-staff user of team A.1 can load and see the unit's content restricted to the A.1 team, this should happen with each team.
  • A non-staff user of team A.1 and B.1, can load and see the unit's content restricted to the A.1 and B.1 team. The blocks should be listed in the course outline for the user.
  • A user cannot be part of two teams in the same team-set, so they can't load content restricted to more than one team within a team-set.

@BryanttV
Copy link
Contributor

Hi @mariajgrimaldi, doing some tests with the masquerade I see that when I set the view from a specific team, it does not show me the units to which that team has access. e.g.

I have this structure in the course. Team A1 has access to the Unit 1.1.1:
image

When I use the masquerade with Team A1, I can't see the unit.
image

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Code looks great, thanks @mariajgrimaldi !

Unfortunately I think I see another bug on the sandbox.

  • I have created a user learner2 (password p@ssw0rd) who is enrolled in the demo course but belongs to no teams or content groups
  • In the subsection Module 1 > Intro to Open edX, I have restricted all units to Content Group A.
  • In the subsection Module 1 > Structure of Open edX, I have restircted all units to Majo's team.

As expected, learner2 is unable to access any unit in either subsection. However, I'd expect that both subsections are hidden from learner2's course outline, but only the Intro to Open edX subsection is hidden:
image

For comparison, here is the Module 1 outline from the perspective of the openedx superuser:

image

My guess is that the OutlineProcessor is not removing subsections whose units are fully hidden by team access.

I would consider this a presentation issue rather than an integrity issue, so if you'd like to merge this PR at your discretion and then do a follow-up fix, that's fine by me 👍🏻

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mariajgrimaldi. I tested this latest version and it worked just as expected.

I tried combinations again using feature flag overrides per course and tried turning the feature on and off for a course that already had the team partitions configured to try and cause errors in mismatched config, but nothing. The code handled them all gracefully.

I think this is good for merging in this form and we can address the outline issue in a smaller follow up PR.

@mariajgrimaldi
Copy link
Member Author

@kdmccormick @felipemontoya: Thank you! I'll merge this PR in the next few days and work on fixing what you found. I'll let you know how it goes.

@kdmccormick
Copy link
Member

Sounds good. @mariajgrimaldi and @felipemontoya , this is an awesome feature... thank you both for taking the time to work it through product and eng review and get it upstreamed!

@mariajgrimaldi mariajgrimaldi merged commit 809ffc3 into master Apr 25, 2024
67 checks passed
@mariajgrimaldi mariajgrimaldi deleted the MJG/dynamic-team-partition branch April 25, 2024 17:02
@openedx-webhooks
Copy link

@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
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@mariajgrimaldi
Copy link
Member Author

mariajgrimaldi commented Apr 26, 2024

Hello, @kdmccormick @felipemontoya! In the last few days I've been working on the issue Kyle reported and I have a few comments:

I haven't been able to reproduce the issue in a dev tutor nightly environment. So I imported and set up the course, similar to what was in the sandbox, in a remote tutor nightly environment we've previously used to test this functionality, this environment is up-to-date with yesterday's afternoon master.

In that environment, we found a similar, but not quite the same, behavior. Whenever I loaded the course outline page, subsections with restricted units, ones with content groups and the others with teams content groups, were rendered. To debug further, I added some logs in the outline processor for teams' restricted content; after deploying it to the remote environment, I discovered that the Django flag was set to unknown instead of everyone, so it was off. After setting it to everyone, everything worked as usual, meaning the section with subsections with restricted units was hidden.

I merged this PR yesterday afternoon, so we can no longer access the sandbox to test whether this was a misconfiguration issue. I can't reproduce the error in our remote environment after turning on the flag correctly.

@kdmccormick: I think we can either test there with today's master in our remote environment and confirm we can't replicate it, or we can create a new sandbox with your help. While testing this, I discovered this other issue so we can use it for the sandbox if that's what we prefer.

Let me know! Thank you

@robrap
Copy link
Contributor

robrap commented Jun 24, 2024

@mariajgrimaldi @kdmccormick: Would you mind looking at #35028 when you get the chance? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants