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

feat(tags/view): Add api to get tag by uuid #3229

Merged
merged 3 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions api/projects/tags/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,13 @@ def has_permission(self, request, view):
project_pk = view.kwargs.get("project_pk")
if not project_pk:
return False

project = Project.objects.get(pk=project_pk)

if request.user.is_project_admin(project):
return True

if view.action == "list" and request.user.has_project_permission(
VIEW_PROJECT, project
):
return True
if view.action in ["list", "get_by_uuid"]:
return request.user.has_project_permission(VIEW_PROJECT, project)

# move on to object specific permissions
return view.detail
Expand Down
2 changes: 1 addition & 1 deletion api/projects/tags/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
class TagSerializer(serializers.ModelSerializer):
class Meta:
model = Tag
fields = ("id", "label", "color", "description", "project")
fields = ("id", "label", "color", "description", "project", "uuid")
read_only_fields = ("project",)
18 changes: 16 additions & 2 deletions api/projects/tags/views.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
from rest_framework import viewsets
from rest_framework.decorators import action
from rest_framework.generics import get_object_or_404
from rest_framework.permissions import IsAuthenticated
from rest_framework.request import Request
from rest_framework.response import Response

from projects.permissions import VIEW_PROJECT

Expand All @@ -26,9 +29,20 @@ def get_queryset(self):
return queryset

def perform_create(self, serializer):
project_id = self.kwargs["project_pk"]
project_id = int(self.kwargs["project_pk"])
serializer.save(project_id=project_id)

def perform_update(self, serializer):
project_id = self.kwargs["project_pk"]
project_id = int(self.kwargs["project_pk"])
Copy link
Member Author

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)

Copy link
Contributor

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?

Copy link
Member Author

@gagantrivedi gagantrivedi Jan 4, 2024

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?

Copy link
Contributor

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 the is_valid check should remove the requirement for the int cast, no?

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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 method

Copy link
Member Author

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

serializer.save(project_id=project_id)

@action(
detail=False,
url_path=r"get-by-uuid/(?P<uuid>[0-9a-f-]+)",
methods=["get"],
)
def get_by_uuid(self, request: Request, project_pk: int, uuid: str):
qs = self.get_queryset()
tag = get_object_or_404(qs, uuid=uuid)
serializer = self.get_serializer(tag)
return Response(serializer.data)
71 changes: 71 additions & 0 deletions api/tests/unit/projects/tags/test_views.py
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)