Enable/ disable revert using only a settings attribute (#632)

erikvw · web-flow · commit 184ecdd67d2e · 2020-04-23T09:52:19.000-04:00
* show revert text / buttons based on value of revert_disabled.

* add tests for revert disabled functionality

* add SIMPLE_HISTORY_REVERT_DISABLED as attr to control visibility of Revert button.

* capture --tag from args

* correct variable name to revert_disabled

* update docs

* CHANGES.rst

* remove unused test tags

* fix minor formatting issues in templates

* fix, only show text if revert is NOT disabled

* use random file name in tests (uuid)
diff --git a/.gitignore b/.gitignore
@@ -16,3 +16,4 @@ htmlcov/
 MANIFEST
 test_files/
 venv/
+.DS_Store
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -9,6 +9,8 @@ Unreleased
 - import model `ContentType` in `SimpleHistoryAdmin` using `django_apps.get_model`
   to avoid possible `AppRegistryNotReady` exception (gh-630)
 - Fix `utils.update_change_reason` when user specifies excluded_fields
+- settings.SIMPLE_HISTORY_REVERT_DISABLED if True removes the Revert
+  button from the history form for all historical models (gh-632))
 
 2.8.0 (2019-12-02)
 ------------------
diff --git a/docs/admin.rst b/docs/admin.rst
@@ -68,3 +68,16 @@ admin class
 
 
 .. image:: screens/5_history_list_display.png
+
+Disabling the option to revert an object
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+By default, an object can be reverted to its previous version. To disable this option update your settings with the following:
+
+.. code-block:: python
+
+    SIMPLE_HISTORY_REVERT_DISABLED=True
+
+When ``SIMPLE_HISTORY_REVERT_DISABLED`` is set to ``True``, the revert button is removed from the form.
+
+.. image:: screens/10_revert_disabled.png
diff --git a/docs/screens/10_revert_disabled.png b/docs/screens/10_revert_disabled.png
diff --git a/runtests.py b/runtests.py
@@ -73,12 +73,14 @@ def __getitem__(self, item):
 
 
 def main():
-
     if not settings.configured:
         settings.configure(**DEFAULT_SETTINGS)
     django.setup()
-    failures = DiscoverRunner(failfast=False).run_tests(["simple_history.tests"])
-    failures |= DiscoverRunner(failfast=False).run_tests(
+    tags = [t.split("=")[1] for t in sys.argv if t.startswith("--tag")]
+    failures = DiscoverRunner(failfast=False, tags=tags).run_tests(
+        ["simple_history.tests"]
+    )
+    failures |= DiscoverRunner(failfast=False, tags=tags).run_tests(
         ["simple_history.registry_tests"]
     )
     sys.exit(failures)
diff --git a/simple_history/admin.py b/simple_history/admin.py
@@ -88,7 +88,7 @@ def history_view(self, request, object_id, extra_context=None):
             content_type.model,
         )
         context = {
-            "title": _("Change history: %s") % force_str(obj),
+            "title": self.history_view_title(obj),
             "action_list": action_list,
             "module_name": capfirst(force_str(opts.verbose_name_plural)),
             "object": obj,
@@ -97,6 +97,7 @@ def history_view(self, request, object_id, extra_context=None):
             "opts": opts,
             "admin_user_view": admin_user_view,
             "history_list_display": history_list_display,
+            "revert_disabled": self.revert_disabled,
         }
         context.update(self.admin_site.each_context(request))
         context.update(extra_context or {})
@@ -105,6 +106,12 @@ def history_view(self, request, object_id, extra_context=None):
             request, self.object_history_template, context, **extra_kwargs
         )
 
+    def history_view_title(self, obj):
+        if self.revert_disabled and not SIMPLE_HISTORY_EDIT:
+            return _("View history: %s") % force_str(obj)
+        else:
+            return _("Change history: %s") % force_str(obj)
+
     def response_change(self, request, obj):
         if "_change_history" in request.POST and SIMPLE_HISTORY_EDIT:
             verbose_name = obj._meta.verbose_name
@@ -175,7 +182,7 @@ def history_form_view(self, request, object_id, version_id, extra_context=None):
         model_name = original_opts.model_name
         url_triplet = self.admin_site.name, original_opts.app_label, model_name
         context = {
-            "title": _("Revert %s") % force_str(obj),
+            "title": self.history_form_view_title(obj),
             "adminform": admin_form,
             "object_id": object_id,
             "original": obj,
@@ -188,6 +195,7 @@ def history_form_view(self, request, object_id, version_id, extra_context=None):
             "change_url": reverse("%s:%s_%s_change" % url_triplet, args=(obj.pk,)),
             "history_url": reverse("%s:%s_%s_history" % url_triplet, args=(obj.pk,)),
             "change_history": change_history,
+            "revert_disabled": self.revert_disabled,
             # Context variables copied from render_change_form
             "add": False,
             "change": True,
@@ -212,6 +220,12 @@ def history_form_view(self, request, object_id, version_id, extra_context=None):
             request, self.object_history_form_template, context, **extra_kwargs
         )
 
+    def history_form_view_title(self, obj):
+        if self.revert_disabled:
+            return _("View %s") % force_str(obj)
+        else:
+            return _("Revert %s") % force_str(obj)
+
     def render_history_view(self, request, template, context, **kwargs):
         """Catch call to render, to allow overriding."""
         return render(request, template, context, **kwargs)
@@ -226,3 +240,7 @@ def content_type_model_cls(self):
         """Returns the ContentType model class.
         """
         return django_apps.get_model("contenttypes.contenttype")
+
+    @property
+    def revert_disabled(self):
+        return getattr(settings, "SIMPLE_HISTORY_REVERT_DISABLED", False)
diff --git a/simple_history/templates/simple_history/object_history.html b/simple_history/templates/simple_history/object_history.html
@@ -7,9 +7,8 @@
 
 {% block content %}
   <div id="content-main">
-
-    <p>{% blocktrans %}Choose a date from the list below to revert to a previous version of this object.{% endblocktrans %}</p>
-
+    {% if not revert_disabled %}<p>
+      {% blocktrans %}Choose a date from the list below to revert to a previous version of this object.{% endblocktrans %}</p>{% endif %}
     <div class="module">
       {% if action_list %}
         {% display_list %}
@@ -19,4 +18,3 @@
     </div>
   </div>
 {% endblock %}
-
diff --git a/simple_history/templates/simple_history/object_history_form.html b/simple_history/templates/simple_history/object_history_form.html
@@ -9,7 +9,7 @@
     <a href="{{changelist_url}}">{{opts.verbose_name_plural|capfirst}}</a> &rsaquo;
     <a href="{{change_url}}">{{original|truncatewords:"18"}}</a> &rsaquo;
     <a href="../">{% trans "History" %}</a> &rsaquo;
-    {% blocktrans with original_opts.verbose_name as verbose_name %}Revert {{verbose_name}}{% endblocktrans %}
+    {% if revert_disabled %}{% blocktrans with original_opts.verbose_name as verbose_name %}View {{verbose_name}}{% endblocktrans %}{% else %}{% blocktrans with original_opts.verbose_name as verbose_name %}Revert {{verbose_name}}{% endblocktrans %}{% endif %}
   </div>
 {% endblock %}
 
@@ -18,5 +18,5 @@
 {% endblock %}
 
 {% block form_top %}
-<p>{% blocktrans %}Press the 'Revert' button below to revert to this version of the object.{% endblocktrans %} {% if change_history %}{% blocktrans %}Or press the 'Change History' button to edit the history.{% endblocktrans %}{% endif %}</p>
+<p>{% if not revert_disabled %}{% blocktrans %}Press the 'Revert' button below to revert to this version of the object. {% endblocktrans %}{% endif %}{% if change_history %}{% blocktrans %}Press the 'Change History' button below to edit the history.{% endblocktrans %}{% endif %}</p>
 {% endblock %}
diff --git a/simple_history/templates/simple_history/submit_line.html b/simple_history/templates/simple_history/submit_line.html
@@ -1,5 +1,8 @@
 {% load i18n %}
 <div class="submit-row">
-<input type="submit" value="{% trans 'Revert' %}" class="default" name="_save" {{ onclick_attrib }}/>
-{% if change_history %}<input type="submit" value="{% trans 'Change History' %}" class="default" name="_change_history" {{ onclick_attrib }}/>{% endif %}
+  {% if not revert_disabled %}
+    <input type="submit" value="{% trans 'Revert' %}" class="default" name="_save" {{ onclick_attrib }}/>{% endif %}
+  {% if change_history %}
+    <input type="submit" value="{% trans 'Change History' %}" class="default" name="_change_history" {{ onclick_attrib }}/>{% endif %}
+  <a href="{{ history_url }}" class="closelink">{% trans 'Close' %}</a>
 </div>
