-
Notifications
You must be signed in to change notification settings - Fork 429
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
feat(tags/view): Add api to get tag by uuid #3229
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
from typing import Callable | ||
|
||
import pytest | ||
from django.urls import reverse | ||
from pytest_lazyfixture import lazy_fixture | ||
from rest_framework import status | ||
from rest_framework.test import APIClient | ||
|
||
from projects.models import Project | ||
from projects.permissions import VIEW_PROJECT | ||
from projects.tags.models import Tag | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"client", | ||
[(lazy_fixture("admin_master_api_key_client")), (lazy_fixture("admin_client"))], | ||
) | ||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're prioritizing using lower levels of access over the Also, it would be good to check the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense |
||
def test_get_tag_by_uuid(client: APIClient, project: Project, tag: Tag): | ||
url = reverse("api-v1:projects:tags-get-by-uuid", args=[project.id, str(tag.uuid)]) | ||
|
||
# When | ||
response = client.get(url) | ||
|
||
# Then | ||
assert response.status_code == status.HTTP_200_OK | ||
assert response.json()["uuid"] == str(tag.uuid) | ||
|
||
|
||
def test_get_tag_by_uuid__returns_403_for_user_without_permission( | ||
staff_client: APIClient, | ||
organisation_one_project_two: Project, | ||
project: Project, | ||
tag: Tag, | ||
with_project_permissions: Callable[[list[str], int], None], | ||
): | ||
# Given | ||
# user with view permission for a different project | ||
with_project_permissions([VIEW_PROJECT], organisation_one_project_two.id) | ||
|
||
url = reverse( | ||
"api-v1:projects:tags-get-by-uuid", | ||
args=[project.id, str(tag.uuid)], | ||
) | ||
|
||
# When | ||
response = staff_client.get(url) | ||
|
||
# Then | ||
assert response.status_code == status.HTTP_403_FORBIDDEN | ||
|
||
|
||
def test_get_tag_by__uuid_returns_200_for_user_with_view_project_permission( | ||
staff_client: APIClient, | ||
project: Project, | ||
tag: Tag, | ||
with_project_permissions: Callable[[list[str], int], None], | ||
): | ||
# Given | ||
with_project_permissions([VIEW_PROJECT]) | ||
|
||
url = reverse( | ||
"api-v1:projects:tags-get-by-uuid", | ||
args=[project.id, str(tag.uuid)], | ||
) | ||
|
||
# When | ||
response = staff_client.get(url) | ||
|
||
# Then | ||
assert response.status_code == status.HTTP_200_OK | ||
assert response.json()["uuid"] == str(tag.uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the serializer with
is_valid
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand how that will help change the data type of the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the data type with
validated_data
after running theis_valid
check should remove the requirement for theint
cast, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 passvalidated_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this for now, but let me know if disagree