Rpenido/fal 3474 implement tagging rest api tag object by rpenido · Pull Request #9 · open-craft/openedx-learning

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 wants to merge 5 commits intoyusuf-musleh/implement-tagging-rest-api-retrieve-object-tagsfrom
rpenido/fal-3474-implement_tagging_rest_api_tag_object

rpenido commented Aug 15, 2023

Uh oh!

rpenido left a comment

Uh oh!

rpenido Aug 16, 2023

Uh oh!

ChrisChV Aug 17, 2023

Uh oh!

rpenido Aug 16, 2023

Uh oh!

ChrisChV Aug 17, 2023

Uh oh!

rpenido Aug 16, 2023

Uh oh!

ChrisChV Aug 17, 2023

Uh oh!

rpenido Aug 17, 2023 •

edited

Loading

Uh oh!

rpenido Aug 17, 2023

Uh oh!

ChrisChV Aug 18, 2023

Uh oh!

rpenido Aug 16, 2023

Uh oh!

rpenido commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rpenido commented Aug 15, 2023

Uh oh!

rpenido left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido Aug 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rpenido Aug 17, 2023 •

edited

Loading