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

fix: enroll users when added to a course team role #32561

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

navinkarkera
Copy link
Contributor

@navinkarkera navinkarkera commented Jun 23, 2023

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 > Membership tab, go to Course Team Management section.
  • 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.

image

Other information

A point of discussion: Should we revoke user's course related roles when they unroll from a course?

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

openedx-webhooks commented Jun 23, 2023

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.

@mphilbrick211
Copy link

Hi @navinkarkera! Is this all set for review?

@navinkarkera
Copy link
Contributor Author

@mphilbrick211 Thanks for checking. Yes, it is ready,

@mphilbrick211 mphilbrick211 requested a review from a team July 25, 2023 20:48
@mphilbrick211
Copy link

Hello @openedx/teaching-and-learning! Would someone be able to help merge this? Thanks!

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 7, 2023
@mphilbrick211
Copy link

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

@mphilbrick211
Copy link

Hi @openedx/teaching-and-learning! Friendly ping on this.

@gabor-boros gabor-boros force-pushed the navin/staff-access-api branch from d91c0c8 to 6dd54c4 Compare October 1, 2023 07:34
@connorhaugh connorhaugh requested a review from hsinkoff October 23, 2023 15:02
@ashultz0
Copy link
Contributor

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?

@mphilbrick211
Copy link

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 :)

@navinkarkera
Copy link
Contributor Author

@mphilbrick211 Thanks for the reminder. I'll get back to this PR soon.

@open-craft-grove
Copy link

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 mphilbrick211 added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Dec 11, 2023
@mphilbrick211 mphilbrick211 added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 21, 2024
@mphilbrick211
Copy link

@mphilbrick211 Thanks for the reminder. I'll get back to this PR soon.

Hi @navinkarkera - just checking in on this.

@mphilbrick211 mphilbrick211 removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024
@navinkarkera navinkarkera force-pushed the navin/staff-access-api branch from 6dd54c4 to a3a353e Compare March 4, 2024 06:31
@navinkarkera
Copy link
Contributor Author

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.
image

If we don't want to change the behaviour, we should update the help text to reflect the actual changes.

@navinkarkera
Copy link
Contributor Author

@mariajgrimaldi Would you like to review this as well as it is related to #32436?
cc: @mphilbrick211

@mphilbrick211
Copy link

@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?

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Mar 20, 2024

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 #32436

I left a message in #wg-product-core asking the same question. Thanks!

@mariajgrimaldi
Copy link
Member

See https://openedx.slack.com/archives/C057J2D1WU9/p1710940996542269 for more context on the status change

@itsjeyd itsjeyd removed the request for review from hsinkoff March 21, 2024 09:49
@itsjeyd itsjeyd added product review PR requires product review before merging core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Mar 21, 2024
@crathbun428
Copy link

crathbun428 commented Mar 21, 2024

@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).

@navinkarkera
Copy link
Contributor Author

@crathbun428 That also makes sense. I am open to suggestions here as my only issue is with the text and behaviour mismatch.

@crathbun428
Copy link

@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 crathbun428 added product review complete PR has gone through product review and removed product review PR requires product review before merging labels Mar 25, 2024
@navinkarkera navinkarkera force-pushed the navin/staff-access-api branch 3 times, most recently from 6d6c1bd to c2737e4 Compare March 28, 2024 07:37
@navinkarkera
Copy link
Contributor Author

@crathbun428 Thanks! Should we also do the same for forum related roles: #32436

@mariajgrimaldi The PR is ready for your review.

@itsjeyd itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Mar 28, 2024
@navinkarkera
Copy link
Contributor Author

@mariajgrimaldi Gentle reminder.

@mariajgrimaldi
Copy link
Member

@navinkarkera: on it! Thanks for the reminder

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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.
@navinkarkera navinkarkera force-pushed the navin/staff-access-api branch from c2737e4 to 5aaaf06 Compare April 24, 2024 14:26
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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!

@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Apr 25, 2024
@mariajgrimaldi mariajgrimaldi changed the title fix: allow only enrolled users in course team roles fix: enroll users when added to a course team role Apr 26, 2024
@mariajgrimaldi mariajgrimaldi merged commit 61b8961 into openedx:master Apr 26, 2024
66 checks passed
@openedx-webhooks
Copy link

@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.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U product review complete PR has gone through product review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants