Implement Visibility for Listings / Versions #1112

x753 · 2025-03-07T21:22:10Z

Rework VisibilityMixin to take active() into account and use abstract methods Add VisibilityMixin and VisibilityQuerySet to PackageListing Add update_visibility and is_visible_to_user functions for PackageListing and PackageVersion Add review status to PackageVersion
Add Listing and Version specific AuditAction enums

This PR implements Visibility, but none of our views or API endpoints use it, so nothing should change as far as what users see. Doing PRs in pieces like these will hopefully make review easier than looking over 60+ commits from this.

django/thunderstore/webhooks/models/audit.py

Roffenlund · 2025-03-13T07:06:50Z

The update_visibility functions would benefit from documentation to provide context for readers, particularly regarding the precedence of visibility rules.

Are there tests for the update_visibility functions in the larger PR?

x753 · 2025-03-14T17:15:26Z

The update_visibility functions would benefit from documentation to provide context for readers, particularly regarding the precedence of visibility rules.

Are there tests for the update_visibility functions in the larger PR?

I've added tests, but I'm not too sure what kind of documentation would fit here. Any ideas?

Roffenlund · 2025-03-17T08:35:23Z

The update_visibility functions would benefit from documentation to provide context for readers, particularly regarding the precedence of visibility rules.
Are there tests for the update_visibility functions in the larger PR?

I've added tests, but I'm not too sure what kind of documentation would fit here. Any ideas?

I was thinking a docstring would do here—just to document the basics, such as the precedence of how these visibility rules are handled.

django/thunderstore/repository/models/package.py

Rework VisibilityMixin to take active() into account and use abstract methods Add VisibilityMixin and VisibilityQuerySet to PackageListing Add update_visibility and is_visible_to_user functions for PackageListing and PackageVersion Add review status to PackageVersion Add Listing and Version specific AuditAction enums

Update visibility when package saves so changing is_active properly updates visibility

Small update_visibility optimization, does not affect worst case

codecov · 2025-05-07T15:18:20Z

Codecov Report

Attention: Patch coverage is 79.72973% with 45 lines in your changes missing coverage. Please review.

Project coverage is 93.07%. Comparing base (56ab32b) to head (05a2da3).
Report is 23 commits behind head on master.

Files with missing lines	Patch %	Lines
.../thunderstore/repository/models/package_version.py	54.92%	32 Missing ⚠️
...o/thunderstore/repository/admin/package_version.py	57.14%	6 Missing ⚠️
django/thunderstore/permissions/mixins.py	86.20%	4 Missing ⚠️
...o/thunderstore/community/models/package_listing.py	95.34%	2 Missing ⚠️
django/thunderstore/webhooks/models/audit.py	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
+ Coverage   93.03%   93.07%   +0.03%     
==========================================
  Files         321      324       +3     
  Lines        9675    10304     +629     
  Branches      857     1024     +167     
==========================================
+ Hits         9001     9590     +589     
- Misses        553      598      +45     
+ Partials      121      116       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

...go/thunderstore/community/migrations/0037_create_default_visibility_for_existing_listings.py

django/thunderstore/repository/models/package.py

django/thunderstore/permissions/models/visibility.py

django/thunderstore/community/models/package_listing.py

MythicManiac · 2025-05-08T17:43:31Z

django/thunderstore/repository/models/package_version.py

+    @transaction.atomic
+    def update_visibility(self):
+        """
+        Updates the version's visibility based on whether it or its package is active and its review status.
+        This may also change the visibility of related listings, so it calls update_visibility() on them.
+
+        By default, versions are visible to everyone (for now). Rejected versions aren't publicly visible,
+        and inactive versions or versions with inactive packages aren't visible at all.
+        """
+        original_visibility_bitstring = self.visibility.bitstring()
+
+        self.visibility.public_detail = True
+        self.visibility.public_list = True
+        self.visibility.owner_detail = True
+        self.visibility.owner_list = True
+        self.visibility.moderator_detail = True
+        self.visibility.moderator_list = True
+
+        if not self.is_active or not self.package.is_active:
+            self.visibility.public_detail = False
+            self.visibility.public_list = False
+            self.visibility.owner_detail = False
+            self.visibility.owner_list = False
+            self.visibility.moderator_detail = False
+            self.visibility.moderator_list = False
+
+        if (
+            self.review_status == PackageVersionReviewStatus.rejected
+            or self.review_status == PackageVersionReviewStatus.pending
+        ):
+            self.visibility.public_detail = False
+            self.visibility.public_list = False
+
+        if self.visibility.bitstring != original_visibility_bitstring:
+            self.visibility.save()
+            for listing in self.package.community_listings.all():
+                listing.update_visibility()
+            self.package.recache_latest()


