Historical records: set the change reason explicitly on the instance

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

@@ -43,8 +43,8 @@ def perform_update(self, serializer):
         return obj
 
     def perform_destroy(self, instance):
+        instance._change_reason = self.get_change_reason()
         super().perform_destroy(instance)
-        safe_update_change_reason(instance, self.get_change_reason())
 
 
 class NestedParentObjectMixin:

readthedocs/core/forms.py

@@ -7,8 +7,6 @@
 from django.forms.fields import CharField
 from django.utils.translation import ugettext_lazy as _
 
-from readthedocs.core.history import safe_update_change_reason
-
 from .models import UserProfile
 
 log = logging.getLogger(__name__)
@@ -39,10 +37,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())
+            user._change_reason = self.get_change_reason()
+            user.save()
         return profile
 
     def get_change_reason(self):

readthedocs/core/history.py

@@ -12,7 +12,23 @@
 
 
 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, set the ``_change_reason``
+       attribute 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 +95,14 @@ def get_change_reason(self):
         return f'origin=admin class={klass}'
 
     def save_model(self, request, obj, form, change):
+        if obj:
+            obj._change_reason = 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:
+            obj._change_reason = self.get_change_reason()
         super().delete_model(request, obj)
-        safe_update_change_reason(obj, self.change_reason)
 
 
 class SimpleHistoryModelForm(forms.ModelForm):
@@ -100,9 +118,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:
+            self.instance._change_reason = self.get_change_reason()
+        return super().save(commit=commit)
 
 
 class UpdateChangeReasonPostView:
@@ -121,8 +139,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()
+        obj._change_reason = self.get_change_reason()
+        return obj

readthedocs/profiles/views.py

@@ -30,7 +30,6 @@
     UserDeleteForm,
     UserProfileForm,
 )
-from readthedocs.core.history import safe_update_change_reason
 from readthedocs.core.mixins import PrivateViewMixin
 from readthedocs.core.models import UserProfile
 from readthedocs.core.utils.extend import SettingsOverrideObject
@@ -88,8 +87,8 @@ def get_object(self):
     def form_valid(self, form):
         user = self.get_object()
         logout(self.request)
+        user._change_reason = 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):
@@ -100,6 +99,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):