bug 621800, reorders don't log changes

apps/addons/models.py

@@ -1062,6 +1062,11 @@ class AddonUser(caching.CachingMixin, models.Model):
 
     objects = caching.CachingManager()
 
+    def __init__(self, *args, **kwargs):
+        super(AddonUser, self).__init__(*args, **kwargs)
+        self._original_role = self.role
+        self._original_user_id = self.user_id
+
     class Meta:
         db_table = 'addons_users'
 

apps/amo/log.py

@@ -377,7 +377,7 @@ def log(action, *args, **kw):
             if arg[0] == Addon:
                 AddonLog(addon_id=arg[1], activity_log=al).save()
             elif arg[0] == UserProfile:
-                AddonLog(user_id=arg[1], activity_log=al).save()
+                UserLog(user_id=arg[1], activity_log=al).save()
         if isinstance(arg, Addon):
             AddonLog(addon=arg, activity_log=al).save()
         elif isinstance(arg, UserProfile):

apps/devhub/tests/test_views_ownership.py

@@ -178,6 +178,29 @@ class TestEditLicense(TestOwnership):
 
 
 class TestEditAuthor(TestOwnership):
+    def test_reorder_authors(self):
+        """
+        Re-ordering authors should not generate role changes in the
+        ActivityLog.
+        """
+        # flip form-0-position
+        f = self.client.get(self.url).context['user_form'].initial_forms[0]
+        u = dict(user='regular@mozilla.com', listed=True,
+                 role=amo.AUTHOR_ROLE_DEV, position=0)
+        data = self.formset(f.initial, u, initial_count=1)
+        r = self.client.post(self.url, data)
+        eq_(r.status_code, 302)
+        f = self.client.get(self.url).context['user_form'].initial_forms[0]
+        u1 = f.initial
+        u1['position'] = 1
+        f = self.client.get(self.url).context['user_form'].initial_forms[1]
+        u2 = f.initial
+        data = self.formset(u1, u2)
+
+        eq_(ActivityLog.objects.all().count(), 2)
+        r = self.client.post(self.url, data)
+        eq_(r.status_code, 302)
+        eq_(ActivityLog.objects.all().count(), 2)
 
     def test_success_add_user(self):
         q = (AddonUser.objects.no_cache().filter(addon=3615)
@@ -236,18 +259,6 @@ class TestEditAuthor(TestOwnership):
         eq_(r.status_code, 302)
         eq_(999, AddonUser.objects.get(addon=3615).user_id)
 
-    def test_logs(self):
-        # A copy of switch ownership to test logs
-        f = self.client.get(self.url).context['user_form'].initial_forms[0]
-        f.initial['user'] = 'regular@mozilla.com'
-        data = self.formset(f.initial, initial_count=1)
-        o = ActivityLog.objects
-        eq_(o.count(), 0)
-        r = self.client.post(self.url, data)
-        eq_(o.filter(action=amo.LOG.CHANGE_USER_WITH_ROLE.id).count(), 1)
-        eq_(r.status_code, 302)
-        eq_(999, AddonUser.objects.get(addon=3615).user_id)
-
     def test_switch_owner(self):
         # See if we can transfer ownership in one POST.
         f = self.client.get(self.url).context['user_form'].initial_forms[0]
@@ -256,6 +267,10 @@ class TestEditAuthor(TestOwnership):
         r = self.client.post(self.url, data)
         eq_(r.status_code, 302)
         eq_(999, AddonUser.objects.get(addon=3615).user_id)
+        eq_(ActivityLog.objects.filter(
+            action=amo.LOG.ADD_USER_WITH_ROLE.id).count(), 1)
+        eq_(ActivityLog.objects.filter(
+            action=amo.LOG.REMOVE_USER_WITH_ROLE.id).count(), 1)
 
     def test_only_owner_can_edit(self):
         f = self.client.get(self.url).context['user_form'].initial_forms[0]

apps/devhub/views.py

@@ -38,6 +38,7 @@ from devhub.models import ActivityLog, RssKey, SubmitStep
 from files.models import File, FileUpload
 from files.utils import parse_addon
 from translations.models import delete_translation
+from users.models import UserProfile
 from versions.models import License, Version
 
 from . import forms, tasks, feeds
@@ -366,11 +367,21 @@ def ownership(request, addon_id, addon):
         # Authors.
         authors = user_form.save(commit=False)
         for author in authors:
-            action = (amo.LOG.CHANGE_USER_WITH_ROLE if author.id
-                      else amo.LOG.ADD_USER_WITH_ROLE)
-            author.addon = addon
+            action = None
+            if not author.id or author.user_id != author._original_user_id:
+                action = amo.LOG.ADD_USER_WITH_ROLE
+                author.addon = addon
+            elif author.role != author._original_role:
+                action = amo.LOG.CHANGE_USER_WITH_ROLE
+
             author.save()
-            amo.log(action, author.user, author.get_role_display(), addon)
+            if action:
+                amo.log(action, author.user, author.get_role_display(), addon)
+            if (author._original_user_id and
+                author.user_id != author._original_user_id):
+                amo.log(amo.LOG.REMOVE_USER_WITH_ROLE,
+                        (UserProfile, author._original_user_id),
+                        author.get_role_display(), addon)
 
         for author in user_form.deleted_objects:
             amo.log(amo.LOG.REMOVE_USER_WITH_ROLE, author.user,
@@ -1003,7 +1014,6 @@ def _resume(addon, step):
     return redirect('devhub.versions', addon.slug)
 
 
-
 @login_required
 @dev_required
 def submit_bump(request, addon_id, addon):