Merge pull request #9262 from edx/christina/search-sort

Add sorting by team_count, as well as secondary sort (by name).

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"""
+        # 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)
+        """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'])
+
 
-        # 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']]
+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=self.context['course_id'],
+        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 self.data['results']:
+    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

@@ -436,7 +436,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
@@ -740,10 +742,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
@@ -752,6 +764,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}
         )
@@ -170,7 +175,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.
 
@@ -326,14 +331,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")),
@@ -349,8 +355,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
@@ -408,6 +412,28 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
             return Response(CourseTeamSerializer(team).data)
 
 
+class IsEnrolledOrIsStaff(permissions.BasePermission):
+    """Permission that checks to see if the user is enrolled in the course or is staff."""
+
+    def has_object_permission(self, request, view, obj):
+        """Returns true if the user is enrolled or is staff."""
+        return has_team_api_access(request.user, obj.course_id)
+
+
+class IsStaffOrPrivilegedOrReadOnly(IsStaffOrReadOnly):
+    """
+    Permission that checks to see if the user is global staff, course
+    staff, or has discussion privileges. If none of those conditions are
+    met, only read access will be granted.
+    """
+
+    def has_object_permission(self, request, view, obj):
+        return (
+            has_discussion_privileges(request.user, obj.course_id) or
+            super(IsStaffOrPrivilegedOrReadOnly, self).has_object_permission(request, view, obj)
+        )
+
+
 class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView):
     """
         **Use Cases**
@@ -490,26 +516,6 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView):
             method returns a 400 error with all error messages in the
             "field_errors" field of the returned JSON.
     """
-
-    class IsEnrolledOrIsStaff(permissions.BasePermission):
-        """Permission that checks to see if the user is enrolled in the course or is staff."""
-
-        def has_object_permission(self, request, view, obj):
-            """Returns true if the user is enrolled or is staff."""
-            return has_team_api_access(request.user, obj.course_id)
-
-    class IsStaffOrPrivilegedOrReadOnly(IsStaffOrReadOnly):
-        """Permission that checks to see if the user is global staff, course
-        staff, or has discussion privileges. If none of those conditions are
-        met, only read access will be granted.
-        """
-
-        def has_object_permission(self, request, view, obj):
-            return (
-                has_discussion_privileges(request.user, obj.course_id) or
-                IsStaffOrReadOnly.has_object_permission(self, request, view, obj)
-            )
-
     authentication_classes = (OAuth2Authentication, SessionAuthentication)
     permission_classes = (permissions.IsAuthenticated, IsStaffOrPrivilegedOrReadOnly, IsEnrolledOrIsStaff,)
     lookup_field = 'team_id'
@@ -536,8 +542,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.
 
@@ -583,8 +590,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}"""
@@ -613,9 +618,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
@@ -625,22 +628,37 @@ class TopicListView(GenericAPIView):
                 'user_message': _(u"The ordering {ordering} is not supported").format(ordering=ordering),
             }, status=status.HTTP_400_BAD_REQUEST)
 
+        # 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)
-        serializer = self.pagination_serializer_class(page, context={'course_id': course_id, 'sort_order': ordering})
+            # 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):