-
Notifications
You must be signed in to change notification settings - Fork 181
feat: add team groups section in group configurations #1728
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
base: master
Are you sure you want to change the base?
feat: add team groups section in group configurations #1728
Conversation
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1728 +/- ##
=======================================
Coverage 95.18% 95.18%
=======================================
Files 1319 1320 +1
Lines 30022 30031 +9
Branches 6748 6750 +2
=======================================
+ Hits 28575 28584 +9
Misses 1378 1378
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM!!
@bradenmacdonald @PKulkoRaccoonGang @arbrandes I don't have permission over this repo, do you mind have a look to this PR?
|
Sandbox deployment successful 🚀 |
PKulkoRaccoonGang
left a comment
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 for the PR! After some testing, I have some suggestions/observations that probably don't apply to this PR. If they don't apply to your scope of work and they are valid, I don't mind moving them to a separate issue 💯
- If the course unit page is enabled, the link (which is under This group controls access to:) should be followed without reloading the page.
- After I create a team I get an error with status code 500

- After adding a new group, the user must manually scroll the inner content of the Configure teams modal window to the newly added group. Should the modal window content be automatically scrolled to the new group block?

- Is it worth showing the icon to collapse the new group creation block if it is not possible to do so until the group is created? I think this can be a bit misleading for users.

src/group-configurations/team-groups-section/TeamGroupsSection.test.jsx
Outdated
Show resolved
Hide resolved
1acce88 to
7267928
Compare
|
Hi @PKulkoRaccoonGang! Thank you for your review, and I apologize for the delay. I was busy with other things. I've already addressed your suggestions. Regarding what you found, I agree it should be moved to a separate issue. FYI, the 500 error has already been reported here: openedx/openedx-platform#36841, and work is in progress to fix it. |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
|
Hey everyone, what's the status of this PR? |
|
Hi @bradenmacdonald, thanks for your comment! I made the changes requested by @PKulkoRaccoonGang. The PR is ready for re-review. |
|
@PKulkoRaccoonGang do you have time to resume your review here? |
|
@BryanttV Please reach out to me if you don't hear back from him within a few days. |
|
Hi @bradenmacdonald! I haven't received a response yet. I'm tagging you so we can move forward. |
|
Sandbox deployment failed 💥 |
|
Sure @BryanttV , I'll try to review soon then. Could you please merge @crathbun428 or @jmakowski1123 could one of you confirm this is good to go from a product perspective? I can see there's been extensive discussion of the proposal but it's hard for me to tell the final decision(s) and how this aligns. |
9a0ca07 to
c4d79fd
Compare
|
Sandbox deployment failed 💥 |
|
@bradenmacdonald, thanks! rebased with master ✅ |
bradenmacdonald
left a comment
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.
| @@ -0,0 +1,24 @@ | |||
| import { initializeMocks, render } from '../../testUtils'; | |||
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.
| import { initializeMocks, render } from '../../testUtils'; | |
| import { initializeMocks, render } from '@src/testUtils'; |
Nit: could this file be .tsx ? We prefer TypeScript for any new files.
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.
File updated, thanks!
| import { AvailableGroup } from '../constants'; | ||
| import ContentGroupCard from '../content-groups-section/ContentGroupCard'; |
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.
| import { AvailableGroup } from '../constants'; | |
| import ContentGroupCard from '../content-groups-section/ContentGroupCard'; | |
| import { AvailableGroup } from '@src/group-configurations/constants'; | |
| import ContentGroupCard from '@src/group-configurations/content-groups-section/ContentGroupCard'; |
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, done!
| usage: null, | ||
| name: 'Team Group: My Group', | ||
| parameters: { | ||
| course_id: 'course-v1:org+101+101', |
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.
Should this be courseId ? When I rename TeamGroupsSection.test.jsx to .tsx, TypeScript says this is wrong. I guess it depends if you're mocking the raw response or the processed version that's been camelCased.
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.
The file is mixing snake_case and camelCase. But as I see it, the response should be processed in camelCase. I updated it.
|
Hi @BryanttV! Friendly ping on this. |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
|
@BryanttV are you still planning to finish up this PR? |
|
@bradenmacdonald, thanks for the ping! Over the last few weeks I've been catching up on some pending PRs and this one of them. I'll update it as soon as possible, sorry for the delay |
|
Sandbox deployment successful 🚀 |
c4d79fd to
d3289da
Compare
|
Sandbox deployment successful 🚀 |
|
Hi @bradenmacdonald, sorry for the delay, I just made the requested changes! I was testing, and everything works as expected. I couldn't reproduce the errors you reported. Could you check again with the new changes? |
|
Sandbox deployment successful 🚀 |



Description
This PR adds a new section (
TeamGroupsSection) to the Group Configurations page. This section is necessary to render the Teams configured in each Group of the course when the functionality to connect Teams with Content Groups is enabled.Supporting information
Screenshots
Before
After
Testing instructions
Create a Waffle Flag for your course in
{lms_domain}/admin/waffle_utils/waffleflagcourseoverridemodel/teams.content_groups_for_teamsEnables Teams in the platform using the
teams.enable_teams_appflag.Enable teams for your course in Studio > [Your Course] > Content > Pages & Resources > Teams > Teams toggle
In the same modal of Configure teams, create a few Groups.
Then, go to the LMS > [Your Course] > Teams and create a few Teams within those Groups (Topics/team-sets)
From Studio, go to Settings > Group Configurations.
You can see the new section for each of the groups. These sections are not editable.
Also, you can create Content Groups without any problem.
add-team-groups-section-in-group-configurations.webm
Additional Information
This is how it looks in the legacy interface:
teams-groups-legacy-interface.webm