Fix ordering of waitlist in API route

Bjorn pointed out a weird side effect where after setting `manual_order`
on a waitlist, *new* signups (which will have a null manual order) can
end up earlier on the waiting list, when they should instead appear at
the end of the waitlist. We've been properly handling that in the
Django-based views into waitlist ordering, but we've been reporting a
totally different ordering for the API-based view of trips.

In short, this happened because we were expecting the waitlist models to
define their own ordering, but we explicitly use the *preferred*
ordering elsewhere.

Also, let's fix the API route to start returning JSON responses when the
user lacks permissions.

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, QuerySet
+from django.db.models import F, Q, QuerySet
 from django.http import HttpRequest, JsonResponse
 from django.utils.decorators import method_decorator
 from django.views.decorators.csrf import csrf_exempt
@@ -27,7 +27,7 @@
 from ws import enums, models
 from ws.decorators import group_required
 from ws.middleware import RequestWithParticipant
-from ws.mixins import JsonTripLeadersOnlyView, TripLeadersOnlyView
+from ws.mixins import JsonTripLeadersOnlyView
 from ws.utils import membership_api
 from ws.utils.api import jwt_token_from_headers
 from ws.utils.feedback import feedback_is_recent
@@ -43,7 +43,7 @@ class SimpleSignupsView(DetailView):
 
     model = models.Trip
 
-    def get(self, request, *args, **kwargs):
+    def get(self, request: HttpRequest, *args: Any, **kwargs: Any) -> JsonResponse:
         trip = self.get_object()
 
         on_trip = trip.signed_up_participants.filter(signup__on_trip=True)
@@ -54,13 +54,13 @@ def get(self, request, *args, **kwargs):
                 for s in trip.waitlist.signups.select_related("participant")
             ],
         }
-        participant_signups = {}
-        for key, participants in signups.items():
-            participant_signups[key] = [{"participant": par} for par in participants]
 
         return JsonResponse(
             {
-                "signups": participant_signups,
+                "signups": {
+                    key: [{"participant": par} for par in participants]
+                    for key, participants in signups.items()
+                },
                 "leaders": list(trip.leaders.values("name", "email")),
                 "creator": {"name": trip.creator.name, "email": trip.creator.email},
             }
@@ -157,7 +157,9 @@ class JsonSignup(TypedDict):
     deleted: NotRequired["bool"]
 
 
-class AdminTripSignupsView(SingleObjectMixin, FormatSignupMixin, TripLeadersOnlyView):
+class AdminTripSignupsView(
+    SingleObjectMixin, FormatSignupMixin, JsonTripLeadersOnlyView
+):
     model = models.Trip
 
     def get(self, request: HttpRequest, *args: Any, **kwargs: Any) -> JsonResponse:
@@ -273,19 +275,30 @@ def update_signups(self, signup_list: list[JsonSignup], trip: models.Trip) -> No
             signup_utils.next_in_order(signup, order)
 
     def get_signups(self) -> QuerySet[models.SignUp]:
-        """Trip signups with selected models for use in describe_signup."""
+        """Trip signups with selected models for use in describe_signup.
+
+        Unlike in normal Django template uses, this method reports signups that
+        are on the trip in the same collection as signups which represent a
+        spot on the waitlist.
+        """
         trip: models.Trip = self.get_object()
         return (
             trip.on_trip_or_waitlisted.select_related(
-                "participant", "participant__lotteryinfo__paired_with__lotteryinfo"
+                "participant",
+                "participant__lotteryinfo__paired_with__lotteryinfo",
             )
             .prefetch_related(
                 # Older feedback should still be filtered out
                 "participant__feedback_set",
                 "participant__feedback_set__leader",
                 "participant__feedback_set__trip",
             )
-            .order_by("-on_trip", "waitlistsignup", "last_updated")
+            .order_by(
+                "-on_trip",
+                F("waitlistsignup__manual_order").desc(nulls_last=True),
+                F("waitlistsignup__time_created").asc(),
+                "last_updated",
+            )
         )
 
     def describe_all_signups(self) -> dict[str, Any]:
@@ -301,7 +314,7 @@ def describe_all_signups(self) -> dict[str, Any]:
                 self.describe_signup(
                     s, trip_participants, other_trips_by_par[s.participant_id]
                 )
-                for s in self.get_signups()
+                for s in signups
             ],
             "leaders": list(trip.leaders.values("name", "email")),
             "creator": {"name": trip.creator.name, "email": trip.creator.email},

ws/models.py

