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

Add good document streaming endpoint #1818

Merged
merged 4 commits into from
Feb 12, 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
4 changes: 4 additions & 0 deletions api/conf/settings_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,7 @@
SUPPRESS_TEST_OUTPUT = True

AWS_ENDPOINT_URL = None

INSTALLED_APPS += [
"api.core.tests.apps.CoreTestsConfig",
]
17 changes: 17 additions & 0 deletions api/core/filters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
from rest_framework import filters

from django.core.exceptions import ImproperlyConfigured


class ParentFilter(filters.BaseFilterBackend):
def filter_queryset(self, request, queryset, view):
parent_filter_id_lookup_field = getattr(view, "parent_filter_id_lookup_field", None)
if not parent_filter_id_lookup_field:
raise ImproperlyConfigured(
f"Cannot use {self.__class__.__name__} on a view which does not have a parent_filter_id_lookup_field attribute"
)

lookup = {
parent_filter_id_lookup_field: view.kwargs["pk"],
}
return queryset.filter(**lookup)
6 changes: 6 additions & 0 deletions api/core/tests/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from django.apps import AppConfig


class CoreTestsConfig(AppConfig):
name = "api.core.tests"
label = "api_core_tests"
32 changes: 32 additions & 0 deletions api/core/tests/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Generated by Django 4.2.9 on 2024-02-09 14:48

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

initial = True

dependencies = []

operations = [
migrations.CreateModel(
name="ParentModel",
fields=[
("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")),
("name", models.CharField(max_length=255)),
],
),
migrations.CreateModel(
name="ChildModel",
fields=[
("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")),
("name", models.CharField(max_length=255)),
(
"parent",
models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="api_core_tests.parentmodel"),
),
],
),
]
Empty file.
10 changes: 10 additions & 0 deletions api/core/tests/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from django.db import models


class ParentModel(models.Model):
name = models.CharField(max_length=255)


class ChildModel(models.Model):
name = models.CharField(max_length=255)
parent = models.ForeignKey(ParentModel, on_delete=models.CASCADE)
12 changes: 12 additions & 0 deletions api/core/tests/serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from rest_framework import serializers

from api.core.tests.models import ChildModel


class ChildModelSerializer(serializers.ModelSerializer):
class Meta:
model = ChildModel
fields = (
"id",
"name",
)
62 changes: 62 additions & 0 deletions api/core/tests/test_filters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import uuid

from django.core.exceptions import ImproperlyConfigured
from django.test import (
override_settings,
SimpleTestCase,
TestCase,
)
from django.urls import reverse

from api.core.tests.models import (
ChildModel,
ParentModel,
)


@override_settings(
ROOT_URLCONF="api.core.tests.urls",
)
class TestMisconfiguredParentFilter(SimpleTestCase):
def test_misconfigured_parent_filter(self):
url = reverse(
"test-misconfigured-parent-filter",
kwargs={
"pk": str(uuid.uuid4()),
"child_pk": str(uuid.uuid4()),
},
)
with self.assertRaises(ImproperlyConfigured):
self.client.get(url)


@override_settings(
ROOT_URLCONF="api.core.tests.urls",
)
class TestParentFilter(TestCase):
def test_parent_filter(self):
parent = ParentModel.objects.create(name="parent")
child = ChildModel.objects.create(parent=parent, name="child")
url = reverse(
"test-parent-filter",
kwargs={
"pk": str(parent.pk),
"child_pk": str(child.pk),
},
)
response = self.client.get(url)
self.assertEqual(response.status_code, 200)

def test_parent_other_parent_filter(self):
parent = ParentModel.objects.create(name="parent")
child = ChildModel.objects.create(parent=parent, name="child")
other_parent = ParentModel.objects.create(name="other_parent")
url = reverse(
"test-parent-filter",
kwargs={
"pk": str(other_parent.pk),
"child_pk": str(child.pk),
},
)
response = self.client.get(url)
self.assertEqual(response.status_code, 404)
19 changes: 19 additions & 0 deletions api/core/tests/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from django.urls import path

from .views import (
MisconfiguredParentFilterView,
ParentFilterView,
)

urlpatterns = [
path(
"misconfigured-parent/<str:pk>/child/<str:child_pk>/",
MisconfiguredParentFilterView.as_view(),
name="test-misconfigured-parent-filter",
),
path(
"parent/<str:pk>/child/<str:child_pk>/",
ParentFilterView.as_view(),
name="test-parent-filter",
),
]
17 changes: 17 additions & 0 deletions api/core/tests/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
from rest_framework.generics import RetrieveAPIView

from api.core.filters import ParentFilter
from api.core.tests.models import ChildModel
from api.core.tests.serializers import ChildModelSerializer


class MisconfiguredParentFilterView(RetrieveAPIView):
filter_backends = (ParentFilter,)
queryset = ChildModel.objects.all()


class ParentFilterView(RetrieveAPIView):
filter_backends = (ParentFilter,)
parent_filter_id_lookup_field = "parent_id"
queryset = ChildModel.objects.all()
serializer_class = ChildModelSerializer
14 changes: 14 additions & 0 deletions api/goods/permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
from rest_framework import permissions

from api.goods.enums import GoodStatus
from api.organisations.libraries.get_organisation import get_request_user_organisation_id


class IsDocumentInOrganisation(permissions.BasePermission):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to unit test these permissions

def has_object_permission(self, request, view, obj):
return obj.good.organisation_id == get_request_user_organisation_id(request)


class IsGoodDraft(permissions.BasePermission):
def has_object_permission(self, request, view, obj):
return obj.good.status == GoodStatus.DRAFT
113 changes: 113 additions & 0 deletions api/goods/tests/test_goods_documents.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
from django.urls import reverse
from rest_framework import status

