Fix bug where leader-supplied notes are ignored

As a leader, you can pass participant notes in the form (and the API
response will include those notes!) - we just don't persist them to the
database, for display on the trip itself.

The root cause of this is that I just never invoked `signup.save()`. In
other words, this method has *never* worked from a notes perspective,
and I never tested it (or received a leader report explaining the bug).

While we're at it, change this JSON route to stop returning HTML for the
equivalent of a 403 error (use HTTP response codes & a JSON body).

Shout-out to Bjorn Poonen for this report.

ws/api_views.py

@@ -21,9 +21,10 @@
 import ws.utils.signups as signup_utils
 from ws import enums, models
 from ws.decorators import group_required
+from ws.mixins import JsonTripLeadersOnlyView, TripLeadersOnlyView
 from ws.templatetags.avatar_tags import avatar_url
 from ws.utils.api import jwt_token_from_headers
-from ws.views import AllLeadersView, TripLeadersOnlyView
+from ws.views.leaders import AllLeadersView
 
 
 class SimpleSignupsView(DetailView):
@@ -146,7 +147,7 @@ def post(self, request, *args, **kwargs):
         """
         trip = self.object = self.get_object()
 
-        postdata = json.loads(self.request.body)
+        postdata = json.loads(self.request.body)  # TODO: assumes valid JSON.
         signup_list = postdata.get('signups', [])
         maximum_participants = postdata.get('maximum_participants')
 
@@ -264,7 +265,7 @@ def describe_all_signups(self):
 
 
 class LeaderParticipantSignupView(
-    SingleObjectMixin, FormatSignupMixin, TripLeadersOnlyView
+    SingleObjectMixin, FormatSignupMixin, JsonTripLeadersOnlyView
 ):
     model = models.Trip
 
@@ -278,6 +279,7 @@ def post(self, request, *args, **kwargs):
 
         postdata = json.loads(self.request.body)
         par_pk = postdata.get('participant_id')
+        notes = postdata.get('notes', '')
 
         try:
             par = models.Participant.objects.get(pk=par_pk)
@@ -286,7 +288,7 @@ def post(self, request, *args, **kwargs):
 
         trip = self.get_object()
         signup, created = models.SignUp.objects.get_or_create(
-            trip=trip, participant=par
+            trip=trip, participant=par, defaults={'notes': notes}
         )
 
         if not created:  # (SignUp exists, but participant may not be on trip)
@@ -301,7 +303,6 @@ def post(self, request, *args, **kwargs):
                     {'message': f"{par.name} is already on the {queue}"}, status=409
                 )
 
-        signup.notes = postdata.get('notes', '')
         signup = signup_utils.trip_or_wait(signup)
 
         trip_participants = {
@@ -322,7 +323,7 @@ def post(self, request, *args, **kwargs):
                 'signup': self.describe_signup(signup, trip_participants, other_trips),
                 'on_trip': signup.on_trip,
             },
-            status=201,
+            status=201 if (created or signup.on_trip) else 200,
         )
 
 

ws/mixins.py

@@ -3,7 +3,7 @@
 """
 from django.contrib.auth.decorators import login_required
 from django.db.models import Q
-from django.http import HttpRequest
+from django.http import HttpRequest, JsonResponse
 from django.shortcuts import render
 from django.utils.decorators import method_decorator
 from django.views.generic import View
@@ -69,19 +69,33 @@ def can_set_attendance(self, participant_to_modify) -> bool:
         return participant_to_modify.user_id == self.request.user.id
 
 
+def _allowed_to_modify_trip(trip: models.Trip, request: HttpRequest) -> bool:
+    activity_enum = trip.required_activity_enum()
+    if activity_enum:
+        is_chair = perm_utils.chair_or_admin(request.user, activity_enum)
+    else:  # (There is no required activity, so no chairs. Allow superusers, though)
+        is_chair = request.user.is_superuser
+
+    participant: models.Participant = request.participant  # type: ignore
+    trip_leader = perm_utils.leader_on_trip(participant, trip, True)
+    return is_chair or trip_leader
+
+
 class TripLeadersOnlyView(View):
     @method_decorator(login_required)
     def dispatch(self, request, *args, **kwargs):
         """Only allow creator, leaders of the trip, and chairs."""
