Expose authors (and their role, included deleted) in add-on admin list (#16133)

* Expose authors (and their role, included deleted) in add-on admin list

src/olympia/addons/admin.py

@@ -15,7 +15,7 @@ from django.http.response import (
 from django.shortcuts import get_object_or_404
 from django.urls import resolve
 from django.utils.encoding import force_text
-from django.utils.html import format_html
+from django.utils.html import format_html, format_html_join
 from django.utils.translation import ugettext, ugettext_lazy as _
 
 import olympia.core.logger
@@ -147,16 +147,22 @@ class FileInline(admin.TabularInline):
 
 class AddonAdmin(admin.ModelAdmin):
     class Media:
-        css = {'all': ('css/admin/l10n.css', 'css/admin/pagination.css')}
+        css = {
+            'all': (
+                'css/admin/l10n.css',
+                'css/admin/pagination.css',
+                'css/admin/addons.css',
+            )
+        }
         js = ('admin/js/jquery.init.js', 'js/admin/l10n.js', 'js/admin/recalc_hash.js')
 
-    exclude = ('authors',)
     list_display = (
         '__str__',
         'type',
         'guid',
         'status',
         'average_rating',
+        'authors_links',
         'reviewer_links',
     )
     list_filter = ('type', 'status')
@@ -243,8 +249,12 @@ class AddonAdmin(admin.ModelAdmin):
     )
     actions = ['git_extract_action']
 
-    def queryset(self, request):
-        return models.Addon.unfiltered
+    def get_queryset(self, request):
+        return (
+            Addon.unfiltered.all()
+            .only_translations()
+            .transform(Addon.attach_all_authors)
+        )
 
     def get_urls(self):
         def wrap(view):
@@ -263,13 +273,45 @@ class AddonAdmin(admin.ModelAdmin):
         ]
         return custom_urlpatterns + urlpatterns
 
+    def authors_links(self, obj):
+        # Note: requires .transform(Addon.attach_all_authors) to have been
+        # applied to fill all_authors property and role on each user in it.
+        authors = obj.all_authors
+        return (
+            format_html(
+                '<ul>{}</ul>',
+                format_html_join(
+                    '',
+                    '<li><a href="{}">{} ({}{})</a></li>',
+                    (
+                        (
+                            urljoin(
+                                settings.EXTERNAL_SITE_URL,
+                                reverse(
+                                    'admin:users_userprofile_change', args=(author.pk,)
+                                ),
+                            ),
+                            author.email,
+                            dict(amo.AUTHOR_CHOICES_UNFILTERED)[author.role],
+                            ', Not listed' if author.listed is False else '',
+                        )
+                        for author in authors
+                    ),
+                ),
+            )
+            if authors
+            else '-'
+        )
+
+    authors_links.short_description = _('Authors')
+
     def total_ratings_link(self, obj):
         return related_content_link(
             obj,
             Rating,
             'addon',
             related_manager='without_replies',
-            count=obj.total_ratings,
+            text=obj.total_ratings,
         )
 
     total_ratings_link.short_description = _(u'Ratings')
@@ -305,10 +347,8 @@ class AddonAdmin(admin.ModelAdmin):
         if lookup_field != 'pk':
             addon = None
             try:
-                if lookup_field == 'slug':
-                    addon = self.queryset(request).all().get(slug=object_id)
-                elif lookup_field == 'guid':
-                    addon = self.queryset(request).get(guid=object_id)
+                if lookup_field in ('slug', 'guid'):
+                    addon = self.get_queryset(request).get(**{lookup_field: object_id})
             except Addon.DoesNotExist:
                 raise http.Http404
             # Don't get in an infinite loop if addon.slug.isdigit().

src/olympia/addons/models.py

@@ -1158,27 +1158,67 @@ class Addon(OnChangeMixin, ModelBase):
             version.addon = addon
 
     @staticmethod
-    def attach_listed_authors(addons, addon_dict=None):
+    def _attach_authors(
+        addons,
+        addon_dict=None,
+        manager='objects',
+        listed=True,
+        to_attr='listed_authors',
+    ):
+        # It'd be nice if this could be done with something like
+        # qs.prefetch_related(
+        #     Prefetch('authors', queryset=UserProfile.objects.annotate(
+        #         role=F('addonuser__role'), listed=F('addonuser__listed'))))
+        # instead, but that doesn't work because the prefetch queryset is
+        # making a different join for addonuser than the one used by the
+        # manytomanyfield, so the results are completely wrong when there are
+        # more than one add-on. Also this wouldn't let us customize the
+        # AddonUser manager to include/exclude deleted roles.
+        # So instead, we do it via AddonUser, copy the properties on the users
+        # and throw away the AddonUser instances afterwards.
         if addon_dict is None:
             addon_dict = {addon.id: addon for addon in addons}
 
