Skip to content

feat: add object tag view permissions [FC-0036]#94

Merged
bradenmacdonald merged 12 commits intoopenedx:mainfrom
open-craft:rpenido/fal-3518-permissions-for-taxonomies
Oct 16, 2023
Merged

feat: add object tag view permissions [FC-0036]#94
bradenmacdonald merged 12 commits intoopenedx:mainfrom
open-craft:rpenido/fal-3518-permissions-for-taxonomies

Conversation

@rpenido
Copy link
Contributor

@rpenido rpenido commented Oct 9, 2023

Description

Add view permission for ObjectTag

  • The oel_tagging.view_objecttag_objectid rule is used in the rest API view to check if the user has permission on the object_id to see its tags

Supporting Information

Testing instructions

  • Please ensure that the tests cover the expected behavior of the view

Private-ref: FAL-3518

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

openedx-webhooks commented Oct 9, 2023

Thanks for the pull request, @rpenido! 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.

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from ed87a26 to 447c7ba Compare October 9, 2023 13:22
@rpenido rpenido marked this pull request as ready for review October 9, 2023 13:23
@rpenido rpenido changed the title feat: add ObjectTag view permissions and filter feat: add ObjectTag view permissions and filters Oct 9, 2023
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch 2 times, most recently from 3087faa to 9b886cf Compare October 9, 2023 13:53
@pomegranited
Copy link
Contributor

@rpenido I don't think this PR is necessary -- We're anticipating functionality that's not specified yet, and that's gotten us into trouble before. I think it's enough for now to have this restriction in the content_tagging app.

CC @bradenmacdonald

@rpenido
Copy link
Contributor Author

rpenido commented Oct 10, 2023

Hi @pomegranited! I agree with you regarding the filter, but I think the permission belongs here:

I'll change the code to make it minimal. Let me know what you think!

@rpenido rpenido changed the title feat: add ObjectTag view permissions and filters feat: add ObjectTag view permissions Oct 10, 2023
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 9b886cf to 4a95e0a Compare October 10, 2023 13:59
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from b8f113b to c1502d0 Compare October 10, 2023 14:23
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

I agree with you regarding the filter, but I think the permission belongs here:

Ok yes, I get your reasoning for putting the rules check and view change here. 👍

But why make oel_tagging.view_objecttag_objectid False by default?

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from b727ed2 to 11c6f46 Compare October 11, 2023 11:50
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 11c6f46 to d7f78c1 Compare October 11, 2023 12:21
@rpenido rpenido requested a review from pomegranited October 11, 2023 19:44
@rpenido rpenido changed the title feat: add ObjectTag view permissions feat: add object tag view permissions Oct 11, 2023
rpenido and others added 2 commits October 12, 2023 13:26
…ized

Co-authored-by: Jillian <jill@opencraft.com>
The "view objecttag" rule now checks permissions for the taxonomy and
object_id if provided.
@rpenido
Copy link
Contributor Author

rpenido commented Oct 12, 2023

Hi @pomegranited! I think we are close! I made the requested changes.
Let me know if I miss something! If everything is right, I will squash it and send it to CC review.

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from d85ee7f to 677260d Compare October 12, 2023 16:57
@rpenido rpenido requested a review from pomegranited October 12, 2023 21:24
@bradenmacdonald bradenmacdonald changed the title feat: add object tag view permissions feat: add object tag view permissions [FC-0036] Oct 13, 2023
@bradenmacdonald bradenmacdonald added the FC Relates to an Axim Funded Contribution project label Oct 13, 2023
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 This is looking good, thank you @rpenido , and ready for CC review!

Could you make sure there's a version bump before it gets merged?

  • I tested this by checking the unit tests, and by ensuring this branch is installed for the tests run by openedx/openedx-platform#33413
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation -- good code comments
  • Commit structure follows OEP-0051

@rpenido
Copy link
Contributor Author

rpenido commented Oct 15, 2023

Thank you for the review @pomegranited!
@bradenmacdonald , can you make a CC review here?

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Nice! Just one minor question / change request, then I'll approve and merge.

@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from f43f2ad to c082514 Compare October 16, 2023 18:51
@rpenido rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from c082514 to 6b91980 Compare October 16, 2023 18:55
@bradenmacdonald
Copy link
Contributor

@rpenido Would you like me to merge this now?

@rpenido
Copy link
Contributor Author

rpenido commented Oct 16, 2023

Sure @bradenmacdonald! I think we are good here!
We also need to tag the new version (I already bumped the version to 0.2.5

@bradenmacdonald bradenmacdonald merged commit 38da66b into openedx:main Oct 16, 2023
@openedx-webhooks
Copy link

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald bradenmacdonald deleted the rpenido/fal-3518-permissions-for-taxonomies branch October 16, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants