New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rpenido/fal 3474 implement tagging rest api tag object #9
Conversation
83741de
to
1013db9
Compare
e080b6d
to
c18b3f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisChV
I made some changes to the API. Let me know if they are ok or if I need to include something.
if not isinstance(tags, (list, tuple)): | ||
raise ValueError( | ||
_(f"Tags must be a list or tuple, not {type(tags).__name__}.") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit error if not a list or tuple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
raise ValueError( | ||
_(f"Tags must be a list or tuple, not {type(tags).__name__}.") | ||
) | ||
tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing duplicates to clean lists like ["Tag 1", "Tag 1"]
instead of throwing an IntegrityError
on save
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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",
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Otherwise, it should be a list of existing Tag IDs. |
|
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these tests to cover the changes in api.py
1013db9
to
0e6f12f
Compare
* 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
14740c6
to
1ff6adc
Compare
Closes in favor of openedx#74 |
Temp PR