-
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
fix: enroll users when added to a course team role #32561
fix: enroll users when added to a course team role #32561
Conversation
Thanks for the pull request, @navinkarkera! 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. |
Hi @navinkarkera! Is this all set for review? |
@mphilbrick211 Thanks for checking. Yes, it is ready, |
Hello @openedx/teaching-and-learning! Would someone be able to help merge this? Thanks! |
Hi @navinkarkera! Flagging the new tests for you per the recent update: https://discuss.openedx.org/t/minor-change-to-edx-platform-check-statuses/11131 |
e21a4f9
to
3d09851
Compare
Hi @openedx/teaching-and-learning! Friendly ping on this. |
3d09851
to
d91c0c8
Compare
d91c0c8
to
6dd54c4
Compare
How long has the behavior as it is now been in place? It doesn't matter what the message is, if teams have been doing it this way for years changing it is going to confuse everyone. It adds a step to onboarding an assistant, who does that step and where? |
Hi @navinkarkera - flagging this for you :) |
@mphilbrick211 Thanks for the reminder. I'll get back to this PR soon. |
Sandbox deployment failed. Check failure logs here https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5652246006.log Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5652246006-config.yml Please check the settings and requirements and retry deployment by updating the pull request or posting a |
Hi @navinkarkera - just checking in on this. |
6dd54c4
to
a3a353e
Compare
@ashultz0 I think its been at least 10 years.
It is true that this adds an additional step which can be done in the same instructor tab on top. If we don't want to change the behaviour, we should update the help text to reflect the actual changes. |
@mariajgrimaldi Would you like to review this as well as it is related to #32436? |
Hi @mariajgrimaldi! Checking in to see if you're able to review this one? |
Hi folks @mphilbrick211, @navinkarkera! I left a message in #wg-product-core asking the same question. Thanks! |
See https://openedx.slack.com/archives/C057J2D1WU9/p1710940996542269 for more context on the status change |
@navinkarkera - I'm curious if implementing this fix differently to match the behavior in Studio was considered?: In Studio, if an unenrolled user is added as the course staff, the user is auto enrolled in the course. Of course, we'd want to update the text to the fix you make to the LMS as well to let the user know that adding the user will auto-enroll the user in the course if we go this route. This would add less steps for the user and keeping the flow consistent from Studio to the LMS makes sense. (It would be great to update the helper text in Studio at some point, too, but I understand that may be beyond the scope of this ticket). |
@crathbun428 That also makes sense. I am open to suggestions here as my only issue is with the text and behaviour mismatch. |
@navinkarkera - Perfect. If this doesn't prove too difficult, this is how we'd want to implement this instead. This way we're not adding an additional step for users in the flow, while also making things simple process-wise and clear through the helper text. If we were to implement this way instead, the helper text could say something like: Course team members with the Staff role help you manage your course. Staff can enroll and unenroll learners, modify their grades, and access all course data. Staff also have access to your course in Studio. Any users not yet enrolled in the course will be automatically enrolled when added as Staff. I'll mark Product Review as complete for now - happy to do a quick review if needed if and when the implementation is adjusted. Please let me know if any of the above isn't clear. |
6d6c1bd
to
c2737e4
Compare
@crathbun428 Thanks! Should we also do the same for forum related roles: #32436 @mariajgrimaldi The PR is ready for your review. |
@mariajgrimaldi Gentle reminder. |
@navinkarkera: on it! Thanks for the reminder |
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 tested this locally, and it's working as expected. I also reviewed the code, and everything looks right. I left a question to address before giving my thumbs up. Thank you!
The course team management section under Instructor > Membership tab allows users to be added a role even if are not enrolled in the course. This is behaviour does not match the help text displayed in the section. This PR updates modify_access api to enrolls user if they are not enrolled after adding them to a role as well as changes the help text to reflect actual changes.
c2737e4
to
5aaaf06
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.
LGTM! Thank you. I'll merge later this week!
@navinkarkera 🎉 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. |
Description
The course team management section under Instructor > Membership tab
allows users to be added a role even if are not enrolled in the course.
This behaviour does not match the help text displayed in the section.
This PR updates modify_access api to enrolls user if they are not enrolled
after adding them to a role as well as changes the help text to reflect
actual changes.
Inconsistencies in studio and lms interface to add Admin/Staff users found by @kaustavb12
Currently there are two ways to add an user as a course staff/admin. Either through the instructor dashboard in LMS, or through the Studio.
In Studio, if an unenrolled user is added as the course staff, the user is auto enrolled, even though the message in the Studio Course team page says otherwise (It says "All course team members can access content in Studio, the LMS, and Insights, but are not automatically enrolled in the course") :
However, in LMS, if an unenrolled user is added as the course staff, the user is NOT auto-enrolled. Although the message there says that only enrolled users can have course team roles.
This PR is updates LMS instructor dashboard behaviour to match with studio as well as studio text to reflect the real changes.
#32436 Is fixing the API for forum related roles as there is no inconsistency in case of forum related roles.
Supporting information
Private-ref
: BB-7609Testing instructions
Instructor > Membership
tab, go toCourse Team Management
section.Other information
A point of discussion: Should we revoke user's course related roles when they unroll from a course?