@@ -920,6 +920,8 @@ class SignUp(BaseSignUp):
     """
 
     order = models.IntegerField(null=True, blank=True)  # As ranked by participant
+    # TODO: Make this *ascending* (so 1 is first, 2 is second)
+    # Right now, it's descending - which is both confusing & conflicts with SignUp
     manual_order = models.IntegerField(null=True, blank=True)  # Order on trip
 
     on_trip = models.BooleanField(default=False, db_index=True)
@@ -1646,18 +1648,10 @@ class WaitListSignup(models.Model):
     signup = models.OneToOneField(SignUp, on_delete=models.CASCADE)
     waitlist = models.ForeignKey("WaitList", on_delete=models.CASCADE)
     time_created = models.DateTimeField(auto_now_add=True)
+
     # Specify to override ordering by time created
-    # TODO: Make this *ascending* (so 1 is first, 2 is second)
-    # Right now, it's descending - which is both confusing & conflicts with SignUp
     manual_order = models.IntegerField(null=True, blank=True)
 
-    class Meta:
-        # TODO (Django 2): Use F-expressions. [F('manual_order').desc(nulls_last=True), ...]
-        # TODO (Django 2): Also update WaitList.signups property definition
-        # WARNING: Postgres will put nulls first, not last.
-        ordering = ["-manual_order", "time_created", "pk"]  # NOT CORRECT!
-        # WARNING: This default ordering is not fully correct. Manually call `order_by`)
-
     def __str__(self) -> str:
         return f"{self.signup.participant.name} waitlisted on {self.signup.trip}"
 
@@ -1678,32 +1672,29 @@ def signups(self) -> QuerySet[SignUp]:
         This method is useful because the SignUp object has the useful information
         for display, but the WaitListSignup object has information for ordering.
         """
+        # Also see `AdminTripSignupsView.get_signups()`
         return self.unordered_signups.order_by(
             F("waitlistsignup__manual_order").desc(nulls_last=True),
             F("waitlistsignup__time_created").asc(),
         )
 
     @property
-    def first_of_priority(self):
+    def first_of_priority(self) -> int:
         """The 'manual_order' value to be first in the waitlist."""
-        # TODO (Django 2): Just use the below, refactor code to avoid extra lookups
-        # first_wl_signup = self.waitlistsignup_set.first()
         first_signup = self.signups.first()
         if first_signup is None:
             return 10
         return (first_signup.waitlistsignup.manual_order or 0) + 1
 
     @property
-    def last_of_priority(self):
+    def last_of_priority(self) -> int:
         """The 'manual_order' value to be below all manual orders, but above non-ordered.
 
         Waitlist signups are ordered first by `manual_order`, then by time created. This
         method is useful for the scenario when you want to give somebody priority in the
         waitlist, but to not surpass others who were previously added to the top of the
         waitlist.
         """
-        # TODO (Django 2): Just use the below
-        # last_wl_signup = self.waitlistsignup_set.filter(manual_order__isnull=False).last()
         last_wl_signup = (
             self.waitlistsignup_set.filter(manual_order__isnull=False)
             .order_by(F("manual_order").desc(nulls_last=True))

ws/templates/for_templatetags/view_trip.html

@@ -47,7 +47,7 @@ <h3>Participants ({{ signups.on_trip.count }} / {{ trip.maximum_participants }})
   </email-trip-members>
 
   {% if signups.waitlist %}
-    <h3>Waiting List ({{ signups.waitlist.count }}) </h3>
+    <h3>Waiting List ({{ signups.waitlist | length }}) </h3>
     {% signup_table signups.waitlist has_notes show_drivers=viewing_participant.is_leader %}
   {% endif %}
 

ws/tests/views/test_api_views.py

@@ -556,6 +556,173 @@ def test_already_on_waitlist(self):
         )
 
 
