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 unavailable status (TS-2276) #1085

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Roffenlund
Copy link
Contributor

Add a new is_unavailable status to the Package and PackageVersion
models to determine whether a package or package version is unavailable.

A package is considered unavailable if there is no PackageListing
object linking the package to the currently viewed community.

Even if a PackageListing object exists linking the package to the
community, its status might be "rejected," which would render the
package unavailable.

@Roffenlund Roffenlund self-assigned this Jan 13, 2025
@x753
Copy link
Contributor

x753 commented Jan 13, 2025

Unreviewed packages will be unavailable if their Community's require_package_listing_approval field is set to true, so logic for that should probably be added to the is_unavailable function.

(we don't currently use require_package_listing_approval in production but we've been keeping it functional through updates so might as well continue)

@Roffenlund Roffenlund force-pushed the add-is-unavailable-status branch 2 times, most recently from 0b9ea35 to b9a9465 Compare January 16, 2025 09:56
Add new functions to the Package and PackageVersion models to
determine whether a package or package version is unavailable.

A package is considered unavailable if:
- It is not active (is_effectively_active is False), or
- It does not have a related listing in a specific community, or
- It has a listing, but the listing is marked as rejected.

A package version is considered unavailable if:
- The related package is unavailable, or
- The package version itself is not active.
- (If both conditions apply, the package's status takes precedence.)

Additionally, implement unit tests to validate these new functions for
both the Package and PackageVersion models.

Refs. TS-2776
Add the is_unavailable status to the DependencySerializer. Use
SerializerMethodField and implement a function to fetch the
is_unavailable value from the PackageVersion model.

Implement tests for the endpoint using the serializer. The tests
should verify that the field is present and correctly reflects the
result of the is_unavailable function.

Refs. TS-2276
Refs. TS-2276
Comment on lines 61 to 66
def get_is_unavailable(self, obj: PackageVersion) -> bool:
community = Community.objects.filter(
identifier=obj.community_identifier
).first()

return obj.is_unavailable(community)
Copy link
Member

Choose a reason for hiding this comment

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

Only noticed this on a second look, but the implementation of this seems hazardous for performance when sets of items are being rendered. I'd try to find a way to e.g. annotate this information to the queryset as a batch operation instead.

Refactor availability checks to reduce queries:
- Annotate availability in get_custom_package_listing.
- Modify DependencySerializer to use annotations instead of fetching
  the community model.
- Move availability check to PackageListing and update Package's
  is_unavailable.
- Add tests ensuring queries remain under 20 for small datasets;
  larger sets see a drop from 100+ to ~60.

Refs. TS-2276
@Roffenlund Roffenlund force-pushed the add-is-unavailable-status branch from 2d09ba1 to 84e320b Compare January 29, 2025 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants