Tweak ActivityLog to make activity admin pages more useful (#20246)

* Make ActivityLog admin pages more useful
* Make IPLog unique per activity, show it in activity log admin
* Fix log strings for admin actions: {user} can be the admin user making the change

src/olympia/activity/admin.py

@@ -1,7 +1,9 @@
 from django.contrib import admin
 
+from olympia import amo
 from olympia.amo.admin import AMOModelAdmin
 from olympia.reviewers.models import ReviewActionReason
+from olympia.zadmin.admin import related_single_content_link
 
 from .models import ActivityLog, ReviewActionReasonLog
 
@@ -9,23 +11,29 @@ from .models import ActivityLog, ReviewActionReasonLog
 class ActivityLogAdmin(AMOModelAdmin):
     list_display = (
         'created',
-        'user',
-        '__str__',
+        'user_link',
+        'pretty_arguments',
+        'kept_forever',
+        'ip_address',
     )
     raw_id_fields = ('user',)
     readonly_fields = (
         'created',
         'user',
-        '__str__',
+        'pretty_arguments',
+        'kept_forever',
+        'ip_address',
     )
-    date_hierarchy = 'created'
     fields = (
         'user',
         'created',
-        '__str__',
+        'pretty_arguments',
+        'kept_forever',
+        'ip_address',
     )
     raw_id_fields = ('user',)
     view_on_site = False
+    list_select_related = ('iplog',)
 
     def lookup_allowed(self, lookup, value):
         if lookup == 'addonlog__addon':
@@ -38,9 +46,33 @@ class ActivityLogAdmin(AMOModelAdmin):
     def has_delete_permission(self, request, obj=None):
         return False
 
+    @admin.display(description='Arguments')
+    def pretty_arguments(self, obj):
+        return obj.to_string(type_='admin')
+
+    @admin.display(description='User')
+    def user_link(self, obj):
+        return related_single_content_link(obj, 'user')
+
+    @admin.display(description='IP Address', ordering='iplog__ip_address_binary')
+    def ip_address(self, obj):
+        return str(obj.iplog)
+
+    @admin.display(description='Kept forever', boolean=True)
+    def kept_forever(self, obj):
+        return getattr(amo.LOG_BY_ID.get(obj.action), 'keep', False)
+
+    def change_view(self, request, object_id, form_url='', extra_context=None):
+        if extra_context is None:
+            extra_context = {}
+        # The __str__ for ActivityLog contains HTML, so use a simpler subtitle.
+        extra_context['subtitle'] = f'{self.model._meta.verbose_name} {object_id}'
+        return super().change_view(
+            request, object_id, form_url=form_url, extra_context=extra_context
+        )
+
 
 class ReviewActionReasonLogAdmin(AMOModelAdmin):
-    date_hierarchy = 'created'
     fields = (
         'created',
         'activity_log',

src/olympia/activity/migrations/0021_alter_iplog_activity_log.py

@@ -0,0 +1,19 @@
+# Generated by Django 3.2.16 on 2023-01-25 14:04
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('activity', '0020_auto_20221214_1331'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='iplog',
+            name='activity_log',
+            field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='activity.activitylog'),
+        ),
+    ]

src/olympia/activity/migrations/0022_alter_activitylog_user.py

@@ -0,0 +1,34 @@
+# Generated by Django 3.2.16 on 2023-01-26 16:37
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('activity', '0021_alter_iplog_activity_log'),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            # Avoid a database operation for this, as we just need to alter the
+            # python on_delete callback. Database operations would normally be:
+            # ALTER TABLE `log_activity` DROP FOREIGN KEY `log_activity_user_id_6ed3455e_fk_users_id`;
+            # ALTER TABLE `log_activity` MODIFY `user_id` integer NOT NULL;
+            # ALTER TABLE `log_activity` ADD CONSTRAINT `log_activity_user_id_6ed3455e_fk_users_id` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`);
+            database_operations=[],
+            state_operations=[
+                migrations.AlterField(
+                    model_name='activitylog',
+                    name='user',
+                    field=models.ForeignKey(
+                        on_delete=django.db.models.deletion.DO_NOTHING,
+                        to='users.userprofile',
+                    ),
+                )
+            ],
+        )
+    ]

src/olympia/activity/models.py

@@ -1,5 +1,4 @@
 import json
-import string
 import uuid
 
 from collections import defaultdict
@@ -10,10 +9,9 @@ from django.conf import settings
 from django.db import models
 from django.utils import timezone
 from django.utils.functional import cached_property
+from django.utils.html import format_html
 from django.utils.translation import gettext
 
-import markupsafe
-
 import olympia.core.logger
 
 from olympia import amo, constants
@@ -29,7 +27,6 @@ from olympia.reviewers.models import CannedResponse, ReviewActionReason
 
 from olympia.tags.models import Tag
 from olympia.users.models import UserProfile
-from olympia.users.templatetags.jinja_helpers import user_link
 from olympia.versions.models import Version
 
 
@@ -209,7 +206,7 @@ class IPLog(ModelBase):
     actions).
     """
 
-    activity_log = models.ForeignKey('ActivityLog', on_delete=models.CASCADE)
+    activity_log = models.OneToOneField('ActivityLog', on_delete=models.CASCADE)
     _ip_address = models.CharField(max_length=45, db_column='ip_address', null=True)
     ip_address_binary = IPAddressBinaryField(null=True)
 
@@ -227,6 +224,9 @@ class IPLog(ModelBase):
             ),
         ]
 
+    def __str__(self):
+        return str(self.ip_address_binary)
+
     def save(self, *args, **kwargs):
         # ip_address_binary fulfils our needs, but we're keeping filling ip_address for
         # now, until any existing manual queries are updated.
@@ -324,27 +324,18 @@ class ActivityLogManager(ManagerBase):
         )
 
 
-class SafeFormatter(string.Formatter):
-    """A replacement for str.format that escapes interpolated values."""
-
-    def get_field(self, *args, **kw):
-        # obj is the value getting interpolated into the string.
-        obj, used_key = super().get_field(*args, **kw)
-        return markupsafe.escape(obj), used_key
-
-
 class ActivityLog(ModelBase):
     TYPES = sorted(
         (value.id, key) for key, value in constants.activity.LOG_BY_ID.items()
     )
-    user = models.ForeignKey('users.UserProfile', null=True, on_delete=models.SET_NULL)
+    # We should never hard-delete users, so the on_delete can be set to DO_NOTHING,
+    # if somehow a hard-delete still occurs, it will raise an IntegrityError.
+    user = models.ForeignKey('users.UserProfile', on_delete=models.DO_NOTHING)
     action = models.SmallIntegerField(choices=TYPES)
     _arguments = models.TextField(blank=True, db_column='arguments')
     _details = models.TextField(blank=True, db_column='details')
     objects = ActivityLogManager()
 
-    formatter = SafeFormatter()
-
     class Meta:
         db_table = 'log_activity'
         ordering = ('-created',)
@@ -353,11 +344,6 @@ class ActivityLog(ModelBase):
             models.Index(fields=('created',), name='log_activity_created_idx'),
         ]
 
-    def f(self, *args, **kw):
-        """Calls SafeFormatter.format and returns a Markup string."""
-        # SafeFormatter escapes everything so this is safe.
-        return markupsafe.Markup(self.formatter.format(*args, **kw))
-
     @classmethod
     def transformer_anonymize_user_for_developer(cls, logs):
         """Replace the user with a generic user in actions where it shouldn't
@@ -516,6 +502,12 @@ class ActivityLog(ModelBase):
             format = getattr(log_type, '%s_format' % type_)
         else:
             format = log_type.format
+        absolute_url_method = (
+            'get_admin_absolute_url' if type_ == 'admin' else 'get_absolute_url'
+        )
+
+        def get_absolute_url(obj):
+            return getattr(obj, absolute_url_method)() if obj is not None else ''
 
         # We need to copy arguments so we can remove elements from it
         # while we loop over self.arguments.
@@ -528,44 +520,45 @@ class ActivityLog(ModelBase):
         group = None
         file_ = None
         status = None
+        user = None
 
         for arg in self.arguments:
             if isinstance(arg, Addon) and not addon:
-                if arg.has_listed_versions():
-                    addon = self.f(
-                        '<a href="{0}">{1}</a>', arg.get_absolute_url(), arg.name
+                if type_ == 'admin' or arg.has_listed_versions():
+                    addon = format_html(
+                        '<a href="{0}">{1}</a>', get_absolute_url(arg), arg.name
                     )
                 else:
-                    addon = self.f('{0}', arg.name)
+                    addon = format_html('{0}', arg.name)
                 arguments.remove(arg)
             if isinstance(arg, Rating) and not rating:
-                rating = self.f(
-                    '<a href="{0}">{1}</a>', arg.get_absolute_url(), gettext('Review')
+                rating = format_html(
+                    '<a href="{0}">{1}</a>', get_absolute_url(arg), gettext('Review')
                 )
                 arguments.remove(arg)
             if isinstance(arg, Version) and not version:
                 text = gettext('Version {0}')
-                if arg.channel == amo.CHANNEL_LISTED:
-                    version = self.f(
+                if type_ == 'admin' or arg.channel == amo.CHANNEL_LISTED:
+                    version = format_html(
                         '<a href="{1}">%s</a>' % text,
                         arg.version,
-                        arg.get_absolute_url(),
+                        get_absolute_url(arg),
                     )
                 else:
-                    version = self.f(text, arg.version)
+                    version = format_html(text, arg.version)
                 arguments.remove(arg)
             if isinstance(arg, Collection) and not collection:
-                collection = self.f(
-                    '<a href="{0}">{1}</a>', arg.get_absolute_url(), arg.name
+                collection = format_html(
+                    '<a href="{0}">{1}</a>', get_absolute_url(arg), arg.name
                 )
                 arguments.remove(arg)
             if isinstance(arg, Tag) and not tag:
                 if arg.can_reverse():
-                    tag = self.f(
-                        '<a href="{0}">{1}</a>', arg.get_absolute_url(), arg.tag_text
+                    tag = format_html(
+                        '<a href="{0}">{1}</a>', get_absolute_url(arg), arg.tag_text
                     )
                 else:
-                    tag = self.f('{0}', arg.tag_text)
+                    tag = format_html('{0}', arg.tag_text)
             if isinstance(arg, Group) and not group:
                 group = arg.name
                 arguments.remove(arg)
@@ -577,13 +570,18 @@ class ActivityLog(ModelBase):
                 ):
                     validation = 'ignored'
 
-                file_ = self.f(
+                file_ = format_html(
                     '<a href="{0}">{1}</a> (validation {2})',
-                    arg.get_absolute_url(),
+                    get_absolute_url(arg),
                     arg.pretty_filename,
                     validation,
                 )
                 arguments.remove(arg)
+            if isinstance(arg, UserProfile) and not user:
+                user = format_html(
+                    '<a href="{0}">{1}</a>', get_absolute_url(arg), arg.name
+                )
+                arguments.remove(arg)
             if self.action == amo.LOG.CHANGE_STATUS.id and not isinstance(arg, Addon):
                 # Unfortunately, this action has been abused in the past and
                 # the non-addon argument could be a string or an int. If it's
@@ -596,7 +594,9 @@ class ActivityLog(ModelBase):
                     status = arg
                 arguments.remove(arg)
 
-        user = user_link(self.user)
+        user_responsible = format_html(
+            '<a href="{0}">{1}</a>', get_absolute_url(self.user), self.user.name
+        )
 
         try:
             kw = {
@@ -606,11 +606,12 @@ class ActivityLog(ModelBase):
                 'collection': collection,
                 'tag': tag,
                 'user': user,
+                'user_responsible': user_responsible,
                 'group': group,
                 'file': file_,
                 'status': status,
             }
-            return self.f(str(format), *arguments, **kw)
+            return format_html(str(format), *arguments, **kw)
         except (AttributeError, KeyError, IndexError):
             log.warning('%d contains garbage data' % (self.id or 0))
             return 'Something magical happened.'

src/olympia/activity/tests/test_admin.py

@@ -3,11 +3,77 @@ from django.urls import reverse
 from pyquery import PyQuery as pq
 
 from olympia import amo
-from olympia.amo.tests import TestCase, user_factory
+from olympia import activity
+from olympia.amo.tests import TestCase, addon_factory, user_factory
 from olympia.activity.models import ActivityLog, ReviewActionReasonLog
 from olympia.reviewers.models import ReviewActionReason
 
 
+class TestActivityLogAdmin(TestCase):
+    def setUp(self):
+        self.list_url = reverse('admin:activity_activitylog_changelist')
+
+    def test_list(self):
+        author = user_factory()
+        addon1 = addon_factory()
+        activity.log_create(
+            amo.LOG.ADD_VERSION, addon1.current_version, addon1, user=author
+        )
+        addon2 = addon_factory()
+        activity.log_create(
+            amo.LOG.ADD_VERSION, addon2.current_version, addon2, user=author
+        )
+        addon3 = addon_factory()
+        activity.log_create(
+            amo.LOG.ADD_VERSION, addon3.current_version, addon3, user=author
+        )
+
+        user = user_factory(email='someone@mozilla.com')
+        self.grant_permission(user, '*:*')
+        self.client.force_login(user)
+
+        with self.assertNumQueries(11):
+            # - 2 savepoints/release
+            # - 2 user and groups
+            # - 1 count for pagination
+            # - 1 activities
+            # - 1 all users from activities
+            # - 1 all versions from activities
+            # - 1 all translations from those versions
+            # - 1 all add-ons from activities
+            # - 1 all translations for those add-ons
+            response = self.client.get(self.list_url)
+        assert response.status_code == 200
+        doc = pq(response.content)
+        assert len(doc('#result_list tbody tr')) == 4  # 3 add versions, 1 log in.
+
+    def test_escaping_and_links(self):
+        user = user_factory(
+            email='someone@mozilla.com', display_name='<script>alert(52)</script>'
+        )
+        addon = addon_factory(name='<script>alert(41)</script>')
+        activity.log_create(
+            amo.LOG.ADD_VERSION, addon.current_version, addon, user=user
+        )
+        self.grant_permission(user, '*:*')
+        self.client.force_login(user)
+        response = self.client.get(self.list_url)
+        assert response.status_code == 200
+        content = response.content.decode('utf-8)')
+        assert (
+            '<a href="http://testserver/en-US/admin/models/users/userprofile/'
+            f'{user.pk}/change/">&lt;script&gt;alert(52)&lt;/script&gt;</a> '
+            'logged in.'
+        ) in content
+        assert (
+            '<a href="http://testserver/en-US/admin/models/versions/version/'
+            f'{addon.current_version.pk}/change/">Version '
+            f'{addon.current_version.version}</a> added to '
+            f'<a href="http://testserver/en-US/admin/models/addons/addon/'
+            f'{addon.pk}/change/">&lt;script&gt;alert(41)&lt;/script&gt;</a>'
+        ) in content
+
+
 class TestReviewActionReasonLogAdmin(TestCase):
     def setUp(self):
         self.admin_home_url = reverse('admin:index')
@@ -42,7 +108,9 @@ class TestReviewActionReasonLogAdmin(TestCase):
         user = user_factory(email='someone@mozilla.com')
         self.grant_permission(user, '*:*')
         self.client.force_login(user)
-        activity_log = ActivityLog.objects.create(action=amo.LOG.APPROVE_VERSION.id)
+        activity_log = ActivityLog.objects.create(
+            action=amo.LOG.APPROVE_VERSION.id, user=user
+        )
         reason_log = ReviewActionReasonLog.objects.create(
             activity_log=activity_log,
             reason=reason_1,

src/olympia/activity/tests/test_models.py

@@ -162,8 +162,10 @@ class TestActivityLog(TestCase):
 
     def test_fancy_rendering(self):
         """HTML for Rating, and Collection."""
-        activity_log = ActivityLog.objects.create(action=amo.LOG.ADD_RATING.id)
         user = UserProfile.objects.create()
+        activity_log = ActivityLog.objects.create(
+            action=amo.LOG.ADD_RATING.id, user=user
+        )
         rating = Rating.objects.create(user=user, addon_id=3615)
         activity_log.arguments = [activity_log, rating]
         assert '>Review</a> for None written.' in activity_log.to_string()
@@ -172,7 +174,7 @@ class TestActivityLog(TestCase):
         assert 'None added to <a href="http://testserver/' in activity_log.to_string()
 
     def test_bad_arguments(self):
-        activity_log = ActivityLog()
+        activity_log = ActivityLog(user=UserProfile.objects.create())
         activity_log.arguments = []
         activity_log.action = amo.LOG.ADD_USER_WITH_ROLE.id
         assert activity_log.to_string() == 'Something magical happened.'
@@ -428,9 +430,10 @@ class TestActivityLog(TestCase):
         log = ActivityLog.objects.get()
 
         log_expected = (
-            'Yolo role changed to Owner for <a href="http://testserver/en-US/'
+            f'<a href="http://testserver/en-US/firefox/user/{self.user.pk}/">Yolo</a> '
+            'role changed to Owner for <a href="http://testserver/en-US/'
             'firefox/addon/a3615/">Delicious &lt;script src='
-            '&#34;x.js&#34;&gt;Bookmarks</a>.'
+            '&quot;x.js&quot;&gt;Bookmarks</a>.'
         )
         assert log.to_string() == log_expected
 

src/olympia/addons/tests/test_models.py

@@ -2061,7 +2061,10 @@ class TestAddonDelete(TestCase):
         FrozenAddon.objects.create(addon=addon)
 
         AddonLog.objects.create(
-            addon=addon, activity_log=ActivityLog.objects.create(action=0)
+            addon=addon,
+            activity_log=ActivityLog.objects.create(
+                action=0, user=UserProfile.objects.create()
+            ),
         )
         RssKey.objects.create(addon=addon)
 

src/olympia/addons/tests/test_utils_.py

@@ -214,9 +214,7 @@ class TestRestrictionChecker(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1
         activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get()
         assert activity.user == self.request.user
-        assert activity.iplog_set.all().count() == 1
-        ip_log = activity.iplog_set.all().get()
-        assert ip_log.ip_address_binary == IPv4Address('10.0.0.1')
+        assert activity.iplog.ip_address_binary == IPv4Address('10.0.0.1')
 
     def test_is_submission_allowed_bypassing_read_dev_agreement(self, incr_mock):
         self.request.user.update(read_dev_agreement=None)
@@ -262,9 +260,7 @@ class TestRestrictionChecker(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1
         activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get()
         assert activity.user == self.request.user
-        assert activity.iplog_set.all().count() == 1
-        ip_log = activity.iplog_set.all().get()
-        assert ip_log.ip_address_binary == IPv4Address('10.0.0.1')
+        assert activity.iplog.ip_address_binary == IPv4Address('10.0.0.1')
 
     def test_is_submission_allowed_email_restricted(self, incr_mock):
         EmailUserRestriction.objects.create(email_pattern=self.request.user.email)
@@ -290,9 +286,7 @@ class TestRestrictionChecker(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1
         activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get()
         assert activity.user == self.request.user
-        assert activity.iplog_set.all().count() == 1
-        ip_log = activity.iplog_set.all().get()
-        assert ip_log.ip_address_binary == IPv4Address('10.0.0.1')
+        assert activity.iplog.ip_address_binary == IPv4Address('10.0.0.1')
 
     def test_is_submission_allowed_bypassing_read_dev_agreement_restricted(
         self, incr_mock
@@ -325,9 +319,7 @@ class TestRestrictionChecker(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1
         activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get()
         assert activity.user == self.request.user
-        assert activity.iplog_set.all().count() == 1
-        ip_log = activity.iplog_set.all().get()
-        assert ip_log.ip_address_binary == IPv4Address('10.0.0.1')
+        assert activity.iplog.ip_address_binary == IPv4Address('10.0.0.1')
 
     def test_is_auto_approval_allowed_email_restricted_only_for_submission(
         self, incr_mock
@@ -380,11 +372,9 @@ class TestRestrictionChecker(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1
         activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get()
         assert activity.user == self.request.user
-        assert activity.iplog_set.all().count() == 1
-        ip_log = activity.iplog_set.all().get()
         # Note that there is no request in this case, the ip_adress is coming
         # from the upload.
-        assert ip_log.ip_address_binary == IPv4Address('10.0.0.2')
+        assert activity.iplog.ip_address_binary == IPv4Address('10.0.0.2')
 
     def test_no_request_or_upload_at_init(self, incr_mock):
         with self.assertRaises(ImproperlyConfigured):

src/olympia/constants/activity.py

@@ -50,7 +50,8 @@ class EDIT_CATEGORIES(_LOG):
 class ADD_USER_WITH_ROLE(_LOG):
     id = 5
     action_class = 'add'
-    format = _('{0.name} ({1}) added to {addon}.')
+    # L10n: {0} is the user role.
+    format = _('{user} ({0}) added to {addon}.')
     keep = True
     show_user_to_developer = True
 
@@ -58,8 +59,8 @@ class ADD_USER_WITH_ROLE(_LOG):
 class REMOVE_USER_WITH_ROLE(_LOG):
     id = 6
     action_class = 'delete'
-    # L10n: {0} is the user being removed, {1} is their role.
-    format = _('{0.name} ({1}) removed from {addon}.')
+    # L10n: {0} is the user role.
+    format = _('{user} ({0}) removed from {addon}.')
     keep = True
     show_user_to_developer = True
 
@@ -120,7 +121,7 @@ class DELETE_VERSION(_LOG):
 class ADD_FILE_TO_VERSION(_LOG):
     id = 19
     action_class = 'add'
-    format = _('File {0.name} added to {version} of {addon}.')
+    format = _('File {file} added to {version} of {addon}.')
 
 
 class DELETE_FILE_FROM_VERSION(_LOG):
@@ -313,8 +314,8 @@ class CHANGE_USER_WITH_ROLE(_LOG):
     """Expects: author.user, role, addon"""
 
     id = 36
-    # L10n: {0} is a user, {1} is their role
-    format = _('{0.name} role changed to {1} for {addon}.')
+    # L10n: {0} is the user role
+    format = _('{user} role changed to {0} for {addon}.')
     keep = True
     show_user_to_developer = True
 
@@ -346,7 +347,7 @@ class APPROVE_RATING(_LOG):
     id = 40
     action_class = 'approve'
     format = _('{rating} for {addon} approved.')
-    reviewer_format = _('{user} approved {rating} for {addon}.')
+    reviewer_format = '{user_responsible} approved {rating} for {addon}.'
     keep = True
     reviewer_event = True
 
@@ -357,7 +358,7 @@ class DELETE_RATING(_LOG):
     id = 41
     action_class = 'review'
     format = _('Review {rating} for {addon} deleted.')
-    reviewer_format = _('{user} deleted {rating} for {addon}.')
+    reviewer_format = '{user_responsible} deleted {rating} for {addon}.'
     keep = True
     reviewer_event = True
 
@@ -438,27 +439,27 @@ class OBJECT_DELETED(_LOG):
 
 class ADMIN_USER_EDITED(_LOG):
     id = 103
-    format = _('User {user} edited, reason: {1}')
+    format = 'User {user} edited by {user_responsible}'
     admin_event = True
 
 
 class ADMIN_USER_ANONYMIZED(_LOG):
     id = 104
-    format = _('User {user} anonymized.')
+    format = 'User {user} anonymized by {user_responsible}.'
     keep = True
     admin_event = True
 
 
 class ADMIN_USER_RESTRICTED(_LOG):
     id = 105
-    format = _('User {user} restricted.')
+    format = 'User {user} restricted by {user_responsible}.'
     keep = True
     admin_event = True
 
 
 class ADMIN_VIEWED_LOG(_LOG):
     id = 106
-    format = _('Admin {0} viewed activity log for {user}.')
+    format = 'Admin {user_responsible} viewed activity log for {user}.'
     admin_event = True
 
 
@@ -493,7 +494,7 @@ class ADMIN_USER_PICTURE_DELETED(_LOG):
 class GROUP_USER_ADDED(_LOG):
     id = 120
     action_class = 'access'
-    format = _('User {0.name} added to {group}.')
+    format = _('User {user} added to {group}.')
     keep = True
     admin_event = True
 
@@ -501,7 +502,7 @@ class GROUP_USER_ADDED(_LOG):
 class GROUP_USER_REMOVED(_LOG):
     id = 121
     action_class = 'access'
-    format = _('User {0.name} removed from {group}.')
+    format = _('User {user} removed from {group}.')
     keep = True
     admin_event = True
 
@@ -773,7 +774,7 @@ class ADMIN_USER_SESSION_RESET(_LOG):
 
 class THROTTLED(_LOG):
     id = 163
-    format = _('User {user} throttled for scope "{0}"')
+    format = 'User {user_responsible} throttled for scope "{0}"'
     admin_event = True
 
 
@@ -815,8 +816,8 @@ class FORCE_DISABLE(_LOG):
     # add-on is likely malicious.
     hide_developer = True
     reviewer_review_action = True
-    format = _('{user} force-disabled {addon}.')
-    short = _('Force disabled')
+    format = '{user_responsible} force-disabled {addon}.'
+    short = 'Force disabled'
 
 
 class FORCE_ENABLE(_LOG):
@@ -824,8 +825,8 @@ class FORCE_ENABLE(_LOG):
     keep = True
     hide_developer = True
     reviewer_review_action = True
-    format = _('{user} force-enabled {addon}.')
-    short = _('Force enabled')
+    format = '{user_responsible} force-enabled {addon}.'
+    short = 'Force enabled'
 
 
 class LOG_IN(_LOG):
@@ -835,7 +836,7 @@ class LOG_IN(_LOG):
     keep = True
     admin_event = True
     store_ip = True
-    format = _('{user} logged in.')
+    format = '{user_responsible} logged in.'
 
 
 class RESTRICTED(_LOG):
@@ -843,7 +844,7 @@ class RESTRICTED(_LOG):
     keep = True
     admin_event = True
     store_ip = True
-    format = _('{user} restricted.')
+    format = '{user_responsible} restricted.'
 
 
 class UNREJECT_VERSION(_LOG):
@@ -864,7 +865,7 @@ class LOG_IN_API_TOKEN(_LOG):
     keep = True
     admin_event = True
     store_ip = True
-    format = _('{user} authenticated through an API token.')
+    format = '{user_responsible} authenticated through an API token.'
 
 
 LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG]

src/olympia/devhub/tests/test_views_submit.py

@@ -2264,10 +2264,7 @@ class TestVersionSubmitUploadListed(VersionSubmitUploadMixin, UploadMixin, TestC
         )
         assert logs_qs.count() == 1
         log = logs_qs.get()
-        assert log.iplog_set.count() == 1
-        assert log.iplog_set.get().ip_address_binary == IPv4Address(
-            self.upload.ip_address
-        )
+        assert log.iplog.ip_address_binary == IPv4Address(self.upload.ip_address)
         self.statsd_incr_mock.assert_any_call('devhub.submission.version.listed')
 
     def test_experiment_inside_webext_upload_without_permission(self):

src/olympia/ratings/admin.py

@@ -117,9 +117,9 @@ class RatingAdmin(AMOModelAdmin):
     )
     list_display = (
         'id',
-        'addon',
+        'addon_link',
         'created',
-        'user',
+        'user_link',
         'known_ip_adresses',
         'rating',
         'is_reply',

src/olympia/ratings/tests/test_views.py

@@ -1734,9 +1734,7 @@ class TestRatingViewSetPost(TestCase):
         assert activity_log.user == self.user
         assert activity_log.arguments == [self.addon, review]
         assert activity_log.action == amo.LOG.ADD_RATING.id
-
-        ip_log = activity_log.iplog_set.get()
-        assert ip_log.ip_address_binary == IPv4Address('213.225.31.25')
+        assert activity_log.iplog.ip_address_binary == IPv4Address('213.225.31.25')
 
         assert len(mail.outbox) == 1
         assert mail.outbox[0].to == [addon_author.email]

src/olympia/reviewers/tests/test_views.py

@@ -4742,26 +4742,26 @@ class TestReview(ReviewBase):
         # change and deletion.
         author = self.addon.addonuser_set.get()
         core.set_user(author.user)
-        ActivityLog.create(
+        activity0 = ActivityLog.create(
             amo.LOG.ADD_USER_WITH_ROLE,
             author.user,
             str(author.get_role_display()),
             self.addon,
         )
-        ActivityLog.create(
+        activity1 = ActivityLog.create(
             amo.LOG.CHANGE_USER_WITH_ROLE,
             author.user,
             str(author.get_role_display()),
             self.addon,
         )
-        ActivityLog.create(
+        activity2 = ActivityLog.create(
             amo.LOG.REMOVE_USER_WITH_ROLE,
             author.user,
             str(author.get_role_display()),
             self.addon,
         )
-        ActivityLog.create(amo.LOG.FORCE_DISABLE, self.addon)
-        ActivityLog.create(
+        activity3 = ActivityLog.create(amo.LOG.FORCE_DISABLE, self.addon)
+        activity4 = ActivityLog.create(
             amo.LOG.FORCE_ENABLE,
             self.addon,
         )
@@ -4783,20 +4783,29 @@ class TestReview(ReviewBase):
         # Make sure the logs are displayed in the page.
         important_changes = doc('#important-changes-history li')
         assert len(important_changes) == 5
-        assert '(Owner) added to ' in important_changes[0].text_content()
+        assert important_changes[0].text_content() == (
+            f'{format_datetime(activity0.created)}: {activity1.user.name} '
+            '(Owner) added to Public.'
+        )
+        assert 'class' not in important_changes[0].attrib
+        assert important_changes[1].text_content() == (
+            f'{format_datetime(activity1.created)}: {activity1.user.name} '
+            'role changed to Owner for Public.'
+        )
         assert 'class' not in important_changes[1].attrib
-        assert 'role changed to Owner for ' in important_changes[1].text_content()
-        assert 'class' not in important_changes[1].attrib
-        assert '(Owner) removed from ' in important_changes[2].text_content()
+        assert important_changes[2].text_content() == (
+            f'{format_datetime(activity2.created)}: {activity1.user.name} '
+            '(Owner) removed from Public.'
+        )
         assert 'class' not in important_changes[2].attrib
-        assert (
-            'regularuser التطب force-disabled Public.'
-            in important_changes[3].text_content()
+        assert important_changes[3].text_content() == (
+            f'{format_datetime(activity3.created)}: {activity1.user.name} '
+            'force-disabled Public.'
         )
         assert important_changes[3].attrib['class'] == 'reviewer-review-action'
-        assert (
-            'regularuser التطب force-enabled Public.'
-            in important_changes[4].text_content()
+        assert important_changes[4].text_content() == (
+            f'{format_datetime(activity4.created)}: {activity1.user.name} '
+            'force-enabled Public.'
         )
         assert important_changes[4].attrib['class'] == 'reviewer-review-action'
 

src/olympia/users/tests/test_models.py

@@ -56,8 +56,7 @@ class TestUserProfile(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.LOG_IN.id).count() == 1
         log = ActivityLog.objects.filter(action=amo.LOG.LOG_IN.id).latest('pk')
         assert log.user == user
-        ip_log = log.iplog_set.get()
-        assert ip_log.ip_address_binary == IPv4Address('4.8.15.16')
+        assert log.iplog.ip_address_binary == IPv4Address('4.8.15.16')
 
         with core.override_remote_addr('23.42.42.42'):
             self.client.force_login(user)
@@ -65,8 +64,7 @@ class TestUserProfile(TestCase):
         assert ActivityLog.objects.filter(action=amo.LOG.LOG_IN.id).count() == 2
         log = ActivityLog.objects.filter(action=amo.LOG.LOG_IN.id).latest('pk')
         assert log.user == user
-        ip_log = log.iplog_set.get()
-        assert ip_log.ip_address_binary == IPv4Address('23.42.42.42')
+        assert log.iplog.ip_address_binary == IPv4Address('23.42.42.42')
 
     def test_is_addon_developer(self):
         user = user_factory()