openedx / modular-learning

3 stars 1 forks source link

[Tagging] Implement tagging REST API: Update object tags #82

Closed pomegranited closed 1 year ago

pomegranited commented 1 year ago

Story

"As a content author, I want to update the tags applied to my content."

Description

Implement a REST API that allows users to update the list of tags applied to a given object. This ticket implements the API in the oel_tagging library, but a later ticket will use to update the tags applied to content edited in Studio.

Completion criteria

Build on the work from https://github.com/openedx/modular-learning/issues/81 to enhance this endpoint to add support for :

PUT /api/oel_tagging/v1/object_tags/<object_id>/?taxonomy=<pk>

Note: uncertain which HTTP method to use here -- PUT? PATCH? POST?

Automated testing must cover common paths in behavioral specification.

Behavioral specifications

Suppose the Open edX instance has these taxonomies:

and the following users:

PUT /api/oel_tagging/v1/object_tags/abc/?taxonomy=st1

Documentation updates & improvements criteria

Relevant PRs/repositories

rpenido commented 1 year ago

Hi, @ChrisChV! CC @pomegranited I started working on the feature and got to this:

Note: Not sure how best to do this.. Currently the oel_tagging.api does not get involved with any rules, only views care about them. Maybe pass a callback through to tag_object that lets us check this rule for the request.user before saving changes to an ObjectTag?

Some premises:

I would like to suggest the following change in can_change_object_tag to keep this simpler, but I want to validate with you if I'm missing something:

-def can_change_object_tag(user: User, object_tag: ObjectTag = None) -> bool:
+def can_change_object_tag(user: User, taxonomy: Taxonomy = None) -> bool:

That way, we can check the permission only once before the API call in the view. What do you think?

PS: We have a temp PR here with the proposed view and rule:

ChrisChV commented 1 year ago

@rpenido I agree with your changes, makes sense to me, but I think you should keep the logic that taxonomy admins can edit object tags of disabled taxonomies, since all the other rules maintain that logic.

rpenido commented 1 year ago

Agreed @ChrisChV!

I diverged some things from the spec. I will list here to see if it is ok:

ChrisChV commented 1 year ago

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!

@rpenido About this I left a comment on the PR