Merge pull request #7428 from diox/status-disabling-activity-log

Make ActivityLog creation with amo.LOG.CHANGE_STATUS more consistent

src/olympia/activity/models.py

@@ -366,6 +366,7 @@ class ActivityLog(ModelBase):
         tag = None
         group = None
         file_ = None
+        status = None
 
         for arg in self.arguments:
             if isinstance(arg, Addon) and not addon:
@@ -414,6 +415,21 @@ class ActivityLog(ModelBase):
                                arg.filename,
                                validation)
                 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
+                # an int, we want to retrieve the string and translate it.
+                # Note that we use STATUS_CHOICES_PERSONA because it's a
+                # superset of STATUS_CHOICES_ADDON, and we need to handle all
+                # statuses.
+                if isinstance(arg, int) and arg in amo.STATUS_CHOICES_PERSONA:
+                    status = ugettext(amo.STATUS_CHOICES_PERSONA[arg])
+                else:
+                    # It's not an int or not one of the choices, so assume it's
+                    # a string or an unknown int we want to display as-is.
+                    status = arg
+                arguments.remove(arg)
 
         user = user_link(self.user)
 
@@ -427,6 +443,7 @@ class ActivityLog(ModelBase):
                 'user': user,
                 'group': group,
                 'file': file_,
+                'status': status,
             }
             return self.f(format, *arguments, **kw)
         except (AttributeError, KeyError, IndexError):

src/olympia/activity/tests/test_models.py

@@ -292,6 +292,35 @@ class TestActivityLog(TestCase):
             reverse('files.list', args=[file_.pk]), file_.filename)
         assert log == msg
 
+    def test_change_status(self):
+        addon = Addon.objects.get()
+        log = ActivityLog.create(
+            amo.LOG.CHANGE_STATUS, addon, amo.STATUS_PUBLIC)
+        expected = ('<a href="/en-US/firefox/addon/a3615/">'
+                    'Delicious Bookmarks</a> status changed to Approved.')
+        assert unicode(log) == expected
+
+        log.arguments = [amo.STATUS_DISABLED, addon]
+        expected = ('<a href="/en-US/firefox/addon/a3615/">'
+                    'Delicious Bookmarks</a> status changed to '
+                    'Disabled by Mozilla.')
+        assert unicode(log) == expected
+
+        log.arguments = [addon, amo.STATUS_REJECTED]
+        expected = ('<a href="/en-US/firefox/addon/a3615/">'
+                    'Delicious Bookmarks</a> status changed to Rejected.')
+        assert unicode(log) == expected
+
+        log.arguments = [addon, 666]
+        expected = ('<a href="/en-US/firefox/addon/a3615/">'
+                    'Delicious Bookmarks</a> status changed to 666.')
+        assert unicode(log) == expected
+
+        log.arguments = [addon, 'Some String']
+        expected = ('<a href="/en-US/firefox/addon/a3615/">'
+                    'Delicious Bookmarks</a> status changed to Some String.')
+        assert unicode(log) == expected
+
 
 class TestActivityLogCount(TestCase):
     fixtures = ['base/addon_3615']

src/olympia/addons/models.py

@@ -971,8 +971,7 @@ class Addon(OnChangeMixin, ModelBase):
             log.info('Changing add-on status [%s]: %s => %s (%s).'
                      % (self.id, self.status, status, reason))
             self.update(status=status)
-            activity.log_create(amo.LOG.CHANGE_STATUS,
-                                self.get_status_display(), self)
+            activity.log_create(amo.LOG.CHANGE_STATUS, self, self.status)
 
         self.update_version(ignore=ignore_version)
 

src/olympia/constants/activity.py

@@ -76,8 +76,8 @@ class USER_ENABLE(_LOG):
 
 class CHANGE_STATUS(_LOG):
     id = 12
-    # L10n: {0} is the status
-    format = _(u'{addon} status changed to {0}.')
+    # L10n: {status} is the status
+    format = _(u'{addon} status changed to {status}.')
     keep = True
 
 

src/olympia/devhub/views.py

@@ -244,12 +244,14 @@ def _get_activities(request, action):
 
 
 def _get_items(action, addons):
-    filters = dict(updates=(amo.LOG.ADD_VERSION, amo.LOG.ADD_FILE_TO_VERSION),
-                   status=(amo.LOG.USER_DISABLE, amo.LOG.USER_ENABLE,
-                           amo.LOG.CHANGE_STATUS, amo.LOG.APPROVE_VERSION,),
-                   collections=(amo.LOG.ADD_TO_COLLECTION,
-                                amo.LOG.REMOVE_FROM_COLLECTION,),
-                   reviews=(amo.LOG.ADD_RATING,))
+    filters = {
+        'updates': (amo.LOG.ADD_VERSION, amo.LOG.ADD_FILE_TO_VERSION),
+        'status': (amo.LOG.USER_DISABLE, amo.LOG.USER_ENABLE,
+                   amo.LOG.CHANGE_STATUS, amo.LOG.APPROVE_VERSION,),
+        'collections': (amo.LOG.ADD_TO_COLLECTION,
+                        amo.LOG.REMOVE_FROM_COLLECTION,),
+        'reviews': (amo.LOG.ADD_RATING,)
+    }
 
     filter_ = filters.get(action)
     items = (ActivityLog.objects.for_addons(addons)
@@ -406,8 +408,7 @@ def enable(request, addon_id, addon):
 def cancel(request, addon_id, addon):
     if addon.status == amo.STATUS_NOMINATED:
         addon.update(status=amo.STATUS_NULL)
-        ActivityLog.create(amo.LOG.CHANGE_STATUS, addon.get_status_display(),
-                           addon)
+        ActivityLog.create(amo.LOG.CHANGE_STATUS, addon, addon.status)
     latest_version = addon.find_latest_version(
         channel=amo.RELEASE_CHANNEL_LISTED)
     if latest_version:
@@ -1701,8 +1702,7 @@ def request_review(request, addon_id, addon):
     else:
         messages.success(request, _(
             'You must provide further details to proceed.'))
-    ActivityLog.create(amo.LOG.CHANGE_STATUS, addon.get_status_display(),
-                       addon)
+    ActivityLog.create(amo.LOG.CHANGE_STATUS, addon, addon.status)
     return redirect(addon.get_dev_url('versions'))