from moto import mock_aws
from parameterized import parameterized

from django.http import FileResponse

from api.applications.models import GoodOnApplication
from test_helpers.clients import DataTestClient

from api.applications.tests.factories import StandardApplicationFactory
from api.goods.enums import GoodStatus
from api.goods.tests.factories import GoodFactory
from api.organisations.tests.factories import OrganisationFactory

Expand Down Expand Up @@ -121,3 +127,110 @@ def test_edit_product_document_description(self):
response = self.client.get(url, **self.exporter_headers)
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.json()["document"]["description"], "Updated document description")


@mock_aws
class GoodDocumentStreamTests(DataTestClient):
def setUp(self):
super().setUp()
self.good = GoodFactory(
organisation=self.organisation,
status=GoodStatus.DRAFT,
)
self.create_default_bucket()
self.put_object_in_default_bucket("thisisakey", b"test")

def test_get_good_document_stream(self):
good_document = self.create_good_document(
good=self.good,
user=self.exporter_user,
organisation=self.organisation,
s3_key="thisisakey",
name="doc1.pdf",
)

url = reverse(
"goods:document_stream",
kwargs={
"pk": str(self.good.pk),
"doc_pk": str(good_document.pk),
},
)
response = self.client.get(url, **self.exporter_headers)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertIsInstance(response, FileResponse)
self.assertEqual(b"".join(response.streaming_content), b"test")

def test_get_good_document_stream_invalid_good_pk(self):
another_good = GoodFactory(organisation=self.organisation)
good_document = self.create_good_document(
good=self.good,
user=self.exporter_user,
organisation=self.organisation,
s3_key="thisisakey",
name="doc1.pdf",
)

url = reverse(
"goods:document_stream",
kwargs={
"pk": str(another_good.pk),
"doc_pk": str(good_document.pk),
},
)
response = self.client.get(url, **self.exporter_headers)

self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)

def test_get_good_document_stream_other_organisation(self):
other_organisation = self.create_organisation_with_exporter_user()[0]
self.good.organisation = other_organisation
self.good.save()
good_document = self.create_good_document(
good=self.good,
user=self.exporter_user,
organisation=other_organisation,
s3_key="thisisakey",
name="doc1.pdf",
)

url = reverse(
"goods:document_stream",
kwargs={
"pk": str(self.good.pk),
"doc_pk": str(good_document.pk),
},
)
response = self.client.get(url, **self.exporter_headers)

self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

@parameterized.expand(
[
GoodStatus.SUBMITTED,
GoodStatus.QUERY,
GoodStatus.VERIFIED,
Comment on lines +211 to +213
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see these are the "not draft" statuses, and that they test the logic above obj.good.status == GoodStatus.DRAFT, but I can't see here how a good gets draft status to begin with. I'm guessing it is because there is default=GoodStatus.DRAFT in the good model? and therefore it is not worth having test cases to test the document stream works for the draft status (as it is the default)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test that checks that the stream works is the first one, it doesn't explicitly set the status as it's already being set by the default value, as you said, so there is a test already. I can explicitly set this to make it clearer.

],
)
def test_get_good_document_stream_good_not_draft(self, good_status):
self.good.status = good_status
self.good.save()
good_document = self.create_good_document(
good=self.good,
user=self.exporter_user,
organisation=self.organisation,
s3_key="thisisakey",
name="doc1.pdf",
)

url = reverse(
"goods:document_stream",
kwargs={
"pk": str(self.good.pk),
"doc_pk": str(good_document.pk),
},
)
response = self.client.get(url, **self.exporter_headers)

self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
5 changes: 5 additions & 0 deletions api/goods/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@
views.GoodDocumentDetail.as_view(),
name="document",
),
path(
"<uuid:pk>/documents/<uuid:doc_pk>/stream/",
views.GoodDocumentStream.as_view(),
name="document_stream",
),
path(
"document_internal_good_on_application/<str:goods_on_application_pk>/",
views.DocumentGoodOnApplicationInternalView.as_view(),
Expand Down
21 changes: 21 additions & 0 deletions api/goods/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
from api.core.authentication import ExporterAuthentication, SharedAuthentication, GovAuthentication
from api.core.exceptions import BadRequestError
from api.core.helpers import str_to_bool
from api.core.filters import ParentFilter
from api.core.views import DocumentStreamAPIView
from api.documents.libraries.delete_documents_on_bad_request import delete_documents_on_bad_request
from api.documents.models import Document
from api.goods.enums import GoodStatus, GoodPvGraded, ItemCategory
Expand All @@ -31,6 +33,10 @@
from api.goods.libraries.get_goods import get_good, get_good_document
from api.goods.libraries.save_good import create_or_update_good
from api.goods.models import Good, GoodDocument
from api.goods.permissions import (
IsDocumentInOrganisation,
IsGoodDraft,
)
from api.goods.serializers import (
GoodAttachingSerializer,
GoodCreateSerializer,
Expand Down Expand Up @@ -539,6 +545,21 @@ def delete(self, request, pk, doc_pk):
return JsonResponse({"document": "deleted success"})


class GoodDocumentStream(DocumentStreamAPIView):
authentication_classes = (ExporterAuthentication,)
filter_backends = (ParentFilter,)
parent_filter_id_lookup_field = "good_id"
lookup_url_kwarg = "doc_pk"
queryset = GoodDocument.objects.all()
permission_classes = (
IsDocumentInOrganisation,
IsGoodDraft,
)

def get_document(self, instance):
return instance


class DocumentGoodOnApplicationInternalView(APIView):
authentication_classes = (GovAuthentication,)
serializer_class = GoodOnApplicationInternalDocumentCreateSerializer
Expand Down
6 changes: 0 additions & 6 deletions api/organisations/filters.py

This file was deleted.

Loading
Loading