Rpenido/fal 3474 implement tagging rest api tag object #9

rpenido · 2023-08-15T20:32:52Z

Temp PR

rpenido

@ChrisChV
I made some changes to the API. Let me know if they are ok or if I need to include something.

rpenido · 2023-08-16T20:29:00Z

openedx_tagging/core/tagging/models/base.py

+        if not isinstance(tags, (list, tuple)):
+            raise ValueError(
+                _(f"Tags must be a list or tuple, not {type(tags).__name__}.")
+            )


Explicit error if not a list or tuple

@rpenido In which cases can the tags be a tuple? In my opinion I would just keep it as a list, so as not to complicate the typing here

rpenido · 2023-08-16T20:30:27Z

openedx_tagging/core/tagging/models/base.py

+            raise ValueError(
+                _(f"Tags must be a list or tuple, not {type(tags).__name__}.")
+            )
+        tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order


Removing duplicates to clean lists like ["Tag 1", "Tag 1"] instead of throwing an IntegrityError on save

rpenido · 2023-08-16T20:33:07Z

openedx_tagging/core/tagging/models/base.py

+            # Find the existing object_tag index, if any
+            object_tag_index = next(
+                (
+                    i
+                    for i, object_tag in enumerate(current_tags)
+                    if object_tag.tag_ref == tag_ref or object_tag.value == tag_ref
+                ),
+                -1,
+            )
+
+            if object_tag_index >= 0:
+                object_tag = current_tags.pop(object_tag_index)


I'm using a list here to check the tag_ref and also the value.
I changed this because the following code was throwing IntegrityError on save, violating UNIQUE index.

def test_tag_object_same_value(self): # Tag the object with the same value twice tagging_api.tag_object( self.taxonomy, ["Eubacteria"], "biology101", ) tagging_api.tag_object( self.taxonomy, ["Eubacteria"], "biology101", )

Let me know if this is a problem.

I want to understand this change? Why is the IntegrityError occurring? From what I see, it is because the value of the tag is being used in closed taxonomies when it is necessary to use the id of the tag. I think that verification should not be done here. It should be done on the view. Because these changes I feel complicate this function.

I think you should revert the change you made here:

Changed the input from tags = [{"value": "Interesting stuff"}, {"value": "will surely"}, {"value": "go here"}] to simply tags = ["Interesting stuff", "will surely", "go here"]. The tag_object handle ids and values transparently, so I didn't see a need to treat this in the view explicitly. Let me know if I'm wrong!

In the view it is good to know if it is an id or a value to do the corresponding verifications

From what I see, it is because the value of the tag is being used in closed taxonomies when it is necessary to use the id of the tag. I think that verification should not be done here.

Exactly @ChrisChV !

Should I find the Tag from the value and pass its id in this case?

The first tag_object call works, but it shouldn't, right? This raises IntegrityError on the second call:

def test_tag_object_same_value(self): taxonomy = Taxonomy.objects.get(pk=-1) # Tag the object with the same value twice tagging_api.tag_object( taxonomy, ["Portuguese"], "biology101", ) tagging_api.tag_object( taxonomy, ["Portuguese"], "biology101", )

Should we always do this?

def test_tag_object_same_value(self): taxonomy = Taxonomy.objects.get(pk=-1) portuguese_tag = Tag.objects.get(value="Portuguese") # Tag the object with the same value twice tagging_api.tag_object( taxonomy, [portuguese_tag.id], "biology101", ) tagging_api.tag_object( taxonomy, [portuguese_tag.id], "biology101", )

I will take a step back here: I really liked the way the code below just works, and resync does its magic (well done!).

tagging_api.tag_object( taxonomy, ["Portuguese"], "biology101", )

If you think this is not desirable and can lead to side effects (that I do not know enough to evaluate) I will revert and also find a way to raise an error on this call.

Also, if this code is ok but the find by id and value in the tag_object function let the code unreadable, I can work to try to make it better (maybe creating a function to wrap the check?)

Hi @rpenido!

Also, if this code is ok but the find by id and value in the tag_object function let the code unreadable, I can work to try to make it better (maybe creating a function to wrap the check?)

Yes, it works for me.

Also you need to update the docstring to allow values for closed taxonomies

openedx-learning/openedx_tagging/core/tagging/models/base.py

Line 384 in b108896

Otherwise, it should be a list of existing Tag IDs.

rpenido · 2023-08-16T20:37:03Z

tests/openedx_tagging/core/tagging/test_api.py