+
         trip = self.get_object()
+        if not _allowed_to_modify_trip(trip, request):
+            return render(request, 'not_your_trip.html', {'trip': trip})
+        return super().dispatch(request, *args, **kwargs)
 
-        activity_enum = trip.required_activity_enum()
-        if activity_enum:
-            is_chair = perm_utils.chair_or_admin(request.user, activity_enum)
-        else:  # (There is no required activity, so no chairs. Allow superusers, though)
-            is_chair = request.user.is_superuser
 
-        trip_leader = perm_utils.leader_on_trip(request.participant, trip, True)
-        if not (is_chair or trip_leader):
-            return render(request, 'not_your_trip.html', {'trip': trip})
+class JsonTripLeadersOnlyView(View):
+    @method_decorator(login_required)
+    def dispatch(self, request, *args, **kwargs):
+        """Only allow creator, leaders of the trip, and chairs."""
+        if not _allowed_to_modify_trip(self.get_object(), request):
+            return JsonResponse({"message": "Must be a leader"}, status=403)
         return super().dispatch(request, *args, **kwargs)

ws/tests/views/test_api_views.py

@@ -5,8 +5,9 @@
 from freezegun import freeze_time
 
 import ws.utils.perms as perm_utils
-from ws import enums, settings
+from ws import enums, models, settings
 from ws.tests import TestCase, factories
+from ws.utils.signups import add_to_waitlist
 
 
 class JWTSecurityTest(TestCase):
@@ -383,6 +384,175 @@ def test_exact_id(self):
         )
 
 
