edx-platform
edx-platform copied to clipboard
openedx
fix: allow only enrolled users in course team roles
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-7609
Testing instructions
- Setup master devstack or tutor locally.
- Register two users, one of which should have staff access.
- Using the staff user, enroll to a course.
- Under
Instructor > Membershiptab, go toCourse Team Managementsection. - Without the changes in this PR, you will be able to add the other user (who did not enroll in this course) to any role.
- With the changes in this PR, it will raise an error with appropriate msg.
Other information
A point of discussion: Should we revoke user's course related roles when they unroll from a course?
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:
- 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.
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
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?
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 Instance requirements : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5652246006-requirements.txt
Please check the settings and requirements and retry deployment by updating the pull request or posting a /grove sandbox update comment.
@mphilbrick211 Thanks for the reminder. I'll get back to this PR soon.
Hi @navinkarkera - just checking in on this.
How long has the behavior as it is now been in place?
@ashultz0 I think its been at least 10 years.
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?
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 https://github.com/openedx/edx-platform/pull/32436? cc: @mphilbrick211
@mariajgrimaldi Would you like to review this as well as it is related to #32436? cc: @mphilbrick211
Hi @mariajgrimaldi! Checking in to see if you're able to review this one?
Hi folks @mphilbrick211, @navinkarkera! What @ashultz0 says is pretty valid. Although I understand the text is misleading, we're proposing changing the current behavior. So, should this go through a product review? This question also goes to https://github.com/openedx/edx-platform/pull/32436
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. cc: @mariajgrimaldi; @mphilbrick211
@crathbun428 Thanks! Should we also do the same for forum related roles: https://github.com/openedx/edx-platform/pull/32436
@mariajgrimaldi The PR is ready for your review.
@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.