Tagging: serialize object permissions to REST API [FC-0036] - #34004
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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:
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. |
8359e78 to
0c070c8
Compare
5a820ef to
9db5e18
Compare
rpenido
left a comment
rpenido
left a comment
There was a problem hiding this comment.
This is looking good @pomegranited!
Good work also fixing the types.
I added a comment about the can_change_object_tag_objectid function.
There was a problem hiding this comment.
If we use change_objecttag_taxonomy at openedx-learning, we don't need to change it here.
There was a problem hiding this comment.
I've added TODO statements marking these query counts, since they're awfully high. But fixing them is out of scope for this task -- here, I only needed to verify that adding permissions to the response doesn't add to the query count.
There was a problem hiding this comment.
Out of curiosity, did you discover if it was related to the permissions?
I now we get the Org list and tag count (I'm not sure if in the same query that we get the Taxonomy)
There was a problem hiding this comment.
Yep, the extra queries seem to be the ones for getting the user orgs and matching them with the taxonomies.
There was a problem hiding this comment.
Sounds like we're missing a select_related somewhere.
There was a problem hiding this comment.
Good work @pomegranited! 👍
- I tested this using the Testing Instructions from openedx/frontend-app-authoring#787
- I read through the code
-
I checked for accessibility issues - Includes documentation
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-securerepository.
ToDo: update requirements after merge:
to add user permissions to the Tagging REST API.
7c8cd6e to
4013627
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
If you can confirm that the high query counts you flagged are not related to the permissions changes, this LGTM. I can merge tomorrow.
Would you mind creating a ticket on the modular learning board on GitHub to investigate those query counts, and ping me on it?
Confirmed. My apologies, I squashed out the commits that demonstrated the query counts were unchanged with and without calculating those permission flags in the response. If there had been a difference, I would have kept that Thank you for your help merging this!
Created openedx/modular-learning#185, but wasn't sure which milestone to attach it to. |
|
@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. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
5 participants
Description
Updates the installed version of openedx-learning and relevant tests to use the updated REST API serializers in openedx/openedx-core#138.
The Tagging feature is disabled in edX/2U production, so this change should not impact anyone.
Supporting information
Part of openedx/modular-learning#160
Testing instructions
See openedx/frontend-app-authoring#787 for manual testing instructions.
Deadline
None
Other information
Depends on:
Private ref: FAL-3577