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: Adds a fix to remove "Add a post" button when discussion is restricted #742

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

farhaanbukhsh
Copy link
Member

@farhaanbukhsh farhaanbukhsh commented Oct 23, 2024

Description

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.

Useful information to include:

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

  1. Docstring for has_discussion_privileges
  2. 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 front we need to change isPriviledge to just use:

userHasModerationPrivileges || isUserGroupTA

and we should be aligned with the original idea.

Supporting information

Private Ref: BB-9249

Testing instructions

  1. Setup tutor on master
  2. Make sure to install and enable forum
  3. tutor plugins install forum
  4. tutor plugin enable forum
  5. Mount the discussions MFE
  6. tutor mounts add <path/to/frontend-app-discussions>
  7. Build the image tutor images build openedx-dev
  8. If you face error with forum image being pulled just pull docker pull overhangio/openedx-forum:18.1.1
  9. You can change the tag using docker image tag overhangio/openedx-forum:<current_tag> overhangio/openedx-forum:<new_tag>.
  10. You might have to run tutor dev run forum bin/rake search:initialize and tutor dev run forum bin/rake search:validate_indices
  11. Now log in to studio go the course and navigate to Content --> Page and Resources --> Disccussion
  12. Click the settings icon on discussion and proceed to the next page, add Restrictions here.
  13. Assuming you are using superuser go to the discussion page and you should see the banner that says Posting is restricted but you should see the "Add a post" button. Try creating a post and you will observer it will silently fail.
  14. If you check LMS log you will see 403 being raised.
  15. Now create a learner user and visit the discussion you should see the "Add a post" button is not visible. (✅ The permission works fine for learners)
  16. This proves that "Add a POST" button is hidden for learners when the discussion is restricted.
  17. Now switch the branch in frontend-app-discussion to open-craft:farhaan/fix-add-post-button
  18. Now visit the discussion again and this time you shouldn't see "Add a post" button if you are a superuser.
  19. To enable it Go to instructor tab and navigate to Course Team Management and give a user role such as Community TA or group TA to the user.
  20. Refresh the Discussion page and you should be able to see "Add a post" buttons , try to make a post and that should create a post.
  21. Try visiting the same restricted discussion with a learner account and the button shouldn't be visible.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

@openedx-webhooks
Copy link

openedx-webhooks commented Oct 23, 2024

Thanks for the pull request, @farhaanbukhsh!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/edx-infinity. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 23, 2024
@farhaanbukhsh farhaanbukhsh marked this pull request as draft October 24, 2024 15:27
@farhaanbukhsh farhaanbukhsh marked this pull request as ready for review October 24, 2024 20:40
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.17%. Comparing base (af6cd18) to head (55b85d9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   93.18%   93.17%   -0.01%     
==========================================
  Files         161      161              
  Lines        3404     3401       -3     
  Branches      919      919              
==========================================
- Hits         3172     3169       -3     
  Misses        215      215              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@Agrendalath Agrendalath 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: checked that the "Add a post" button is displayed according to the permissions defined in the LMS
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@farhaanbukhsh
Copy link
Member Author

@mphilbrick211 @itsjeyd can we schedule this ticket for upstream review? cc: @xitij2000

@itsjeyd
Copy link

itsjeyd commented Oct 29, 2024

@farhaanbukhsh Have you checked the criteria for product review to see if this PR qualifies? That would be the next step to complete here.

CC @mphilbrick211

Copy link
Contributor

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

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

@farhaanbukhsh I looked at the PR, and I think eventually the best place to fix this would be edx-platform. You are right to use that as the source of truth, and I think the value of isPostingEnabled should in itself be sufficient to know if posting is enabled, instead of duplicating that logic in the frontend and backend. That said, this change definitely makes sense since it is a bug fix and changing it in edx-platform could have other side effects at this point.

However, what we need to figure out is which one is the bug. Is the bug in the backend, i.e. should staff and course admins also be able to post during restrictions, OR, is the bug in the frontend, which is fixed here.

I feel it's a frontend bug, since if it were a backend bug, it would have been noticed immediately and fixed earlier. The old discussions didn't allow course admins to post, so we should retain that and if someone wants to fix it they can make a product proposal and PR.

That said, let's also hear back from @mphilbrick211 before merging this.

@farhaanbukhsh
Copy link
Member Author

@xitij2000 Thanks a lot 😄

@farhaanbukhsh
Copy link
Member Author

@itsjeyd As @xitij2000 pointed out it's a bugfix but maybe the proposal will give us clarify on what should be the right set of permissions. cc: @mphilbrick211

@farhaanbukhsh farhaanbukhsh changed the title fix: Adds a fix to remove "Add a post" button when discussion is rest… fix: Adds a fix to remove "Add a post" button when discussion is restricted Nov 6, 2024
@farhaanbukhsh
Copy link
Member Author

farhaanbukhsh commented Nov 6, 2024

@mphilbrick211 I have created a proposal; I didn't make a Wiki since it was a fix for what already exists in the platform.
cc: @itsjeyd @xitij2000

openedx/platform-roadmap#391

@itsjeyd
Copy link

itsjeyd commented Nov 7, 2024

Thanks for the updates @farhaanbukhsh!

Next steps would be to ping #wg-product-core on Slack and find a Coordinator:

product-submitter-creates-proposal

Details here.

CC @mphilbrick211

@farhaanbukhsh
Copy link
Member Author

@itsjeyd already pinged :)

@itsjeyd
Copy link

itsjeyd commented Nov 14, 2024

@farhaanbukhsh Awesome 🙂

…ricted

"Add a post" button  was visible even though the banner says that posting is
restricted. This change helps in removing the button when posting is restricted.

Signed-off-by: Farhaan Bukhsh <[email protected]>
@farhaanbukhsh farhaanbukhsh force-pushed the farhaan/fix-add-post-button branch from dae2720 to 55b85d9 Compare November 19, 2024 13:51
@farhaanbukhsh
Copy link
Member Author

@xitij2000 Rebased :)

@xitij2000 xitij2000 merged commit 3cc39d8 into openedx:master Nov 21, 2024
6 checks passed
@xitij2000 xitij2000 deleted the farhaan/fix-add-post-button branch November 21, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants