diff --git a/ws/api_views.py b/ws/api_views.py index 6a25e17c..c31ec2b1 100644 --- a/ws/api_views.py +++ b/ws/api_views.py @@ -12,7 +12,7 @@ from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db import transaction -from django.db.models import Q +from django.db.models import Q, QuerySet from django.http import HttpRequest, JsonResponse from django.utils.decorators import method_decorator from django.views.decorators.csrf import csrf_exempt @@ -152,13 +152,18 @@ class SignupsChangedError(Exception): """ +class JsonSignup(TypedDict): + id: int + deleted: NotRequired["bool"] + + class AdminTripSignupsView(SingleObjectMixin, FormatSignupMixin, TripLeadersOnlyView): model = models.Trip - def get(self, request, *args, **kwargs): + def get(self, request: HttpRequest, *args: Any, **kwargs: Any) -> JsonResponse: return JsonResponse(self.describe_all_signups()) - def post(self, request, *args, **kwargs): + def post(self, request: HttpRequest, *args: Any, **kwargs: Any) -> JsonResponse: """Take a list of exactly how signups should be ordered and apply it. To avoid dealing with concurrency, calculating diffs, etc. we just @@ -173,10 +178,10 @@ def post(self, request, *args, **kwargs): trip = self.object = self.get_object() postdata = json.loads(self.request.body) # TODO: assumes valid JSON. - signup_list = postdata.get("signups", []) - maximum_participants = postdata.get("maximum_participants") + signup_list: list[JsonSignup] = postdata.get("signups", []) + maximum_participants: int | None = postdata.get("maximum_participants") - def error(msg): + def error(msg: str) -> JsonResponse: return JsonResponse({"message": msg}, status=400) # Any non-validation errors will trigger rollback @@ -192,7 +197,12 @@ def error(msg): ) return JsonResponse({}) - def update(self, trip, signup_list, maximum_participants): + def update( + self, + trip: models.Trip, + signup_list: list[JsonSignup], + maximum_participants: int | None, + ) -> None: """Take parsed input data and apply the changes.""" if maximum_participants: trip.maximum_participants = maximum_participants @@ -202,7 +212,10 @@ def update(self, trip, signup_list, maximum_participants): self.update_signups(signup_list, trip) @staticmethod - def signups_to_update(signup_list, trip): + def signups_to_update( + signup_list: list[JsonSignup], + trip: models.Trip, + ) -> tuple[QuerySet[models.SignUp], QuerySet[models.SignUp]]: """From the payload, break signups into deletion & those that stay. All signups are given (in order) in `signup_list`. If the `deleted` key @@ -238,7 +251,7 @@ def signups_to_update(signup_list, trip): return (keep_on_trip, to_delete) - def update_signups(self, signup_list, trip): + def update_signups(self, signup_list: list[JsonSignup], trip: models.Trip) -> None: """Mark all signups as not on trip, then add signups in order.""" keep_on_trip, to_delete = self.signups_to_update(signup_list, trip) @@ -246,7 +259,10 @@ def update_signups(self, signup_list, trip): # Both methods (update and skip_signals) ignore waitlist-bumping keep_on_trip.update(on_trip=False) for kill_signup in to_delete: - kill_signup.skip_signals = True + # Setting properties dynamically on an object is a huge anti-pattern. + # *Almost* as bad as using Django signals in the first place... + # Hopefully we change this at a later date. + kill_signup.skip_signals = True # type: ignore[attr-defined] kill_signup.delete() # `all()` hits the db, will fetch current (`on_trip=False`) signups @@ -256,9 +272,9 @@ def update_signups(self, signup_list, trip): signup_utils.trip_or_wait(signup, trip_must_be_open=False) signup_utils.next_in_order(signup, order) - def get_signups(self): + def get_signups(self) -> QuerySet[models.SignUp]: """Trip signups with selected models for use in describe_signup.""" - trip = self.get_object() + trip: models.Trip = self.get_object() return ( trip.on_trip_or_waitlisted.select_related( "participant", "participant__lotteryinfo__paired_with__lotteryinfo" @@ -272,7 +288,7 @@ def get_signups(self): .order_by("-on_trip", "waitlistsignup", "last_updated") ) - def describe_all_signups(self): + def describe_all_signups(self) -> dict[str, Any]: """Get information about the trip's signups.""" trip = self.get_object() signups = self.get_signups() diff --git a/ws/models.py b/ws/models.py index 271db9c9..b31d0545 100644 --- a/ws/models.py +++ b/ws/models.py @@ -1672,14 +1672,12 @@ def __str__(self) -> str: return f"Waitlist for {self.trip}" @property - def signups(self): + def signups(self) -> QuerySet[SignUp]: """Return signups ordered with the waitlist rules. This method is useful because the SignUp object has the useful information for display, but the WaitListSignup object has information for ordering. """ - # TODO (Django 2): Just use the below, once we can use F-expressions in `ordering` - # return self.unordered_signups.order_by('waitlistsignup') return self.unordered_signups.order_by( F("waitlistsignup__manual_order").desc(nulls_last=True), F("waitlistsignup__time_created").asc(), diff --git a/ws/templatetags/signup_tags.py b/ws/templatetags/signup_tags.py index b21f5366..f8c7cc8b 100644 --- a/ws/templatetags/signup_tags.py +++ b/ws/templatetags/signup_tags.py @@ -2,7 +2,7 @@ from typing import Any from django import template -from django.db.models import Q +from django.db.models import Q, QuerySet from django.forms import HiddenInput import ws.utils.perms as perm_utils @@ -179,7 +179,12 @@ def drop_off_trip(trip, existing_signup): @register.inclusion_tag("for_templatetags/signup_table.html") -def signup_table(signups, has_notes=False, show_drivers=False, all_participants=None): +def signup_table( + signups: QuerySet[models.SignUp], + has_notes: bool = False, + show_drivers: bool = False, + all_participants: QuerySet[models.Participant] | None = None, +) -> dict[str, Any]: """Display a table of signups (either leaders or participants). The all_participants argument is especially useful for including leaders @@ -188,12 +193,18 @@ def signup_table(signups, has_notes=False, show_drivers=False, all_participants= """ # If there are participants not included in the signup list, put these # participants in the front of the list + if all_participants: signed_up = {signup.participant.id for signup in signups} no_signup = all_participants.exclude(id__in=signed_up) fake_signups = [{"participant": leader} for leader in no_signup] - signups = chain(fake_signups, signups) - return {"signups": signups, "has_notes": has_notes, "show_drivers": show_drivers} + # TODO: Combining fake dictionaries & real models isn't smart. + signups = chain(fake_signups, signups) # type: ignore[assignment] + return { + "signups": signups, + "has_notes": has_notes, + "show_drivers": show_drivers, + } @register.inclusion_tag("for_templatetags/trip_summary.html", takes_context=True) diff --git a/ws/templatetags/trip_tags.py b/ws/templatetags/trip_tags.py index 89c14be0..6df7af75 100644 --- a/ws/templatetags/trip_tags.py +++ b/ws/templatetags/trip_tags.py @@ -4,6 +4,7 @@ from typing import TYPE_CHECKING, Any, TypedDict from django import template +from django.contrib.auth.models import User from django.db.models import Case, Count, IntegerField, QuerySet, Sum, When import ws.utils.dates as date_utils @@ -233,47 +234,45 @@ def trip_edit_buttons(trip, participant, user, hide_approve=False): @register.inclusion_tag("for_templatetags/view_trip.html") -def view_trip(trip, participant, user): - # For efficiency, the trip should be called with: - # select_related('info') - # prefetch_related('leaders', 'leaders__leaderrating_set') - - def get_signups(model=models.SignUp): - """Signups, with related fields used in template preselected.""" - signups = model.objects.filter(trip=trip) - signups = signups.select_related("participant", "trip") - return signups.select_related("participant__lotteryinfo") - +def view_trip( + trip: models.Trip, # Should select `info`, prefetch `leaders` & `leaders__leaderrating_set` + participant: models.Participant, + user: User, +) -> dict[str, Any]: trip_leaders = trip.leaders.all() - leader_signups = get_signups(models.LeaderSignUp) - context = { + leader_signups = models.LeaderSignUp.objects.filter(trip=trip).select_related( + "participant", "participant__lotteryinfo", "trip" + ) + signups = models.SignUp.objects.filter(trip=trip).select_related( + "participant", "participant__lotteryinfo", "trip" + ) + wl_signups = trip.waitlist.signups.select_related( + "participant", "participant__lotteryinfo" + ) + return { "trip": trip, "is_trip_leader": perm_utils.leader_on_trip(participant, trip), "viewing_participant": participant, "user": user, + "has_notes": ( + bool(trip.notes) + or any(s.notes for s in signups) + or any(s.notes for s in leader_signups) + ), + "signups": { + "waitlist": wl_signups, + "off_trip": signups.filter(on_trip=False).exclude(pk__in=wl_signups), + "on_trip": signups.filter(on_trip=True), + "leaders_on_trip": [ + s for s in leader_signups if s.participant in trip_leaders + ], + "leaders_off_trip": [ + s for s in leader_signups if s.participant not in trip_leaders + ], + }, + "par_signup": signups.filter(participant=participant).first(), } - signups = get_signups(models.SignUp) - context["par_signup"] = signups.filter(participant=participant).first() - wl_signups = trip.waitlist.signups.select_related( - "participant", "participant__lotteryinfo" - ) - context["signups"] = { - "waitlist": wl_signups, - "off_trip": signups.filter(on_trip=False).exclude(pk__in=wl_signups), - "on_trip": signups.filter(on_trip=True), - "leaders_on_trip": [s for s in leader_signups if s.participant in trip_leaders], - "leaders_off_trip": [ - s for s in leader_signups if s.participant not in trip_leaders - ], - } - context["has_notes"] = ( - bool(trip.notes) - or any(s.notes for s in signups) - or any(s.notes for s in leader_signups) - ) - return context - @register.inclusion_tag("for_templatetags/wimp_trips.html") def wimp_trips(participant, user): diff --git a/ws/utils/signups.py b/ws/utils/signups.py index abed1289..24abb778 100644 --- a/ws/utils/signups.py +++ b/ws/utils/signups.py @@ -2,13 +2,13 @@ from django.contrib import messages from django.db import transaction -from django.db.models import Q +from django.db.models import Q, QuerySet from django.http import HttpRequest from ws import models -def next_in_order(signup, manual_order=None): +def next_in_order(signup: models.SignUp, manual_order: int | None = None) -> None: """Add the signup to the next-ordered spot on the trip or in waitlist. Waitlists are ordered in reverse so that those without a manual override @@ -63,16 +63,24 @@ def add_to_waitlist( def trip_or_wait( - signup, request=None, prioritize=False, top_spot=False, trip_must_be_open=False -): + signup: models.SignUp, + request: HttpRequest | None = None, + prioritize: bool = False, + top_spot: bool = False, + trip_must_be_open: bool = False, +) -> models.SignUp: """Given a signup object, attempt to place the participant on the trip. If the trip is full, instead place that person on the waiting list. - :param request: If given, will supply messages to the request - :param prioritize: Give any waitlist signup priority - :param top_spot: Give any waitlist signup top priority - :param trip_must_be_open: If true, don't sign up on closed trip + Args: + ---- + signup: Signup object to add to the trip *or* the waitlist + request: If given, will supply messages to the request + prioritize: Give any waitlist signup priority + top_spot: Give any waitlist signup top priority + trip_must_be_open: If true, don't sign up on closed trip + """ trip = signup.trip if signup.on_trip: @@ -100,7 +108,7 @@ def trip_or_wait( return signup -def update_queues_if_trip_open(trip): +def update_queues_if_trip_open(trip: models.Trip) -> None: """Update queues if the trip is an open, first-come, first-serve trip. This is intended to be used when the trip size changes (either from changing @@ -119,13 +127,14 @@ def update_queues_if_trip_open(trip): elif diff < 0: # Trip is shrinking, move lowest signups to waitlist for _ in range(abs(diff)): last = trip.signup_set.filter(on_trip=True).last() + assert last is not None # TODO: Don't use implicit ordering, use `latest()` last.on_trip = False last.save() # Make sure they're at the top! add_to_waitlist(last, prioritize=True, top_spot=True) -def non_trip_participants(trip): +def non_trip_participants(trip: models.Trip) -> QuerySet[models.Participant]: """All participants not currently on the given trip.""" all_participants = models.Participant.objects.all() signups = trip.signup_set.filter(on_trip=True)