I feel like we should think about how to build this in a way that's easy to test and maintain, if nothing else by splitting it to more atomic components that are easier to write standalone tests for rather than having to test everything at once.

Right now it seems like we're actually dealing with a pretty simple map of conditions that need to match to flags that should be flipped to False. If that's a paradigm we can assume is true, this shouldn't be too hard to turn into a system of sorts that functions in a more "data driven" fashion, making it easier to reliably test.

Child-updates & their propagation of course is a separate concern and probably fine as it is

This comment applies to the overall design of visibility updates and theri implementations in other places of the codebase too, not just this specific block

Should note that this approach does have some advantages (mainly simplicity to read), but I suspect a light abstraction layer could still prove useful long-term and can actually make it easier to reason about if you have the required contextual info. Reading through dozens of lines of code isn't as straight forward as seeing "if the package is not active, visibility rules of inactive packages are applied" in a single line after all. What those rules are isn't as relevant on this level of context in my opinion

Refactor AuditAction into AuditTarget & AuditAction

...go/thunderstore/repository/migrations/0059_create_default_visibility_for_existing_records.py

django/thunderstore/repository/tests/test_package_version.py

graphite-app · 2025-05-13T22:37:55Z

...go/thunderstore/repository/migrations/0059_create_default_visibility_for_existing_records.py

+    for instance in PackageVersion.objects.filter(visibility__isnull=True):
+        visibility_flags = VisibilityFlags.objects.create(
+            public_list=False,
+            public_detail=False,
+            owner_list=True,
+            owner_detail=True,
+            moderator_list=True,
+            moderator_detail=True,
+            admin_list=True,
+            admin_detail=True,
+        )
+        instance.visibility = visibility_flags
+        instance.save()
+        update_version_visibility(instance)


It appears that the migration 0059_create_default_visibility_for_existing_records.py is attempting to set visibility fields for PackageVersion objects, but there's no corresponding migration that adds the visibility field to the PackageVersion model.

Before this migration runs, a separate migration should be created to add the visibility field to the PackageVersion model, similar to how it was done for Package in migration 0058_package_visibility.py.

This would ensure that when update_version_visibility() is called, the visibility field actually exists on the model instances.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Refactor update_visibility Add VisibilityMixin to Package Optimize Package visibility from versions querying Use tuple instead of bitstring for comparing to original visibility

graphite-app · 2025-05-13T22:44:01Z

django/thunderstore/repository/models/package.py

@@ -44,7 +45,7 @@ def get_package_dependants_list(package_pk: int):
    return list(get_package_dependants(package_pk))


-class Package(AdminLinkMixin, models.Model):
+class Package(VisibilityMixin, AdminLinkMixin):


The Package class inherits from VisibilityMixin which requires implementing the abstract method is_visible_to_user(). This method is implemented in both PackageListing and PackageVersion classes, but is missing from Package. Please add an implementation similar to the other classes to ensure proper visibility control for packages.

Suggested change

class Package(VisibilityMixin, AdminLinkMixin):

class Package(VisibilityMixin, AdminLinkMixin):

def is_visible_to_user(self, user=None):

# Implementation similar to PackageListing and PackageVersion

if user and user.is_staff:

return True

return self.is_active

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-05-13T22:44:02Z

django/thunderstore/repository/admin/package_version.py

+        version.review_status = PackageVersionReviewStatus.rejected
+        version.save(update_fields=("review_status",))


The use of update_fields=("review_status",) in the admin actions bypasses the visibility update mechanism. When a version is rejected or approved through the admin interface, its visibility flags won't be updated because the VisibilityMixin.update_visibility() method is only called during a full save operation.

Two options to fix this:

Remove the update_fields parameter to ensure the full save method runs:

version.review_status = PackageVersionReviewStatus.rejected version.save()

Better approach: use the model methods that handle both status and visibility updates:

version.reject(request.user)

version.approve(request.user)

These methods already handle the review status change, visibility updates, and audit logging in a single transaction.

Suggested change

version.review_status = PackageVersionReviewStatus.rejected

version.save(update_fields=("review_status",))

version.reject(request.user)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

x753 requested a review from Roffenlund March 11, 2025 15:48

Roffenlund reviewed Mar 13, 2025

View reviewed changes

django/thunderstore/webhooks/models/audit.py Outdated Show resolved Hide resolved

x753 force-pushed the visibility-and-version-review branch from 65291df to 1d05852 Compare March 24, 2025 16:33

x753 requested a review from Roffenlund March 26, 2025 20:33