+class JsonLeaderParticipantSignupTest(TestCase):
+    def setUp(self):
+        super().setUp()
+        self.trip = factories.TripFactory.create(algorithm='lottery')
+        self.url = f'/trips/{self.trip.pk}/signup/'
+        self.leader = factories.ParticipantFactory.create()
+        self.trip.leaders.add(self.leader)
+        self.client.force_login(self.leader.user)
+
+    def test_just_user(self):
+        """A user without a participant record gets a 403 trying to sign up a user."""
+        user = factories.UserFactory.create()
+        self.client.force_login(user)
+        response = self.client.post(self.url)
+        self.assertEqual(response.status_code, 403)
+
+    def test_unauthenticated(self):
+        """One must obviously be logged in to sign up others."""
+        self.client.logout()
+        response = self.client.post(self.url)
+        # This is a JSON route - it probably shouldn't return with a redirect. Ah, well.
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, f"/accounts/login/?next={self.url}")
+
+    def test_not_a_leader(self):
+        """Participants must be a leader for the trip in question."""
+        par = factories.ParticipantFactory.create()
+        self.client.force_login(par.user)
+        response = self.client.post(self.url)
+        self.assertEqual(response.status_code, 403)
+
+    def test_leader_but_not_on_the_trip(self):
+        """Participants must be a leader for the trip in question."""
+        other_trip = factories.TripFactory.create()
+        response = self.client.post(f'/trips/{other_trip.pk}/signup/')
+        self.assertEqual(response.status_code, 403)
+
+    def test_unknown_target_participant(self):
+        """If the caller is a leader, we'll tell them if the participant isn't found."""
+        response = self.client.post(
+            self.url,
+            {'participant_id': -37},
+            content_type='application/json',
+        )
+        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.json(), {'message': "No participant found"})
+
+    def test_signup_exists_but_not_on_lottery_trip(self):
+        """We won't edit notes and the trip remains in lottery mode."""
+        signup = factories.SignUpFactory.create(
+            on_trip=False, trip=self.trip, notes='original, participant-supplied notes'
+        )
+        response = self.client.post(
+            self.url,
+            {'participant_id': signup.participant.pk, 'notes': 'leader notes'},
+            content_type='application/json',
+        )
+
+        # Participant was already signed up for the lottery
+        self.assertEqual(self.trip.algorithm, 'lottery')
+        self.assertEqual(response.status_code, 200)
+        signup.refresh_from_db()
+        self.assertFalse(signup.on_trip)
+        self.assertEqual(signup.notes, 'original, participant-supplied notes')
+
+    def test_signup_exists_but_not_on_fcfs_trip(self):
+        """We won't edit notes, but we can add the participant to the trip."""
+        signup = factories.SignUpFactory.create(
+            on_trip=False, trip=self.trip, notes='original, participant-supplied notes'
+        )
+
+        self.trip.algorithm = 'fcfs'
+        self.trip.save()
+
+        response = self.client.post(
+            self.url,
+            {'participant_id': signup.participant.pk, 'notes': 'leader notes'},
+            content_type='application/json',
+        )
+
+        self.assertEqual(response.status_code, 201)
+        signup.refresh_from_db()
+        self.assertTrue(signup.on_trip)
+        self.assertEqual(signup.notes, 'original, participant-supplied notes')
+
+    def test_new_signup_to_fcfs_trip(self):
+        """Test the simplest case: adding a participant to a FCFS trip with spaces."""
+        self.trip.algorithm = 'fcfs'
+        self.trip.save()
+
+        par = factories.ParticipantFactory.create()
+
+        response = self.client.post(
+            self.url,
+            {'participant_id': par.pk, 'notes': 'leader notes'},
+            content_type='application/json',
+        )
+
+        self.assertEqual(response.status_code, 201)
+        signup = models.SignUp.objects.get(participant=par, trip=self.trip)
+        self.assertTrue(signup.on_trip)
+        self.assertEqual(signup.notes, 'leader notes')
+
+    def test_add_new_waitlist_entry(self):
+        """We can add a participant to the waitlist for a full trip."""
+        self.trip.algorithm = 'fcfs'
+        self.trip.maximum_participants = 1
+        self.trip.save()
+
+        # Trip is full now
+        factories.SignUpFactory.create(trip=self.trip)
+
+        par = factories.ParticipantFactory.create()
+
+        response = self.client.post(
+            self.url,
+            {'participant_id': par.pk},
+            content_type='application/json',
+        )
+
+        self.assertEqual(response.status_code, 201)
+        signup = models.SignUp.objects.get(participant=par, trip=self.trip)
+        self.assertFalse(signup.on_trip)
+        self.assertTrue(signup.waitlistsignup)
+        self.assertEqual(signup.notes, '')
+
+    def test_already_on_trip(self):
+        """Leaders can't add a participant already on the trip!"""
+        signup = factories.SignUpFactory.create(
+            trip=self.trip, on_trip=True, participant__name='Abdul McTest'
+        )
+
+        response = self.client.post(
+            self.url,
+            {'participant_id': signup.participant_id},
+            content_type='application/json',
+        )
+
+        self.assertEqual(response.status_code, 409)
+        self.assertEqual(
+            response.json(), {'message': "Abdul McTest is already on the trip"}
+        )
+
+    def test_already_on_waitlist(self):
+        """Leaders can't add a participant already on the waitlist!"""
+        self.trip.algorithm = 'fcfs'
+        self.trip.maximum_participants = 1
+        self.trip.save()
+
+        # Trip is full now
+        factories.SignUpFactory.create(trip=self.trip)
+
+        signup = factories.SignUpFactory.create(
+            trip=self.trip, participant__name='Jane McJaney'
+        )
+        add_to_waitlist(signup)
+
+        response = self.client.post(
+            self.url,
+            {'participant_id': signup.participant_id},
+            content_type='application/json',
+        )
+
+        self.assertEqual(response.status_code, 409)
+        self.assertEqual(
+            response.json(), {'message': "Jane McJaney is already on the waitlist"}
+        )
+
+
 class ApproveTripViewTest(TestCase):
     def setUp(self):
         super().setUp()