feat: CRUD API for Taxonomy Tags [FC-0036]

[yusuf-musleh]

Description

This PR introduces the CRUD python and REST APIs for interacting with Taxonomy Tags. Creating, updating and deleting them.

Supporting Information

Related Tickets:

• Closes [Tagging] CRUD API for editing tags in a taxonomy modular-learning#136

Testing Instructions

Make sure all the tests pass, and they cover all the cases.

---

Private-ref: FAL-3519

[openedx-webhooks]

Thanks for the pull request, @yusuf-musleh! 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.

[pomegranited]

This is looking good @yusuf-musleh !
Let me know when you're ready for a full review?

[pomegranited]

Your view changes are looking great @yusuf-musleh , and you're correct, there's some rules/permission changes needed.

[pomegranited]

@yusuf-musleh I think your best approach would be to modify (and rename) TagListPermissions to provide a perm_map like we did with ObjectTagObjectPermissions.

That will enforce the oel_tagging.view_tag, oel_tagging.change_tag, and oel_tagging.delete_tag rules before invoking the view methods.

But I think you're correct in your question -- these oel_tagging.*_tag rules need to be updated to reflect the changes to the taxonomy view/change rules made by #94.

I think something like this would be sufficient?

def can_view_tag(user: UserType, tag: Tag | None = None) -> bool:
    """
    Users can view tags for any taxonomy they can view.
    """"
    taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None
    return  user.has_perm(
        "oel_tagging.view_taxonomy",
        taxonomy,
    )

def can_change_tag(user: UserType, tag: Tag | None = None) -> bool:
    """
    Users can change tags for any taxonomy they can modify.
    """"
    taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None
    return  user.has_perm(
        "oel_tagging.change_taxonomy",
        taxonomy,
    )

rules.add_perm("oel_tagging.add_tag", can_change_tag)
rules.add_perm("oel_tagging.change_tag", can_change_tag)
rules.add_perm("oel_tagging.delete_tag", can_change_tag)
rules.add_perm("oel_tagging.view_tag", can_view_tag)
rules.add_perm("oel_tagging.list_tag", can_view_tag)

[yusuf-musleh]

@pomegranited Thanks for the pointers! I went ahead with what you suggested, however I faced a few issues and took me quite some time to figure them out:

1. The get_queryset method in TaxonomyTagsView was initially returning a list, as it does some custom population of an attribute subtags and later serialized using the TagsForSearchSerializer. For the updated permissions (using perm_map) to work,get_queryset needs to return a queryset as it is used internally by DRF. The current solution I have (though not a huge fan of it) is getting the result list and constructing a queryset using filter and then repopulating the subtags again if applicable. Ideally, I think the approach should be to refactor the underlying implementation of get_matching_tags to use querysets instead. I couldn't get to that today, but I thought I'd mention it here to get your thoughts.
2. After adding the perms_map for TagObjectPermissions, I left the has_object_permission method in place, since it's a special case for the GET list endpoint. There is not tag object in that case, we have the Taxonomy object, so we utilize the can_view_taxonomy permission for the oel_tagging.list_tag case. It gets called in the get_taxonomy method to and handle whether the use can view the Tags under this Taxonomy or not.

[pomegranited]

@yusuf-musleh

1... For the updated permissions (using perm_map) to work, get_queryset needs to return a queryset as it is used internally by DRF... Ideally, I think the approach should be to refactor the underlying implementation of get_matching_tags to use querysets instead.

I agree.. have you seen @bradenmacdonald 's #92 ? I think that PR does what you need already. We ran out of budget to complete it, but if it's needed for this work, then maybe we can complete it here?

And what you did for the permissions looks great, thank you.

[yusuf-musleh]

@pomegranited I went through @bradenmacdonald 's #92, lots of great work there, and already does alot of the heavy lifting for the refactoring, however since there are substantial changes made in that PR (and some more work remaining there) I feel it might get pretty messy to include it as part of this PR. Would it make sense to leave the current code in this PR as is and then continue working on #92 and along with the refactoring needed here as part of a separate ticket?

Alternatively, I could copy the relevant changes from that #92 and include it as part of this PR? What do you guys think?

[bradenmacdonald]

Would it make sense to leave the current code in this PR as is and then continue working on #92 and along with the refactoring needed here as part of a separate ticket?

Yes, I think that makes sense :)

[pomegranited]

Created openedx/modular-learning#142 for this.

[yusuf-musleh]

Great, thanks for creating it @pomegranited !

[bradenmacdonald]

I would generally prefer that we use value to identify tags in these API methods, rather than having an int tag ID. Same for parent_tag_id -> parent_tag_value and tag_ids -> list[str].

The reasons are:

