-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
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:
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. |
ac1f568
to
7e1c023
Compare
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) |
51c2ddb
to
829315e
Compare
e63622f
to
575d950
Compare
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. |
6bdcf19
to
4ee64b0
Compare
There was a problem hiding this 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.
# .. toggle_implementation: CourseWaffleFlag | ||
# .. toggle_default: False | ||
# .. toggle_description: This flag enables the use of content groups for teams. | ||
# .. toggle_use_cases: open_edx |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Hi @kdmccormick! Thank you for the feedback and the patience; I'll address your comments ASAP. :) |
4ee64b0
to
d018b96
Compare
6321396
to
37bf55e
Compare
openedx/core/djangoapps/content/learning_sequences/api/tests/test_outlines.py
Show resolved
Hide resolved
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:
Test casesWith this setup:
Here's the list of test cases we've considered:
|
18b37d5
to
11d77c4
Compare
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: When I use the masquerade with Team A1, I can't see the unit. |
a839a96
to
740fa7d
Compare
Sandbox deployment successful 🚀 |
There was a problem hiding this 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:
For comparison, here is the Module 1 outline from the perspective of the openedx superuser:
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 👍🏻
There was a problem hiding this 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.
@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. |
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 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
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 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 |
@mariajgrimaldi @kdmccormick: Would you mind looking at #35028 when you get the chance? Thank you! |
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:
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):
dynamic_partition_generator
function calledcreate_team_set_partition
that creates aTeamUserPartition
object in memory for each course's team-set created via Pages & Resources > Teams UI in Studio.UserPartitionTransformer
transformer collects the blocks the current user can access. When doing so, the registered course outline processors are called. Among them is theTeamPartitionGroupsOutlineProcessor
, which gets the information in the blocksgroup_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.
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
Enable teams for the course: Studio > Select course > Content > Pages & resources > Teams > Teams toggle
Create a few team sets (or groups) from the Course Authoring MFE for your course:
Then, go to the LMS and create a few teams within those team-set (or topics, or groups):
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.
Now, restrict topics for each team as you would with cohorts or enrollment tracks:
As a student, try loading the course sections again. You won't be able to see any content.
As a student, join one of the teams and try again: you'll be able to see the content within your team
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