diff --git a/simple_history/tests/admin.py b/simple_history/tests/admin.py
@@ -13,6 +13,7 @@
     FileModel,
     Paper,
     Person,
+    Planet,
     Poll,
 )
 
@@ -33,6 +34,13 @@ def test_method(self, obj):
     history_list_display = ["title", "test_method"]
 
 
+class PlanetAdmin(SimpleHistoryAdmin):
+    def test_method(self, obj):
+        return "test_method_value"
+
+    history_list_display = ["title", "test_method"]
+
+
 admin.site.register(Poll, SimpleHistoryAdmin)
 admin.site.register(Choice, ChoiceAdmin)
 admin.site.register(Person, PersonAdmin)
@@ -43,3 +51,4 @@ def test_method(self, obj):
 admin.site.register(ConcreteExternal, SimpleHistoryAdmin)
 admin.site.register(ExternalModelWithCustomUserIdField, SimpleHistoryAdmin)
 admin.site.register(FileModel, FileModelAdmin)
+admin.site.register(Planet, PlanetAdmin)
diff --git a/simple_history/tests/models.py b/simple_history/tests/models.py
@@ -381,6 +381,17 @@ class City(models.Model):
     history = HistoricalRecords()
 
 
+class Planet(models.Model):
+    star = models.CharField(max_length=30)
+    history = HistoricalRecords()
+
+    def __str__(self):
+        return self.star
+
+    class Meta:
+        verbose_name = "Planet"
+
+
 class Contact(models.Model):
     name = models.CharField(max_length=30)
     email = models.EmailField(max_length=255, unique=True)
@@ -576,7 +587,6 @@ class UUIDRegisterModel(models.Model):
 
 register(UUIDRegisterModel, history_id_field=models.UUIDField(default=uuid.uuid4))
 
-
 # Set the SIMPLE_HISTORY_HISTORY_ID_USE_UUID
 setattr(settings, "SIMPLE_HISTORY_HISTORY_ID_USE_UUID", True)
 
@@ -589,7 +599,6 @@ class UUIDDefaultModel(models.Model):
 # Clear the SIMPLE_HISTORY_HISTORY_ID_USE_UUID
 delattr(settings, "SIMPLE_HISTORY_HISTORY_ID_USE_UUID")
 
-
 # Set the SIMPLE_HISTORY_HISTORY_CHANGE_REASON_FIELD
 setattr(settings, "SIMPLE_HISTORY_HISTORY_CHANGE_REASON_USE_TEXT_FIELD", True)
 
diff --git a/simple_history/tests/tests/test_admin.py b/simple_history/tests/tests/test_admin.py
@@ -26,6 +26,7 @@
     Person,
     Poll,
     State,
+    Planet,
 )
 
 if django.VERSION < (2,):
@@ -65,8 +66,13 @@ def tearDown(self):
         except AttributeError:
             pass
 
-    def login(self, user=None):
-        self.client.force_login(user or self.user)
+    def login(self, user=None, superuser=None):
+        user = user or self.user
+        if superuser is not None:
+            user.is_superuser = True if superuser is None else superuser
+            user.is_active = True
+            user.save()
+        self.client.force_login(user)
 
     def test_history_list(self):
         model_name = self.user._meta.model_name
@@ -465,6 +471,7 @@ def test_history_form_view_without_getting_history(self):
             "has_add_permission": admin.has_add_permission(request),
             "has_change_permission": admin.has_change_permission(request, poll),
             "has_delete_permission": admin.has_delete_permission(request, poll),
+            "revert_disabled": admin.revert_disabled,
             "has_file_field": True,
             "has_absolute_url": False,
             "form_url": "",
@@ -518,6 +525,7 @@ def test_history_form_view_getting_history(self):
             "has_add_permission": admin.has_add_permission(request),
             "has_change_permission": admin.has_change_permission(request, poll),
             "has_delete_permission": admin.has_delete_permission(request, poll),
+            "revert_disabled": admin.revert_disabled,
             "has_file_field": True,
             "has_absolute_url": False,
             "form_url": "",
@@ -571,6 +579,7 @@ def test_history_form_view_getting_history_with_setting_off(self):
             "has_add_permission": admin.has_add_permission(request),
             "has_change_permission": admin.has_change_permission(request, poll),
             "has_delete_permission": admin.has_delete_permission(request, poll),
