From ba7ee42690d21273acfe2a073b06b7ab41f97756 Mon Sep 17 00:00:00 2001 From: Thomas Steen Rasmussen Date: Mon, 20 May 2024 15:08:55 +0200 Subject: [PATCH] fix album list django view filtering by files, add first unit tests for album django views, move some stuff to utils/tests.py to keep tests DRY --- src/albums/filters.py | 29 ++++++++++++++++++++--- src/albums/tables.py | 1 - src/albums/tests.py | 54 ++++++++++++++++++++++++++++++++----------- src/albums/views.py | 2 +- src/files/filters.py | 2 +- src/files/models.py | 17 ++++++++++++++ src/files/tests.py | 4 ++-- src/utils/tests.py | 22 ++++++++++++++++++ 8 files changed, 110 insertions(+), 21 deletions(-) diff --git a/src/albums/filters.py b/src/albums/filters.py index d682fcd..983d312 100644 --- a/src/albums/filters.py +++ b/src/albums/filters.py @@ -3,6 +3,10 @@ from typing import ClassVar import django_filters +from django.db.models import QuerySet +from django.http import HttpRequest +from django.utils import timezone +from files.models import BaseFile from ninja import Field from utils.filters import ListFilters @@ -10,20 +14,39 @@ class AlbumFilters(ListFilters): - """The filters used for the album_list endpoint.""" + """The filters used for the album_list django-ninja API endpoint.""" files: list[uuid.UUID] = Field(None, alias="files") +def get_permitted_files(request: HttpRequest) -> QuerySet[BaseFile]: + """Called by AlbumFilter to get files for the albumlist filter multiselect form field.""" + return BaseFile.permitted_files.get_queryset(user=request.user).all() + + class AlbumFilter(django_filters.FilterSet): - """The Album filter.""" + """The Album filter used by django-filters.""" + + # when filtering by files only show files the user has permission for in the form + files = django_filters.filters.ModelMultipleChoiceFilter( + field_name="files", + queryset=get_permitted_files, + method="filter_files", + ) + + def filter_files(self, queryset: QuerySet[Album], name: str, value: str) -> QuerySet[Album]: + """When filtering by files only consider currently active memberships.""" + # we want AND so loop over files and filter for each, + # finally returning only albums containing all the files in value + for f in value: + queryset = queryset.filter(memberships__basefile__in=[f], memberships__period__contains=timezone.now()) + return queryset class Meta: """Set model and fields.""" model = Album fields: ClassVar[dict[str, list[str]]] = { - "files": ["exact"], "title": ["exact", "icontains"], "description": ["icontains"], } diff --git a/src/albums/tables.py b/src/albums/tables.py index 5d140cd..3537564 100644 --- a/src/albums/tables.py +++ b/src/albums/tables.py @@ -19,7 +19,6 @@ class Meta: "title", "description", "owner", - "files", "active_memberships", "historic_memberships", "future_memberships", diff --git a/src/albums/tests.py b/src/albums/tests.py index 8ed6076..b2f3c6b 100644 --- a/src/albums/tests.py +++ b/src/albums/tests.py @@ -1,4 +1,5 @@ """Tests for the Album API.""" +from bs4 import BeautifulSoup from django.urls import reverse from oauth2_provider.models import get_access_token_model from oauth2_provider.models import get_application_model @@ -20,18 +21,7 @@ def test_album_create( files: list[str] | None = None, ) -> None: """Test creating an album.""" - response = self.client.post( - reverse("api-v1-json:album_create"), - { - "title": title, - "description": description, - "files": files if files else [], - }, - headers={"authorization": self.curator6.auth}, - content_type="application/json", - ) - assert response.status_code == 201 - self.album_uuid = response.json()["bma_response"]["uuid"] + self.album_uuid = self.album_create(title=title, description=description, files=files) def test_album_create_with_files( self, @@ -42,7 +32,7 @@ def test_album_create_with_files( self.files = [] for _ in range(10): self.files.append(self.file_upload()) - self.test_album_create(title=title, description=description, files=self.files) + self.album_uuid = self.album_create(title=title, description=description, files=self.files) def test_album_update(self) -> None: """First replace then update.""" @@ -201,3 +191,41 @@ def test_album_list(self) -> None: assert response.status_code == 200 assert len(response.json()["bma_response"]) == 5 assert response.json()["bma_response"][0]["title"] == "album5" + + +class TestAlbumViews(ApiTestBase): + """Unit tests for regular django Album views.""" + + def create_albums(self) -> None: + """Create som albums for testing.""" + # upload some files as creator2 + self.files = [] + for _ in range(10): + self.files.append(self.file_upload()) + # add them to an album created by creator6 + self.album_uuid = self.album_create(title="creator2 files", files=self.files) + # upload some files as creator3 + for _ in range(10): + self.files.append(self.file_upload(uploader="creator3")) + self.album_create(title="creator3 files", files=self.files[10:], creator="curator7") + + def test_album_list_view(self) -> None: + """Test the basics of the album list view.""" + url = reverse("albums:album_list") + self.create_albums() + self.client.login(username="creator2", password="secret") + + # test listing both albums, no filters + response = self.client.get(url) + content = response.content.decode() + soup = BeautifulSoup(content, "html.parser") + rows = soup.select("div.table-container > table > tbody > tr") + self.assertEqual(len(rows), 2, "album list does not return 2 albums") + + # test filtering by files to show albums containing a single file + url += f"?files={self.files[0]}" + response = self.client.get(url) + content = response.content.decode() + soup = BeautifulSoup(content, "html.parser") + rows = soup.select("div.table-container > table > tbody > tr") + self.assertEqual(len(rows), 1, "filtering by files does not return 1 album") diff --git a/src/albums/views.py b/src/albums/views.py index b5f94b3..90c6a1f 100644 --- a/src/albums/views.py +++ b/src/albums/views.py @@ -34,7 +34,7 @@ class AlbumListView(SingleTableMixin, FilterView): filterset_class = AlbumFilter context_object_name = "albums" - def get_table_data(self) -> QuerySet[Album]: + def get_queryset(self) -> QuerySet[Album]: """Add membership counts.""" qs = super().get_queryset() active_memberships = Count("memberships", filter=Q(memberships__period__contains=timezone.now())) diff --git a/src/files/filters.py b/src/files/filters.py index ab60a2f..e04f178 100644 --- a/src/files/filters.py +++ b/src/files/filters.py @@ -34,7 +34,7 @@ class FileFilter(django_filters.FilterSet): not_albums = django_filters.filters.UUIDFilter(field_name="albums", method="filter_albums", exclude=True) def filter_albums(self, queryset: QuerySet[BaseFile], name: str, value: str) -> QuerySet[BaseFile]: - """When filtering for albums only consider currently active memberships.""" + """When filtering by albums only consider currently active memberships.""" return queryset.filter(memberships__album__in=[value], memberships__period__contains=timezone.now()) class Meta: diff --git a/src/files/models.py b/src/files/models.py index 78b6bc5..5969dbe 100644 --- a/src/files/models.py +++ b/src/files/models.py @@ -11,6 +11,7 @@ from django.http import HttpRequest from django.urls import reverse from guardian.shortcuts import assign_perm +from guardian.shortcuts import get_objects_for_user from guardian.shortcuts import remove_perm from polymorphic.managers import PolymorphicQuerySet from polymorphic.models import PolymorphicManager @@ -93,6 +94,20 @@ def unpublish(self) -> int: return updated +class PermittedFilesManager(PolymorphicManager): + """A custom manager which only returns the files the user has access to.""" + + def get_queryset(self, user: User) -> models.QuerySet["BaseFile"]: # type: ignore[valid-type] + """Return PUBLISHED files and files where the user has view_basefile perms.""" + files = super().get_queryset().filter(status="PUBLISHED") | get_objects_for_user( + user=user, + perms="files.view_basefile", + klass=super().get_queryset(), + ) + # do not return duplicates when a file is PUBLISHED and a user also has files.view_basefile + return files.distinct() # type: ignore[no-any-return] + + class BaseFile(PolymorphicModel): """The polymorphic base model inherited by the Picture, Video, Audio, and Document models.""" @@ -111,6 +126,8 @@ class Meta: objects = PolymorphicManager.from_queryset(BaseFileQuerySet)() + permitted_files = PermittedFilesManager() + uuid = models.UUIDField( primary_key=True, default=uuid.uuid4, diff --git a/src/files/tests.py b/src/files/tests.py index 0587fd5..b746ce8 100644 --- a/src/files/tests.py +++ b/src/files/tests.py @@ -623,7 +623,7 @@ def test_file_missing_on_disk(self) -> None: self.assertEqual(response.json()["bma_response"]["size_bytes"], 0) -class FileAdminTests(ApiTestBase): +class TestFileAdmin(ApiTestBase): """Tests for the FileAdmin.""" def test_file_list_status_code(self) -> None: @@ -758,7 +758,7 @@ def test_file_list_html(self) -> None: ) -class FileViewTests(ApiTestBase): +class TestFileViews(ApiTestBase): """Unit tests for regular django views.""" def test_file_list_view(self) -> None: # noqa: PLR0915 diff --git a/src/utils/tests.py b/src/utils/tests.py index 32f2aea..b505e58 100644 --- a/src/utils/tests.py +++ b/src/utils/tests.py @@ -170,3 +170,25 @@ def file_upload( # noqa: PLR0913 if return_full: return data return data["uuid"] + + def album_create( + self, + *, + title: str = "album title here", + description: str = "album description here", + files: list[str] | None = None, + creator: str = "curator6", + ) -> str: + """Create an album optionally with some files.""" + response = self.client.post( + reverse("api-v1-json:album_create"), + { + "title": title, + "description": description, + "files": files if files else [], + }, + headers={"authorization": getattr(self, creator).auth}, + content_type="application/json", + ) + assert response.status_code == 201 + return response.json()["bma_response"]["uuid"]