+        filters = {'addon__in': addons}
+        if listed is not None:
+            filters['listed'] = listed
+        addonuser_qs = getattr(AddonUser, manager).all()
         addonuser_qs = (
-            AddonUser.objects.filter(addon__in=addons, listed=True)
+            addonuser_qs.filter(**filters)
             .order_by('addon_id', 'position')
             .select_related('user')
         )
         seen = set()
         groupby = itertools.groupby(addonuser_qs, key=lambda u: u.addon_id)
         for addon_id, addonusers in groupby:
-            addon_dict[addon_id].listed_authors = [au.user for au in addonusers]
+            authors = []
+            for addonuser in addonusers:
+                setattr(addonuser.user, 'role', addonuser.role)
+                setattr(addonuser.user, 'listed', addonuser.listed)
+                authors.append(addonuser.user)
+            setattr(addon_dict[addon_id], to_attr, authors)
             seen.add(addon_id)
-        # set listed_authors to empty list on addons without listed authors.
+        # set authors to empty list on addons without any.
         [
-            setattr(addon, 'listed_authors', [])
+            setattr(addon, to_attr, [])
             for addon in addon_dict.values()
             if addon.id not in seen
         ]
 
+    @staticmethod
+    def attach_listed_authors(addons, addon_dict=None):
+        Addon._attach_authors(addons, addon_dict=addon_dict)
+
+    @staticmethod
+    def attach_all_authors(addons, addon_dict=None):
+        Addon._attach_authors(
+            addons,
+            addon_dict=addon_dict,
+            manager='unfiltered',
+            listed=None,
+            to_attr='all_authors',
+        )
+
     @staticmethod
     def attach_previews(addons, addon_dict=None, no_transforms=False):
         if addon_dict is None:

src/olympia/addons/tests/test_admin.py

@@ -154,6 +154,21 @@ class TestAddonAdmin(TestCase):
         assert b'Reviewer Tools (listed)' in response.content
         assert b'Reviewer Tools (unlisted)' in response.content
 
+    def test_list_queries(self):
+        addon_factory(guid='@foo')
+        addon_factory(guid='@bar')
+        addon_factory(guid='@xyz')
+        user = user_factory(email='someone@mozilla.com')
+        self.grant_permission(user, 'Addons:Edit')
+        self.client.login(email=user.email)
+
+        with self.assertNumQueries(12):
+            # FIXME: explain each query, lower count (#16132 should fix it
+            # the scaling, currently there is one query per add-on for the
+            # unlisted reviewer exists() query)
+            response = self.client.get(self.list_url, follow=True)
+            assert response.status_code == 200
+
     def test_can_edit_with_addons_edit_permission(self):
         addon = addon_factory(guid='@foo')
         self.detail_url = reverse('admin:addons_addon_change', args=(addon.pk,))
@@ -499,16 +514,18 @@ class TestAddonAdmin(TestCase):
         self.grant_permission(user, 'Addons:Edit')
         self.grant_permission(user, 'Admin:Advanced')
         self.client.login(email=user.email)
-        with self.assertNumQueries(26):
+        with self.assertNumQueries(20):
             # It's very high because most of AddonAdmin is unoptimized but we
             # don't want it unexpectedly increasing.
+            # FIXME: explain each query
             response = self.client.get(self.detail_url, follow=True)
         assert response.status_code == 200
         assert addon.guid in response.content.decode('utf-8')
 
         version_factory(addon=addon)
-        with self.assertNumQueries(26):
-            # confirm it scales
+        with self.assertNumQueries(20):
+            # Confirm it scales
+            # FIXME: explain each query
             response = self.client.get(self.detail_url, follow=True)
         assert response.status_code == 200
         assert addon.guid in response.content.decode('utf-8')

src/olympia/addons/tests/test_models.py

@@ -489,7 +489,7 @@ class TestAddonModels(TestCase):
     def test_transformer(self):
         author = UserProfile.objects.get(pk=55021)
         new_author = AddonUser.objects.create(
-            addon_id=3615, user=user_factory(), listed=True
+            addon_id=3615, user=user_factory(username='new_author'), listed=True
         ).user
         # make the new_author a deleted author of another addon
         AddonUser.objects.create(
@@ -498,12 +498,24 @@ class TestAddonModels(TestCase):
         # Deleted, so shouldn't show up below.
         AddonUser.objects.create(
             addon_id=3615,
-            user=user_factory(),
+            user=user_factory(username='deleted_author'),
             listed=True,
             role=amo.AUTHOR_ROLE_DELETED,
-        ).user
+        )
+        # Not listed, should not show up.
+        AddonUser.objects.create(
+            addon_id=3615,
+            user=user_factory(username='not_listed_author'),
+            listed=False,
+            role=amo.AUTHOR_ROLE_OWNER,
+        )
+        # Different author on another add-on - should not show up
+        AddonUser.objects.create(addon=addon_factory(), user=user_factory())
+        # First author, but on another add-on, not deleted - should not show up
+        AddonUser.objects.create(addon=addon_factory(), user=author)
 
-        addon = Addon.objects.get(pk=3615)
+        # Force evaluation of the queryset and test a single add-on
+        addon = list(Addon.objects.all().order_by('pk'))[0]
 
         # If the transformer works then we won't have any more queries.
         with self.assertNumQueries(0):
@@ -515,7 +527,55 @@ class TestAddonModels(TestCase):
         # repeat to check the position ordering works
         author.addonuser_set.filter(addon=addon).update(position=1)
         addon = Addon.objects.get(pk=3615)
-        assert [u.pk for u in addon.listed_authors] == [new_author.pk, author.pk]
+        with self.assertNumQueries(0):
+            assert [u.pk for u in addon.listed_authors] == [new_author.pk, author.pk]
+
+    def test_transformer_all_authors(self):
+        author = UserProfile.objects.get(pk=55021)
+        new_author = AddonUser.objects.create(
+            addon_id=3615, user=user_factory(username='new_author'), listed=True
+        ).user
+        # make the new_author a deleted author of another addon
+        AddonUser.objects.create(
+            addon=addon_factory(), user=new_author, role=amo.AUTHOR_ROLE_DELETED
+        )
+        # Deleted, so it should show up cause we're looking at *all* authors
+        # explicitly.
+        deleted_author = AddonUser.objects.create(
+            addon_id=3615,
+            user=user_factory(username='deleted_author'),
+            listed=True,
+            role=amo.AUTHOR_ROLE_DELETED,
+        ).user
+        # Not listed, should also show up.
+        not_listed_author = AddonUser.objects.create(
+            addon_id=3615,
+            user=user_factory(username='not_listed_author'),
+            listed=False,
+            role=amo.AUTHOR_ROLE_OWNER,
+        ).user
+        # Different author on another add-on - should not show up
+        AddonUser.objects.create(addon=addon_factory(), user=user_factory())
+        # First author, but on another add-on, not deleted - should not show up
+        AddonUser.objects.create(addon=addon_factory(), user=author)
+
+        # Force evaluation of the queryset and test a single add-on
+        addon = list(Addon.objects.transform(Addon.attach_all_authors).order_by('pk'))[
+            0
+        ]
+
+        # If the transformer works then we won't have any more queries.
+        with self.assertNumQueries(0):
+            assert [u.pk for u in addon.all_authors] == [
+                author.pk,
+                new_author.pk,
+                deleted_author.pk,
+                not_listed_author.pk,
+            ]
+        for user in addon.all_authors:
+            addonuser = AddonUser.unfiltered.filter(user=user, addon=addon).get()
+            assert user.role == addonuser.role
+            assert user.listed == addonuser.listed
 
     def _delete(self, addon_id):
         """Test deleting add-ons."""

src/olympia/users/admin.py

@@ -4,6 +4,7 @@ from django import http
 from django.conf.urls import url
 from django.contrib import admin, messages
 from django.contrib.admin.utils import unquote
+from django.db.models import Count, Q
 from django.db.utils import IntegrityError
 from django.http import (
     Http404,
@@ -21,7 +22,7 @@ from olympia import amo
 from olympia.abuse.models import AbuseReport
 from olympia.access import acl
 from olympia.activity.models import ActivityLog, UserLog
-from olympia.addons.models import Addon
+from olympia.addons.models import Addon, AddonUser
 from olympia.amo.admin import CommaSearchInAdminMixin
 from olympia.api.models import APIKey, APIKeyConfirmation
 from olympia.bandwagon.models import Collection
@@ -66,9 +67,9 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
         'last_login_ip',
         'known_ip_adresses',
         'last_known_activity_time',
-        'ratings_created',
-        'collections_created',
-        'addons_created',
+        'ratings_authorship',
+        'collections_authorship',
+        'addons_authorship',
         'activity',
         'abuse_reports_by_this_user',
         'abuse_reports_for_this_user',
@@ -104,7 +105,13 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
         ),
         (
             'Content',
-            {'fields': ('addons_created', 'collections_created', 'ratings_created')},
+            {
+                'fields': (
+                    'addons_authorship',
+                    'collections_authorship',
+                    'ratings_authorship',
+                )
+            },
         ),
         (
             'Abuse Reports',
@@ -360,20 +367,37 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
 
     has_active_api_key.boolean = True
 
-    def collections_created(self, obj):
+    def collections_authorship(self, obj):
         return related_content_link(obj, Collection, 'author')
 
-    collections_created.short_description = _('Collections')
+    collections_authorship.short_description = _('Collections')
 
-    def addons_created(self, obj):
-        return related_content_link(obj, Addon, 'authors', related_manager='unfiltered')
+    def addons_authorship(self, obj):
+        counts = (
+            AddonUser.unfiltered.filter(user=obj)
+            .order_by()
+            .aggregate(
+                active_role=Count('role', filter=~Q(role=amo.AUTHOR_ROLE_DELETED)),
+                deleted_role=Count('role', filter=Q(role=amo.AUTHOR_ROLE_DELETED)),
+            )
+        )
+        return related_content_link(
+            obj,
+            Addon,
+            'authors',
+            text=format_html(
+                '{} (active role), {} (deleted role)',
+                counts['active_role'],
+                counts['deleted_role'],
+            ),
+        )
 
-    addons_created.short_description = _('Addons')
+    addons_authorship.short_description = _('Addons')
 
-    def ratings_created(self, obj):
+    def ratings_authorship(self, obj):
         return related_content_link(obj, Rating, 'user')
 
-    ratings_created.short_description = _('Ratings')
+    ratings_authorship.short_description = _('Ratings')
 
     def activity(self, obj):
         return related_content_link(obj, ActivityLog, 'user')

src/olympia/users/tests/test_admin.py

@@ -557,11 +557,11 @@ class TestUserAdmin(TestCase):
         link = pq(result)('a')[0]
         return link.attrib['href'], link.text
 
-    def test_collections_created(self):
+    def test_collections_authorship(self):
         Collection.objects.create()
         Collection.objects.create(author=self.user)
         Collection.objects.create(author=self.user, listed=False)
-        url, text = self._call_related_content_method('collections_created')
+        url, text = self._call_related_content_method('collections_authorship')
         expected_url = (
             reverse('admin:bandwagon_collection_changelist')
             + '?author=%d' % self.user.pk
@@ -569,7 +569,7 @@ class TestUserAdmin(TestCase):
         assert url == expected_url
         assert text == '2'
 
-    def test_addons_created(self):
+    def test_addons_authorship(self):
         addon_factory()
         another_user = user_factory()
         addon_factory(users=[self.user, another_user])
@@ -578,14 +578,21 @@ class TestUserAdmin(TestCase):
         addon_factory(
             users=[self.user], version_kw={'channel': amo.RELEASE_CHANNEL_UNLISTED}
         )
-        url, text = self._call_related_content_method('addons_created')
+        # This next add-on shouldn't be counted.
+        addon_where_user_has_deleted_role = addon_factory(
+            users=[self.user, another_user]
+        )
+        addon_where_user_has_deleted_role.addonuser_set.filter(user=self.user).update(
+            role=amo.AUTHOR_ROLE_DELETED
+        )
+        url, text = self._call_related_content_method('addons_authorship')
         expected_url = (
             reverse('admin:addons_addon_changelist') + '?authors=%d' % self.user.pk
         )
         assert url == expected_url
-        assert text == '4'
+        assert text == '4 (active role), 1 (deleted role)'
 
-    def test_ratings_created(self):
+    def test_ratings_authorship(self):
         Rating.objects.create(addon=addon_factory(), user=self.user)
         dummy_addon = addon_factory()
         Rating.objects.create(
@@ -599,7 +606,7 @@ class TestUserAdmin(TestCase):
         Rating.objects.create(
             addon=dummy_addon, user=user_factory(), ip_address='255.255.0.0'
         )
-        url, text = self._call_related_content_method('ratings_created')
+        url, text = self._call_related_content_method('ratings_authorship')
         expected_url = (
             reverse('admin:ratings_rating_changelist') + '?user=%d' % self.user.pk
         )

src/olympia/zadmin/admin.py

@@ -11,7 +11,7 @@ from . import models
 
 
 def related_content_link(
-    obj, related_class, related_field, related_manager='objects', count=None
+    obj, related_class, related_field, related_manager='objects', text=None
 ):
     """
     Return a link to the admin changelist for the instances of related_class
@@ -20,11 +20,11 @@ def related_content_link(
     url = 'admin:{}_{}_changelist'.format(
         related_class._meta.app_label, related_class._meta.model_name
     )
-    queryset = getattr(related_class, related_manager).filter(**{related_field: obj})
-    if count is None:
-        count = queryset.count()
+    if text is None:
+        qs = getattr(related_class, related_manager).filter(**{related_field: obj})
+        text = qs.count()
     return format_html(
-        '<a href="{}?{}={}">{}</a>', reverse(url), related_field, obj.pk, count
+        '<a href="{}?{}={}">{}</a>', reverse(url), related_field, obj.pk, text
     )
 
 

static/css/admin/addons.css

@@ -0,0 +1,8 @@
+#changelist ul {
+    margin-left: 0;
+    padding-left: 0;
+}
+
+#changelist li {
+    list-style-type: none;
+}