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

[Pull Request]: Remove "Add a post" button when discussion is restricted for specific roles #391

Open
farhaanbukhsh opened this issue Nov 6, 2024 · 2 comments

Comments

@farhaanbukhsh
Copy link
Member

farhaanbukhsh commented Nov 6, 2024

Abstract

The issues we faced were when the discussions were restricted the "Add a post" button was shown to global staff and superusers but when they create a post the MFE doesn't let them create the post and the backend raises a 403 error.

Detailed Product Proposal

No response

Context & Background (in brief, if a Product Proposal is linked above)

The issues we faced were when the discussions were restricted the "Add a post" button was shown to global staff and superusers but when they create a post the MFE doesn't let them create the post and the backend raises a 403 error.

This happens because there exists a mismatch in permission on who can create a post when discussion is restricted.

There is a mismatch in the roles, hence I am taking edx-platform as my source of truth and what the implementation should be hence I dug a little deeper. From edx-platform we can confirm that only the user who has one of the following roles Discussion Admin, Moderator, Community TA or Group Moderator are allowed to make the post even during the restriction period. The proof for this is:

Docstring for has_discussion_privileges

This proves that course staff doesn't have priviledge

The MFE uses api at <LMS_URL>/api/discussion/v2/courses/<COURSE_ID>/ which uses get_course and here if the user has has_moderation_privileges or is_group_ta then they can post on a restricted discussion.

Hence on the MFE we need to change isPriviledge to use:

userHasModerationPrivileges || isUserGroupTA

and we should be aligned with the original idea.

Scope & Approach (in brief, if a Product Proposal is linked above)

This is a fix led by OpenCraft since the behavior already existed in the Legacy UI but they were missing in MFE.

Value & Impact (in brief, if a Product Proposal is linked above)

This brings the permission in the edx-platform inline with the usage in MFE.

Milestones and/or Epics

PR: openedx/frontend-app-discussions#742

Named Release

Teak

Timeline (in brief, if a Product Proposal is linked above)

It is ready to merge.

Proposed By

OpenCraft

Additional Info

No response

Copy link

github-actions bot commented Nov 6, 2024

Thanks for your submission, @openedx/openedx-product-managers will review shortly.

@marcotuts
Copy link

I'm not sure if what the legacy experience has is ideal, having previously run into issues / bug reports here only to realize that for that given course the permissions were missing for discussions specific roles, but that's a topic for another day. :) Perhaps in the future we could decide that superusers / course admins should automatically inherit discussion moderation and other needs, otherwise you have course operators / admins unsure of why they can't do basic discussions operations. This is all a low incidence / frequency thing so I don't think this is a rush to consider but for future RBAC work perhaps.

Matching the behavior as you described above makes sense, +1 to merge IMO. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: [Prod Proposals] NEW
Development

No branches or pull requests

2 participants