#4031: Patch to help prevent stale domain status saves [dg]

src/registrar/admin.py

@@ -13,6 +13,7 @@
 )
 
 from django.db.models.functions import Concat, Coalesce
+from django.core.exceptions import ValidationError
 from django.http import HttpResponseRedirect
 from registrar.models.federal_agency import FederalAgency
 from registrar.models.portfolio_invitation import PortfolioInvitation
@@ -71,6 +72,7 @@
 from django.contrib.admin.widgets import FilteredSelectMultiple
 from django.utils.html import format_html
 from django.utils.translation import gettext_lazy as _
+from django.utils.dateparse import parse_datetime
 
 
 logger = logging.getLogger(__name__)
@@ -3088,6 +3090,20 @@ def get_fieldsets(self, request, obj=None):
             modified_fieldsets.append((name, {**data, "fields": fields}))
         return modified_fieldsets
 
+    def _has_version_conflict(self, request, obj):
+        """
+        Returns True if there is a version conflict, False otherwise.
+        """
+        version = request.POST.get("version")
+        snap = parse_datetime(version) if version else None
+        current = type(obj).objects.only("updated_at").filter(pk=obj.pk).values_list("updated_at", flat=True).first()
+
+        if snap and current and current != snap:
+            return True
+        else:
+            obj._original_updated_at = snap
+            return False
+
     # Trigger action when a fieldset is changed
     def save_model(self, request, obj, form, change):
         """Custom save_model definition that handles edge cases"""
@@ -3118,8 +3134,12 @@ def save_model(self, request, obj, form, change):
 
             return None
 
-        # == Check if we're making a change or not == #
+        # == OPTIMISTIC LOCK: Temp fix check for version conflict == #
+        if self._has_version_conflict(request, obj):
+            request._domain_request_version_conflict = True
+            return None
 
+        # == Check if we're making a change or not == #
         # If we're not making a change (adding a record), run save model as we do normally
         if not change:
             return super().save_model(request, obj, form, change)
@@ -3147,6 +3167,13 @@ def save_model(self, request, obj, form, change):
             if should_save:
                 return super().save_model(request, obj, form, change)
 
+    def response_change(self, request, obj):
+        if getattr(request, "_domain_request_version_conflict", False):
+            change_url = reverse("admin:registrar_domainrequest_change", args=[obj.pk])
+            self.message_user(request, "A newer version of this form exists. Please try again.", messages.ERROR)
+            return HttpResponseRedirect(change_url)
+        return super().response_change(request, obj)
+
     def _handle_custom_emails(self, obj):
         if obj.status == DomainRequest.DomainRequestStatus.ACTION_NEEDED:
             if obj.action_needed_reason and not obj.action_needed_reason_email:
@@ -4310,6 +4337,12 @@ def do_extend_expiration_date(self, request, obj):
                 "Error connecting to the registry. No expiration date was found.",
                 messages.ERROR,
             )
+        except ValidationError:
+            self.message_user(
+                request,
+                "A newer version of this form exists. Please try again.",
+                messages.ERROR,
+            )
         except Exception as err:
             logger.error(err, stack_info=True)
             self.message_user(request, "Could not delete: An unspecified error occured", messages.ERROR)
@@ -4333,7 +4366,7 @@ def do_delete_domain(self, request, obj):
 
         try:
             obj.deletedInEpp()
-            obj.save()
+            obj.save(optimistic_lock=True)
         except RegistryError as err:
             # Using variables to get past the linter
             message1 = f"Cannot delete Domain when in state {obj.state}"
@@ -4369,6 +4402,8 @@ def do_delete_domain(self, request, obj):
                     ),
                     messages.ERROR,
                 )