+class AdminTripSignupsViewTest(TestCase):
+    maxDiff = None
+
+    def setUp(self) -> None:
+        super().setUp()
+        self.user = factories.UserFactory.create()
+        self.client.force_login(self.user)
+
+    def test_not_leader(self) -> None:
+        trip = factories.TripFactory()
+        resp = self.client.get(f"/trips/{trip.pk}/admin/signups/")
+        self.assertEqual(resp.status_code, 403)
+
+    # Note to future me -- this test relies on signals creating waitlist signups
+    # Ideally, we can change to just invoking `trip_or_wait`
+    def test_signup_ordering(self) -> None:
+        par = factories.ParticipantFactory.create(name="Tim B", user=self.user)
+        trip = factories.TripFactory(
+            algorithm="fcfs", creator=par, maximum_participants=2
+        )
+
+        first_on_trip = factories.SignUpFactory(trip=trip, notes="Hi")
+        second_on_trip = factories.SignUpFactory(trip=trip)
+
+        # Two signups come in, but the leader manually ordered them!
+        second_on_waitlist = factories.SignUpFactory(trip=trip, on_trip=False)
+        second_on_waitlist.waitlistsignup.manual_order = -3
+        second_on_waitlist.waitlistsignup.save()
+
+        top_of_waitlist = factories.SignUpFactory(trip=trip, on_trip=False)
+        top_of_waitlist.waitlistsignup.manual_order = -2
+        top_of_waitlist.waitlistsignup.save()
+
+        # This represents somebody who added themselves to the waitlist
+        # Previously, they would appear at the *top* of the list!
+        bottom_of_waitlist = factories.SignUpFactory(trip=trip)
+
+        self.assertEqual(
+            list(trip.waitlist.signups),
+            [top_of_waitlist, second_on_waitlist, bottom_of_waitlist],
+        )
+
+        resp = self.client.get(f"/trips/{trip.pk}/admin/signups/")
+        expected = {
+            "signups": [
+                {
+                    "id": first_on_trip.pk,
+                    "participant": {
+                        "id": first_on_trip.participant.pk,
+                        "name": mock.ANY,
+                        "email": mock.ANY,
+                    },
+                    "missed_lectures": True,
+                    "feedback": [],
+                    "also_on": [],
+                    "paired_with": None,
+                    "car_status": None,
+                    "number_of_passengers": None,
+                    "notes": "Hi",
+                },
+                {
+                    "id": second_on_trip.pk,
+                    "participant": {
+                        "id": second_on_trip.participant.pk,
+                        "name": mock.ANY,
+                        "email": mock.ANY,
+                    },
+                    "missed_lectures": True,
+                    "feedback": [],
+                    "also_on": [],
+                    "paired_with": None,
+                    "car_status": None,
+                    "number_of_passengers": None,
+                    "notes": "",
+                },
+                {
+                    "id": top_of_waitlist.pk,
+                    "participant": {
+                        "id": top_of_waitlist.participant.pk,
+                        "name": mock.ANY,
+                        "email": mock.ANY,
+                    },
+                    "missed_lectures": True,
+                    "feedback": [],
+                    "also_on": [],
+                    "paired_with": None,
+                    "car_status": None,
+                    "number_of_passengers": None,
+                    "notes": "",
+                },
+                {
+                    "id": second_on_waitlist.pk,
+                    "participant": {
+                        "id": second_on_waitlist.participant.pk,
+                        "name": mock.ANY,
+                        "email": mock.ANY,
+                    },
+                    "missed_lectures": True,
+                    "feedback": [],
+                    "also_on": [],
+                    "paired_with": None,
+                    "car_status": None,
+                    "number_of_passengers": None,
+                    "notes": "",
+                },
+                {
+                    "id": bottom_of_waitlist.pk,
+                    "participant": {
+                        "id": bottom_of_waitlist.participant.pk,
+                        "name": mock.ANY,
+                        "email": mock.ANY,
+                    },
+                    "missed_lectures": True,
+                    "feedback": [],
+                    "also_on": [],
+                    "paired_with": None,
+                    "car_status": None,
+                    "number_of_passengers": None,
+                    "notes": "",
+                },
+            ],
+            "leaders": [],
+            "creator": {"name": "Tim B", "email": mock.ANY},
+        }
+        self.assertEqual(resp.json(), expected)
+        simple_resp = self.client.get(f"/trips/{trip.pk}/signups/")
+        self.assertEqual(
+            simple_resp.json()["signups"],
+            {
+                "onTrip": [
+                    {
+                        "participant": {
+                            "name": first_on_trip.participant.name,
+                            "email": first_on_trip.participant.email,
+                        }
+                    },
+                    {
+                        "participant": {
+                            "name": second_on_trip.participant.name,
+                            "email": second_on_trip.participant.email,
+                        }
+                    },
+                ],
+                "waitlist": [
+                    {
+                        "participant": {
+                            "name": top_of_waitlist.participant.name,
+                            "email": top_of_waitlist.participant.email,
+                        }
+                    },
+                    {
+                        "participant": {
+                            "name": second_on_waitlist.participant.name,
+                            "email": second_on_waitlist.participant.email,
+                        }
+                    },
+                    {
+                        "participant": {
+                            "name": bottom_of_waitlist.participant.name,
+                            "email": bottom_of_waitlist.participant.email,
+                        }
+                    },
+                ],
+            },
+        )
+
+
 class ApproveTripViewTest(TestCase):
     def setUp(self):
         super().setUp()