feat(tags/view): Add api to get tag by uuid #3229
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3229 +/- ##
===========================================
- Coverage 95.95% 76.25% -19.71%
===========================================
Files 1062 1035 -27
Lines 32299 31345 -954
===========================================
- Hits 30993 23901 -7092
- Misses 1306 7444 +6138 ☔ View full report in Codecov by Sentry. |
|
Uffizzi Preview |
gagantrivedi
requested review from
a team and
zachaysan
and removed request for
a team
| serializer.save(project_id=project_id) | ||
|
|
||
| def perform_update(self, serializer): | ||
| project_id = self.kwargs["project_pk"] | ||
| project_id = int(self.kwargs["project_pk"]) |
There was a problem hiding this comment.
This is done to make sure the response have correct data type for project(i.e: int instead of string)
There was a problem hiding this comment.
Why not use the serializer with is_valid?
There was a problem hiding this comment.
Sorry, I don't understand how that will help change the data type of the response?
There was a problem hiding this comment.
Getting the data type with validated_data after running the is_valid check should remove the requirement for the int cast, no?
There was a problem hiding this comment.
No, that does not work, I don't think any data passed to save() is part of validated data 🤔 unless I am doing something wrong?
There was a problem hiding this comment.
Maybe we're talking past each other, and this is a minor thing so I'll approve the PR just to unblock you, but typically when I use serializers I'm able to call is_valid on the serializer then pass validated_data into whatever I need. This works for all sorts of situations including stuff like query params serializers. If the kwargs can be passed into the serializer and the serializer can validate the data then why do it manually with type casts?
There was a problem hiding this comment.
I think the part that you are talking about is handled by drf(update method in this case) and I honestly don't see much value in overriding the update method
There was a problem hiding this comment.
I will merge this for now, but let me know if disagree
| @pytest.mark.parametrize( | ||
| "client", | ||
| [(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], | ||
| ) |
There was a problem hiding this comment.
We're prioritizing using lower levels of access over the admin_client when it comes to checking permissions. So since this endpoint follows TagPermissions you'll want to use staff_client with the with_project_permissions([VIEW_PROJECT]) to set access level to match.
Also, it would be good to check the HTTP 403 case to make sure the endpoint isn't open to a user from a different project.
There was a problem hiding this comment.
That's a good point… Although I wonder if that should be done by the unit test that test the permission class itself instead of the test that test the view?
There was a problem hiding this comment.
Since permissions can be misconfigured when adding to a viewset (which we've found wide open ones because of a decorator misordering) I think testing through the view overall is a better strategy.
There was a problem hiding this comment.
Yeah, that makes sense
| serializer.save(project_id=project_id) | ||
|
|
||
| def perform_update(self, serializer): | ||
| project_id = self.kwargs["project_pk"] | ||
| project_id = int(self.kwargs["project_pk"]) |
There was a problem hiding this comment.
Maybe we're talking past each other, and this is a minor thing so I'll approve the PR just to unblock you, but typically when I use serializers I'm able to call is_valid on the serializer then pass validated_data into whatever I need. This works for all sorts of situations including stuff like query params serializers. If the kwargs can be passed into the serializer and the serializer can validate the data then why do it manually with type casts?
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
Add API to fetch tag by uuid - (will be) Used by our terraform provider to import tag
How did you test this code?
Adds unit test