• In Simplify Tag Models [FC-0030] #87 I found that using value (which the DB enforces to be a unique ID) lets us handle free-text and closed taxonomies consistently, without need different identifiers for different types of taxonomies. This reduced the overall line count and made it simpler to reason about.
• This lets us use a similar argument type (value) for add_tag_to_taxonomy and update_tag_in_taxonomy
• The tag_object API now uses value to identify tags, so I think we should be consistent with that.
• My proposed optimization/simplification of getting tags emphasizes value over ID, and I'm still hoping I'll be able to finish that up at some point.
• IDs aren't necessarily stable in the test suite, but value is so you can simplify your tests

But I know there are downsides as well so let me know if you guys don't like that direction.

[pomegranited]

Ooh good points, @bradenmacdonald -- I agree that using value where we can makes sense.

[yusuf-musleh]

I see, that makes sense, I updated the implementation to use values instead of IDs. 3abce26

[pomegranited]

We're getting there.. I've left a couple of minor comments, but would like to re-review once you've addressed Braden's suggestions.

[pomegranited]

I don't understand why this validation needs to be this complicated, when the same validation on the Taxonomy.delete_tags is much simpler?

Also, do we need to this extra validation in these serializers, since the implementation also enforces the same rules? I'm genuinely asking, I'm not sure where this should sit, but I don't think it needs to be duplicated.

[yusuf-musleh]

The implementation in the Taxonomy.delete_tags is actually an incorrect one, since the simple value__in filter doesnt account for case-insensitve values, the current tests didn't catch it. Thanks for pointing it out, I'll update it.

Also, do we need to this extra validation in these serializers, since the implementation also enforces the same rules?

My initial thought process was to add the validation in the api incase the functions were called directly rather than only in the rest_api/views (that go through serializers), however then I figured we could also make use of the validation that DRF provides in addition to that.

The rules they enforce are slightly different, you'll notice the validation that happens in the serializer checks if the Tag values provided are valid (i.e exist in the DB regardless to which taxonomy they belong to) however the validation that happens in the api method checks if the Taxonomy has the provided (Tag) values, slight difference where the former would return a 400 and the later would return a 404. I guess the end result is technically the same (the request fails), just thought this give more granularity in checks and errors.

[yusuf-musleh]

The implementation in the Taxonomy.delete_tags is actually an incorrect one, since the simple value__in filter doesnt account for case-insensitve values, the current tests didn't catch it.

Actually, doing some tests, surprising the simple implementation works 😮, It might be because of the special custom case_insensitive_char_field we use in the model? Since that is generally not the default behavior for django's charfields. In any case, that was a pleasant surprise! I went ahead and simplified the validation.

[pomegranited]

Ah lovely, I'm glad to hear that case-insensitive fields aren't so hard to interact with!

I'm still not convinced that the extra Serializer validation is needed, since the model does what it does and more. But I'll leave that to you & upstream's discretion.

[pomegranited]

👍 Looks great to me @yusuf-musleh ! Ready for your eyes @bradenmacdonald .

Note: needs a version bump before merging.

• I tested this using the django devserver and the DRF REST API UI.
• I read through the code -- great tests!
• I checked for accessibility issues
• Includes documentation
• Commit structure follows OEP-0051

[bradenmacdonald]

Almost there! You don't have to do all of these as changes, just let me know your thoughts.

See also my suggestion on the edx-platform PR about changing how we use reverse so that this API can be hosted in any namespace, not just oel_tagging

[bradenmacdonald]

Personally I would put this validation in the Taxonomy.add_tag method, where most other validation is anyways. That way it gets used for both the python API and REST API. Currently, only the REST API has this validation. You can leave it as-is though.

Removing the validation method(s) from these three serializers would also make the serializer code much more compact.

[yusuf-musleh]

After trying it out, it's definitely more concise and having the validations all in one place is cleaner, thanks @pomegranited and @bradenmacdonald for the suggestions! I updated it. b0e4b26

[bradenmacdonald]

Nit: I think it would be a bit more RESTful to put the old value in the URL and just use value as the name of the new value field. Though since value is a mutable ID it's not a huge deal. So no need to change for now.

What I would definitely recommend we do now is remove the PUT example from the docstring. Because if we add the ability to change other fields like external_id via this API endpoint in the future, anyone who is using PUT to change the name will now be accidentally overwriting/deleting the external_id, since PUT means "completely replace the tag with this new spec". But PATCH explicitly means "update the fields I specify to the new values and leave others unchanged".

[yusuf-musleh]

I see, for sure, I removed the PUT example as suggested, and kept the endpoint as is for now.

[yusuf-musleh]

@bradenmacdonald Thanks for the review! I address/applied your suggestions, including the name space suggestion, I also bumped the version, it should hopefully be good to go now.

[bradenmacdonald]

Thanks for those changes! This is good to merge, but I would like to see those duplicate checks removed first, unless there's some reason to keep them in that I'm missing.

[yusuf-musleh]

@bradenmacdonald Thanks for the review! I've updated the PR to remove the duplicate checks, and the version bump is already included.

[openedx-webhooks]

@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.