Merge pull request #9714 from edx/peter-fogg/fix-slow-memberships

Prevent over-expansion of team memberships.

lms/djangoapps/teams/serializers.py

@@ -112,7 +112,8 @@ class MembershipSerializer(serializers.ModelSerializer):
             view_name='teams_detail',
             read_only=True,
         ),
-        expanded_serializer=CourseTeamSerializer(read_only=True)
+        expanded_serializer=CourseTeamSerializer(read_only=True),
+        exclude_expand_fields={'user'},
     )
 
     class Meta(object):

lms/djangoapps/teams/static/teams/js/spec_helpers/team_spec_helpers.js

@@ -68,8 +68,11 @@ define([
         return _.map(_.range(startIndex, stopIndex + 1), function (i) {
             return {
                 user: {
-                    'username': testUser,
-                    'url': 'https://openedx.example.com/api/user/v1/accounts/' + testUser
+                    username: testUser,
+                    url: 'https://openedx.example.com/api/user/v1/accounts/' + testUser,
+                    profile_image: {
+                        image_url_small: 'test_profile_image'
+                    }
                 },
                 team: teams[i-1]
             };

lms/djangoapps/teams/static/teams/js/views/team_card.js

@@ -32,11 +32,13 @@
 
             initialize: function (options) {
                 this.maxTeamSize = options.maxTeamSize;
+                this.memberships = options.memberships;
             },
 
             render: function () {
-                var allMemberships = _(this.model.get('membership'))
-                        .sortBy(function (member) {return new Date(member.last_activity_at);}).reverse(),
+                var allMemberships = _(this.memberships).sortBy(function (member) {
+                    return new Date(member.last_activity_at);
+                }).reverse(),
                     displayableMemberships = allMemberships.slice(0, 5),
                     maxMemberCount = this.maxTeamSize;
                 this.$el.html(this.template({
@@ -97,7 +99,7 @@
                 CardView.prototype.initialize.apply(this, arguments);
                 // TODO: show last activity detail view
                 this.detailViews = [
-                    new TeamMembershipView({model: this.teamModel(), maxTeamSize: this.maxTeamSize}),
+                    new TeamMembershipView({memberships: this.getMemberships(), maxTeamSize: this.maxTeamSize}),
                     new TeamCountryLanguageView({
                         model: this.teamModel(),
                         countries: this.countries,
@@ -105,6 +107,9 @@
                     }),
                     new TeamActivityView({date: this.teamModel().get('last_activity_at')})
                 ];
+                this.model.on('change:membership', function () {
+                    this.detailViews[0].memberships = this.getMemberships();
+                }, this);
             },
 
             teamModel: function () {
@@ -112,6 +117,15 @@
                 return this.model;
             },
 
+            getMemberships: function () {
+                if (this.model.has('team')) {
+                    return [this.model.attributes];
+                }
+                else {
+                    return this.model.get('membership');
+                }
+            },
+
             configuration: 'list_card',
             cardClass: 'team-card',
             title: function () { return this.teamModel().get('name'); },

lms/djangoapps/teams/tests/test_serializers.py

@@ -3,18 +3,24 @@
 Tests for custom Teams Serializers.
 """
 from django.core.paginator import Paginator
+from django.test.client import RequestFactory
 
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from student.tests.factories import CourseEnrollmentFactory, UserFactory
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
-from lms.djangoapps.teams.tests.factories import CourseTeamFactory
+from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory
 from lms.djangoapps.teams.serializers import (
-    BaseTopicSerializer, PaginatedTopicSerializer, BulkTeamCountPaginatedTopicSerializer,
-    TopicSerializer, add_team_count
+    BaseTopicSerializer,
+    PaginatedTopicSerializer,
+    BulkTeamCountPaginatedTopicSerializer,
+    TopicSerializer,
+    MembershipSerializer,
+    add_team_count
 )
 
 
-class TopicTestCase(ModuleStoreTestCase):
+class SerializerTestCase(SharedModuleStoreTestCase):
     """
     Base test class to set up a course with topics
     """
@@ -22,7 +28,7 @@ class TopicTestCase(ModuleStoreTestCase):
         """
         Set up a course with a teams configuration.
         """
-        super(TopicTestCase, self).setUp()
+        super(SerializerTestCase, self).setUp()
         self.course = CourseFactory.create(
             teams_configuration={
                 "max_team_size": 10,
@@ -31,7 +37,46 @@ class TopicTestCase(ModuleStoreTestCase):
         )
 
 
-class BaseTopicSerializerTestCase(TopicTestCase):
+class MembershipSerializerTestCase(SerializerTestCase):
+    """
+    Tests for the membership serializer.
+    """
+
+    def setUp(self):
+        super(MembershipSerializerTestCase, self).setUp()
+        self.team = CourseTeamFactory.create(
+            course_id=self.course.id,
+            topic_id=self.course.teams_topics[0]['id']
+        )
+        self.user = UserFactory.create()
+        CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id)
+        self.team_membership = CourseTeamMembershipFactory.create(team=self.team, user=self.user)
+
+    def test_membership_serializer_expand_user_and_team(self):
+        """Verify that the serializer only expands the user and team one level."""
+        data = MembershipSerializer(self.team_membership, context={
+            'expand': [u'team', u'user'],
+            'request': RequestFactory().get('/api/team/v0/team_membership')
+        }).data
+        username = self.user.username
+        self.assertEqual(data['user'], {
+            'url': 'http://testserver/api/user/v1/accounts/' + username,
+            'username': username,
+            'profile_image': {
+                'image_url_full': 'http://testserver/static/default_500.png',
+                'image_url_large': 'http://testserver/static/default_120.png',
+                'image_url_medium': 'http://testserver/static/default_50.png',
+                'image_url_small': 'http://testserver/static/default_30.png',
+                'has_image': False
+            }
+        })
+        self.assertEqual(data['team']['membership'][0]['user'], {
+            'url': 'http://testserver/api/user/v1/accounts/' + username,
+            'username': username
+        })
+
+
+class BaseTopicSerializerTestCase(SerializerTestCase):
     """
     Tests for the `BaseTopicSerializer`, which should not serialize team count
     data.
@@ -46,7 +91,7 @@ class BaseTopicSerializerTestCase(TopicTestCase):
             )
 
 
-class TopicSerializerTestCase(TopicTestCase):
+class TopicSerializerTestCase(SerializerTestCase):
     """
     Tests for the `TopicSerializer`, which should serialize team count data for
     a single topic.
@@ -95,7 +140,7 @@ class TopicSerializerTestCase(TopicTestCase):
             )
 
 
-class BasePaginatedTopicSerializerTestCase(TopicTestCase):
+class BasePaginatedTopicSerializerTestCase(SerializerTestCase):
     """
     Base class for testing the two paginated topic serializers.
     """

openedx/core/lib/api/fields.py

@@ -5,18 +5,26 @@ from rest_framework.serializers import CharField, Field
 
 
 class ExpandableField(Field):
-    """Field that can dynamically use a more detailed serializer based on a user-provided "expand" parameter."""
+    """Field that can dynamically use a more detailed serializer based on a user-provided "expand" parameter.
+
+    Kwargs:
+      collapsed_serializer (Serializer): the serializer to use for a non-expanded representation.
+      expanded_serializer (Serializer): the serializer to use for an expanded representation.
+      exclude_expand_fields (set(str)): a set of fields which will not be expanded by sub-serializers.
+    """
     def __init__(self, **kwargs):
         """Sets up the ExpandableField with the collapsed and expanded versions of the serializer."""
         assert 'collapsed_serializer' in kwargs and 'expanded_serializer' in kwargs
         self.collapsed = kwargs.pop('collapsed_serializer')
         self.expanded = kwargs.pop('expanded_serializer')
+        self.exclude_expand_fields = kwargs.pop('exclude_expand_fields', set())
         super(ExpandableField, self).__init__(**kwargs)
 
     def field_to_native(self, obj, field_name):
         """Converts obj to a native representation, using the expanded serializer if the context requires it."""
         if 'expand' in self.context and field_name in self.context['expand']:
             self.expanded.initialize(self, field_name)
+            self.expanded.context['expand'] = list(set(self.expanded.context['expand']) - self.exclude_expand_fields)
             return self.expanded.field_to_native(obj, field_name)
         else:
             self.collapsed.initialize(self, field_name)