From e961162e52d8bfa01f233ae37ada3d904a6c0294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antti=20M=C3=A4ki?= Date: Tue, 2 Jan 2024 12:16:36 +0200 Subject: [PATCH 1/3] Add PackageVersion.is_effectively_active Helper method checks that both the PackageVersion and the related Package are active. This is acts as a pair for the existing Package.is_effectively_active helper. Refs TS-2013 --- .../repository/models/package_version.py | 4 ++++ .../repository/tests/test_package_version.py | 22 ++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/django/thunderstore/repository/models/package_version.py b/django/thunderstore/repository/models/package_version.py index 3e1fffaa4..b3332f69f 100644 --- a/django/thunderstore/repository/models/package_version.py +++ b/django/thunderstore/repository/models/package_version.py @@ -192,6 +192,10 @@ def owner(self): def is_deprecated(self): return self.package.is_deprecated + @cached_property + def is_effectively_active(self): + return self.is_active and self.package.is_active + @cached_property def full_version_name(self): return f"{self.package.full_package_name}-{self.version_number}" diff --git a/django/thunderstore/repository/tests/test_package_version.py b/django/thunderstore/repository/tests/test_package_version.py index f080cef71..3ee0dfaf7 100644 --- a/django/thunderstore/repository/tests/test_package_version.py +++ b/django/thunderstore/repository/tests/test_package_version.py @@ -5,7 +5,7 @@ from thunderstore.community.factories import PackageListingFactory from thunderstore.community.models.package_listing import PackageListing -from thunderstore.repository.factories import PackageVersionFactory +from thunderstore.repository.factories import PackageFactory, PackageVersionFactory from thunderstore.repository.models import PackageVersion from thunderstore.repository.package_formats import PackageFormats @@ -109,3 +109,23 @@ def test_package_version_chunked_enumerate() -> None: package_ids.remove(entry.pk) assert len(package_ids) == 0 + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("package_is_active", "version_is_active"), + ( + (False, False), + (True, False), + (False, True), + (True, True), + ), +) +def test_package_version_is_effectively_active( + package_is_active: bool, + version_is_active: bool, +) -> None: + package = PackageFactory(is_active=package_is_active) + version = PackageVersionFactory(package=package, is_active=version_is_active) + + assert version.is_effectively_active == (package_is_active and version_is_active) From ceccff51a4b2847195932defc736784546e5f413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antti=20M=C3=A4ki?= Date: Tue, 2 Jan 2024 12:45:01 +0200 Subject: [PATCH 2/3] Cyberstorm API: include deactived dependencies for package listing Even if a dependency is deactived, it's still a dependency of the package, which might not work without it. To avoid misleading information on the website, include deactived dependencies too. PackageVersion is considered deactived if the parent Package is deactived, even if the specific version would be active. Annotating the "effective active status" to the QuerySet was considered, but since the related package is prefetched for other purposes anyway, it seemed like an unnecessary step. Refs TS-2013 --- .../cyberstorm/tests/test_package_listing.py | 34 ++++++++++++++++++- .../api/cyberstorm/views/package_listing.py | 4 +-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/django/thunderstore/api/cyberstorm/tests/test_package_listing.py b/django/thunderstore/api/cyberstorm/tests/test_package_listing.py index 52147180f..3047f62a8 100644 --- a/django/thunderstore/api/cyberstorm/tests/test_package_listing.py +++ b/django/thunderstore/api/cyberstorm/tests/test_package_listing.py @@ -4,7 +4,10 @@ import pytest from rest_framework.test import APIClient -from thunderstore.api.cyberstorm.views.package_listing import get_custom_package_listing +from thunderstore.api.cyberstorm.views.package_listing import ( + DependencySerializer, + get_custom_package_listing, +) from thunderstore.community.factories import ( CommunityFactory, PackageCategoryFactory, @@ -280,5 +283,34 @@ def test_package_listing_view__serializes_url_correctly(api_client: APIClient) - assert actual["website_url"] is None +@pytest.mark.django_db +@pytest.mark.parametrize( + ("package_is_active", "version_is_active"), + ( + (False, False), + (True, False), + (False, True), + (True, True), + ), +) +def test_dependency_serializer__reads_is_active_from_correct_field( + package_is_active: bool, + version_is_active: bool, +) -> None: + dependant = PackageVersionFactory() + dependency = PackageVersionFactory(is_active=version_is_active) + dependency.package.is_active = package_is_active + dependency.package.save() + dependant.dependencies.set([dependency]) + + # community_identifier is normally added using annotations, but + # it's irrelavant for this test case. + dependency.community_identifier = "greendale" + + actual = DependencySerializer(dependency).data + + assert actual["is_active"] == (package_is_active and version_is_active) + + def _date_to_z(value: datetime) -> str: return value.strftime("%Y-%m-%dT%H:%M:%S.%fZ") diff --git a/django/thunderstore/api/cyberstorm/views/package_listing.py b/django/thunderstore/api/cyberstorm/views/package_listing.py index e34ad39c5..56a4550af 100644 --- a/django/thunderstore/api/cyberstorm/views/package_listing.py +++ b/django/thunderstore/api/cyberstorm/views/package_listing.py @@ -34,6 +34,7 @@ class DependencySerializer(serializers.Serializer): community_identifier = serializers.CharField() description = serializers.CharField() icon_url = serializers.CharField(source="icon.url") + is_active = serializers.BooleanField(source="is_effectively_active") name = serializers.CharField() namespace = serializers.CharField(source="package.namespace.name") version_number = serializers.CharField() @@ -165,8 +166,7 @@ def get_custom_package_listing( ) dependencies = ( - listing.package.latest.dependencies.active() - .listed_in(community_id) + listing.package.latest.dependencies.listed_in(community_id) .annotate( community_identifier=Value(community_id, CharField()), ) From 6e1b65d6e55db748798db88a5efbccc98979a49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antti=20M=C3=A4ki?= Date: Tue, 2 Jan 2024 15:22:17 +0200 Subject: [PATCH 3/3] Cyberstorm API: censor deactived package dependencies The reason why a package has been deactivated may be an obnoxious icon or description text, so don't send those to clients. Method serializers are used instead of annotations, since especially for the icon field the annotation isn't straightforward: the ImageField can't be directly attached to the object, and parsing the absolute URL correctly from the path representation stored in the database is error-prone. Refs TS-2013 --- .../cyberstorm/tests/test_package_listing.py | 20 ++++++++++++++++ .../api/cyberstorm/views/package_listing.py | 24 +++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/django/thunderstore/api/cyberstorm/tests/test_package_listing.py b/django/thunderstore/api/cyberstorm/tests/test_package_listing.py index 3047f62a8..6918795a5 100644 --- a/django/thunderstore/api/cyberstorm/tests/test_package_listing.py +++ b/django/thunderstore/api/cyberstorm/tests/test_package_listing.py @@ -312,5 +312,25 @@ def test_dependency_serializer__reads_is_active_from_correct_field( assert actual["is_active"] == (package_is_active and version_is_active) +@pytest.mark.django_db +def test_dependency_serializer__when_dependency_is_not_active__censors_icon_and_description() -> None: + # community_identifier is normally added using annotations, but + # it's irrelavant for this test case. + dependency = PackageVersionFactory() + dependency.community_identifier = "greendale" + + actual = DependencySerializer(dependency).data + + assert actual["description"].startswith("Desc_") + assert actual["icon_url"].startswith("http") + + dependency.is_active = False + del dependency.is_effectively_active # Clear cached property + actual = DependencySerializer(dependency).data + + assert actual["description"] == "This package has been removed." + assert actual["icon_url"] is None + + def _date_to_z(value: datetime) -> str: return value.strftime("%Y-%m-%dT%H:%M:%S.%fZ") diff --git a/django/thunderstore/api/cyberstorm/views/package_listing.py b/django/thunderstore/api/cyberstorm/views/package_listing.py index 56a4550af..5a8722cde 100644 --- a/django/thunderstore/api/cyberstorm/views/package_listing.py +++ b/django/thunderstore/api/cyberstorm/views/package_listing.py @@ -1,3 +1,5 @@ +from typing import Optional + from django.db.models import ( BooleanField, CharField, @@ -27,18 +29,32 @@ class DependencySerializer(serializers.Serializer): """ Dependencies of a given PackageVersion, listed in a given Community. - community_identifier and namespace are not present by default and - need to be annotated to the object. + community_identifier is not present by default and needs to be + annotated to the object. + + Description and icon is not shown to clients if the dependency is + deactivated, since the fields may contain the very reason for the + deactivation. """ community_identifier = serializers.CharField() - description = serializers.CharField() - icon_url = serializers.CharField(source="icon.url") + description = serializers.SerializerMethodField() + icon_url = serializers.SerializerMethodField() is_active = serializers.BooleanField(source="is_effectively_active") name = serializers.CharField() namespace = serializers.CharField(source="package.namespace.name") version_number = serializers.CharField() + def get_description(self, obj: PackageVersion) -> str: + return ( + obj.description + if obj.is_effectively_active + else "This package has been removed." + ) + + def get_icon_url(self, obj: PackageVersion) -> Optional[str]: + return obj.icon.url if obj.is_effectively_active else None + class TeamSerializer(serializers.Serializer): """