Roffenlund reviewed Mar 31, 2025

View reviewed changes

django/thunderstore/repository/models/package.py Outdated Show resolved Hide resolved

x753 force-pushed the visibility-and-version-review branch from 521bf08 to 8496a98 Compare April 1, 2025 14:28

x753 requested a review from Roffenlund April 10, 2025 19:11

Roffenlund approved these changes May 7, 2025

View reviewed changes

x753 added 6 commits May 7, 2025 11:13

Update visibility when package saves

aa6e09a

Update visibility when package saves so changing is_active properly updates visibility

Add tests for Visibility and Audit

45b718c

Added docstrings for update_visibility()

c3cc688

Rename migrations

507208b

Small update_visibility optimization

dd38bd9

Small update_visibility optimization, does not affect worst case

x753 force-pushed the visibility-and-version-review branch from 8496a98 to dd38bd9 Compare May 7, 2025 15:13

Update migration numbers

29e94b8

graphite-app bot reviewed May 7, 2025

View reviewed changes

...go/thunderstore/community/migrations/0037_create_default_visibility_for_existing_listings.py Outdated Show resolved Hide resolved

Improve visibility test coverage

313c143

MythicManiac reviewed May 8, 2025

View reviewed changes

django/thunderstore/repository/models/package.py Outdated Show resolved Hide resolved

MythicManiac reviewed May 8, 2025

View reviewed changes

django/thunderstore/permissions/models/visibility.py Outdated Show resolved Hide resolved

MythicManiac reviewed May 8, 2025

View reviewed changes

django/thunderstore/community/models/package_listing.py Outdated Show resolved Hide resolved

MythicManiac reviewed May 8, 2025

View reviewed changes

Refactor AuditAction into Target & Action

35898ee

Refactor AuditAction into AuditTarget & AuditAction

x753 force-pushed the visibility-and-version-review branch from ba0f6fa to 929a32b Compare May 13, 2025 22:35

graphite-app bot reviewed May 13, 2025

View reviewed changes

...go/thunderstore/repository/migrations/0059_create_default_visibility_for_existing_records.py Outdated Show resolved Hide resolved

django/thunderstore/repository/tests/test_package_version.py Outdated Show resolved Hide resolved

graphite-app bot reviewed May 13, 2025

View reviewed changes

Refactor update_visibility

05a2da3

Refactor update_visibility Add VisibilityMixin to Package Optimize Package visibility from versions querying Use tuple instead of bitstring for comparing to original visibility

x753 force-pushed the visibility-and-version-review branch from 929a32b to 05a2da3 Compare May 13, 2025 22:40

graphite-app bot reviewed May 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Visibility for Listings / Versions #1112

Implement Visibility for Listings / Versions #1112

x753 commented Mar 7, 2025

Roffenlund commented Mar 13, 2025

x753 commented Mar 14, 2025

Roffenlund commented Mar 17, 2025

codecov bot commented May 7, 2025 •

edited

Loading

MythicManiac May 8, 2025 •

edited

Loading

MythicManiac May 8, 2025 •

edited

Loading

graphite-app bot May 13, 2025

graphite-app bot May 13, 2025

graphite-app bot May 13, 2025

-class Package(VisibilityMixin, AdminLinkMixin):
+class Package(VisibilityMixin, AdminLinkMixin):
+    def is_visible_to_user(self, user=None):
+        # Implementation similar to PackageListing and PackageVersion
+        if user and user.is_staff:
+            return True
+        return self.is_active

		version.review_status = PackageVersionReviewStatus.rejected
		version.save(update_fields=("review_status",))

	version.review_status = PackageVersionReviewStatus.rejected
	version.save(update_fields=("review_status",))
	version.reject(request.user)

Implement Visibility for Listings / Versions #1112

Are you sure you want to change the base?

Implement Visibility for Listings / Versions #1112

Conversation

x753 commented Mar 7, 2025

Roffenlund commented Mar 13, 2025

x753 commented Mar 14, 2025

Roffenlund commented Mar 17, 2025

codecov bot commented May 7, 2025 • edited Loading

Codecov Report

MythicManiac May 8, 2025 • edited Loading

Choose a reason for hiding this comment

MythicManiac May 8, 2025 • edited Loading

Choose a reason for hiding this comment

graphite-app bot May 13, 2025

Choose a reason for hiding this comment

graphite-app bot May 13, 2025

Choose a reason for hiding this comment

graphite-app bot May 13, 2025

Choose a reason for hiding this comment

codecov bot commented May 7, 2025 •

edited

Loading

MythicManiac May 8, 2025 •

edited

Loading

MythicManiac May 8, 2025 •

edited

Loading