feat: implement tagging rest api tag object [FC-0030]#74
Conversation
|
Thanks for the pull request, @rpenido! 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:
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. |
14740c6 to
1ff6adc
Compare
ChrisChV
left a comment
There was a problem hiding this comment.
Thanks @rpenido!
I've requested small changes, but once that's done it's good to go into @bradenmacdonald review.
- I ran the tests
- I read through the code
- Includes documentation for the added REST API
| assert response.status_code == expected_status | ||
|
|
||
| @ddt.data( | ||
| (None, "language_taxonomy", ["Portuguese"], status.HTTP_403_FORBIDDEN), |
There was a problem hiding this comment.
@rpenido Could you add a comment describing each case? It's easy to get lost here, comments can help :)
There was a problem hiding this comment.
Hi @ChrisChV.
Could you please give me your opinion on this (5bf5de8) ?
The tests are exhaustive: I test the same condition for all users (None, "user", and "staff"), all taxonomies, and all conditions (single, empty, multiple, invalid..).
After writing the comments, it's clear that we have redundant tests.
Should I clear some of them?
There was a problem hiding this comment.
I think you can delete the test that use language_taxonomy, I think are the same of enabled_taxonomy
There was a problem hiding this comment.
@rpenido I was thinking, and if we separate and group the tests in different functions? For example:
test_add_tag: with the three user types and taxonomy typestest_add_invalid_tags: with the three user types and taxonomy typestest_clear_tags: with the three user types and taxonomy types- Etc.
There was a problem hiding this comment.
I think you can delete the test that use
language_taxonomy, I think are the same ofenabled_taxonomy
I'm using the language taxonomy to test the required=true case
@rpenido I was thinking, and if we separate and group the tests in different functions? For example:
* `test_add_tag`: with the three user types and taxonomy types * `test_add_invalid_tags`: with the three user types and taxonomy types * `test_clear_tags`: with the three user types and taxonomy types * Etc.
I liked it. Let me know what you think.
| """ | ||
| Taxonomy admins can create or modify object tags on enabled taxonomies. | ||
| Everyone can potentially create/edit object tags (taxonomy=None). The object permission must be checked | ||
| to determine if the user can create/edit a object_tag for a specific taxonomy. |
There was a problem hiding this comment.
I think we need to emphasize this more, if I'm understanding it correctly. Something like:
Everyone can potentially create/edit object tags, IF they have permission
to use the taxonomy that the tag comes from, and IF they have permission
to edit the object in question.
❗This function only checks if they have permission to use the taxonomy! You
need to combine this with a separate check for their permission to edit the
object, which depends on the type of object.
The permissions for the taxonomy usage are simple: Everyone can create
or modify object tags using an enabled taxonomy. Only taxonomy admins
can create or modify object tags using a disabled taxonomies.
But I'm also wondering, why are admins allowed to create object tags using disabled taxonomies? Do we actually need that?
There was a problem hiding this comment.
It is a bit different. Because we are using the DjangoObjectPermission in the view, this function is called two times when we make a rest call:
- First, for checking if the user has the METHOD permission (PUT, in this specific case). In this call, we have taxonomy=None.
- After that, we call this function passing a taxonomy to check if the user has permission for this taxonomy
Everyone passes on 1. This is what I wanted to say with Everyone can potentially create/edit object tags (taxonomy=None).. I was trying to justify the if not taxonomy: return True
This is an implementation detail. Do you think we should let this out of the docstring?
This text covers the actual permission as of today:
Everyone can create or modify object tags using an enabled taxonomy. Only taxonomy admins
can create or modify object tags using a disabled taxonomy.
But I'm also wondering, why are admins allowed to create object tags using disabled taxonomies? Do we actually need that?
I'm not sure. It makes sense to me to let an admin clear the tags from a disabled taxonomy, for example.
There was a problem hiding this comment.
@rpenido That makes sense, thanks. Can you please explain that in the code, either in the docstring or in comments? It wasn't super clear to me what's happening until you replied.
This text covers the actual permission as of today:
But also we need to remind developers to check the object permissions too. We can't let just anyone apply object tags to a course, if they don't also have permission to edit that course in Studio!
There was a problem hiding this comment.
Actually, how will that work with this API? Does it have a way of checking if the user has edit permission for the object? (If not, we probably need to make the "edit object tags" API python-only, so that more specific REST APIs like in edx-platform can check user edit permissions before updating an object). Or we need to make this a base class that can be inherited and customized e.g. in edx-platform but not used directly, since it doesn't know how to check permissions. Or we need to add a pluggable permissions check.
There was a problem hiding this comment.
@rpenido That makes sense, thanks. Can you please explain that in the code, either in the docstring or in comments? It wasn't super clear to me what's happening until you replied.
Sure @bradenmacdonald! I'll work on this.
Or we need to make this a base class that can be inherited and customized e.g. in edx-platform but not used directly, since it doesn't know how to check permissions. Or we need to add a pluggable permissions check.
I think this is the strategy: inherit this class in edx-platform to enhance the permission.
About the object permission, look at the discussion in the issue here. Maybe I oversimplified reducing the permission check to only taxonomy.
In the openedx-learning side, as a stand-alone app, we basically apply tags to "strings" without any object knowledge.
I agree that we can have a callback to check some type of "Object" permission, but I'm not sure if we need this for the MVP.
Maybe aggregating more permissions in edx-platform (not on Object level, but Taxonomy level) is enough?
CC @ChrisChV
|
Let me know when you want a full review of this. |
|
I think it's ready for a review @bradenmacdonald! |
|
@rpenido I went to review but I'm getting hung up on the permissions issue. We can't allow users to tag any objects they want; they can only tag objects that they have permission to edit. So I think this API needs to be an abstract class that is not actually accessible, and then in edx-platform we can inherit it and implement a "get/set content object tags" API that uses this same code but adds appropriate permissions checks like "is this user allowed to edit this XBlock". What do you think? |
I agree with you. But with YAGNI in mind, I think that the MVP permission model is broad enough to allow this (if a user can create a course, it can tag objects). But I wouldn't want to make assumptions without checking. Can you help us here with the requirements @ChrisChV? |
|
@rpenido
If the written requirements anywhere are different than this, please point it out and I'll get them updated. |
201871f to
5851ee2
Compare
|
Hi @bradenmacdonald! I was thinking about it and would like to make sure we are on the right path. I still can remove the update method from the View and create a Mixin to be used in Also, as soon we change the object_id permission here to False, we will not be able to test the endpoint here. Let me know what you think! The initial requirements are here: |
I was thinking the whole view should just be an independent class, because every CRUD action depends on the user's permissions. Even just reading the tags should only be allowed if the user can "read" the object that is tagged too. So what if we just make the view so that it is designed to be used as an abstract view. It should work, but it won't be in Then, in edx-platform, we can subclass the view, and give it a new URL endpoint, and implement the permissions checks for "can user read/write this content object". This view also will only work with ContentObjectTags. Then if there are other use cases for tagging in the future, each use case can subclass the base view and implement its own permission checks on top of it. Another way is what you are saying, where the base implementation in openedx-learning just checks the |
| exc.exception | ||
| assert "Invalid object tag for taxonomy (1): Eukaryota Xenomorph" in str(exc.exception) | ||
|
|
||
| def test_tag_object_string(self): |
There was a problem hiding this comment.
If there are no type declarations on the function definition, please add -> None so it becomes def test(self) -> None: . Specifying any type hints, including just -> None will enable type checking for the code in this function. But if there are no type annotations at all in the function declaration then the whole function will be ignored and can contain type errors.
There was a problem hiding this comment.
Thank you for pointing out! I think we have some type errors here. I will check it out.
It is more like this rule is for 'oel_tagging' (
Yes, if "one rule" per app_label is enough for us (of course we could do any type check inside our rule function to handle anything we want). I think we should stick with the rules only because the other rest apis are using it. And I if the requirements where made that way because |
|
One rule per app_label is fine yep. But how does "rules" know to use a different app label for content tags like tags on an XBlock? Won't it still think the app is |
The But now I understand what you are saying. We can't use the taxonomy_class model app (i.e. LanguageTaxonomy is from oel_tagging). But I'm intrigued by how it worked in the Taxonomy API (the rules work out of the box without issues); I will take a look at it tomorrow. Thank you for your insight and input @bradenmacdonald ! |
|
I got it. We explicitly override the permission from Do you fell that we can go on this route @bradenmacdonald? |
|
I'm not sure. But if you try it out, and it allows edx-platform to specify the permissions for content objects, other future apps to specify the permissions for tagging their own models, and openedx-tagging to test its API code, then it sounds good. I see there is both https://github.com/openedx/openedx-learning/blob/f73adefbcf9748dafe8ebfb7e8e6590df6780bb2/openedx_tagging/core/tagging/rules.py#L71 and https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/rules.py#L113 - how do we know that the platform one always overrides the other? And what is the point of the override when the implementation of |
|
I checked now (using breakpoints), and I'm sure that NOW the Is it safe to say that the platform will always override because we do
The code started differently (we had an Organization permission check) and converged when we simplified it. I didn't notice that in the Taxonomy Org PR. My bad! |
186e6c6 to
c9d4ccd
Compare
I think so, yes. But there should be some unit tests in edx-platform to check that the permissions are correct just in case. |
Yes! We have the rule tests here: The view tests also cover that! So, are we good to go that way? PS: It is better to handle the duplicate rule in the next |
|
You can add the cleanup to FAL-3477. I'm not sure those tests are comprehensive. Because there was a missed requirement here: the requirements/specs for this ticket focused too much on the taxonomies and org-level permissions; those tests are right. But we also need to check if the user has permissions on the object, and I don't see any tests doing that. For example https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/tests/test_rules.py#L380-L394 is not checking the object permissions at all. We'll have to address that missed requirement in the future. Obviously if I can't edit an XBlock in Studio, I shouldn't be able to edit that XBlock's tags regardless of what other permissions I have at the org and taxonomy level. |
Yes. The rules don't cover the object permission. This could be done when we create the tag object rest api (I didn't find this task in the project) |
|
Are we good for a final review here @bradenmacdonald? I think we addressed all the questions here. The only loose end is the object permission in Am I missing something? Thank you! |
bradenmacdonald
left a comment
There was a problem hiding this comment.
Please make these minor changes but it looks good! I can merge once you let me know.
| def update(self, request, object_id, partial=False): | ||
| """ | ||
| Update ObjectTags that belong to a given object_id and | ||
| return the list of these ObjecTags paginated. |
There was a problem hiding this comment.
The response isn't really paginated is it? I don't think we need pagination.
Also, do we have a limit on how many tags can be applied to one object? If not, I'd like to add a limit in. Something like 100 so that we can avoid any need for pagination in the object tag APIs. That can come in a separate task though, no need to do in this PR. Just let me know what the current status is.
There was a problem hiding this comment.
It calls the retrieve method, which is paginated.
We don't have a limit on tags per object.
I think the idea here is to return the default GET format to make the refresh on the frontend easier and save 1 extra call.
Personally, I prefer to return no data in the PUT/POST requests and let the frontend handle the logic to make another GET call to get refreshed data. We can fine-tune this when we make the frontend.
There was a problem hiding this comment.
@rpenido We will need to have a limit on tags per object. I created openedx/modular-learning#99 but it's not ready yet as I'm waiting for the product folks to confirm what the actual limit is. When we implement that ticket we can probably remove pagination from the GET API.
I personally usually like to make the POST/PUT API return the same thing as the GET API. It saves the frontend from making an extra request.
| """ | ||
| Nobody can create or modify object tags without checking the permission for the tagged object. | ||
|
|
||
| This rule should be defined in other apps for proper permission checking. |
There was a problem hiding this comment.
We have some options here:
- return True for this app (I think it is ok to tag everything here: we are tagging
str) - return False and change the tests accordingly (everything will return 403 and will barely cover the implemented code in the rest_api)
- return False and change the tests accordingly (everything will return 403) AND duplicate the tests patching this permission (forcing True) to actually test the workflow.
In the actual context, 1 is the faster/easier path. For 2 and 3, I must fix the test_rules.py and test_views.py. Let me know what you think!
There was a problem hiding this comment.
In edx-platform, we'll be adding a rule that defines how "content objects" are tagged.
So in the tests, can't we add a new rule just for tests that defines how "Foo objects" are tagged? And then confirms that the rule is enforced by trying to tag some Foo objects?
In other words, the tests should work in a similar way to how the platform code is going to work. I would prefer no patching and I want to see the actual functionality being tested so I don't like 2 or 3.
Please review openedx/modular-learning#98 , I created this ticket for next sprint to clearly define the missed requirements here around permissions. If you think these tests will take a while, we can just return True for now and make sure the permissions are properly tested when we implement that ticket but I'd prefer to make sure that the base functionality in openedx-learning is implemented correctly now.
There was a problem hiding this comment.
So in the tests, can't we add a new rule just for tests that defines how "Foo objects" are tagged? And then confirms that the rule is enforced by trying to tag some Foo objects?
Awesome idea @bradenmacdonald!
There was a problem hiding this comment.
@bradenmacdonald I would like to add some tests to check if we can't tag anything but "abc". Just give me a minute before merge!
There was a problem hiding this comment.
Phew! It had a copy/paste error. Tested and fixed 85b320a
I think we are good to go now @bradenmacdonald!
There was a problem hiding this comment.
Looks like a build error. No rush, just let me know when it's green and ready :)
There was a problem hiding this comment.
I think we are good to go now @bradenmacdonald!
Too soon.
openedx_tagging/core/tagging/rules.py:92: error: Argument 2 to "has_perm" of "PermissionsMixin" has incompatible type "str"; expected "Optional[Union[Model, AnonymousUser]]" [arg-type]
openedx_tagging/core/tagging/rules.py:92: error: Argument 2 to "has_perm" of "AnonymousUser" has incompatible type "str"; expected "Optional[Union[Model, AnonymousUser]]" [arg-type]
Where mypi gets these types from?
There was a problem hiding this comment.
@rpenido According to the django docs for has_perm it seems like the second object is usually an object, but here we're passing in a string (the object ID). So I think the type check is mostly correct. BUT here we don't have objects, we have object IDs, and the API doesn't actually care how you define the permissions checks. I think defining them to use the object_id is quite reasonable. So just add a comment explaining that and put #type: ignore[arg-type] at the end of the line to suppress the warning.
There was a problem hiding this comment.
Thank you @bradenmacdonald! We are good now!²
|
@rpenido Sorry, it says it needs to be rebased before it merges. |
416d34f to
2c0b997
Compare
|
Rebased and squashed @bradenmacdonald ! |
|
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR implements the tagging REST API for updating object tags.
Supporting Information
Testing instructions
Private-ref: FAL-3474