Use a revision counter to prevent stale trip edits

This should hopefully guard against a few rare edge cases when multiple
leaders are editing the same trip at the same time!

ws/forms.py

@@ -33,6 +33,10 @@ def _bind_input(
     field = form.fields[field_name]
     initial = field.initial if initial is None else initial
 
+    if field_name in form.data:
+        # (e.g. an invalid POST, we tell Angular about the submitted value)
+        initial = form.data[field_name]
+
     model_name = model_name or field_name
     field.widget.attrs['data-ng-model'] = model_name
 

ws/templates/trips/__create_or_edit.html

@@ -10,6 +10,7 @@
 
 <form name="{{ form.form_name }}" method="post" action=".">
   {% csrf_token %}
+  {{ form.edit_revision }}
   <div class="row">
     <div class="col-md-6">
       <h2>Basics</h2>

ws/tests/views/test_trips.py

@@ -19,7 +19,10 @@ def _form_data(form):
             yield elem['name'], elem.text
 
         for elem in form.find_all('input'):
-            yield elem['name'], elem.get('value', '')
+            if elem['type'] == 'checkbox' and elem.get('checked') is not None:
+                yield elem['name'], 'on'
+            else:
+                yield elem['name'], elem.get('value', '')
 
         for select in form.find_all('select'):
             selection = select.find('option', selected=True)
@@ -256,6 +259,8 @@ def test_creation(self):
 
         trip = models.Trip.objects.get(pk=trip_pk)
         self.assertEqual(trip.creator, trip_leader)
+        self.assertEqual(trip.last_updated_by, trip_leader)
+        self.assertEqual(trip.edit_revision, 0)
         self.assertEqual(trip.name, 'My Great Trip')
 
 
@@ -398,6 +403,48 @@ def test_update_rescinds_approval(self):
         trip = models.Trip.objects.get(pk=trip.pk)
         self.assertFalse(trip.chair_approved)
 
+    @freeze_time("2019-02-15 12:25:00 EST")
+    def test_updates_on_stale_trips(self):
+        leader = factories.ParticipantFactory.create()
+        self.client.force_login(leader.user)
+        factories.LeaderRatingFactory.create(
+            participant=leader, activity=enums.Activity.CLIMBING.value
+        )
+        trip = factories.TripFactory.create(
+            edit_revision=0,
+            creator=leader,
+            program=enums.Program.CLIMBING.value,
+            level=None,
+            trip_date=date(2019, 3, 2),
+        )
+
+        # Simulate a stale page content by loading data *first*
+        _edit_resp, initial_soup = self._get(f'/trips/{trip.pk}/edit/')
+        form_data = dict(self._form_data(initial_soup.find('form')))
+
+        # (Pretend that two others have updated edited the trip)
+        trip.edit_revision += 2
+        trip.leaders.add(factories.ParticipantFactory.create())
+        trip.description = 'Other edits changed this description!'
+        trip.last_updated_by = factories.ParticipantFactory.create(name='Joe Schmoe')
+        trip.save()
+
+        resp = self.client.post(f'/trips/{trip.pk}/edit/', form_data, follow=False)
+        soup = BeautifulSoup(resp.content, 'html.parser')
+        warning = strip_whitespace(soup.find(class_='alert alert-danger').text)
+        self.assertIn(
+            'This trip has already been edited 2 times, most recently by Joe Schmoe.',
+            warning,
+        )
+        self.assertIn(
+            'To make updates to the trip, please load the page again.', warning
+        )
+        self.assertIn('Fields which differ: Description, Leaders', warning)
+
+        # No edit was made; we have form errors
+        trip.refresh_from_db()
+        self.assertEqual(trip.edit_revision, 2)
+
 
 class ApproveTripsViewTest(TestCase):
     def setUp(self):

ws/views/trips.py

@@ -6,10 +6,13 @@
 """
 from collections import defaultdict
 from datetime import date, timedelta
+from typing import Optional
 
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
 from django.core.exceptions import PermissionDenied
+from django.db import transaction
+from django.forms.utils import ErrorList
 from django.http import Http404, HttpResponseBadRequest
 from django.shortcuts import redirect, render
 from django.urls import reverse, reverse_lazy
@@ -342,6 +345,11 @@ def get_context_data(self, **kwargs):
     def get_success_url(self):
         return reverse('view_trip', args=(self.object.pk,))
 
+    def _leaders_changed(self, form) -> bool:
+        old_pks = {leader.pk for leader in self.object.leaders.all()}
+        new_pks = {leader.pk for leader in form.cleaned_data['leaders']}
+        return bool(old_pks.symmetric_difference(new_pks))
+
     def _ignore_leaders_if_unchanged(self, form):
         """Don't update the leaders m2m field if unchanged.
 
@@ -352,21 +360,65 @@ def _ignore_leaders_if_unchanged(self, form):
         A compromise: Only send emails when the leader list is changed.
         See ticket 6707 for an eventual fix to this behavior
         """
-        old_pks = {leader.pk for leader in self.object.leaders.all()}
-        new_pks = {leader.pk for leader in form.cleaned_data['leaders']}
-        if not old_pks.symmetric_difference(new_pks):
+        if not self._leaders_changed(form):
             form.cleaned_data.pop('leaders')
 
+    def _stale_revision_message(self, form, current_trip, new_trip) -> Optional[str]:
+        """Produce a message describing a stale edit, if one exists.."""
+        if current_trip.edit_revision == new_trip.edit_revision:
+            return None
+
+        fields_with_difference = {
+            field
+            for name, field in form.fields.items()
+            if name != 'edit_revision'
+            and getattr(current_trip, name) != getattr(new_trip, name)
+        }
+        # (Account for the fact that we might have stripped `leaders`)
+        if 'leaders' in form.cleaned_data and self._leaders_changed(form):
+            fields_with_difference.add(form.fields['leaders'])
+
+        if current_trip.last_updated_by is None:
+            # This shouldn't ever happen, but the data model makes it possible
+            editor_name = "an unknown user"
+        elif current_trip.last_updated_by == self.request.participant:  # type: ignore
+            editor_name = "you"
+        else:
+            editor_name = current_trip.last_updated_by.name
+
+        assert current_trip.edit_revision > new_trip.edit_revision
+        edit_count = current_trip.edit_revision - new_trip.edit_revision
+        plural = '' if edit_count == 1 else 's'
+        return "\n".join(
+            [
+                f"This trip has already been edited {edit_count} time{plural}, most recently by {editor_name}.",
+                "To make updates to the trip, please load the page again.",
+                f"Fields which differ: {', '.join(field.label for field in fields_with_difference) or '???'}",
+            ]
+        )
+
     def form_valid(self, form):
         self._ignore_leaders_if_unchanged(form)
 
         trip = form.save(commit=False)
-        trip.last_updated_by = self.request.participant
         if self.update_rescinds_approval:
             trip.chair_approved = False
 
         trip.activity = trip.get_legacy_activity()
-        return super().form_valid(form)
+
+        # Make sure that nobody else edits the trip while doing this comparison!
+        # (We do this here instead of in form `clean` so we can guarantee lock at save)
+        with transaction.atomic():
+            current_trip = models.Trip.objects.select_for_update().get(pk=trip.pk)
+
+            stale_msg = self._stale_revision_message(form, current_trip, trip)
+            if stale_msg:
+                form.errors['__all__'] = ErrorList([stale_msg])
+                return self.form_invalid(form)
+
+            trip.last_updated_by = self.request.participant
+            trip.edit_revision += 1
+            return super().form_valid(form)
 
 
 class TripListView(ListView):