release review by MythicManiac · Pull Request #1199 · thunderstore-io/Thunderstore

MythicManiac · 2025-11-13T10:04:07Z

this PR is just so I can comment on diffs between the releases

Add file with endpoints and PATCH/POST data to a separate file in order to not clutter the actual test fiels with this data. Implement helper functions for getting superuser and superuser with a package and other testing setup. Refs. TS-2584

Implement new tests for testing the query count for GET endpoints in the cyberstorm API. Implement separate tests for list endpoints to assert they do not exceed the limit of 15 queries. Update the existing endpoint tests accordingly to the refactored changes. Refs. TS-2584

Refs. TS-2584

Remove setup_superuser and use the UserFactory directly. Move get_parameter_values to utils instead of implementing it twice. Fix typo in test. Refs. TS-2597

Query count tests (TS-2584)

Add "latest" field to select_related in order to slightly improve database query when fetching latest package version. Refs. TS-2639

Add a new file for storing pagination classes as these ought to be in one place. Implement a new class for package dependencies list apiview. Refs. TS-2639

Add new serializer to be used with the package dependencies list apiview. Refs. TS-2639

Implement a view responsible for fetching dependencies for a package. The view returns a list of paginated and serialized package versions related to a specific package. Pagination is set to 20 items per page. The view expects a version number, or alternatively "latest" in the URL kwargs to be present. When using "latest", the version is set to None which is handled by get_package_version(). Refs. TS-2639

Add URL and tests for package dependencies API list view. Refs. TS-2639

Refs. TS-2637

Implement a DELETE endpoint for removing team members from a team in the cyberstorm API. Implement URL and tests. Refs. TS-2637

Update the existing form and form view responsible for removing team members from a team to use the service layer function. Update tests accordingly. Refs. TS-2637

Add remove team member endpoint tests. Update test_cyberstorm_DELETE_endpoint_schemas to create a team and add member to team. Update get_parameter_values. Refs. TS-2637

* Add new test to query count tests * Remove the existing query count tests in test file * Update util for asserting query count to check response status code * Add endpoint to endpoint_data Refs. TS-2639

Fix test which returned a 403 due to faulty setup. Refs. TS-2639

…cies-endpoint Cyberstorm api dependencies endpoint (TS-2639)

Cyberstorm api remove team member endpoint (TS-2637)

Implement serializer to be user for serializing package listing status responses. Refs. TS-2598

Implement a view which fetches internal_notes, rejection_reason, review_status, listing_admin_url for package listings. Return the correct data depending on what permissions the user has. These permissions mirror the legacy view's permissions. Implement unit tests and URL for the endpoint. Refs. TS-2598

Add a separate check for who can view the listing_admin_url. Refactor tests as the previous test would have gotten too big by adding one more permission. Refs. TS-2598

Refs. TS-2598

…status Cyberstorm api listing status (TS-2598)

Implement services for the following: * Create service account * Delete service account Implement tests for the services. Refs. TS-2320

Refs. TS-2320

Update forms and views for create and delete service accounts. Refs. TS-2320

Add create and delete endpoints to the endpoint test suite. Update the delete view swagger request_body parameter to display explicitly None. Refs. TS-2320

Update the import location for UserFactory. Refs. TS-2320

Sort endpoints in endpoint_data.py in alphabetical order. Refs. TS-2597

Use empty dict instead of None as default value in test_package_listing_action test. Refs. TS-2597

Use conditional_swagger_auto_schema in order to hide endpoint depending on settings. Refs. TS-2597

expected_status_code -> expected_status_code_map. Refs. TS-2597

Annotate the queryset with active_package_versions instead of calling PackageVersion.is_removed, which avoids triggering an N+1 query. Refs. TS-2672

Remove function which is not being used by any tests. Refs. TS-2597

Add the /unlist endpoint to test. Refs. TS-2597

…ackage Cyberstorm api unlist package (TS-2597)

Implement a service in the service layer in the cyberstorm API for updating team members. Refs. TS-2316

Refs. TS-2316

Implement an APIView in the cyberstorm API responsible for updating team members. The view utilizes the service layer functionality for permission checks and performing the update action to the database object. Refs. TS-2316

Add a new endpoint for updating team members in the Cyberstorm API. Implement functional tests. Refs. TS-2316

Update the form which is responsible for updating team members in the legacy application to use the service layer functionality. Update the view and tests accordingly to the new changes. Refs. TS-2316

Add endpoint to endpoint tests, update view swagger docs with request body. Add username to get_parameter_values util. Refs. TS-2316

Refs. TS-2282

…dependencies-endpoint-db-query Optimize dependencies list queryset query (TS-2672)

…eam-member Implement update team member endpoint (TS-2316)

Implement test for checking the return types of the plugin_registry. Refs. TS-2282