+        except ValidationError:
+            self.message_user(request, "A newer version of this form exists. Please try again.", messages.ERROR)
         except Exception:
             self.message_user(
                 request,
@@ -4396,9 +4431,12 @@ def do_get_status(self, request, obj):
         return HttpResponseRedirect(".")
 
     def do_place_client_hold(self, request, obj):
+
         try:
             obj.place_client_hold()
-            obj.save()
+            obj.save(optimistic_lock=True)
+        except ValidationError:
+            self.message_user(request, "A newer version of this form exists. Please try again.", messages.ERROR)
         except Exception as err:
             # if error is an error from the registry, display useful
             # and readable error
@@ -4427,7 +4465,9 @@ def do_place_client_hold(self, request, obj):
     def do_remove_client_hold(self, request, obj):
         try:
             obj.revert_client_hold()
-            obj.save()
+            obj.save(optimistic_lock=True)
+        except ValidationError:
+            self.message_user(request, "A newer version of this form exists. Please try again.", messages.ERROR)
         except Exception as err:
             # if error is an error from the registry, display useful
             # and readable error

src/registrar/forms/domain_request_wizard.py

@@ -580,7 +580,7 @@ def to_database(self, obj):
             else:
                 requested_domain = DraftDomain.objects.create(name=f"{domain}.gov")
                 obj.requested_domain = requested_domain
-                obj.save()
+                obj.save(optimistic_lock=True)
 
         obj.save()
 

src/registrar/forms/utility/wizard_form_helper.py

@@ -33,7 +33,11 @@ def to_database(self, obj: DomainRequest | Contact):
             return
         for name, value in self.cleaned_data.items():
             setattr(obj, name, value)
-        obj.save()
+
+        if isinstance(obj, DomainRequest):
+            obj.save(optimistic_lock=True)
+        else:
+            obj.save()
 
     @classmethod
     def from_database(cls, obj: DomainRequest | Contact | None):
@@ -101,7 +105,7 @@ def _to_database(
         """
         if not self.is_valid():
             return
-        obj.save()
+        obj.save(optimistic_lock=True)
 
         query = getattr(obj, join).order_by("created_at").all()  # order matters
 
@@ -209,7 +213,7 @@ def to_database(self, obj):
         else:
             for name, value in self.cleaned_data.items():
                 setattr(obj, name, value)
-        obj.save()
+        obj.save(optimistic_lock=True)
 
 
 class BaseYesNoForm(RegistrarForm):

src/registrar/models/domain.py

@@ -10,6 +10,7 @@
 from django_fsm import FSMField, transition, TransitionNotAllowed  # type: ignore
 from django.utils import timezone
 from functools import cached_property
+from django.core.exceptions import ValidationError
 from typing import Any
 from registrar.models.domain_invitation import DomainInvitation
 from registrar.models.host import Host
@@ -88,6 +89,7 @@ class Meta:
     def __init__(self, *args, **kwargs):
         self._cache = {}
         super(Domain, self).__init__(*args, **kwargs)
+        self._original_updated_at = self.__dict__.get("updated_at", None)
 
     class Status(models.TextChoices):
         """
@@ -241,12 +243,19 @@ def __delete__(self, obj):
             """Called during delete. Example: `del domain.registrant`."""
             super().__delete__(obj)
 
-    def save(self, force_insert=False, force_update=False, using=None, update_fields=None):
+    def save(self, force_insert=False, force_update=False, using=None, update_fields=None, optimistic_lock=False):
+        # -------- Optimistic locking (quick-fix) --------
+        if optimistic_lock and self.pk:
+            current_updated_at = type(self).objects.only("updated_at").get(pk=self.pk).updated_at
+            if getattr(self, "_original_updated_at", None) and current_updated_at != self._original_updated_at:
+                raise ValidationError("A newer version of this form exists. Please try again.")
+
         # If the domain is deleted we don't want the expiration date to be set
         if self.state == self.State.DELETED and self.expiration_date:
             self.expiration_date = None
 
         super().save(force_insert, force_update, using, update_fields)
+        self._original_updated_at = self.updated_at
 
     @classmethod
     def available(cls, domain: str) -> bool:
@@ -369,7 +378,9 @@ def registry_expiration_date(self, ex_date: date):
         To update the expiration date, use renew_domain method."""
         raise NotImplementedError()
 
-    def renew_domain(self, length: int = 1, unit: epp.Unit = epp.Unit.YEAR):
+    def renew_domain(
+        self, length: int = 1, unit: epp.Unit = epp.Unit.YEAR, persist: bool = True, optimistic_lock: bool = False
+    ) -> bool:
         """
         Renew the domain to a length and unit of time relative to the current
         expiration date.
@@ -392,8 +403,13 @@ def renew_domain(self, length: int = 1, unit: epp.Unit = epp.Unit.YEAR):
             # expiration date in the registrar, and in the cache
             self._cache["ex_date"] = registry.send(request, cleaned=True).res_data[0].ex_date
             self.expiration_date = self._cache["ex_date"]
-            self.save()
-
+            if persist:
+                if optimistic_lock:
+                    self.save(optimistic_lock=True)
+                else:
+                    self.save()
+                return True
+            return False
         except RegistryError as err:
             # if registry error occurs, log the error, and raise it as well
             logger.error(f"Registry error renewing domain '{self.name}': {err}")

src/registrar/models/domain_request.py

@@ -723,6 +723,7 @@ def _cache_status_and_status_reasons(self):
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
+        self._original_updated_at = self.__dict__.get("updated_at", None)
         # Store original values for caching purposes. Used to compare them on save.
         self._cache_status_and_status_reasons()
 
@@ -779,16 +780,29 @@ def clean(self):
                         )
                 raise ValidationError(errors)
 
-    def save(self, *args, **kwargs):
+    def save(self, *args, optimistic_lock=False, **kwargs):
         """Save override for custom properties"""
+        if optimistic_lock and self.pk is not None:
+            # Get the current DB value to compare with our snapshot
+            current_updated_at = (
+                type(self).objects.only("updated_at").filter(pk=self.pk).values_list("updated_at", flat=True).first()
+            )
+            # If someone else saved after we loaded, block the save
+            if (
+                self._original_updated_at is not None
+                and current_updated_at is not None
+                and current_updated_at != self._original_updated_at
+            ):
+                raise ValidationError("A newer version of this form exists. Please try again.")
+
         self.sync_organization_type()
         self.sync_yes_no_form_fields()
 
         if self._cached_status != self.status:
             self.last_status_update = timezone.now().date()
 
         super().save(*args, **kwargs)
-
+        self._original_updated_at = self.updated_at
         # Handle custom status emails.
         # An email is sent out when a, for example, action_needed_reason is changed or added.
         statuses_that_send_custom_emails = [self.DomainRequestStatus.ACTION_NEEDED, self.DomainRequestStatus.REJECTED]

src/registrar/templates/django/admin/domain_change_form.html

@@ -44,7 +44,6 @@
             </div>
         </div>
     {% endif %}
-
     {% for fieldset in adminform %}
         {% include "django/admin/includes/domain_fieldset.html" with state_help_message=state_help_message %}
     {% endfor %}

src/registrar/templates/django/admin/domain_request_change_form.html

@@ -10,6 +10,7 @@
 {% endblock content %}
 
 {% block field_sets %}
+    <input type="hidden" name="version" value="{{ original.updated_at|date:'c' }}">
     {# Create an invisible <a> tag so that we can use a click event to toggle the modal. #}
     <a id="invisible-ineligible-modal-toggler" class="display-none" href="#toggle-set-ineligible" aria-controls="toggle-set-ineligible" data-open-modal></a>
     {# Store the current object id so we can access it easier #}

src/registrar/templates/domain_request_form.html

@@ -85,6 +85,7 @@ <h1> {{form_titles|get_item:steps.current}} </h1>
 {% block form_fields %}{% endblock %}
 
 {% block form_buttons %}
+        <input type="hidden" name="version" value="{{ version_token }}">
           <div class="stepnav">
             {% if steps.prev %}
             <button

src/registrar/tests/test_views_request.py

@@ -743,22 +743,25 @@ def test_domain_request_form_submission_incomplete(self):
         requirements_page = additional_details_result.follow()
         requirements_form = requirements_page.forms[0]
 
-        requirements_form["requirements-is_policy_acknowledged"] = True
-
         # Before we go to the review page, let's remove some of the data from the request:
         domain_request = DomainRequest.objects.get()  # there's only one
 
         domain_request.generic_org_type = None
         domain_request.save()
 
-        # test next button
+        # Refresh the Requirements page so snapshot matches the new updated_at
+        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
+        requirements_page = self.app.get(reverse("domain-request:requirements", args=[domain_request.id]))
+        requirements_form = requirements_page.forms[0]
+        requirements_form["requirements-is_policy_acknowledged"] = True
+
+        # Submit and test next button
         self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
         requirements_result = requirements_form.submit()
         # validate that data from this step are being saved
-
         domain_request.refresh_from_db()
-
         self.assertEqual(domain_request.is_policy_acknowledged, True)
+
         # the post request should return a redirect to the next form in
         # the domain request page
         self.assertEqual(requirements_result.status_code, 302)
@@ -2415,7 +2418,8 @@ def test_domain_request_so_dynamic_text(self):
 
         # Go back to organization type page and change type
         self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
-        so_page.click(str(self.TITLES["generic_org_type"]), index=0)
+        type_page = so_page.click(str(self.TITLES["generic_org_type"]), index=0)
+        type_form = type_page.forms[0]  # IMPORTANT re-acquire a fresh form (new hidden version token)
         self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
         type_form["generic_org_type-generic_org_type"] = "city"
         type_result = type_form.submit()
@@ -2527,8 +2531,8 @@ def test_domain_request_dotgov_domain_dynamic_text(self):
         self.assertContains(dotgov_page, "medicare.gov")
 
         # Go back to organization type page and change type
-        self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
-        dotgov_page.click(str(self.TITLES["generic_org_type"]), index=0)
+        type_page = dotgov_page.click(str(self.TITLES["generic_org_type"]), index=0)
+        type_form = type_page.forms[0]  # IMPORTANT re-acquire a fresh form (new hidden version token)
         self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id)
         type_form["generic_org_type-generic_org_type"] = "city"
         type_result = type_form.submit()