Skip to content
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

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

Closed
pomegranited opened this issue Jul 24, 2023 · 4 comments · Fixed by openedx/openedx-learning#74
Closed

Comments

@pomegranited
Copy link

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 #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?

  • Uses oel_tagging.api.tag_object to replace the list of ObjectTags associated with the given <object_id> and taxonomy <pk>
  • Respects rules.can_change_object_tag for the existing object tags.
    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?
  • On success, returns the updated list of object tags, as per the GET method defined in [Tagging] Implement tagging REST API: Retrieve object tags #81
  • On error, no updates are made, and the error code and explanation is returned.
  • taxonomy parameter: (required) Taxonomy pk
    • Update only ObjectTags for the given taxonomy, if it exists.
  • tags PUT data parameter: (required) list of tag "references", where either "id" or "value" is required:
    • id: corresponds to a Tag.external_id or Tag.id (required for closed taxonomies)
    • value: string, free-text tag value

Automated testing must cover common paths in behavioral specification.

Behavioral specifications

Suppose the Open edX instance has these taxonomies:

  • system-defined language taxonomy: st1 (enabled) linked to 1 (valid) object tag for object_id="abc": "Spanish"
  • closed taxonomies created by taxonomy admins: t1 (enabled), t2 (disabled), each linked to 20 object tags for object_id="abc"
  • 2 free-text taxonomies created by taxonomy admins: o1 (enabled), o2 (disabled), each linked to 200 object tags for object_id="abc"

and the following users:

  • userA: non-staff
  • userS: global staff user

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

  • When userA or userS sends the following data to ^, they replace the "Spanish" language tag from st1 applied to object "abc" with "Portuguese"
    tags = [{"id": "pt"}]
  • When userA or userS sends the following data to ^, they remove any language tags from st1 applied to object "abc".
    tags = []
    PUT /api/oel_tagging/v1/object_tags/abc/?taxonomy=t2
  • When userA sends data to ^, they receive an error -- o2 and t2 are disabled taxonomies, so normal users cannot add object tags linked to them.
  • When userS sends the following data to ^, they replace the existing tags on "abc" with taxonomy=t2 with these two (example) tags:
    tags = [{"id": "Science"}, {"id": "Chemistry"}]
    PUT /api/oel_tagging/v1/object_tags/abc/?taxonomy=o1
  • When user A or userS sends the following (example) data to ^, they replace the existing free-text tags on "abc" linked to taxonomy=o1 with these three (example) tags:
    tags = [{"value": "Interesting stuff"}, {"value": "will surely"}, {"value": "go here"}]

Documentation updates & improvements criteria

  • API endpoint and its parameters should be well-documented
  • Note: this API should be marked as “unstable” for the MVP.

Relevant PRs/repositories

@rpenido
Copy link

rpenido commented Aug 15, 2023

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:

  • We are creating/updating ObjectTags in batch for an object_id and Taxonomy and not calling a single add/update ObjectTag.
  • Our current rule logic is more attached to the Taxonomy than to the actual ObjectTag itself (and I don't foresee any use case that can change that)
  • We are writing a custom update method to handle this logic (because we are not dealing with a single object here), so we must make the has_perm call ourselves anyway.

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
Copy link

@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
Copy link

rpenido commented Aug 16, 2023

Agreed @ChrisChV!

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

  • 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!
  • Added a taxonomy with allow_multiple=True in the tests

@ChrisChV
Copy link

ChrisChV commented Aug 17, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

4 participants