diff --git a/ws/api_views.py b/ws/api_views.py index c31ec2b1..c4b96f1b 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, 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,11 +275,17 @@ 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 @@ -285,7 +293,12 @@ def get_signups(self) -> QuerySet[models.SignUp]: "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}, diff --git a/ws/models.py b/ws/models.py index b31d0545..f8b0e59a 100644 --- a/ws/models.py +++ b/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,23 +1672,22 @@ 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 @@ -1702,8 +1695,6 @@ def last_of_priority(self): 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)) diff --git a/ws/templates/for_templatetags/view_trip.html b/ws/templates/for_templatetags/view_trip.html index d87c4ca8..187a8506 100644 --- a/ws/templates/for_templatetags/view_trip.html +++ b/ws/templates/for_templatetags/view_trip.html @@ -47,7 +47,7 @@

Participants ({{ signups.on_trip.count }} / {{ trip.maximum_participants }}) {% if signups.waitlist %} -

Waiting List ({{ signups.waitlist.count }})

+

Waiting List ({{ signups.waitlist | length }})

{% signup_table signups.waitlist has_notes show_drivers=viewing_participant.is_leader %} {% endif %} diff --git a/ws/tests/views/test_api_views.py b/ws/tests/views/test_api_views.py index fc062ccb..5bebe536 100644 --- a/ws/tests/views/test_api_views.py +++ b/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()