+
+    def test_tag_object_string(self):
+        with self.assertRaises(ValueError) as exc:
+            tagging_api.tag_object(
+                self.taxonomy,
+                'string',
+                "biology101",
+            )
+        assert "Tags must be a list or tuple, not str." in str(exc.exception)
+
+    def test_tag_object_integer(self):
+        with self.assertRaises(ValueError) as exc:
+            tagging_api.tag_object(
+                self.taxonomy,
+                1,
+                "biology101",
+            )
+        assert "Tags must be a list or tuple, not int." in str(exc.exception)
+
+    def test_tag_object_same_id(self):
+        # Tag the object with the same value twice
+        tagging_api.tag_object(
+            self.taxonomy,
+            [self.eubacteria.id],
+            "biology101",
+        )
+        tagging_api.tag_object(
+            self.taxonomy,
+            [self.eubacteria.id],
+            "biology101",
+        )
+        tagging_api.tag_object(
+            self.taxonomy,
+            ["Eubacteria"],
+            "biology101",
+        )
+
+    def test_tag_object_same_value(self):
+        # Tag the object with the same value twice
+        tagging_api.tag_object(
+            self.taxonomy,
+            ["Eubacteria"],
+            "biology101",
+        )
+        tagging_api.tag_object(
+            self.taxonomy,
+            ["Eubacteria"],
+            "biology101",
        )

+    def test_tag_object_same_id_multiple(self):
+        self.taxonomy.allow_multiple = True
+        self.taxonomy.save()
+        # Tag the object with the same value twice
+        object_tags = tagging_api.tag_object(
+            self.taxonomy,
+            [self.eubacteria.id, self.eubacteria.id],
+            "biology101",
+        )
+        assert len(object_tags) == 1
+
+    def test_tag_object_same_value_multiple(self):
+        self.taxonomy.allow_multiple = True
+        self.taxonomy.save()
+        # Tag the object with the same value twice
+        object_tags = tagging_api.tag_object(
+            self.taxonomy,
+            ["Eubacteria", "Eubacteria"],
+            "biology101",
+        )
+        assert len(object_tags) == 1
+
+    def test_tag_object_same_value_multiple_free(self):
+        self.taxonomy.allow_multiple = True
+        self.taxonomy.allow_free_text = True
+        self.taxonomy.save()
+        # Tag the object with the same value twice
+        object_tags = tagging_api.tag_object(
+            self.taxonomy,
+            ["tag1", "tag1"],
+            "biology101",
+        )
+        assert len(object_tags) == 1
+


Added these tests to cover the changes in api.py

* chore: Remove is_valid checks from get_object_tags * fix: Rename ObjectTag perms to match model name * feat: Implement ObjectTag retrieve REST API Retrieve ObjectTags for given Object IDs, and optionally filter by taxonomy. * chore: bumped version

rpenido · 2023-08-21T18:50:29Z

Closes in favor of openedx#74

rpenido mentioned this pull request Aug 15, 2023

[Tagging] Implement tagging REST API: Update object tags openedx/modular-learning#82

Closed

yusuf-musleh force-pushed the yusuf-musleh/implement-tagging-rest-api-retrieve-object-tags branch 3 times, most recently from 83741de to 1013db9 Compare August 16, 2023 08:33

rpenido force-pushed the rpenido/fal-3474-implement_tagging_rest_api_tag_object branch 3 times, most recently from e080b6d to c18b3f7 Compare August 16, 2023 14:31

rpenido commented Aug 16, 2023

View reviewed changes

yusuf-musleh force-pushed the yusuf-musleh/implement-tagging-rest-api-retrieve-object-tags branch from 1013db9 to 0e6f12f Compare August 17, 2023 10:17

yusuf-musleh and others added 2 commits August 18, 2023 15:54

feat: add tag_object rest api

1ff6adc

rpenido force-pushed the rpenido/fal-3474-implement_tagging_rest_api_tag_object branch from 14740c6 to 1ff6adc Compare August 21, 2023 16:29

rpenido added 3 commits August 21, 2023 14:30

chore: update requirements

abe8cd5

style: cleaning code

994a82f

style: revert styling issues

898fa13

rpenido closed this Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rpenido/fal 3474 implement tagging rest api tag object #9

Rpenido/fal 3474 implement tagging rest api tag object #9

rpenido commented Aug 15, 2023

rpenido left a comment

rpenido Aug 16, 2023

ChrisChV Aug 17, 2023

rpenido Aug 16, 2023

ChrisChV Aug 17, 2023

rpenido Aug 16, 2023

ChrisChV Aug 17, 2023

rpenido Aug 17, 2023 •

edited

rpenido Aug 17, 2023

ChrisChV Aug 18, 2023

rpenido Aug 16, 2023

rpenido commented Aug 21, 2023

Rpenido/fal 3474 implement tagging rest api tag object #9

Rpenido/fal 3474 implement tagging rest api tag object #9

Conversation

rpenido commented Aug 15, 2023

rpenido 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

rpenido Aug 17, 2023 • edited

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

rpenido commented Aug 21, 2023

rpenido Aug 17, 2023 •

edited