diff --git a/src/olympia/addons/admin.py b/src/olympia/addons/admin.py
index 5e693fe24f..98482fe96e 100644
--- a/src/olympia/addons/admin.py
+++ b/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(
+ '',
+ format_html_join(
+ '',
+ '{} ({}{})',
+ (
+ (
+ 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().
diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py
index a81f54bdcf..8a6c7447d2 100644
--- a/src/olympia/addons/models.py
+++ b/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:
diff --git a/src/olympia/addons/tests/test_admin.py b/src/olympia/addons/tests/test_admin.py
index a1be78f6b0..61c8db30f3 100644
--- a/src/olympia/addons/tests/test_admin.py
+++ b/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')
diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py
index e261c02d51..74abf2b855 100644
--- a/src/olympia/addons/tests/test_models.py
+++ b/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."""
diff --git a/src/olympia/users/admin.py b/src/olympia/users/admin.py
index 17f7859a98..e706644db9 100644
--- a/src/olympia/users/admin.py
+++ b/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')
diff --git a/src/olympia/users/tests/test_admin.py b/src/olympia/users/tests/test_admin.py
index e0c4ea3f02..b82e1b9b57 100644
--- a/src/olympia/users/tests/test_admin.py
+++ b/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
)
diff --git a/src/olympia/zadmin/admin.py b/src/olympia/zadmin/admin.py
index 089a016cea..30b5679cc4 100644
--- a/src/olympia/zadmin/admin.py
+++ b/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(
- '{}', reverse(url), related_field, obj.pk, count
+ '{}', reverse(url), related_field, obj.pk, text
)
diff --git a/static/css/admin/addons.css b/static/css/admin/addons.css
new file mode 100644
index 0000000000..b47245096e
--- /dev/null
+++ b/static/css/admin/addons.css
@@ -0,0 +1,8 @@
+#changelist ul {
+ margin-left: 0;
+ padding-left: 0;
+}
+
+#changelist li {
+ list-style-type: none;
+}