Tagging: serialize object permissions to REST API [FC-0036]

pomegranited commented 8 months ago

Description

Adds "user permissions" to the Tagging REST API to show what permissions the current request.user has on the objects serialized in the response.

Supporting information

Part of https://github.com/openedx/modular-learning/issues/160

Testing instructions

See https://github.com/openedx/frontend-app-course-authoring/pull/787 for manual testing instructions.

Deadline

None

Other information

TO DO (Re comment:)

[x] Please include a test that loads a bunch of taxonomies/tags/objecttags as a non-superuser and makes sure the query count is only increased by at most 1 when permissions are included in the response.

Done, see test_views.py.
[x] Do we want to allow API consumers to choose whether the perms info is included in the REST API responses or not? It's probably simpler to just always include them, unless there is some performance or response size penalty.

I decided to add an include_perms parameter which triggers the permission checks. These checks are cheap to run in this library, but users of the library may customize their permissions (like we did with content_tagging) and so the checks may not always be this quick to run.
[x] bump version

Private ref: FAL-3577

openedx-webhooks commented 8 months ago

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

bradenmacdonald commented 8 months ago

A few thoughts:

please include a test that loads a bunch of taxonomies/tags/objecttags as a non-superuser and makes sure the query count is only increased by at most 1 when permissions are included in the response
The permissions for all tags in a taxonomy are always the same, right? And the permissions for an ObjectTag always are the same as the permissions for the Object itself? So I'm tempted to say don't include permissions for them - just include perms at the taxonomy level or course component level. But I realize that's making an assumption that this won't change in the future. If we include the perms now with every Taxonomy/Tag/ObjectTag as you're proposing, and make the frontend work accordingly, then we can be super flexible about how permissions work in the future if we want to, and won't need to make any changes to the frontend. So that makes sense.
Do we want to allow API consumers to choose whether the perms info is included in the REST API responses or not? It's probably simpler to just always include them, unless there is some performance or response size penalty.

pomegranited commented 7 months ago

@bradenmacdonald could I get your review here?

bradenmacdonald commented 7 months ago

@rpenido @pomegranited Are you both happy with this PR and ready for final review + merge from me?

pomegranited commented 7 months ago

Thanks for your reviews @bradenmacdonald ! I'm going to merge & tag this and get https://github.com/openedx/edx-platform/pull/34004 ready for merge too.

openedx-webhooks commented 7 months ago

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

openedx / openedx-learning