Refactor code slightly, add type annotations

Let's expand mypy coverage, by adding types to bits of code which could
benefit from more robust checking. As a result, I can refactor out some
extraneous variable assignments that were made purely to keep lines
short.

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,15 +251,18 @@ 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)
 
         # Clear the trip first (delete removals, set others to not on 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()

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(),

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)

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):

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)