chore(nimbus): readonly changelogs in admin

experimenter/experimenter/experiments/admin.py

@@ -1,12 +1,13 @@
 from typing import Any
-from weakref import WeakKeyDictionary
 
 from django import forms
 from django.contrib import admin
 from django.contrib.auth.models import User
 from django.contrib.postgres import forms as pgforms
 from django.db import transaction
+from django.template.loader import render_to_string
 from django.utils.encoding import force_str
+from django.utils.safestring import mark_safe
 from import_export import fields, resources
 from import_export.admin import ExportActionMixin, ImportMixin
 from import_export.widgets import DecimalWidget, ForeignKeyWidget
@@ -233,43 +234,6 @@ class NimbusDocumentationLinkInlineAdmin(
     extra = 1
 
 
-class NimbusExperimentChangeLogInlineAdmin(
-    NoDeleteAdminMixin, admin.StackedInline[NimbusChangeLog]
-):
-    model = NimbusChangeLog
-
-    # A mapping of Django HttpRequests to the choices for the "changed_by"
-    # field.
-    #
-    # This uses a WeakKeyDictionary to ensure we don't leak requests.
-    CHANGED_BY_CHOICES = WeakKeyDictionary()
-
-    def formfield_for_foreignkey(self, db_field, request, **kwargs):
-        if db_field.name == "changed_by":
-            field = super().formfield_for_foreignkey(db_field, request, **kwargs)
-            if field is not None:
-                field.choices = self._get_changed_by_choices(request)
-
-        else:
-            field = super().formfield_for_foreignkey(db_field, request, **kwargs)
-
-        return field
-
-    def _get_changed_by_choices(self, request):
-        """Return a cached list of all available Users.
-
-        If the list of users has not been computed for the changed_by field this
-        request, it will be computed and cached.
-        """
-        choices = self.CHANGED_BY_CHOICES.get(request)
-
-        if not choices:
-            choices = list(forms.ModelChoiceField(User.objects.all()).choices)
-            self.CHANGED_BY_CHOICES[request] = choices
-
-        return choices
-
-
 class NimbusExperimentBucketRangeInlineAdmin(
     ReadOnlyAdminMixin, admin.StackedInline[NimbusBucketRange]
 ):
@@ -361,7 +325,6 @@ class NimbusExperimentAdmin(
         NimbusDocumentationLinkInlineAdmin,
         NimbusBranchInlineAdmin,
         NimbusExperimentBucketRangeInlineAdmin,
-        NimbusExperimentChangeLogInlineAdmin,
     )
     list_display = (
         "name",
@@ -387,7 +350,21 @@ class NimbusExperimentAdmin(
     form = NimbusExperimentAdminForm
     actions = [force_fetch_jetstream_data]
     resource_class = NimbusExperimentResource
-    readonly_fields = ("_firefox_min_version_parsed",)
+    readonly_fields = ("_firefox_min_version_parsed", "changelog_display")
+
+    @admin.display(description="Change History")
+    def changelog_display(self, obj):
+        if obj.pk:
+            changes = obj.changes.all().order_by("-changed_on")
+            if not changes:
+                return "No change history"
+
+            html = render_to_string(
+                "admin/changelog_display.html",
+                {"changes": changes},
+            )
+            return mark_safe(html)
+        return "No change history"
 
     @transaction.atomic
     def save_form(self, request, form, change):

experimenter/experimenter/experiments/templates/admin/changelog_display.html

@@ -0,0 +1,26 @@
+<table class="listing">
+    <thead>
+        <tr>
+            <th>Changed On</th>
+            <th>Changed By</th>
+            <th>Old Status</th>
+            <th>New Status</th>
+            <th>Old Publish Status</th>
+            <th>New Publish Status</th>
+            <th>Message</th>
+        </tr>
+    </thead>
+    <tbody>
+        {% for change in changes %}
+        <tr class="{% cycle 'row1' 'row2' %}">
+            <td>{{ change.changed_on }}</td>
+            <td>{{ change.changed_by.email|default:"Unknown" }}</td>
+            <td>{{ change.old_status|default:"-" }}</td>
+            <td>{{ change.new_status|default:"-" }}</td>
+            <td>{{ change.old_publish_status|default:"-" }}</td>
+            <td>{{ change.new_publish_status|default:"-" }}</td>
+            <td>{{ change.message|default:"No message" }}</td>
+        </tr>
+        {% endfor %}
+    </tbody>
+</table>

experimenter/experimenter/experiments/tests/test_admin.py

@@ -2,7 +2,6 @@
 from unittest import mock
 
 from django.conf import settings
-from django.contrib.admin.sites import AdminSite
 from django.contrib.auth.models import User
 from django.test import RequestFactory, TestCase
 from django.urls import reverse
@@ -13,7 +12,6 @@
     NimbusBranchForeignKeyWidget,
     NimbusExperimentAdmin,
     NimbusExperimentAdminForm,
-    NimbusExperimentChangeLogInlineAdmin,
     NimbusExperimentResource,
 )
 from experimenter.experiments.changelog_utils import NimbusBranchChangeLogSerializer
@@ -176,11 +174,10 @@ def test_admin_force_jetstream_fetch(self, mock_fetch_experiment_data):
         mock_fetch_experiment_data.delay.assert_called_with(experiment.id)
 
     def test_admin_save_creates_changelog(self):
+        admin = NimbusExperimentAdmin(NimbusExperiment, None)
         user = UserFactory.create()
         request = RequestFactory().post("/")
         request.user = user
-        site = AdminSite()
-        admin = NimbusExperimentAdmin(model=NimbusExperiment, admin_site=site)
 
         experiment = NimbusExperimentFactory.create_with_lifecycle(
             NimbusExperimentFactory.Lifecycles.CREATED
@@ -217,36 +214,44 @@ def test_admin_save_creates_changelog(self):
 
 
 class TestNimbusExperimentChangeLogInlineAdmin(TestCase):
-    def setUp(self):
-        self.user = UserFactory.create()
-        self.request = RequestFactory().get("/")
-        self.request.user = self.user
-        self.site = AdminSite()
-
-    def test_query_count(self):
+    def test_changelog_display_with_changes(self):
+        admin = NimbusExperimentAdmin(NimbusExperiment, None)
+        user = UserFactory.create()
         experiment = NimbusExperimentFactory.create_with_lifecycle(
             NimbusExperimentFactory.Lifecycles.CREATED,
-            owner=self.user,
+            owner=user,
         )
 
-        for _ in range(100):
-            NimbusChangeLogFactory.create(
-                experiment=experiment,
-                experiment_data={"some": "data"},
-                changed_by=UserFactory.create(),
-            )
+        NimbusChangeLogFactory.create(
+            experiment=experiment,
+            changed_by=user,
+            old_status=NimbusExperiment.Status.DRAFT,
+            new_status=NimbusExperiment.Status.PREVIEW,
+            old_publish_status=NimbusExperiment.PublishStatus.IDLE,
+            new_publish_status=NimbusExperiment.PublishStatus.REVIEW,
+            message="Test change",
+        )
 
-        admin = NimbusExperimentChangeLogInlineAdmin(
-            parent_model=NimbusExperiment,
-            admin_site=self.site,
+        result = admin.changelog_display(experiment)
+        self.assertIn("Test change", result)
+
+    def test_changelog_display_without_changes(self):
+        admin = NimbusExperimentAdmin(NimbusExperiment, None)
+        experiment = NimbusExperimentFactory.create_with_lifecycle(
+            NimbusExperimentFactory.Lifecycles.CREATED,
         )
+        # Clear any changelog entries
+        experiment.changes.all().delete()
+
+        result = admin.changelog_display(experiment)
+        self.assertEqual(result, "No change history")
 
-        # This does some queries that are unaffected by our model admin.
-        formset_cls = admin.get_formset(self.request)
+    def test_changelog_display_without_pk(self):
+        admin = NimbusExperimentAdmin(NimbusExperiment, None)
+        experiment = NimbusExperiment()
 
-        with self.assertNumQueries(1):
-            formset = formset_cls(instance=experiment)
-            formset.render()
+        result = admin.changelog_display(experiment)
+        self.assertEqual(result, "No change history")
 
 
 class TestNimbusExperimentExport(TestCase):