Historical records: set the change reason explicitly on the instance (#8627)

The implementation of django-simple-history's `update_change_reason`
is very brittle, as it queries for a previous historical record
that matches the attributes of the instance to update the ``change_reason``,
which could end up updating the wrong record, or not finding it.

https://github.com/jazzband/django-simple-history/blob/b8e43df626a03ce4817beaf6a20995b40a209f37/simple_history/utils.py#L27

I saw this mostly happening when:

- Saving a team without changing anything (update_change_reason won't
  find the record for some reason)
- Deleting a team (update_change_reason will override the previous
  record instead)

But simple history also mentions that you can set the change reason
explicitly by setting the `_change_reason` attribute,
which is more safer as it sets the change_reason at the creation time of
the record.

https://django-simple-history.readthedocs.io/en/latest/historical_model.html#change-reason

Author: stsewd

readthedocs/api/v3/mixins.py

@@ -3,7 +3,10 @@
 from rest_framework.response import Response
 
 from readthedocs.builds.models import Version
-from readthedocs.core.history import safe_update_change_reason
+from readthedocs.core.history import (
+    safe_update_change_reason,
+    set_change_reason,
+)
 from readthedocs.organizations.models import Organization
 from readthedocs.projects.models import Project
 
@@ -38,13 +41,13 @@ def perform_create(self, serializer):
         return obj
 
     def perform_update(self, serializer):
+        set_change_reason(serializer.instance, self.get_change_reason())
         obj = serializer.save()
-        safe_update_change_reason(obj, self.get_change_reason())
         return obj
 
     def perform_destroy(self, instance):
+        set_change_reason(instance, self.get_change_reason())
         super().perform_destroy(instance)
-        safe_update_change_reason(instance, self.get_change_reason())
 
 
 class NestedParentObjectMixin:

readthedocs/core/forms.py

@@ -7,7 +7,7 @@
 from django.forms.fields import CharField
 from django.utils.translation import ugettext_lazy as _
 
-from readthedocs.core.history import safe_update_change_reason
+from readthedocs.core.history import set_change_reason
 
 from .models import UserProfile
 
@@ -39,10 +39,10 @@ def save(self, commit=True):
             user = profile.user
             user.first_name = first_name
             user.last_name = last_name
-            user.save()
             # SimpleHistoryModelForm isn't used here
             # because the model of this form is `UserProfile`, not `User`.
-            safe_update_change_reason(user, self.get_change_reason())
+            set_change_reason(user, self.get_change_reason())
+            user.save()
         return profile
 
     def get_change_reason(self):

readthedocs/core/history.py

@@ -11,8 +11,37 @@
 log = logging.getLogger(__name__)
 
 
+def set_change_reason(instance, reason):
+    """
+    Set the change reason for the historical record created from the instance.
+
+    This method should be called before calling ``save()`` or ``delete``.
+    It sets `reason` to the `_change_reason` attribute of the instance,
+    that's used to create the historical record on the save/delete signals.
+
+    https://django-simple-history.readthedocs.io/en/latest/historical_model.html#change-reason  # noqa
+    """
+    instance._change_reason = reason
+
+
 def safe_update_change_reason(instance, reason):
-    """Wrapper around update_change_reason to catch exceptions."""
+    """
+    Wrapper around update_change_reason to catch exceptions.
+
+    .. warning::
+
+       The implementation of django-simple-history's `update_change_reason`
+       is very brittle, as it queries for a previous historical record
+       that matches the attributes of the instance to update the ``change_reason``,
+       which could end up updating the wrong record, or not finding it.
+
+       If you already have control over the object, use `set_change_reason`
+       before updating/deleting the object instead.
+       That's more safe, since the attribute is passed to the signal
+       and used at the creation time of the record.
+
+        https://django-simple-history.readthedocs.io/en/latest/historical_model.html#change-reason  # noqa
+    """
     try:
         update_change_reason(instance=instance, reason=reason)
     except Exception:
@@ -79,12 +108,14 @@ def get_change_reason(self):
         return f'origin=admin class={klass}'
 
     def save_model(self, request, obj, form, change):
+        if obj:
+            set_change_reason(obj, self.get_change_reason())
         super().save_model(request, obj, form, change)
-        safe_update_change_reason(obj, self.get_change_reason())
 
     def delete_model(self, request, obj):
+        if obj:
+            set_change_reason(obj, self.get_change_reason())
         super().delete_model(request, obj)
-        safe_update_change_reason(obj, self.change_reason)
 
 
 class SimpleHistoryModelForm(forms.ModelForm):
@@ -100,9 +131,9 @@ def get_change_reason(self):
         return f'origin=form class={klass}'
 
     def save(self, commit=True):
-        obj = super().save(commit=commit)
-        safe_update_change_reason(obj, self.get_change_reason())
-        return obj
+        if self.instance:
+            set_change_reason(self.instance, self.get_change_reason())
+        return super().save(commit=commit)
 
 
 class UpdateChangeReasonPostView:
@@ -121,8 +152,7 @@ def get_change_reason(self):
         klass = self.__class__.__name__
         return f'origin=form class={klass}'
 
-    def post(self, request, *args, **kwargs):
-        obj = self.get_object()
-        response = super().post(request, *args, **kwargs)
-        safe_update_change_reason(obj, self.get_change_reason())
-        return response
+    def get_object(self):
+        obj = super().get_object()
+        set_change_reason(obj, self.get_change_reason())
+        return obj

readthedocs/profiles/views.py

@@ -28,7 +28,7 @@
     UserDeleteForm,
     UserProfileForm,
 )
-from readthedocs.core.history import safe_update_change_reason
+from readthedocs.core.history import set_change_reason
 from readthedocs.core.mixins import PrivateViewMixin
 from readthedocs.core.models import UserProfile
 from readthedocs.core.utils.extend import SettingsOverrideObject
@@ -86,8 +86,8 @@ def get_object(self):
     def form_valid(self, form):
         user = self.get_object()
         logout(self.request)
+        set_change_reason(user, self.get_change_reason())
         user.delete()
-        safe_update_change_reason(user, 'Changed from: form')
         return super().form_valid(form)
 
     def get_form(self, data=None, files=None, **kwargs):
@@ -98,6 +98,10 @@ def get_form(self, data=None, files=None, **kwargs):
     def get_success_url(self):
         return reverse('homepage')
 
+    def get_change_reason(self):
+        klass = self.__class__.__name__
+        return f'origin=form class={klass}'
+
 
 class ProfileDetailBase(DetailView):
 

readthedocs/projects/admin.py

@@ -8,10 +8,7 @@
 from django.utils.translation import ugettext_lazy as _
 
 from readthedocs.builds.models import Version
-from readthedocs.core.history import (
-    ExtraSimpleHistoryAdmin,
-    safe_update_change_reason,
-)
+from readthedocs.core.history import ExtraSimpleHistoryAdmin, set_change_reason
 from readthedocs.core.utils import trigger_build
 from readthedocs.notifications.views import SendNotificationView
 from readthedocs.redirects.models import Redirect
@@ -274,8 +271,8 @@ def ban_owner(self, request, queryset):
             if project.users.count() == 1:
                 user = project.users.first()
                 user.profile.banned = True
+                set_change_reason(user.profile, self.get_change_reason())
                 user.profile.save()
-                safe_update_change_reason(user.profile, self.get_change_reason())
                 total += 1
             else:
                 messages.add_message(