…e-source-data Add endpoint for fetching package source (TS-2282)

Remove raising ValidationError in Team.create if namespace exists. This check is performed in Team.validation() and does not have to be checked in Team.create(). Do this change in order to have consistent error messaging when using the Team.create function. Refs. TS-2426

Update disband_team service and create_team service. Utilize Team.create() in create_team service and remove redundant validation errors. Use objects as parameters in the services instead of trying to fetch and raise 404 errors in services. Refs. TS-2426

Update the create team and disband team views since the parameter names changed of the services. Refs. TS-2426

Use the create_team and disband_team services in the forms and views responsible for disband teams and creating teams. Update tests accordingly. Update the form for creating teams to clean the name manually in order to keep the error messaging consistent, otherwise tests will fail since because it is a ModelForm which throws a different formatted error by default. Refs. TS-2426

Some actions raise PermissionValidationError instead of ValidationError. Although PermissionValidationError inherits from ValidationError, tests that specifically expect these errors should use PermissionValidationError to ensure accurate validation. Refs. TS-2426

Refactor team disband and create views(TS-2426)

coderabbitai · 2025-11-13T10:04:15Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch review-dest

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2025-11-13T10:10:19Z

Codecov Report

❌ Patch coverage is 99.21260% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.81%. Comparing base (f06b9b4) to head (1ba470a).

Files with missing lines	Patch %	Lines
django/thunderstore/repository/forms/team.py	95.12%	1 Missing and 1 partial ⚠️
...o/thunderstore/api/cyberstorm/serializers/utils.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           review-base    #1199      +/-   ##
===============================================
+ Coverage        92.23%   92.81%   +0.57%     
===============================================
  Files              331      337       +6     
  Lines            10088    10358     +270     
  Branches           927      938      +11     
===============================================
+ Hits              9305     9614     +309     
+ Misses             657      617      -40     
- Partials           126      127       +1

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

MythicManiac · 2025-11-13T10:06:14Z



+@pytest.mark.django_db
+def test_service_account_create_error_on_save(user, team):


Could be a good idea to explain why the test should fail

MythicManiac · 2025-11-13T10:06:20Z



+@pytest.mark.django_db
+def test_service_account_delete_error_on_save(service_account):


Could be a good idea to explain why the test should fail

MythicManiac · 2025-11-13T10:12:51Z

+        if self.errors:
+            raise ValueError("Cannot save form with errors")


Is this actually necessary/useful to guard against? Asking because I'd expect Django to have this built in if yes

MythicManiac · 2025-11-13T10:12:57Z

+        if self.errors:
+            raise ValueError("Cannot save form with errors")


Is this actually necessary/useful to guard against? Asking because I'd expect Django to have this built in if yes

MythicManiac · 2025-11-13T10:24:27Z

+    def get_is_removed(self, obj: PackageVersion) -> bool:
+        package_is_removed = not (
+            obj.package.is_active and obj.package_has_active_versions
+        )
+        if package_is_removed:
+            return True
+        return not obj.is_active
+
+    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


This seems like it could generate lots of queries if this serializer is used in a list context, but I don't know if that's the case (and perhaps we had query count tests already protecting against it?)

MythicManiac · 2025-11-13T11:27:38Z

+from rest_framework.pagination import PageNumberPagination
+
+
+class PackageDependenciesPaginator(PageNumberPagination):
+    page_size = 20


Does this really need its own code file, moreover does it need to be called PackageDependenciesPaginatior if it has nothing specific for package dependencies?

I'd add it next to the view itself if it's specific for a single view, and if not, then call it something more generic

MythicManiac · 2025-11-13T11:38:33Z

        name="cyberstorm.listing",
    ),
+    path(
+        "listing/<str:community_id>/<str:namespace_id>/<str:package_name>/status/",


Since this was a view that varies based on the current user, I think it should be under the current-user URL path

MythicManiac · 2025-11-13T11:41:33Z

+    path(
+        "user/delete/",
+        DeleteUserAPIView.as_view(),
+        name="cyberstorm.user.delete",
+    ),
+    path(
+        "user/linked-account/<str:provider>/disconnect/",
+        DisconnectUserLinkedAccountAPIView.as_view(),
+        name="cyberstorm.user.linked-account.disconnect",
+    ),
+    path(
+        "team/<str:team_name>/member/<str:team_member>/update/",
+        UpdateTeamMemberAPIView.as_view(),
+        name="cyberstorm.team.member.update",
+    ),


These also strike me as something that should be under the current-user API path as that's what we're operating on.

Alternatively we could use some magic value for referring to the current user and parametrize the API, e.g. @me like Discord does.

MythicManiac · 2025-11-13T11:53:34Z

+def validate_max_queries(
+    client: APIClient,
+    method: str,
+    path: str,
+    max_queries: int,
+    data: Optional[dict] = None,
+    **kwargs,
+):
+    request_func = getattr(client, method.lower())
+
+    with CaptureQueriesContext(connection) as ctx:
+        response = request_func(path, data=data or {}, **kwargs)
+
+    allowed_statuses = [200, 201, 202, 204]
+    if response.status_code not in allowed_statuses:
+        raise AssertionError(
+            f"{method} {path} returned status {response.status_code}, "
+            f"expected one of {allowed_statuses}."
+        )
+
+    num_queries = len(ctx.captured_queries)
+    if num_queries > max_queries:
+        queries_str = "\n".join(q["sql"] for q in ctx.captured_queries)
+        raise AssertionError(
+            f"{method} {path} executed {num_queries} queries "
+            f"(allowed {max_queries}).\n"
+            f"Queries:\n{queries_str}"
+        )
+
+    return response


Instead of doing the whole getattr(client, method.lower()) logic and making this utility HTTP specific, this could've been implemented as a 2 parameter function, where one parameter is a callable (allowing you to pass in whatever you want to measure) and the other is the query limit.

MythicManiac · 2025-11-13T12:07:20Z

submodule changes:

the latest parameter should not be supported
if version_number == "latest": return package.latest

the API response is currently returning all source data in a single response (granted it can be truncated); this is not a good way to design the API and will result in potentially tens of megabytes per API response

should ideally return links to the raw source files instead, which API clients can fetch separately (and over CDN at that)

large API responses are bad and can choke the service, and currently the API has room to return very large responses

also why do we need datetime_created in the returned API data? it's not the actual creation date of the file, just when it was extracted/processed on the server, which doesn't seem particularly useful for anything?

Roffenlund and others added 30 commits August 28, 2025 14:02

Refactor and update test utils

a506458

Add file with endpoints and PATCH/POST data to a separate file in order to not clutter the actual test fiels with this data. Implement helper functions for getting superuser and superuser with a package and other testing setup. Refs. TS-2584

Fix styling with black

fa1a437

Refs. TS-2584

Refactor test utils

59bf19d

Remove setup_superuser and use the UserFactory directly. Move get_parameter_values to utils instead of implementing it twice. Fix typo in test. Refs. TS-2597

Merge pull request #1173 from thunderstore-io/query-count-tests

42339ba

Query count tests (TS-2584)

Improve get_package_version

9843753

Add "latest" field to select_related in order to slightly improve database query when fetching latest package version. Refs. TS-2639

Implement pagination.py file in cyberstorm API

6731f28

Add a new file for storing pagination classes as these ought to be in one place. Implement a new class for package dependencies list apiview. Refs. TS-2639

Add dependencies serializer

adb6766

Add new serializer to be used with the package dependencies list apiview. Refs. TS-2639

Add URL for package dependencies api list view

562a3b4

Add URL and tests for package dependencies API list view. Refs. TS-2639

Implement service for remove team member

631291b

Refs. TS-2637

Implement api view for removing team member

d3fe9c3

Implement a DELETE endpoint for removing team members from a team in the cyberstorm API. Implement URL and tests. Refs. TS-2637

Update existing form and form view

7848f5a

Update the existing form and form view responsible for removing team members from a team to use the service layer function. Update tests accordingly. Refs. TS-2637

Add remove team member endpoint to tests

88ddc7f

Add remove team member endpoint tests. Update test_cyberstorm_DELETE_endpoint_schemas to create a team and add member to team. Update get_parameter_values. Refs. TS-2637

Add query count test

5f83436

* Add new test to query count tests * Remove the existing query count tests in test file * Update util for asserting query count to check response status code * Add endpoint to endpoint_data Refs. TS-2639

Fix broken test

46fa16e

Fix test which returned a 403 due to faulty setup. Refs. TS-2639

Merge pull request #1184 from thunderstore-io/cyberstorm-api-dependen…

1e6fc6f

…cies-endpoint Cyberstorm api dependencies endpoint (TS-2639)

Merge pull request #1182 from thunderstore-io/cyberstorm-api-leave-team

f6c2187

Cyberstorm api remove team member endpoint (TS-2637)

Implement listing status serializer

b893ad5

Implement serializer to be user for serializing package listing status responses. Refs. TS-2598

Add permission check for listing_admin_url

87ada42

Add a separate check for who can view the listing_admin_url. Refactor tests as the previous test would have gotten too big by adding one more permission. Refs. TS-2598

Add endpoint to endpoint tests

a0fb687

Refs. TS-2598

Merge pull request #1179 from thunderstore-io/cyberstorm-api-listing-…

04e1017

…status Cyberstorm api listing status (TS-2598)

Implement team services

1ac0481

Implement services for the following: * Create service account * Delete service account Implement tests for the services. Refs. TS-2320

Implement serializer for create service account

6821d01

Refs. TS-2320

Implement create & delete service account views

7c1270f

Refs. TS-2320

Implement create/delete service account endpoints

e665e38

Refs. TS-2320

Update service account form and views

12defc4

Update forms and views for create and delete service accounts. Refs. TS-2320

Add endpoints to endpoint tests

38522a2

Add create and delete endpoints to the endpoint test suite. Update the delete view swagger request_body parameter to display explicitly None. Refs. TS-2320

Update test_team_services import

dd16ce9

Update the import location for UserFactory. Refs. TS-2320

Roffenlund and others added 27 commits October 2, 2025 14:59

Sort endpoints in alphabetical order

09e29c4

Sort endpoints in endpoint_data.py in alphabetical order. Refs. TS-2597

Update parameter type in test

a84219c

Use empty dict instead of None as default value in test_package_listing_action test. Refs. TS-2597

Use conditional_swagger_auto_schema

4f3b42d

Use conditional_swagger_auto_schema in order to hide endpoint depending on settings. Refs. TS-2597

Fix variable name in tests

d5eb376

expected_status_code -> expected_status_code_map. Refs. TS-2597

Optimize dependencies list queryset query

a51c2c1

Annotate the queryset with active_package_versions instead of calling PackageVersion.is_removed, which avoids triggering an N+1 query. Refs. TS-2672

Remove redundant test function

3cd0109

Remove function which is not being used by any tests. Refs. TS-2597

Add the unlist action to tests

2a4b8b8

Add the /unlist endpoint to test. Refs. TS-2597

Merge pull request #1178 from thunderstore-io/cyberstorm-api-unlist-p…

0f2eef0

…ackage Cyberstorm api unlist package (TS-2597)

Implement service for updating team member

271aa1a

Implement a service in the service layer in the cyberstorm API for updating team members. Refs. TS-2316

Implement serializer for updating team members

0891dd3

Refs. TS-2316

Implement view for updating team member

826b545

Implement an APIView in the cyberstorm API responsible for updating team members. The view utilizes the service layer functionality for permission checks and performing the update action to the database object. Refs. TS-2316

Add URL endpoint for update team member

bae8455

Add a new endpoint for updating team members in the Cyberstorm API. Implement functional tests. Refs. TS-2316

Update form and form view for update team member

b47ddc6

Update the form which is responsible for updating team members in the legacy application to use the service layer functionality. Update the view and tests accordingly to the new changes. Refs. TS-2316

Add endpoint to endpoint tests

94f5809

Add endpoint to endpoint tests, update view swagger docs with request body. Add username to get_parameter_values util. Refs. TS-2316

python-packages update

003ffd5

Refs. TS-2282

Add package source endpoint

f249529

Refs. TS-2282

Skip tests if python-packages endpoint not available

b6aa784

Refs. TS-2282

Merge pull request #1187 from thunderstore-io/cyberstorm-api-improve-…

3777922

…dependencies-endpoint-db-query Optimize dependencies list queryset query (TS-2672)

Merge pull request #1117 from thunderstore-io/cyberstorm-api-update-t…

db03a91

…eam-member Implement update team member endpoint (TS-2316)

Implement basic test for plugin_registry

805dbf8

Implement test for checking the return types of the plugin_registry. Refs. TS-2282

Merge pull request #1094 from thunderstore-io/api-endpoint-for-packag…

2e7df1c

…e-source-data Add endpoint for fetching package source (TS-2282)

Update team services

c8ba27d

Update disband_team service and create_team service. Utilize Team.create() in create_team service and remove redundant validation errors. Use objects as parameters in the services instead of trying to fetch and raise 404 errors in services. Refs. TS-2426

Update create team and disband team views

62751e6

Update the create team and disband team views since the parameter names changed of the services. Refs. TS-2426

Merge pull request #1139 from thunderstore-io/refactor-team-form-views

1ba470a

Refactor team disband and create views(TS-2426)

MythicManiac commented Nov 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release review#1199

release review#1199
MythicManiac wants to merge 75 commits intoreview-basefrom
review-dest

MythicManiac commented Nov 13, 2025

Uh oh!

coderabbitai bot commented Nov 13, 2025

Review skipped

Uh oh!

codecov bot commented Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

MythicManiac Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants



		@pytest.mark.django_db
		def test_service_account_create_error_on_save(user, team):



		@pytest.mark.django_db
		def test_service_account_delete_error_on_save(service_account):

		if self.errors:
		raise ValueError("Cannot save form with errors")

Conversation

MythicManiac commented Nov 13, 2025

Uh oh!

coderabbitai bot commented Nov 13, 2025

Review skipped

Uh oh!

codecov bot commented Nov 13, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants