Log name|summary changes in more detail (#23725)

* Log name|summary changes in more detail

* fix tests for nondeterministic ordering

* store removed, added strings in arguments instead

Author: eviljeff

src/olympia/addons/serializers.py

@@ -1,4 +1,5 @@
 import copy
+import json
 import re
 
 from django.core.exceptions import ValidationError as DjangoValidationError
@@ -80,6 +81,7 @@
 from .tasks import resize_icon, resize_preview
 from .utils import (
     fetch_translations_from_addon,
+    get_translation_differences,
     validate_version_number_is_gt_latest_signed_listed_version,
 )
 from .validators import (
@@ -1298,7 +1300,7 @@ def _save_icon(self, uploaded_icon):
             remove_icons(self.instance)
             self.instance.update(icon_type='')
 
-    def log(self, instance, validated_data):
+    def log(self, instance, validated_data, changes):
         validated_data = {**validated_data}  # we want to modify it, so take a copy
         user = self.context['request'].user
 
@@ -1319,6 +1321,15 @@ def log(self, instance, validated_data):
             # version is always a new object, and not a property either
             validated_data.pop('version')
 
+        for field, addedremoved in changes.items():
+            ActivityLog.objects.create(
+                amo.LOG.EDIT_ADDON_PROPERTY,
+                instance,
+                field,
+                json.dumps(addedremoved),
+                user=user,
+            )
+            validated_data.pop(field)
         if validated_data:
             ActivityLog.objects.create(
                 amo.LOG.EDIT_PROPERTIES,
@@ -1356,17 +1367,19 @@ def update(self, instance, validated_data):
             if instance.has_listed_versions()
             else None
         )
+        changes = {}
 
         old_slug = instance.slug
 
         if 'icon' in validated_data:
             self._save_icon(validated_data['icon'])
         instance = super().update(instance, validated_data)
 
-        if old_metadata is not None and old_metadata != fetch_translations_from_addon(
-            instance, fields_to_review
+        if old_metadata is not None and old_metadata != (
+            new_metadata := fetch_translations_from_addon(instance, fields_to_review)
         ):
             statsd.incr('addons.submission.metadata_content_review_triggered')
+            changes = get_translation_differences(old_metadata, new_metadata)
             AddonApprovalsCounter.reset_content_for_addon(addon=instance)
         if 'all_categories' in validated_data:
             del instance.all_categories  # super.update will have set it.
@@ -1386,7 +1399,7 @@ def update(self, instance, validated_data):
                 amo.LOG.ADDON_SLUG_CHANGED, instance, old_slug, instance.slug
             )
 
-        self.log(instance, validated_data)
+        self.log(instance, validated_data, changes)
         return instance
 
 

src/olympia/addons/tests/test_views.py

@@ -1961,6 +1961,7 @@ def setUp(self):
         self.statsd_incr_mock = self.patch('olympia.addons.serializers.statsd.incr')
 
     def test_basic(self):
+        original_summary = str(self.addon.summary)
         response = self.request(data={'summary': {'en-US': 'summary update!'}})
         self.addon.reload()
         assert response.status_code == 200, response.content
@@ -1990,8 +1991,12 @@ def test_basic(self):
             )
         ).get()
         assert alog.user == self.user
-        assert alog.action == amo.LOG.EDIT_PROPERTIES.id
-        assert alog.details == ['summary']
+        assert alog.action == amo.LOG.EDIT_ADDON_PROPERTY.id
+        assert alog.arguments == [
+            self.addon,
+            'summary',
+            json.dumps({'removed': [original_summary], 'added': ['summary update!']}),
+        ]
         return data
 
     @override_settings(API_THROTTLING=False)
@@ -2212,6 +2217,10 @@ def test_set_slug_log(self):
     def test_set_extra_data(self):
         self.addon.description = 'Existing description'
         self.addon.save()
+        original_data = {
+            'name': str(self.addon.name),
+            'summary': str(self.addon.summary),
+        }
         patch_data = {
             'developer_comments': {'en-US': 'comments'},
             'homepage': {'en-US': 'https://my.home.page/'},
@@ -2251,17 +2260,48 @@ def test_set_extra_data(self):
         assert addon.support_email == '[email protected]'
         assert data['support_url']['url'] == {'en-US': 'https://my.home.page/support/'}
         assert addon.support_url == 'https://my.home.page/support/'
+
+        summ_log, name_log = list(
+            ActivityLog.objects.filter(action=amo.LOG.EDIT_ADDON_PROPERTY.id)
+        )
+        if name_log.arguments[1] != 'name':
+            # The order isn't deterministic, it doesn't matter, so just switch em.
+            name_log, summ_log = summ_log, name_log
+        assert name_log.arguments == [
+            self.addon,
+            'name',
+            json.dumps(
+                {
+                    'removed': [original_data['name']],
+                    'added': [patch_data['name']['en-US']],
+                }
+            ),
+        ]
+        assert summ_log.arguments == [
+            self.addon,
+            'summary',
+            json.dumps(
+                {
+                    'removed': [original_data['summary']],
+                    'added': [patch_data['summary']['en-US']],
+                }
+            ),
+        ]
+
         alog = ActivityLog.objects.exclude(
             action__in=(
                 amo.LOG.ADD_VERSION.id,
                 amo.LOG.LOG_IN.id,
                 amo.LOG.LOG_IN_API_TOKEN.id,
                 amo.LOG.ADDON_SLUG_CHANGED.id,
+                amo.LOG.EDIT_ADDON_PROPERTY.id,
             )
         ).get()
         assert alog.user == self.user
         assert alog.action == amo.LOG.EDIT_PROPERTIES.id
-        assert alog.details == list(patch_data.keys())
+        assert alog.details == [
+            prop for prop in patch_data if prop not in ('name', 'summary')
+        ]
 
     def test_set_disabled(self):
         response = self.request(
@@ -2500,7 +2540,7 @@ def test_delete_icon_formdata(self):
 
     def _test_metadata_content_review(self):
         response = self.request(
-            data={'name': {'en-US': 'new name'}, 'summary': {'en-US': 'new summary'}},
+            data={'name': {'en-US': 'new name'}, 'summary': {'fr': 'summary nouveau'}},
         )
         assert response.status_code == 200
 
@@ -2526,6 +2566,7 @@ def test_metadata_content_review_unlisted(self, fetch_mock):
         )
 
     def test_metadata_change_triggers_content_review(self):
+        old_name = str(self.addon.name)
         AddonApprovalsCounter.approve_content_for_addon(addon=self.addon)
         assert AddonApprovalsCounter.objects.get(addon=self.addon).last_content_review
 
@@ -2539,6 +2580,26 @@ def test_metadata_change_triggers_content_review(self):
         self.statsd_incr_mock.assert_any_call(
             'addons.submission.metadata_content_review_triggered'
         )
+        assert (
+            ActivityLog.objects.filter(action=amo.LOG.EDIT_ADDON_PROPERTY.id).count()
+            == 2
+        )
+        summ_log, name_log = list(
+            ActivityLog.objects.filter(action=amo.LOG.EDIT_ADDON_PROPERTY.id)
+        )
+        if name_log.arguments[1] != 'name':
+            # The order isn't deterministic, it doesn't matter, so just switch em.
+            name_log, summ_log = summ_log, name_log
+        assert name_log.arguments == [
+            self.addon,
+            'name',
+            json.dumps({'removed': [old_name], 'added': ['new name']}),
+        ]
+        assert summ_log.arguments == [
+            self.addon,
+            'summary',
+            json.dumps({'removed': [], 'added': ['summary nouveau']}),
+        ]
 
     def test_metadata_change_same_content(self):
         AddonApprovalsCounter.approve_content_for_addon(addon=self.addon)
@@ -2547,7 +2608,7 @@ def test_metadata_change_same_content(self):
         ).last_content_review
         assert old_content_review
         self.addon.name = {'en-US': 'new name'}
-        self.addon.summary = {'en-US': 'new summary'}
+        self.addon.summary = {'en-US': str(self.addon.summary), 'fr': 'summary nouveau'}
         self.addon.save()
 
         self._test_metadata_content_review()

src/olympia/addons/utils.py

@@ -1,4 +1,5 @@
 import uuid
+from collections import defaultdict
 
 from django import forms
 from django.core.files.storage import default_storage as storage
@@ -89,10 +90,25 @@ def compute_last_updated(addon):
 
 
 def fetch_translations_from_addon(addon, properties):
-    translation_ids_gen = (getattr(addon, prop + '_id', None) for prop in properties)
-    translation_ids = [id_ for id_ in translation_ids_gen if id_]
+    translation_ids_gen = (
+        (prop, getattr(addon, prop + '_id', None)) for prop in properties
+    )
+    translation_ids = {id_: prop for prop, id_ in translation_ids_gen if id_}
     # Just get all the values together to make it simplier
-    return {str(value) for value in Translation.objects.filter(id__in=translation_ids)}
+    out = defaultdict(set)
+    for trans in Translation.objects.filter(id__in=translation_ids):
+        out[translation_ids.get(trans.id, '_')].add(str(trans))
+    return out
+
+
+def get_translation_differences(old_, new_):
+    out = {}
+    for field in list({*old_, *new_}):
+        removed = list(old_[field] - new_[field])
+        added = list(new_[field] - old_[field])
+        if removed or added:
+            out[field] = {'removed': removed, 'added': added}
+    return out
 
 
 class DeleteTokenSigner(TimestampSigner):

src/olympia/constants/activity.py

@@ -28,7 +28,7 @@ class CREATE_ADDON(_LOG):
 
 
 class EDIT_PROPERTIES(_LOG):
-    """Expects: addon"""
+    """Expects: addon. Consider using EDIT_ADDON_PROPERTY instead"""
 
     id = 2
     action_class = 'edit'
@@ -1238,6 +1238,15 @@ class DISABLE_AUTO_APPROVAL(_LOG):
     hide_developer = True
 
 
+class EDIT_ADDON_PROPERTY(_LOG):
+    """Expects: addon, field. 3rd arg is a json blob."""
+
+    id = 208
+    action_class = 'edit'
+    format = _('{addon} {0} property edited.')
+    show_user_to_developer = True
+
+
 LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG]
 # Make sure there's no duplicate IDs.
 assert len(LOGS) == len({log.id for log in LOGS})

src/olympia/devhub/forms.py

@@ -35,6 +35,7 @@
 )
 from olympia.addons.utils import (
     fetch_translations_from_addon,
+    get_translation_differences,
     remove_icons,
     validate_version_number_is_gt_latest_signed_listed_version,
     verify_mozilla_trademark,
@@ -134,15 +135,17 @@ def save(self, *args, **kwargs):
             else {}
         )
         obj = super().save(*args, **kwargs)
-        if not metadata_content_review:
-            return obj
-        new_data = fetch_translations_from_addon(
-            obj, self.fields_to_trigger_content_review
-        )
-        if existing_data != new_data:
-            # flag for content review
-            statsd.incr('devhub.metadata_content_review_triggered')
-            AddonApprovalsCounter.reset_content_for_addon(addon=obj)
+        if metadata_content_review:
+            new_data = fetch_translations_from_addon(
+                obj, self.fields_to_trigger_content_review
+            )
+            if existing_data != new_data:
+                self.metadata_changes = get_translation_differences(
+                    existing_data, new_data
+                )
+                # flag for content review
+                statsd.incr('devhub.metadata_content_review_triggered')
+                AddonApprovalsCounter.reset_content_for_addon(addon=obj)
         return obj
 
 

src/olympia/devhub/tests/test_views_edit.py

@@ -321,7 +321,37 @@ def test_metadata_change_triggers_content_review(self):
         assert str(addon.name) == data['name']
         assert str(addon.summary) == data['summary']
 
+        if self.listed:
+            # check we logged the changes
+            alogs = ActivityLog.objects.filter(action=amo.LOG.EDIT_ADDON_PROPERTY.id)
+            assert alogs.count() == 2
+            name_log, summ_log = list(
+                ActivityLog.objects.filter(action=amo.LOG.EDIT_ADDON_PROPERTY.id)
+            )
+            if name_log.arguments[1] != 'name':
+                # The order isn't deterministic, it doesn't matter, so just switch em.
+                name_log, summ_log = summ_log, name_log
+            assert name_log.arguments == [
+                self.addon,
+                'name',
+                json.dumps({'removed': ['Delicious Bookmarks'], 'added': ['new name']}),
+            ]
+            assert summ_log.arguments == [
+                self.addon,
+                'summary',
+                json.dumps(
+                    {
+                        'removed': ['Delicious Bookmarks is the official'],
+                        'added': ['new summary'],
+                    }
+                ),
+            ]
+        else:
+            alogs = ActivityLog.objects.filter(action=amo.LOG.EDIT_PROPERTIES.id)
+            assert alogs.count() == 1
+
         # Now repeat, but we won't be changing either name or summary
+        alogs.delete()
         AddonApprovalsCounter.approve_content_for_addon(addon=addon)
         assert AddonApprovalsCounter.objects.get(addon=addon).last_content_review
         data['description'] = 'its a totally new description!'
@@ -332,6 +362,10 @@ def test_metadata_change_triggers_content_review(self):
 
         # Still keeps its date this time, so no new content review
         assert AddonApprovalsCounter.objects.get(addon=addon).last_content_review
+        # no changes logged for name or summary this time
+        assert not ActivityLog.objects.filter(
+            action=amo.LOG.EDIT_ADDON_PROPERTY.id
+        ).exists()
         # And metadata was updated
         assert str(addon.description) == data['description']
 

src/olympia/devhub/views.py

@@ -1,4 +1,5 @@
 import datetime
+import json
 import os
 import time
 from copy import deepcopy
@@ -919,6 +920,15 @@ def addons_section(request, addon_id, addon, section, editable=False):
                 if section == 'media':
                     ActivityLog.objects.create(amo.LOG.CHANGE_MEDIA, addon)
                 else:
+                    metadata_changes = getattr(main_form, 'metadata_changes', {})
+                    for field, addedremoved in metadata_changes.items():
+                        ActivityLog.objects.create(
+                            amo.LOG.EDIT_ADDON_PROPERTY,
+                            addon,
+                            field,
+                            json.dumps(addedremoved),
+                        )
+
                     ActivityLog.objects.create(amo.LOG.EDIT_PROPERTIES, addon)
 
                 if valid_slug != addon.slug: