Skip to content

E
edx-platform
Commit 1bd4b1d6 by cahrens
Options

Add sorting by team_count, as well as secondary sort. 

TNL-3012
parent 9e311931
Showing with 166 additions and 50 deletions
lms/djangoapps/teams/serializers.py
......@@ -139,41 +139,60 @@ class BaseTopicSerializer(serializers.Serializer):
class TopicSerializer(BaseTopicSerializer):
"""
Adds team_count to the basic topic serializer. Use only when
serializing a single topic. When serializing many topics, use
`PaginatedTopicSerializer` to avoid O(N) SQL queries. Requires
that `context` is provided with a valid course_id in order to
filter teams within the course.
Adds team_count to the basic topic serializer, checking if team_count
is already present in the topic data, and if not, querying the CourseTeam
model to get the count. Requires that `context` is provided with a valid course_id
in order to filter teams within the course.
"""
team_count = serializers.SerializerMethodField('get_team_count')
def get_team_count(self, topic):
"""Get the number of teams associated with this topic"""
return CourseTeam.objects.filter(course_id=self.context['course_id'], topic_id=topic['id']).count()
# If team_count is already present (possible if topic data was pre-processed for sorting), return it.
if 'team_count' in topic:
return topic['team_count']
else:
return CourseTeam.objects.filter(course_id=self.context['course_id'], topic_id=topic['id']).count()
class PaginatedTopicSerializer(PaginationSerializer):
"""
Serializes a set of topics, adding team_count field to each topic.
Requires that `context` is provided with a valid course_id in
Serializes a set of topics, adding the team_count field to each topic individually, if team_count
is not already present in the topic data. Requires that `context` is provided with a valid course_id in
order to filter teams within the course.
"""
class Meta(object):
"""Defines meta information for the PaginatedTopicSerializer."""
object_serializer_class = TopicSerializer
class BulkTeamCountPaginatedTopicSerializer(PaginationSerializer):
"""
Serializes a set of topics, adding the team_count field to each topic as a bulk operation per page
(only on the page being returned). Requires that `context` is provided with a valid course_id in
order to filter teams within the course.
"""
class Meta(object):
"""Defines meta information for the BulkTeamCountPaginatedTopicSerializer."""
object_serializer_class = BaseTopicSerializer
def __init__(self, *args, **kwargs):
"""Adds team_count to each topic."""
super(PaginatedTopicSerializer, self).__init__(*args, **kwargs)
# The following query gets all the team_counts for each topic
# and outputs the result as a list of dicts (one per topic).
topic_ids = [topic['id'] for topic in self.data['results']]
teams_per_topic = CourseTeam.objects.filter(
course_id=self.context['course_id'],
topic_id__in=topic_ids
).values('topic_id').annotate(team_count=Count('topic_id'))
topics_to_team_count = {d['topic_id']: d['team_count'] for d in teams_per_topic}
for topic in self.data['results']:
topic['team_count'] = topics_to_team_count.get(topic['id'], 0)
"""Adds team_count to each topic on the current page."""
super(BulkTeamCountPaginatedTopicSerializer, self).__init__(*args, **kwargs)
add_team_count(self.data['results'], self.context['course_id'])
def add_team_count(topics, course_id):
"""
Helper method to add team_count for a list of topics.
This allows for a more efficient single query.
"""
topic_ids = [topic['id'] for topic in topics]
teams_per_topic = CourseTeam.objects.filter(
course_id=course_id,
topic_id__in=topic_ids
).values('topic_id').annotate(team_count=Count('topic_id'))
topics_to_team_count = {d['topic_id']: d['team_count'] for d in teams_per_topic}
for topic in topics:
topic['team_count'] = topics_to_team_count.get(topic['id'], 0)
lms/djangoapps/teams/tests/test_serializers.py
......@@ -8,7 +8,10 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
from lms.djangoapps.teams.tests.factories import CourseTeamFactory
from lms.djangoapps.teams.serializers import BaseTopicSerializer, PaginatedTopicSerializer, TopicSerializer
from lms.djangoapps.teams.serializers import (
BaseTopicSerializer, PaginatedTopicSerializer, BulkTeamCountPaginatedTopicSerializer,
TopicSerializer, add_team_count
)
class TopicTestCase(ModuleStoreTestCase):
......@@ -92,12 +95,14 @@ class TopicSerializerTestCase(TopicTestCase):
)
class PaginatedTopicSerializerTestCase(TopicTestCase):
class BasePaginatedTopicSerializerTestCase(TopicTestCase):
"""
Tests for the `PaginatedTopicSerializer`, which should serialize team count
data for many topics with constant time SQL queries.
Base class for testing the two paginated topic serializers.
"""
__test__ = False
PAGE_SIZE = 5
# Extending test classes should specify their serializer class.
serializer = None
def _merge_dicts(self, first, second):
"""Convenience method to merge two dicts in a single expression"""
......@@ -128,7 +133,8 @@ class PaginatedTopicSerializerTestCase(TopicTestCase):
"""
with self.assertNumQueries(num_queries):
page = Paginator(self.course.teams_topics, self.PAGE_SIZE).page(1)
serializer = PaginatedTopicSerializer(instance=page, context={'course_id': self.course.id})
# pylint: disable=not-callable
serializer = self.serializer(instance=page, context={'course_id': self.course.id})
self.assertEqual(
serializer.data['results'],
[self._merge_dicts(topic, {u'team_count': num_teams_per_topic}) for topic in topics]
......@@ -142,6 +148,15 @@ class PaginatedTopicSerializerTestCase(TopicTestCase):
self.course.teams_configuration['topics'] = []
self.assert_serializer_output([], num_teams_per_topic=0, num_queries=0)
class BulkTeamCountPaginatedTopicSerializerTestCase(BasePaginatedTopicSerializerTestCase):
"""
Tests for the `BulkTeamCountPaginatedTopicSerializer`, which should serialize team_count
data for many topics with constant time SQL queries.
"""
__test__ = True
serializer = BulkTeamCountPaginatedTopicSerializer
def test_topics_with_no_team_counts(self):
"""
Verify that we serialize topics with no team count, making only one SQL
......@@ -181,3 +196,28 @@ class PaginatedTopicSerializerTestCase(TopicTestCase):
)
CourseTeamFactory.create(course_id=second_course.id, topic_id=duplicate_topic[u'id'])
self.assert_serializer_output(first_course_topics, num_teams_per_topic=teams_per_topic, num_queries=1)
class PaginatedTopicSerializerTestCase(BasePaginatedTopicSerializerTestCase):
"""
Tests for the `PaginatedTopicSerializer`, which will add team_count information per topic if not present.
"""
__test__ = True
serializer = PaginatedTopicSerializer
def test_topics_with_team_counts(self):
"""
Verify that we serialize topics with team_count, making one SQL query per topic.
"""
teams_per_topic = 2
topics = self.setup_topics(teams_per_topic=teams_per_topic)
self.assert_serializer_output(topics, num_teams_per_topic=teams_per_topic, num_queries=5)
def test_topics_with_team_counts_prepopulated(self):
"""
Verify that if team_count is pre-populated, there are no additional SQL queries.
"""
teams_per_topic = 8
topics = self.setup_topics(teams_per_topic=teams_per_topic)
add_team_count(topics, self.course.id)
self.assert_serializer_output(topics, num_teams_per_topic=teams_per_topic, num_queries=0)
lms/djangoapps/teams/tests/test_views.py
......@@ -420,7 +420,9 @@ class TestListTeamsAPI(TeamAPITestCase):
@ddt.data(
(None, 200, ['Nuclear Team', u'sólar team', 'Wind Team']),
('name', 200, ['Nuclear Team', u'sólar team', 'Wind Team']),
('open_slots', 200, ['Wind Team', u'sólar team', 'Nuclear Team']),
# Note that "Nuclear Team" and "solar team" have the same number of open slots.
# "Nuclear Team" comes first due to secondary sort by name.
('open_slots', 200, ['Wind Team', 'Nuclear Team', u'sólar team']),
('last_activity', 400, []),
)
@ddt.unpack
......@@ -700,10 +702,20 @@ class TestListTopicsAPI(TeamAPITestCase):
@ddt.data(
(None, 200, ['Coal Power', 'Nuclear Power', u'sólar power', 'Wind Power'], 'name'),
('name', 200, ['Coal Power', 'Nuclear Power', u'sólar power', 'Wind Power'], 'name'),
# Note that "Nuclear Power" and "solar power" both have 2 teams. "Coal Power" and "Window Power"
# both have 0 teams. The secondary sort is alphabetical by name.
('team_count', 200, ['Nuclear Power', u'sólar power', 'Coal Power', 'Wind Power'], 'team_count'),
('no_such_field', 400, [], None),
)
@ddt.unpack
def test_order_by(self, field, status, names, expected_ordering):
# Add 2 teams to "Nuclear Power", which previously had no teams.
CourseTeamFactory.create(
name=u'Nuclear Team 1', course_id=self.test_course_1.id, topic_id='topic_2'
)
CourseTeamFactory.create(
name=u'Nuclear Team 2', course_id=self.test_course_1.id, topic_id='topic_2'
)
data = {'course_id': self.test_course_1.id}
if field:
data['order_by'] = field
......@@ -712,6 +724,35 @@ class TestListTopicsAPI(TeamAPITestCase):
self.assertEqual(names, [topic['name'] for topic in topics['results']])
self.assertEqual(topics['sort_order'], expected_ordering)
def test_order_by_team_count_secondary(self):
"""
Ensure that the secondary sort (alphabetical) when primary sort is team_count
works across pagination boundaries.
"""
# Add 2 teams to "Wind Power", which previously had no teams.
CourseTeamFactory.create(
name=u'Wind Team 1', course_id=self.test_course_1.id, topic_id='topic_1'
)
CourseTeamFactory.create(
name=u'Wind Team 2', course_id=self.test_course_1.id, topic_id='topic_1'
)
topics = self.get_topics_list(data={
'course_id': self.test_course_1.id,
'page_size': 2,
'page': 1,
'order_by': 'team_count'
})
self.assertEqual(["Wind Power", u'sólar power'], [topic['name'] for topic in topics['results']])
topics = self.get_topics_list(data={
'course_id': self.test_course_1.id,
'page_size': 2,
'page': 2,
'order_by': 'team_count'
})
self.assertEqual(["Coal Power", "Nuclear Power"], [topic['name'] for topic in topics['results']])
def test_pagination(self):
response = self.get_topics_list(data={
'course_id': self.test_course_1.id,
......
lms/djangoapps/teams/views.py
......@@ -43,11 +43,12 @@ from .models import CourseTeam, CourseTeamMembership
from .serializers import (
CourseTeamSerializer,
CourseTeamCreationSerializer,
BaseTopicSerializer,
TopicSerializer,
PaginatedTopicSerializer,
BulkTeamCountPaginatedTopicSerializer,
MembershipSerializer,
PaginatedMembershipSerializer,
add_team_count
)
from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam
......@@ -77,10 +78,14 @@ class TeamsDashboardView(View):
not has_access(request.user, 'staff', course, course.id):
raise Http404
# Even though sorting is done outside of the serializer, sort_order needs to be passed
# to the serializer so that the paginated results indicate how they were sorted.
sort_order = 'name'
topics = get_ordered_topics(course, sort_order)
topics = get_alphabetical_topics(course)
topics_page = Paginator(topics, TOPICS_PER_PAGE).page(1)
topics_serializer = PaginatedTopicSerializer(
# BulkTeamCountPaginatedTopicSerializer will add team counts to the topics in a single
# bulk operation per page.
topics_serializer = BulkTeamCountPaginatedTopicSerializer(
instance=topics_page,
context={'course_id': course.id, 'sort_order': sort_order}
)
......@@ -166,7 +171,7 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
* name: Orders results by case insensitive team name (default).
* open_slots: Orders results by most open slots.
* open_slots: Orders results by most open slots (with name as a secondary sort).
* last_activity: Currently not supported.
......@@ -322,14 +327,15 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
)
queryset = CourseTeam.objects.filter(**result_filter)
# We will always use name as either a primary or secondary sort.
queryset = queryset.extra(select={'lower_name': "lower(name)"})
order_by_input = request.QUERY_PARAMS.get('order_by', 'name')
if order_by_input == 'name':
queryset = queryset.extra(select={'lower_name': "lower(name)"})
order_by_field = 'lower_name'
queryset = queryset.order_by('lower_name')
elif order_by_input == 'open_slots':
queryset = queryset.annotate(team_size=Count('users'))
order_by_field = 'team_size'
queryset = queryset.order_by('team_size', 'lower_name')
elif order_by_input == 'last_activity':
return Response(
build_api_error(ugettext_noop("last_activity is not yet supported")),
......@@ -345,8 +351,6 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
'user_message': _(u"The ordering {ordering} is not supported").format(ordering=order_by_input),
}, status=status.HTTP_400_BAD_REQUEST)
queryset = queryset.order_by(order_by_field)
page = self.paginate_queryset(queryset)
serializer = self.get_pagination_serializer(page)
serializer.context.update({'sort_order': order_by_input}) # pylint: disable=maybe-no-member
......@@ -531,8 +535,9 @@ class TopicListView(GenericAPIView):
* course_id: Filters the result to topics belonging to the given
course (required).
* order_by: Orders the results. Currently only 'name' is supported,
and is also the default value.
* order_by: Orders the results. Currently only 'name' and 'team_count' are supported;
the default value is 'name'. If 'team_count' is specified, topics are returned first sorted
by number of teams per topic (descending), with a secondary sort of 'name'.
* page_size: Number of results to return per page.
......@@ -578,8 +583,6 @@ class TopicListView(GenericAPIView):
paginate_by = TOPICS_PER_PAGE
paginate_by_param = 'page_size'
pagination_serializer_class = PaginatedTopicSerializer
serializer_class = BaseTopicSerializer
def get(self, request):
"""GET /api/team/v0/topics/?course_id={course_id}"""
......@@ -608,9 +611,7 @@ class TopicListView(GenericAPIView):
return Response(status=status.HTTP_403_FORBIDDEN)
ordering = request.QUERY_PARAMS.get('order_by', 'name')
if ordering == 'name':
topics = get_ordered_topics(course_module, ordering)
else:
if ordering not in ['name', 'team_count']:
return Response({
'developer_message': "unsupported order_by value {ordering}".format(ordering=ordering),
# Translators: 'ordering' is a string describing a way
......@@ -620,22 +621,37 @@ class TopicListView(GenericAPIView):
'user_message': _(u"The ordering {ordering} is not supported").format(ordering=ordering),
}, status=status.HTTP_400_BAD_REQUEST)
page = self.paginate_queryset(topics)
serializer = self.pagination_serializer_class(page, context={'course_id': course_id, 'sort_order': ordering})
# Always sort alphabetically, as it will be used as secondary sort
# in the case of "team_count".
topics = get_alphabetical_topics(course_module)
if ordering == 'team_count':
add_team_count(topics, course_id)
topics.sort(key=lambda t: t['team_count'], reverse=True)
page = self.paginate_queryset(topics)
# Since team_count has already been added to all the topics, use PaginatedTopicSerializer.
# Even though sorting is done outside of the serializer, sort_order needs to be passed
# to the serializer so that the paginated results indicate how they were sorted.
serializer = PaginatedTopicSerializer(page, context={'course_id': course_id, 'sort_order': ordering})
else:
page = self.paginate_queryset(topics)
# Use the serializer that adds team_count in a bulk operation per page.
serializer = BulkTeamCountPaginatedTopicSerializer(
page, context={'course_id': course_id, 'sort_order': ordering}
)
return Response(serializer.data)
def get_ordered_topics(course_module, ordering):
"""Return a sorted list of team topics.
def get_alphabetical_topics(course_module):
"""Return a list of team topics sorted alphabetically.
Arguments:
course_module (xmodule): the course which owns the team topics
ordering (str): the key belonging to topic dicts by which we sort
Returns:
list: a list of sorted team topics
"""
return sorted(course_module.teams_topics, key=lambda t: t[ordering].lower())
return sorted(course_module.teams_topics, key=lambda t: t['name'].lower())
class TopicDetailView(APIView):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment