feat: implement tag object rest api in cms by rpenido · Pull Request #33181 · openedx/openedx-platform

rpenido · 2023-09-05T22:04:10Z

Description

This PR implements the tagging REST API for creating/updating object tags in Courses and XBlocks.

Supporting Information

Closes [Tagging] Implement tagging REST API: Update content object tags modular-learning#98

Testing instructions

Ensure that the tests cover the expected behavior of the view as described in the related issue.

Private ref: FAL-3499

openedx-webhooks · 2023-09-05T22:04:14Z

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.

…ect_rest_api_in_cms

ChrisChV · 2023-09-13T17:07:18Z

 openedx-filters                     # Open edX Filters from Hooks Extension Framework (OEP-50)
-openedx-learning                    # Open edX Learning core (experimental)
+# openedx-learning                    # Open edX Learning core (experimental)
+openedx-learning @ git+https://github.com/openedx/openedx-learning@main  # Remove this once openedx-learning is tagged


@bradenmacdonald could you please create a tag to deploy the new version of openedx-learning?

Sure, done. We'll need to merge this PR soon because v0.1.6 has a breaking change:

File "/openedx/edx-platform/openedx/features/content_tagging/rules.py", line 99, in <module> def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) -> bool: AttributeError: module 'openedx_tagging.core.tagging.rules' has no attribute 'ObjectTag'

I'm pinning the version to avoid unwanted updates while openedx-learning is in development/unstable.
Does that make sense? bb18095

Yes that makes sense.

ChrisChV

@rpenido Looks good 👍 . I think we can reduce some boilerplate on test_rules.py. I added some other comments too.

I read through the code
I ran the tests

ChrisChV · 2023-09-13T17:44:02Z

 rules.set_perm("oel_tagging.view_object_tag", rules.always_allow)
+
+# Users can tag objects using tags from any taxonomy that they have permission to view
+rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_change_object_tag_taxonomy)


@rpenido Could you update the comments here to give the information that this perms are used on openedx_tagging. A developer who doesn't know this can get lost here.

Done 33dafac

ChrisChV · 2023-09-13T17:52:56Z

+        ("oel_tagging.add_object_tag", "tax_both_course1"),
+        ("oel_tagging.add_object_tag", "tax_both_course2"),
+        ("oel_tagging.add_object_tag", "tax_both_xblock1"),
+        ("oel_tagging.add_object_tag", "tax_both_xblock2"),


@rpenido Maybe to avoid this boilerplate, you can have an static list of all tag_attr (are the same for the three perms) and only leave the three perms on the ddt

Done 7eb4139

Let me know if I got it right!

Yep, Thanks!

Co-authored-by: Chris Chávez <xnpiochv@gmail.com>

bradenmacdonald

I would prefer to see a test or two that actually uses the REST API to change the tags of a component, where it works if the user has edit permission on the course and returns 403 if the user doesn't have edit permission on that course.

Also, we need to update the dependency to use the new tag v0.1.6 that I just published. Otherwise, this looks good! I can merge tomorrow.

…_api_in_cms

rpenido · 2023-09-15T12:02:08Z

Hello @bradenmacdonald! How are you doing?

I would prefer to see a test or two that actually uses the REST API to change the tags of a component, where it works if the user has edit permission on the course and returns 403 if the user doesn't have edit permission on that course.

Done 64107d1

Also, we need to update the dependency to use the new tag v0.1.6 that I just published. Otherwise, this looks good! I can merge tomorrow.

Done 52c529b

…_api_in_cms

rpenido · 2023-09-18T16:30:29Z

Hi @bradenmacdonald!

Is there something I can do to help this get merged?

Thank you!

bradenmacdonald · 2023-09-18T17:22:26Z

@rpenido I'll merge it today.

openedx-webhooks · 2023-09-18T18:07:04Z

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

edx-pipeline-bot · 2023-09-18T19:32:57Z

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

