Skip to content

fix: enroll users when added to a forum role#32436

Merged
mariajgrimaldi merged 1 commit intoopenedx:masterfrom
open-craft:navin/modify-access-api
May 6, 2024
Merged

fix: enroll users when added to a forum role#32436
mariajgrimaldi merged 1 commit intoopenedx:masterfrom
open-craft:navin/modify-access-api

Conversation

@navinkarkera
Copy link
Contributor

@navinkarkera navinkarkera commented Jun 13, 2023 •
edited
Loading

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 is not the expected behaviour based on the help text displayed in the section.

This PR updates update_forum_role_membership api to enroll user before adding them to a role.

Supporting information

Private-ref: BB-7543

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.
  • Adding unenrolled user to any forum related role will enroll them to course.

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 13, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 13, 2023 •
edited
Loading

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.

kaustavb12
kaustavb12 reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@kaustavb12 kaustavb12 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navinkarkera

There are two parts to this PR:

  1. Discussion roles: Restricting this to only enrolled learners make sense here, since only enrolled users have access to the forum pages.

  2. Admin/Staff roles:
    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, we can add unenrolled users as course staff, in which case, the user is auto-enrolled to the course. Not sure if the two flows should behave differently, or if there needs to be a consistency, then what would be the correct behaviour.

@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch from 15d683e to 10377ea Compare June 23, 2023 13:49
@navinkarkera navinkarkera changed the title fix: allow only enrolled users in course team roles fix: allow only enrolled users in course forum related roles Jun 23, 2023
@navinkarkera
Copy link
Contributor Author

navinkarkera commented Jun 23, 2023

@kaustavb12 As per your suggestion, I am splitting up changes related to course team roles (Admin/staff roles) into a separate PR due to inconsistency in studio and lms interfaces. Now this PR holds only forum roles related change, i.e., only enrolled users can be added to forum roles via instructor dashboard.

@navinkarkera navinkarkera mentioned this pull request Jun 23, 2023
Merged
kaustavb12
kaustavb12 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@kaustavb12 kaustavb12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

LGTM

  • I tested this: Tested in devstack
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@navinkarkera navinkarkera mentioned this pull request Jun 23, 2023
Merged
navinkarkera added a commit to open-craft/openedx-platform that referenced this pull request Jun 26, 2023
@navinkarkera
fix: allow only enrolled users in course forum roles
259dbf6 
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 not the expected behaviour based on the help text displayed in
the section.

This PR updates update_forum_role_membership api to check whether user
is enrolled before adding them to a role.

Cherry pick from openedx#32436

(cherry picked from commit 10377ea7d0b1182f5d0b6d389161fab2bac1722a)
navinkarkera added a commit to open-craft/openedx-platform that referenced this pull request Jun 26, 2023
@navinkarkera
fix: allow only enrolled users in course forum roles
e3afa14 
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 not the expected behaviour based on the help text displayed in
the section.

This PR updates update_forum_role_membership api to check whether user
is enrolled before adding them to a role.

Cherry pick from openedx#32436

(cherry picked from commit 10377ea7d0b1182f5d0b6d389161fab2bac1722a)
@mphilbrick211
Copy link

mphilbrick211 commented Jul 25, 2023

Hi @openedx/teaching-and-learning! Would someone be able to review / merge this for us? Thanks!

@mphilbrick211 mphilbrick211 requested a review from a team July 25, 2023 20:48
@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

mphilbrick211 commented Sep 12, 2023

Hi @navinkarkera! Flagging the new tests per the recent update: https://discuss.openedx.org/t/minor-change-to-edx-platform-check-statuses/11131

@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch from 10377ea to 6dc28ad Compare September 13, 2023 10:27
@mphilbrick211
Copy link

mphilbrick211 commented Sep 14, 2023

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

@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch from 6dc28ad to f47fb1b Compare September 15, 2023 15:08
@gabor-boros gabor-boros force-pushed the navin/modify-access-api branch from f47fb1b to 471d39d Compare October 1, 2023 07:18
@mphilbrick211
Copy link

mphilbrick211 commented Oct 3, 2023

Hi @navinkarkera and @gabor-boros! Just flagging that a failing check has popped up.

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 3, 2023
@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Nov 13, 2023
@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

7 similar comments
@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@open-craft-grove
Copy link

open-craft-grove commented Nov 30, 2023

Sandbox destroy request received.

@mphilbrick211
Copy link

mphilbrick211 commented Mar 19, 2024

@navinkarkera - when you have a moment, could you please resolve the branch conflicts so Maria can review / merge? Thanks!

@mphilbrick211 @mariajgrimaldi Resolved conflicts and rebased. Please review.

Hi @mariajgrimaldi - checking in on this!

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Mar 20, 2024

Hi folks @mphilbrick211, @navinkarkera!
Although I understand the text is misleading, we're proposing changing the current behavior. So, should this go through a product review? I left a message in #wg-product-core asking this question. Thanks!

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Mar 20, 2024

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

@itsjeyd itsjeyd added core contributor PR author is a Core Contributor (who may or may not have write access to this repo). product review PR requires product review before merging and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Mar 21, 2024
@jmakowski1123 jmakowski1123 mentioned this pull request Mar 28, 2024
Closed
2 tasks
@navinkarkera
Copy link
Contributor Author

navinkarkera commented Apr 24, 2024

@mariajgrimaldi Should we also update this PR to auto enroll user instead of checking for enrollment similar to #32561?

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Apr 24, 2024

@navinkarkera: I think so since both PRs were linked to the product proposal.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 25, 2024
@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch from 1baf6cd to 91132f2 Compare April 26, 2024 16:51
@navinkarkera navinkarkera changed the title fix: allow only enrolled users in course forum related roles fix: enroll users when added to a forum role Apr 26, 2024
@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch 3 times, most recently from 372961b to 3ed3f67 Compare April 29, 2024 09:17
mariajgrimaldi
mariajgrimaldi reviewed Apr 29, 2024
View reviewed changes
mariajgrimaldi
mariajgrimaldi approved these changes Apr 29, 2024
View reviewed changes
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!

@navinkarkera navinkarkera force-pushed the navin/modify-access-api branch from 3ed3f67 to 9e95bae Compare May 6, 2024 06:02
mariajgrimaldi
mariajgrimaldi approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing all my suggestions. I'll be merging this later today.

@mariajgrimaldi mariajgrimaldi merged commit 8077719 into openedx:master May 6, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented May 6, 2024

@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

edx-pipeline-bot commented May 6, 2024

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

edx-pipeline-bot commented May 6, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

edx-pipeline-bot commented May 6, 2024

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

edx-pipeline-bot commented May 6, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

edx-pipeline-bot commented May 6, 2024

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

edx-pipeline-bot commented May 6, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

edx-pipeline-bot commented May 6, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariajgrimaldi mariajgrimaldi mariajgrimaldi approved these changes

+1 more reviewer

@kaustavb12 kaustavb12 kaustavb12 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mariajgrimaldi mariajgrimaldi

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 PR requires product review before merging

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@navinkarkera @openedx-webhooks @mphilbrick211 @open-craft-grove @mariajgrimaldi @edx-pipeline-bot @kaustavb12 @itsjeyd