Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

DONOTMERGE - [Fix bug 908053] Fix group member counts in admin #838

Closed
wants to merge 1 commit into from

3 participants

@dpoirier
Owner

No description provided.

mozillians/groups/admin.py
@@ -105,6 +106,35 @@ def save(self, *args, **kwargs):
return super(GroupBaseEditAdminForm, self).save(*args, **kwargs)
+class GroupBaseChangeList(ChangeList):
+ # We can't just override queryset() on the ModelAdmin class because the
+ # changelist applies search and filtering on top of whatever that returns,
+ # and we need to apply our annotations after that. So we
+ # need to subclass the changelist itself.
+ def get_query_set(self, request):
+ qset = super(GroupBaseChangeList, self).get_query_set(request)
+
+ # HACK: we want to annotate the result, but if a group has matched a search
+ # in more than one way, the annotations end up double-counting members. So
+ # execute the query to get the final set of groups, then generate a new,
+ # simple query that just includes those groups and annotate that.
+ qset = qset.order_by() # (No point in wasting cycles sorting these results)
+ pks = set(qset.values_list('pk', flat=True))
+ qset = Group.objects.filter(pk__in=pks)
@glogiotatidis Owner

I managed to reproduce the bug locally but I can't say that I understand what we're fixing here. Let me explain:

  • I created the groups web dev and web development
  • I merged web development intoweb dev`
  • I joined web dev
  • I see a count of 4 members without your patch, 1 with your patch.

Correct but:

  • When I break before this line and look at qset there is only web dev in there, so what does set() do?
  • Actually doing Group.objects.filter(pk__in=qset) without set works in the same way.

So maybe there is something deeper in django's orm that needs attention?

BTW IMHO I wouldn't spend much time on fixing this in the admin panel level, since this info can be valueable in other places as well. Maybe it makes sense to code into Groups Queryset.

@dpoirier Owner
dpoirier added a note

The query returns the right records without duplicates (there's even a DISTINCT in the query), but the annotations compute the wrong answers. I'm not SQL expert enough to understand exactly what's wrong with the query, but this sounds like https://code.djangoproject.com/ticket/10060. That's been open for 5 years. I guess it's a hard problem :-)

I'm not sure how we would fix this particular problem outside the admin, since the admin is applying the multiple joins due to the search fields crossing multiple tables, and we have to do the fix after that.

The set in my fix is redundant for multiple reasons and could be removed.

@dpoirier Owner
dpoirier added a note

Another option would be to just compute the numbers of members on the individual records instead of trying to do it as part of the original query (e.g. return self.members.count()). I've been reluctant to do that since in this case it would add two more queries for every group on the page, but we could add methods or properties to do that and use them anywhere.

@glogiotatidis Owner

I'll take a look tomorrow and see what I come up with. Thanks for elaborating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dpoirier
Owner

(Removed unnecessary set())

@dpoirier dpoirier changed the title from DNM - [Fix bug 908053] Fix group member counts in admin to [Fix bug 908053] Fix group member counts in admin
@dpoirier
Owner

This is ready for review and merge, if the approach is acceptable.

@glogiotatidis glogiotatidis changed the title from [Fix bug 908053] Fix group member counts in admin to DONOTMERGE - [Fix bug 908053] Fix group member counts in admin
@Sancus

@glogiotatidis Were you still interested in trying to find an alternate approach for this or whatever you intended by your comment above? :)

Just trying to figure out what to do with this PR.

@glogiotatidis

Yeah this implementation is a not proper workaround imho. I'll work on this but sincei it's not blocking anythings it's low in my todo list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 4, 2014
  1. @dpoirier
This page is out of date. Refresh to see the latest.
Showing with 111 additions and 28 deletions.
  1. +32 −10 mozillians/groups/admin.py
  2. +79 −18 mozillians/groups/tests/test_admin.py
View
42 mozillians/groups/admin.py
@@ -1,6 +1,7 @@
from django import forms
from django.contrib import admin
from django.contrib.admin import SimpleListFilter
+from django.contrib.admin.views.main import ChangeList
from django.contrib.admin.widgets import FilteredSelectMultiple
from django.db.models import Count, Sum
@@ -105,6 +106,35 @@ def save(self, *args, **kwargs):
return super(GroupBaseEditAdminForm, self).save(*args, **kwargs)
+class GroupBaseChangeList(ChangeList):
+ # We can't just override queryset() on the ModelAdmin class because the
+ # changelist applies search and filtering on top of whatever that returns,
+ # and we need to apply our annotations after that. So we
+ # need to subclass the changelist itself.
+ def get_query_set(self, request):
+ qset = super(GroupBaseChangeList, self).get_query_set(request)
+
+ # HACK: we want to annotate the result, but if a group has matched a search
+ # in more than one way, the annotations end up double-counting members. So
+ # execute the query to get the final set of groups, then generate a new,
+ # simple query that just includes those groups and annotate that.
+ qset = qset.order_by() # (No point in wasting cycles sorting these results)
+ pks = qset.values_list('pk', flat=True)
+ qset = Group.objects.filter(pk__in=pks)
+
+ # Restore ordering.
+ qset = qset.order_by(*self.get_ordering(request, qset))
+
+ # Also note:
+ # The Sum('members__is_vouched') annotation only works for
+ # databases where the Boolean type is really an integer. It works
+ # for Sqlite3 or MySQL, but fails on Postgres. If Mozillians ever
+ # switches from MySQL to a database where this won't work, we'll
+ # need to revisit this.
+ return qset.annotate(member_count=Count('members'),
+ vouched_member_count=Sum('members__is_vouched'))
+
+
class GroupBaseAdmin(admin.ModelAdmin):
"""GroupBase Admin."""
save_on_top = True
@@ -121,16 +151,8 @@ def get_form(self, request, obj=None, **kwargs):
defaults.update(kwargs)
return super(GroupBaseAdmin, self).get_form(request, obj, **defaults)
- def queryset(self, request):
- # The Sum('members__is_vouched') annotation only works for
- # databases where the Boolean type is really an integer. It works
- # for Sqlite3 or MySQL, but fails on Postgres. If Mozillians ever
- # switches from MySQL to a database where this won't work, we'll
- # need to revisit this.
- return (super(GroupBaseAdmin, self)
- .queryset(request)
- .annotate(member_count=Count('members'),
- vouched_member_count=Sum('members__is_vouched')))
+ def get_changelist(self, request, **kwargs):
+ return GroupBaseChangeList
def member_count(self, obj):
"""Return number of members in group."""
View
97 mozillians/groups/tests/test_admin.py
@@ -1,12 +1,7 @@
-from django.contrib.admin.sites import site
-from django.http import HttpRequest
-
-from mock import Mock
from nose.tools import eq_
from mozillians.common.tests import TestCase
-from mozillians.groups.admin import GroupAdmin
-from mozillians.groups.models import Group
+from mozillians.groups.models import GroupAlias, Group
from mozillians.groups.tests import GroupFactory
from mozillians.users.tests import UserFactory
@@ -15,6 +10,8 @@ class TestGroupAdmin(TestCase):
def test_member_counts(self):
# The Group admin computes how many vouched members there are
# and how many overall
+ admin_user = UserFactory.create(is_staff=True, is_superuser=True,
+ userprofile={'is_vouched': True})
# IMPORTANT: This test is expected to fail on Postgres, and
# probably other databases where the Boolean type is not just
@@ -24,17 +21,81 @@ def test_member_counts(self):
# test will alert us quickly that we'll need to take another
# approach to this feature.
- # Create group with 1 vouched member and 1 unvouched member
- group = GroupFactory()
- user = UserFactory(userprofile={'is_vouched': False})
- group.add_member(user.userprofile)
- user2 = UserFactory(userprofile={'is_vouched': True})
- group.add_member(user2.userprofile)
+ # Create group with 3 vouched members and 2 unvouched members
+ group = GroupFactory(name='web development')
+ profiles = []
+ for i in range(2):
+ profile = UserFactory(userprofile={'is_vouched': False}).userprofile
+ group.add_member(profile)
+ profiles.append(profile)
+ for i in range(3):
+ profile = UserFactory(userprofile={'is_vouched': True}).userprofile
+ group.add_member(profile)
+ profiles.append(profile)
+
+ # Create an alias for our group that will also match our query.
+ # This can lead to members being counted multiple times.
+ # Another trigger would be searching for a string that appears in the
+ # group name more than once.
+ GroupAlias.objects.create(alias=group, name='web development alias')
+
+ with self.login(admin_user) as client:
+ response = client.get('/admin/groups/group/?q=web+dev')
+ eq_(response.status_code, 200)
+ change_list = response.context['cl']
+ group = change_list.result_list.get(name=group.name)
+ eq_(group.member_count, group.members.count())
+ eq_(5, group.member_count)
+ eq_(group.vouched_member_count, group.members.filter(is_vouched=True).count())
+ eq_(3, group.vouched_member_count)
+
+ def test_group_ordering(self):
+ # Since we're mucking with the group changelist queryset in a way
+ # that could break ordering, make sure it's still working.
+ admin_user = UserFactory.create(is_staff=True, is_superuser=True,
+ userprofile={'is_vouched': True})
+ group1 = GroupFactory(name='web development 1')
+ group2 = GroupFactory(name='web development 2')
+ group3 = GroupFactory(name='web development 3')
+ # Create some profiles we can use
+ profiles = [UserFactory(userprofile={'is_vouched': True}).userprofile
+ for i in range(3)]
+ # Add different numbers of profiles to each group
+ for p in profiles[:3]:
+ group1.add_member(p)
+ for p in profiles[:2]:
+ group2.add_member(p)
+ for p in profiles[:1]:
+ group3.add_member(p)
+
+ # Don't let the staff group confuse the issue
+ Group.objects.filter(name='staff').delete()
+
+ col_number = 7 # where the member_count column is
+ # It would be better to get the column number using
+ # GroupAdmin.list_display.index('member_count') + 1,
+ # but importing GroupAdmin from the test breaks the
+ # admin autodiscovery somehow, and the group admin URLs
+ # don't get registered.
- admin = GroupAdmin(model=Group, admin_site=site)
- mock_request = Mock(spec=HttpRequest)
- qset = admin.queryset(mock_request)
+ # Sort by member count
+ url = '/admin/groups/group/?o=%d.1' % col_number
+ print url
+ with self.login(admin_user) as client:
+ response = client.get(url)
+ eq_(response.status_code, 200)
+ change_list = response.context['cl']
+ qset = change_list.result_list
+ # The group with the fewest members should be first
+ eq_([group3, group2, group1], list(qset))
- g = qset.get(name=group.name)
- eq_(2, g.member_count)
- eq_(1, g.vouched_member_count)
+ # Again, in reverse, to make sure the first success wasn't just luck
+ url = '/admin/groups/group/?o=-%d.1' % col_number
+ print url
+ with self.login(admin_user) as client:
+ response = client.get(url)
+ eq_(response.status_code, 200)
+ change_list = response.context['cl']
+ qset = change_list.result_list
+ # The group with the most members should be first
+ eq_([group1, group2, group3], list(qset))
Something went wrong with that request. Please try again.