feat: Copy object tags [FC-0062] #236

ChrisChV · 2024-10-02T00:37:07Z

This PR adds:

A function copy_tags in api.py to copy tags between objects.
A new field is_copied in ObjectTag

Supporting information

Github issue: Copy tags when using content from a V2 Library in a Course modular-learning#177
It's used in: feat: Copy tags when sync library [FC-0062] edx-platform#35596
Internal ticket: FAL-3616

Testing instructions

Verify that the tests cover the new code.
Follow the testing instructions in feat: Copy tags when sync library [FC-0062] edx-platform#35596

Deadline

ASAP

openedx-webhooks · 2024-10-02T00:37:11Z

Thanks for the pull request, @ChrisChV!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @axim-engineering. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

…et the serializer for tags

navinkarkera

👍 @ChrisChV Perfect 💯

I tested this: (Copied tags, deleted tags, added same tag in source and dest and copied them.)
I read through the code
I checked for accessibility issues
Includes documentation

openedx_tagging/core/tagging/api.py

navinkarkera · 2024-10-09T04:34:09Z

tests/openedx_tagging/core/tagging/test_api.py

+
+        assert len(object_tags) == 2
+        for index, object_tag in enumerate(object_tags):
+            object_tag.full_clean()


Suggested change

object_tag.full_clean()

Is full_clean required here?

Yes, we use full_clean() to validate the object tag in the test. It's used in other tests: [1][2][3]

bradenmacdonald

Suggested some changes. Thinking ahead, we may also need an API to "convert" the tags if/when the link is broken. That is, if the user links some library into a course, then later breaks the link, the tags that were read-only would need to be changed to is_copied=False so that they can be edited. But we can do that in a separate PR if you prefer.

openedx_tagging/core/tagging/rest_api/v1/views.py

openedx_tagging/core/tagging/api.py

bradenmacdonald · 2024-10-11T23:55:58Z

openedx_tagging/core/tagging/models/base.py

+        default=False,
+        help_text=_(
+            "True if this object tag has been copied from one object to another"
+            " using 'copy_tags' api function"


Suggested change

" using 'copy_tags' api function"

" using 'copy_tags' api function. Used to make copied tags read-only in the UI."

I think this should not be here. If the tagging app is reused in another project other than edx-platform, a copied tag does not necessarily mean it is read-only in the UI.

bradenmacdonald · 2024-10-11T23:57:54Z

openedx_tagging/core/tagging/rest_api/v1/serializers.py

@@ -176,6 +176,7 @@ def to_representation(self, instance: list[ObjectTag]) -> dict:
        """
        Convert this list of ObjectTags to the serialized dictionary, grouped by Taxonomy
        """
+        ObjectTagViewMinimalSerializer = self.context["view"].minimal_serilizer_class


What is the purpose of this, vs. just hard-coding the use of ObjectTagMinimalSerializer ?

I do it this way so that that serializer can be overridden. In my other PR I override that serializer to override get_can_delete_objecttag and prevent the user from deleting copied tags.

Would be good to mention this in a comment: "# Allows consumers like edx-platform to override this"

Updated: 7289b22

openedx_tagging/core/tagging/api.py

bradenmacdonald · 2024-10-15T16:29:23Z

openedx_tagging/core/tagging/api.py

+                taxonomy_id=object_tag.taxonomy_id,
+                tag_id=object_tag.tag_id,
+                defaults={"is_copied": True},
+                # Note: _value and _export_id should be set automatically


Did you confirm this is working? (That _value and _export_id are getting set automatically?)

Yes, I confirm that 👍

Update comment: 6395019

bradenmacdonald · 2024-10-15T16:33:04Z

@ChrisChV I can merge this when you fix the conflicts and reply - let me know.

ChrisChV · 2024-10-16T00:58:05Z

@bradenmacdonald It's ready for merge.

Suggested some changes. Thinking ahead, we may also need an API to "convert" the tags if/when the link is broken. That is, if the user links some library into a course, then later breaks the link, the tags that were read-only would need to be changed to is_copied=False so that they can be edited. But we can do that in a separate PR if you prefer.

Sorry I forgot this, yes, we can do this in a separate PR

feat: Copy tags function and read_only Object tags

d653a8e

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

ChrisChV marked this pull request as draft October 2, 2024 00:37

refactor: copy_tags function to keep not-copied tags

3e985d6

ChrisChV changed the title ~~feat: Copy tags function and read_only Object tags~~ feat: Copy object tags Oct 8, 2024

ChrisChV changed the title ~~feat: Copy object tags~~ feat: Copy object tags [FC-0062] Oct 8, 2024

ChrisChV mentioned this pull request Oct 8, 2024

feat: Copy tags when sync library [FC-0062] openedx/edx-platform#35596

Merged

1 task

refactor: Update ObjectTagsByTaxonomySerializer to use context to g…

fcd8437

…et the serializer for tags

ChrisChV marked this pull request as ready for review October 8, 2024 21:19

navinkarkera approved these changes Oct 9, 2024

View reviewed changes

ChrisChV added 3 commits October 9, 2024 20:08

style: Update names of functions and variables

6b73ec4

Merge branch 'main' into chris/FAL-3616-copy-tags

3dc5b0d

chore: Update version

2bb68ee

bradenmacdonald requested changes Oct 12, 2024

View reviewed changes

ChrisChV added 2 commits October 14, 2024 20:20

refactor: Updates on copy_tags and some nits

744a539

chore: Bump version to 0.16.0

adbf967

bradenmacdonald approved these changes Oct 15, 2024

View reviewed changes

ChrisChV added 3 commits October 15, 2024 19:43

chore: fix merge conflicts

4f8c0d3

style: Update comment

6395019

style: Add comment

7289b22

bradenmacdonald merged commit cdb2e3d into openedx:main Oct 16, 2024
9 checks passed

bradenmacdonald deleted the chris/FAL-3616-copy-tags branch October 16, 2024 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Copy object tags [FC-0062] #236

feat: Copy object tags [FC-0062] #236

ChrisChV commented Oct 2, 2024 •

edited

Loading

openedx-webhooks commented Oct 2, 2024 •

edited

Loading

navinkarkera left a comment

navinkarkera Oct 9, 2024

ChrisChV Oct 10, 2024

bradenmacdonald left a comment

bradenmacdonald Oct 11, 2024

ChrisChV Oct 15, 2024

bradenmacdonald Oct 11, 2024

ChrisChV Oct 15, 2024

bradenmacdonald Oct 15, 2024

ChrisChV Oct 16, 2024

bradenmacdonald Oct 15, 2024

ChrisChV Oct 16, 2024

ChrisChV Oct 16, 2024

bradenmacdonald commented Oct 15, 2024 •

edited

Loading

ChrisChV commented Oct 16, 2024

	" using 'copy_tags' api function"
	" using 'copy_tags' api function. Used to make copied tags read-only in the UI."

feat: Copy object tags [FC-0062] #236

feat: Copy object tags [FC-0062] #236

Conversation

ChrisChV commented Oct 2, 2024 • edited Loading

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Oct 2, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

navinkarkera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Oct 15, 2024 • edited Loading

ChrisChV commented Oct 16, 2024

ChrisChV commented Oct 2, 2024 •

edited

Loading

openedx-webhooks commented Oct 2, 2024 •

edited

Loading

bradenmacdonald commented Oct 15, 2024 •

edited

Loading