+            "revert_disabled": admin.revert_disabled,
             "has_file_field": True,
             "has_absolute_url": False,
             "form_url": "",
@@ -626,6 +635,7 @@ def test_history_form_view_getting_history_abstract_external(self):
             "has_add_permission": admin.has_add_permission(request),
             "has_change_permission": admin.has_change_permission(request, obj),
             "has_delete_permission": admin.has_delete_permission(request, obj),
+            "revert_disabled": admin.revert_disabled,
             "has_file_field": True,
             "has_absolute_url": False,
             "form_url": "",
@@ -683,6 +693,7 @@ def test_history_form_view_accepts_additional_context(self):
             "has_add_permission": admin.has_add_permission(request),
             "has_change_permission": admin.has_change_permission(request, poll),
             "has_delete_permission": admin.has_delete_permission(request, poll),
+            "revert_disabled": admin.revert_disabled,
             "has_file_field": True,
             "has_absolute_url": False,
             "form_url": "",
@@ -696,3 +707,51 @@ def test_history_form_view_accepts_additional_context(self):
         mock_render.assert_called_once_with(
             request, admin.object_history_form_template, context
         )
+
+    def test_history_view__title_suggests_revert_by_default(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet))
+        self.assertContains(response, "Change history: Sun")
+
+    @override_settings(SIMPLE_HISTORY_REVERT_DISABLED=False)
+    def test_history_view__title_suggests_revert(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet))
+        self.assertContains(response, "Change history: Sun")
+        self.assertContains(response, "Choose a date")
+
+    @override_settings(SIMPLE_HISTORY_REVERT_DISABLED=True)
+    def test_history_view__title_suggests_view_only(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet))
+        self.assertContains(response, "View history: Sun")
+        self.assertNotContains(response, "Choose a date")
+
+    def test_history_form_view__shows_revert_button_by_default(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet, 0))
+        self.assertContains(response, "Revert Planet")
+        self.assertContains(response, "Revert Sun")
+        self.assertContains(response, "Press the 'Revert' button")
+
+    @override_settings(SIMPLE_HISTORY_REVERT_DISABLED=False)
+    def test_history_form_view__shows_revert_button(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet, 0))
+        self.assertContains(response, "Revert Planet")
+        self.assertContains(response, "Revert Sun")
+        self.assertContains(response, "Press the 'Revert' button")
+
+    @override_settings(SIMPLE_HISTORY_REVERT_DISABLED=True)
+    def test_history_form_view__does_not_show_revert_button(self):
+        self.login()
+        planet = Planet.objects.create(star="Sun")
+        response = self.client.get(get_history_url(planet, 0))
+        self.assertContains(response, "View Planet")
+        self.assertContains(response, "View Sun")
+        self.assertNotContains(response, "Revert")
diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py
@@ -310,11 +310,12 @@ def test_foreignkey_still_allows_reverse_lookup_via_set_attribute(self):
         )
 
     def test_file_field(self):
-        model = FileModel.objects.create(file=get_fake_file("name"))
-        self.assertEqual(model.file.name, "files/name")
+        filename = str(uuid.uuid4())
+        model = FileModel.objects.create(file=get_fake_file(filename))
+        self.assertEqual(model.file.name, "files/{}".format(filename))
         model.file.delete()
         update_record, create_record = model.history.all()
-        self.assertEqual(create_record.file, "files/name")
+        self.assertEqual(create_record.file, "files/{}".format(filename))
         self.assertEqual(update_record.file, "")
 
     def test_file_field_with_char_field_setting(self):
@@ -323,11 +324,12 @@ def test_file_field_with_char_field_setting(self):
         self.assertIs(type(file_field), models.CharField)
         self.assertEqual(file_field.max_length, 100)
         # file field works the same as test_file_field()
-        model = CharFieldFileModel.objects.create(file=get_fake_file("name"))
-        self.assertEqual(model.file.name, "files/name")
+        filename = str(uuid.uuid4())
+        model = CharFieldFileModel.objects.create(file=get_fake_file(filename))
+        self.assertEqual(model.file.name, "files/{}".format(filename))
         model.file.delete()
         update_record, create_record = model.history.all()
-        self.assertEqual(create_record.file, "files/name")
+        self.assertEqual(create_record.file, "files/{}".format(filename))
         self.assertEqual(update_record.file, "")
 
     def test_inheritance(self):
