diff --git a/lms/djangoapps/teams/serializers.py b/lms/djangoapps/teams/serializers.py index 4d9b59e3c9..7f403c2957 100644 --- a/lms/djangoapps/teams/serializers.py +++ b/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) + """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']] - 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) +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) diff --git a/lms/djangoapps/teams/tests/test_serializers.py b/lms/djangoapps/teams/tests/test_serializers.py index 69fa07b14a..2b346fb18a 100644 --- a/lms/djangoapps/teams/tests/test_serializers.py +++ b/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) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 088a3677f7..a2b01c3bcd 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/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, diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index 59f1387706..a44d14fc5a 100644 --- a/lms/djangoapps/teams/views.py +++ b/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) - 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):