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

feat: Update forum role membership api to DRF ( 11th ) #35343

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

awais786
Copy link
Contributor

@awais786 awais786 commented Aug 22, 2024

Issues

Verify via postman

  1. Send post request to this URL
  2. form-data
    unique_student_identifier: username
    rolename: pass any from the list [Administrator, Moderator, Group Moderator, Community TA]
    action: allow or revoke

expected result
{ "course_id": "course-v1:edx+cs222+2311", "action": "allow" }
Verify via instructor dashboard

  1. Go to this page
  2. select the role from drop down and press add. It will appear in the Discussion Moderators list.
  3. Now press revoke link. It will remove the user from the list.
Screenshot 2024-08-22 at 6 36 18 PM


if rolename not in [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_GROUP_MODERATOR,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line, now serializer will do all the validation.

@awais786 awais786 changed the title Update forum role membership feat: Update forum role membership api to DRF ( 11th ) Aug 22, 2024
Comment on lines +2847 to 2850
has_instructor_access = has_access(request.user, 'instructor', course)
has_forum_admin = has_forum_access(
request.user, course_id, FORUM_ROLE_ADMINISTRATOR
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a shortcoming of the permissions.InstructorPermission class? that we can't check these two and the above EDIT_FORUM_ROLES permissions at the same time for a user? Could we adapt the permission class to check for multiple roles or use a different one so that all the permissions checks happen as DRF permission checks?

Copy link
Contributor Author

@awais786 awais786 Aug 27, 2024

Choose a reason for hiding this comment

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

@feanil in separate API which is related with forums I have re-write the functionality.

Related [PR]
(#35366). I will merge 35366 first and then I will rebase this later.

has_instructor_access means user must be instructor.
has_forum_admin means user be added as Administrator in Django_Comment_Common > Roles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants