From 48f72612c345c5bedb89e58cead3bc641b57b6c4 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Thu, 10 Sep 2015 18:05:28 -0400 Subject: [PATCH] Prevent over-expansion of team memberships. TNL-3202 --- lms/djangoapps/teams/serializers.py | 3 +- .../js/spec_helpers/team_spec_helpers.js | 7 ++- .../teams/static/teams/js/views/team_card.js | 20 +++++- .../teams/tests/test_serializers.py | 63 ++++++++++++++++--- openedx/core/lib/api/fields.py | 10 ++- 5 files changed, 87 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/teams/serializers.py b/lms/djangoapps/teams/serializers.py index b33d373287..dae2578bd2 100644 --- a/lms/djangoapps/teams/serializers.py +++ b/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): diff --git a/lms/djangoapps/teams/static/teams/js/spec_helpers/team_spec_helpers.js b/lms/djangoapps/teams/static/teams/js/spec_helpers/team_spec_helpers.js index 63a2faa92b..2d74fbcde6 100644 --- a/lms/djangoapps/teams/static/teams/js/spec_helpers/team_spec_helpers.js +++ b/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] }; diff --git a/lms/djangoapps/teams/static/teams/js/views/team_card.js b/lms/djangoapps/teams/static/teams/js/views/team_card.js index 6ee47fbb79..b9561e0bef 100644 --- a/lms/djangoapps/teams/static/teams/js/views/team_card.js +++ b/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'); }, diff --git a/lms/djangoapps/teams/tests/test_serializers.py b/lms/djangoapps/teams/tests/test_serializers.py index 2b346fb18a..df2dffdf22 100644 --- a/lms/djangoapps/teams/tests/test_serializers.py +++ b/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. """ diff --git a/openedx/core/lib/api/fields.py b/openedx/core/lib/api/fields.py index aae17b3eb6..e46bd9dea3 100644 --- a/openedx/core/lib/api/fields.py +++ b/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)