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

pomegranited · 2024-01-04T07:00:10Z

Description

Updates the installed version of openedx-learning and relevant tests to use the updated REST API serializers in openedx/openedx-learning#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:

Tagging: serialize object permissions to REST API [FC-0036] openedx-learning#138

Private ref: FAL-3577

openedx-webhooks · 2024-01-04T07:00:14Z

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.

rpenido

This is looking good @pomegranited!
Good work also fixing the types.

I added a comment about the can_change_object_tag_objectid function.

rpenido · 2024-01-15T20:15:32Z

openedx/core/djangoapps/content_tagging/rules.py

@@ -219,7 +219,7 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool:
    Everyone that has permission to edit the object should be able to tag it.
    """
    if not object_id:
-        raise ValueError("object_id must be provided")
+        return True


If we use change_objecttag_taxonomy at openedx-learning, we don't need to change it here.

Noted, please see my reply.

pomegranited · 2024-01-16T02:48:58Z

openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py

+        with self.assertNumQueries(16):  # TODO Why so many queries?
+            response = self.client.get(url)


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.

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)

Yep, the extra queries seem to be the ones for getting the user orgs and matching them with the taxonomies.

Sounds like we're missing a select_related somewhere.

rpenido

Good work @pomegranited! 👍

I tested this using the Testing Instructions from Use object permissions in Tagging frontend [FC-0036] 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-secure repository.

ToDo: update requirements after merge:

Tagging: serialize object permissions to REST API [FC-0036] openedx-learning#138

…ning changes

to add user permissions to the Tagging REST API.

bradenmacdonald

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?

pomegranited · 2024-01-25T01:38:50Z

@bradenmacdonald

If you can confirm that the high query counts you flagged are not related to the permissions changes, this LGTM. I can merge tomorrow.

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 include_perms query parameter.

Thank you for your help merging this!

Would you mind creating a ticket on the modular learning board on GitHub to investigate those query counts, and ping me on it?

Created openedx/modular-learning#185, but wasn't sure which milestone to attach it to.

openedx-webhooks · 2024-01-25T18:12:20Z

@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.

edx-pipeline-bot · 2024-01-25T20:44:43Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-01-25T21:04:45Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-01-25T21:04:45Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 4, 2024

pomegranited force-pushed the jill/tagging-rest-perms branch 3 times, most recently from 8359e78 to 0c070c8 Compare January 11, 2024 06:53

pomegranited force-pushed the jill/tagging-rest-perms branch 2 times, most recently from 5a820ef to 9db5e18 Compare January 14, 2024 23:46

pomegranited mentioned this pull request Jan 15, 2024

Use object permissions in Tagging frontend [FC-0036] openedx/frontend-app-authoring#787

Merged

pomegranited marked this pull request as ready for review January 15, 2024 06:40

pomegranited requested a review from rpenido January 15, 2024 06:40

rpenido reviewed Jan 15, 2024

View reviewed changes

pomegranited commented Jan 16, 2024

View reviewed changes

rpenido self-requested a review January 17, 2024 13:33

rpenido approved these changes Jan 17, 2024

View reviewed changes

pomegranited mentioned this pull request Jan 23, 2024

Tagging: serialize object permissions to REST API [FC-0036] openedx/openedx-learning#138

Merged

3 tasks

pomegranited added 3 commits January 23, 2024 18:03

temp: use WIP branch of openedx-learning

4b920e7

fix: fixes tagging view tests and static types broken by openedx-lear…

4013627

…ning changes

feat: bumps openedx-learning to 0.4.4

1426c86

to add user permissions to the Tagging REST API.

pomegranited force-pushed the jill/tagging-rest-perms branch from 7c8cd6e to 4013627 Compare January 24, 2024 00:18

pomegranited requested a review from bradenmacdonald January 24, 2024 01:26

bradenmacdonald approved these changes Jan 24, 2024

View reviewed changes

pomegranited mentioned this pull request Jan 25, 2024

[Tagging] Reduce the number of queries used in content tagging REST API openedx/modular-learning#185

Closed

bradenmacdonald merged commit 7535f9d into openedx:master Jan 25, 2024
64 checks passed

bradenmacdonald deleted the jill/tagging-rest-perms branch January 25, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pomegranited commented Jan 4, 2024 •

edited

Loading

openedx-webhooks commented Jan 4, 2024 •

edited

Loading

rpenido left a comment

rpenido Jan 15, 2024

pomegranited Jan 16, 2024

pomegranited Jan 16, 2024

rpenido Jan 17, 2024 •

edited

Loading

pomegranited Jan 18, 2024

bradenmacdonald Jan 24, 2024 •

edited

Loading

rpenido left a comment •

edited

Loading

bradenmacdonald left a comment

pomegranited commented Jan 25, 2024

openedx-webhooks commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

		with self.assertNumQueries(16): # TODO Why so many queries?
		response = self.client.get(url)

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

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

Conversation

pomegranited commented Jan 4, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Jan 4, 2024 • edited Loading

rpenido left a comment

Choose a reason for hiding this comment

rpenido Jan 15, 2024

Choose a reason for hiding this comment

pomegranited Jan 16, 2024

Choose a reason for hiding this comment

pomegranited Jan 16, 2024

Choose a reason for hiding this comment

rpenido Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Jan 18, 2024

Choose a reason for hiding this comment

bradenmacdonald Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

rpenido left a comment • edited Loading

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

pomegranited commented Jan 25, 2024

openedx-webhooks commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

edx-pipeline-bot commented Jan 25, 2024

pomegranited commented Jan 4, 2024 •

edited

Loading

openedx-webhooks commented Jan 4, 2024 •

edited

Loading

rpenido Jan 17, 2024 •

edited

Loading

bradenmacdonald Jan 24, 2024 •

edited

Loading

rpenido left a comment •

edited

Loading