edx-pipeline-bot · 2023-09-18T19:54:02Z

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

rpenido added 2 commits September 5, 2023 09:21

refactor: simplify CMS tagging rules

49cc836

fix: permissions and tests

74c72f2

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 5, 2023

rpenido and others added 14 commits September 6, 2023 10:20

style: fix pep8

b9a8cd7

chore: update deps

3593a7a

fix: permissions and add tests

bf0fcf3

test: add more tests

0fe4886

feat: add xblock tagging support

1a6c014

chore: temp update requirements

03878b7

fix: requirements

1a59741

style: fix pep8

4216802

test: fixing test_rules WIP

d39f623

test: fix test_rules

06dceb9

style: fix pep8

d7dcca6

style: cleaning code

43f50a7

Merge branch 'openedx:master' into rpenido/fal-3449-implement_tag_obj…

5df7d27

…ect_rest_api_in_cms

style: revert minor style changes

f045f76

ChrisChV reviewed Sep 13, 2023

View reviewed changes

ChrisChV approved these changes Sep 13, 2023

View reviewed changes

rpenido and others added 3 commits September 13, 2023 19:10

docs: add better description on permission override

33dafac

docs: fix typo

d0216ee

Co-authored-by: Chris Chávez <xnpiochv@gmail.com>

refactor: simply test

7eb4139

rpenido changed the title ~~feat: implement tag object rest api in cms WIP~~ feat: implement tag object rest api in cms Sep 13, 2023

style: fix pep8

fe16c83

rpenido mentioned this pull request Sep 14, 2023

[Tagging] Clean up Tagging models openedx/modular-learning#85

Closed

bradenmacdonald approved these changes Sep 14, 2023

View reviewed changes

chore: update openedx-learning version

52c529b

rpenido force-pushed the rpenido/fal-3449-implement_tag_object_rest_api_in_cms branch 2 times, most recently from 2763102 to 03d51ab Compare September 15, 2023 11:56

rpenido force-pushed the rpenido/fal-3449-implement_tag_object_rest_api_in_cms branch from 03d51ab to 52c529b Compare September 15, 2023 11:57

rpenido added 2 commits September 15, 2023 08:57

Merge branch 'master' into rpenido/fal-3449-implement_tag_object_rest…

ed3ede8

…_api_in_cms

test: test tagging without object permissions

64107d1

chore: pin openedx-learning version

bb18095

rpenido force-pushed the rpenido/fal-3449-implement_tag_object_rest_api_in_cms branch from 1822ed9 to bb18095 Compare September 15, 2023 12:06

rpenido added 5 commits September 15, 2023 09:21

chore: fix requirements

84e1af0

style: fix pep8

8eea75d

chore: trigger CD/CI

eab33aa

Merge branch 'master' into rpenido/fal-3449-implement_tag_object_rest…

9ae2c61

…_api_in_cms

Merge branch 'master' into rpenido/fal-3449-implement_tag_object_rest…

09f1362

…_api_in_cms

bradenmacdonald merged commit 3c3306c into openedx:master Sep 18, 2023

bradenmacdonald deleted the rpenido/fal-3449-implement_tag_object_rest_api_in_cms branch September 18, 2023 18:07

Uh oh!

Conversation

rpenido commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting Information

Testing instructions

Uh oh!

openedx-webhooks commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisChV left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

rpenido commented Sep 15, 2023

Uh oh!

rpenido commented Sep 18, 2023

Uh oh!

bradenmacdonald commented Sep 18, 2023

Uh oh!

openedx-webhooks commented Sep 18, 2023

Uh oh!

edx-pipeline-bot commented Sep 18, 2023

Uh oh!

edx-pipeline-bot commented Sep 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rpenido commented Sep 5, 2023 •

edited

Loading

openedx-webhooks commented Sep 5, 2023 •

edited

Loading

rpenido Sep 15, 2023 •

edited

Loading