From 21c6d6b08dbbb7576717441c074d9fa17f832a6a Mon Sep 17 00:00:00 2001 From: Jansen Kantor Date: Thu, 19 Mar 2020 13:56:35 -0400 Subject: [PATCH] EDUCATOR-4952: Allow Users to join one team per open teamset (#23423) *allow users to join one team per teamset --- .../test/acceptance/tests/lms/test_teams.py | 2 +- lms/djangoapps/teams/models.py | 15 ++++- .../views/team_profile_header_actions_spec.js | 25 +++++--- .../js/views/team_profile_header_actions.js | 18 +++--- lms/djangoapps/teams/tests/test_models.py | 6 +- lms/djangoapps/teams/tests/test_views.py | 60 +++++++++++++++++++ lms/djangoapps/teams/views.py | 57 ++++++++++++++++-- 7 files changed, 157 insertions(+), 26 deletions(-) diff --git a/common/test/acceptance/tests/lms/test_teams.py b/common/test/acceptance/tests/lms/test_teams.py index 726cae572f..95b0282465 100644 --- a/common/test/acceptance/tests/lms/test_teams.py +++ b/common/test/acceptance/tests/lms/test_teams.py @@ -1896,7 +1896,7 @@ class TeamPageTest(TeamsTabBase): """ self._set_team_configuration_and_membership(membership_team_index=0, visit_team_index=1) self.team_page.visit() - self.assertEqual(self.team_page.join_team_message, 'You already belong to another team.') + self.assertEqual(self.team_page.join_team_message, 'You already belong to another team in this team set.') self.assert_team_details(num_members=0, is_member=False) def test_team_full_message(self): diff --git a/lms/djangoapps/teams/models.py b/lms/djangoapps/teams/models.py index 99ef3a2ca6..65ab899f8e 100644 --- a/lms/djangoapps/teams/models.py +++ b/lms/djangoapps/teams/models.py @@ -299,7 +299,7 @@ class CourseTeamMembership(models.Model): self.team.reset_team_size() @classmethod - def get_memberships(cls, username=None, course_ids=None, team_id=None): + def get_memberships(cls, username=None, course_ids=None, team_ids=None): """ Get a queryset of memberships. @@ -313,8 +313,8 @@ class CourseTeamMembership(models.Model): queryset = queryset.filter(user__username=username) if course_ids is not None: queryset = queryset.filter(team__course_id__in=course_ids) - if team_id is not None: - queryset = queryset.filter(team__team_id=team_id) + if team_ids is not None: + queryset = queryset.filter(team__team_id__in=team_ids) return queryset @@ -360,3 +360,12 @@ class CourseTeamMembership(models.Model): emit_team_event('edx.team.activity_updated', membership.team.course_id, { 'team_id': membership.team.team_id, }) + + @classmethod + def is_user_on_team(cls, user, team): + """ Is `user` on `team`?""" + try: + cls.objects.get(user=user, team=team) + except ObjectDoesNotExist: + return False + return True diff --git a/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_header_actions_spec.js b/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_header_actions_spec.js index 0d7e16eaf5..a02c8b77b6 100644 --- a/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_header_actions_spec.js +++ b/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_header_actions_spec.js @@ -21,7 +21,8 @@ define([ id: teamId, name: teamName, membership: membership, - url: createTeamsUrl(teamId) + url: createTeamsUrl(teamId), + topic_id: 'topic-id' }; }; @@ -116,7 +117,9 @@ define([ requests, 'GET', TeamSpecHelpers.testContext.teamMembershipsUrl + '?' + $.param({ - username: currentUsername, course_id: TeamSpecHelpers.testCourseID + username: currentUsername, + course_id: TeamSpecHelpers.testCourseID, + teamset_id: 'topic-id' }) ); @@ -163,14 +166,16 @@ define([ requests, 'GET', TeamSpecHelpers.testContext.teamMembershipsUrl + '?' + $.param({ - username: currentUsername, course_id: TeamSpecHelpers.testCourseID + username: currentUsername, + course_id: TeamSpecHelpers.testCourseID, + teamset_id: 'topic-id' }) ); // current user is a member of another team so we should see the correct message AjaxHelpers.respondWithJson(requests, {count: 1}); expect(view.$('.action.action-primary').length).toEqual(0); - expect(view.$('.join-team-message').text().trim()).toBe(view.alreadyMemberMessage); + expect(view.$('.join-team-message').text().trim()).toBe(view.alreadyTeamsetMemberMessage); }); it('shows team full message', function() { @@ -210,7 +215,9 @@ define([ requests, 'GET', TeamSpecHelpers.testContext.teamMembershipsUrl + '?' + $.param({ - username: currentUsername, course_id: TeamSpecHelpers.testCourseID + username: currentUsername, + course_id: TeamSpecHelpers.testCourseID, + teamset_id: 'topic-id' }) ); @@ -241,7 +248,9 @@ define([ requests, 'GET', TeamSpecHelpers.testContext.teamMembershipsUrl + '?' + $.param({ - username: currentUsername, course_id: TeamSpecHelpers.testCourseID + username: currentUsername, + course_id: TeamSpecHelpers.testCourseID, + teamset_id: 'topic-id' }) ); @@ -270,7 +279,9 @@ define([ requests, 'GET', TeamSpecHelpers.testContext.teamMembershipsUrl + '?' + $.param({ - username: currentUsername, course_id: TeamSpecHelpers.testCourseID + username: currentUsername, + course_id: TeamSpecHelpers.testCourseID, + teamset_id: 'topic-id' }) ); diff --git a/lms/djangoapps/teams/static/teams/js/views/team_profile_header_actions.js b/lms/djangoapps/teams/static/teams/js/views/team_profile_header_actions.js index 4f68782973..4de114556f 100644 --- a/lms/djangoapps/teams/static/teams/js/views/team_profile_header_actions.js +++ b/lms/djangoapps/teams/static/teams/js/views/team_profile_header_actions.js @@ -12,7 +12,7 @@ return Backbone.View.extend({ errorMessage: gettext('An error occurred. Try again.'), - alreadyMemberMessage: gettext('You already belong to another team.'), + alreadyTeamsetMemberMessage: gettext('You already belong to another team in this team set.'), teamFullMessage: gettext('This team is full.'), notJoinInstructorManagedTeam: gettext('Cannot join instructor managed team'), @@ -41,9 +41,9 @@ // if user is the member of current team then we wouldn't show anything if (!info.memberOfCurrentTeam) { - if (info.alreadyMember) { + if (info.alreadyInTeamset) { showJoinButton = false; - message = info.memberOfCurrentTeam ? '' : view.alreadyMemberMessage; + message = info.memberOfCurrentTeam ? '' : view.alreadyTeamsetMemberMessage; } else if (!teamHasSpace) { showJoinButton = false; message = view.teamFullMessage; @@ -90,7 +90,7 @@ getUserTeamInfo: function(username, courseMaxTeamSize) { var deferred = $.Deferred(); var info = { - alreadyMember: false, + alreadyInTeamset: false, memberOfCurrentTeam: false, teamHasSpace: false, isAdminOrStaff: false, @@ -108,7 +108,7 @@ info.isInstructorManagedTopic = isInstructorManagedTopic; if (info.memberOfCurrentTeam) { - info.alreadyMember = true; + info.alreadyInTeamset = true; info.memberOfCurrentTeam = true; deferred.resolve(info); } else { @@ -117,9 +117,13 @@ $.ajax({ type: 'GET', url: view.context.teamMembershipsUrl, - data: {username: username, course_id: view.context.courseID} + data: { + username: username, + course_id: view.context.courseID, + teamset_id: view.model.get('topic_id') + } }).done(function(data) { - info.alreadyMember = (data.count > 0); + info.alreadyInTeamset = (data.count > 0); info.memberOfCurrentTeam = false; info.teamHasSpace = teamHasSpace; deferred.resolve(info); diff --git a/lms/djangoapps/teams/tests/test_models.py b/lms/djangoapps/teams/tests/test_models.py index d89dc3421e..8c31a8b624 100644 --- a/lms/djangoapps/teams/tests/test_models.py +++ b/lms/djangoapps/teams/tests/test_models.py @@ -191,13 +191,13 @@ class TeamMembershipTest(SharedModuleStoreTestCase): (None, None, None, 3), ('user1', None, None, 2), ('user1', [COURSE_KEY1], None, 1), - ('user1', None, 'team1', 1), + ('user1', None, ['team1'], 1), ('user2', None, None, 1), ) @ddt.unpack - def test_get_memberships(self, username, course_ids, team_id, expected_count): + def test_get_memberships(self, username, course_ids, team_ids, expected_count): self.assertEqual( - CourseTeamMembership.get_memberships(username=username, course_ids=course_ids, team_id=team_id).count(), + CourseTeamMembership.get_memberships(username=username, course_ids=course_ids, team_ids=team_ids).count(), expected_count ) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 7ac4e9622a..97b3739742 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -1489,6 +1489,66 @@ class TestListMembershipAPI(TeamAPITestCase): result = self.get_membership_list(200, {'team_id': self.solar_team.team_id, 'expand': 'team'}) self.verify_expanded_team(result['results'][0]['team']) + @ddt.data(False, True) + def test_filter_teamset(self, filter_username): + other_username = self.create_and_enroll_student() + self.solar_team.add_user(self.users[other_username]) + filters = { + 'teamset_id': self.solar_team.topic_id, + 'course_id': self.test_course_1.id + } + if filter_username: + filters['username'] = other_username + + result = self.get_membership_list(200, filters) + self.assertEqual(result['count'], 1 if filter_username else 2) + usernames = {enrollment['user']['username'] for enrollment in result['results']} + self.assertIn(other_username, usernames) + if not filter_username: + self.assertIn('student_enrolled', usernames) + + def test_filter_teamset_team_id(self): + # team_id and teamset_id are mutually exclusive + self.get_membership_list( + 400, + { + 'team_id': self.solar_team.team_id, + 'teamset_id': 'topic_0', + 'course_id': 'TestX/TS101/Non_Existent_Course' + } + ) + + def test_filter_teamset_no_course(self): + self.get_membership_list(400, {'teamset_id': 'topic_0'}) + + def test_filter_teamset_not_enrolled_in_course(self): + self.get_membership_list( + 404, + { + 'teamset_id': 'topic_0', + 'course_id': self.test_course_1.id + }, + user='student_unenrolled' + ) + + def test_filter_teamset_course_nonexistant(self): + self.get_membership_list(404, {'teamset_id': 'topic_0', 'course_id': 'TestX/TS101/Non_Existent_Course'}) + + def test_filter_teamset_teamset_nonexistant(self): + self.get_membership_list(404, {'teamset_id': 'nonexistant', 'course_id': self.test_course_1.id}) + + def test_filter_teamset_enrolled_in_course_but_no_team_access(self): + # The requesting user is enrolled in the course, but the requested team is oraganization_protected and + # the requesting user is outside of the bubble + self.get_membership_list( + 404, + { + 'teamset_id': 'private-topic-1-id', + 'course_id': self.test_course_1.id, + 'username': 'student_on_team_1_private_set_1' + } + ) + @ddt.ddt class TestCreateMembershipAPI(EventTestMixin, TeamAPITestCase): diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index bf808fbcc9..ac0b90f91a 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -738,7 +738,7 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): team = get_object_or_404(CourseTeam, team_id=team_id) self.check_object_permissions(request, team) # Note: list() forces the queryset to be evualuated before delete() - memberships = list(CourseTeamMembership.get_memberships(team_id=team_id)) + memberships = list(CourseTeamMembership.get_memberships(team_ids=[team_id])) # Note: also deletes all team memberships associated with this team team.delete() @@ -1009,6 +1009,13 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): specified team. The requesting user must be staff or enrolled in the course associated with the team. + * teamset_id: Returns membership records only for the specified teamset. + if teamset_id is specified, course_id must also be specified. + teamset_id and team_id are mutually exclusive. For open and public_managed + teamsets, the user must be staff or enrolled in the course. For + private_managed teamsets, the user must be course staff, or a member of the + specified teamset. + * course_id: Returns membership records only for the specified course. Username must have access to this course, or else team_id must be in this course. @@ -1118,7 +1125,7 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): """GET /api/team/v0/team_membership""" specified_username_or_team = False username = None - team_id = None + team_ids = None requested_course_id = None requested_course_key = None accessible_course_ids = None @@ -1130,11 +1137,17 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): except InvalidKeyError: return Response(status=status.HTTP_404_NOT_FOUND) - if 'team_id' in request.query_params: + if 'team_id' in request.query_params and 'teamset_id' in request.query_params: + return Response( + build_api_error(ugettext_noop("teamset_id and team_id are mutually exclusive options.")), + status=status.HTTP_400_BAD_REQUEST + ) + elif 'team_id' in request.query_params: specified_username_or_team = True team_id = request.query_params['team_id'] try: team = CourseTeam.objects.get(team_id=team_id) + team_ids = [team.team_id] except CourseTeam.DoesNotExist: return Response(status=status.HTTP_404_NOT_FOUND) if requested_course_key is not None and requested_course_key != team.course_id: @@ -1143,6 +1156,39 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): return Response(status=status.HTTP_404_NOT_FOUND) if not has_specific_team_access(request.user, team): return Response(status=status.HTTP_403_FORBIDDEN) + elif 'teamset_id' in request.query_params: + if 'course_id' not in request.query_params: + return Response( + build_api_error(ugettext_noop("teamset_id requires course_id to also be provided.")), + status=status.HTTP_400_BAD_REQUEST + ) + + if not has_team_api_access(request.user, requested_course_key): + return Response(status=status.HTTP_404_NOT_FOUND) + + course_module = modulestore().get_course(requested_course_key) + if not course_module: + return Response(status=status.HTTP_404_NOT_FOUND) + specified_username_or_team = True + teamsets = course_module.teams_configuration.teamsets_by_id + teamset_id = request.query_params['teamset_id'] + teamset = teamsets.get(teamset_id, None) + if not teamset: + return Response( + build_api_error(ugettext_noop("No teamset found in given course with given id")), + status=status.HTTP_404_NOT_FOUND + ) + teamset_teams = CourseTeam.objects.filter(course_id=requested_course_key, topic_id=teamset_id) + teams_with_access = [ + team for team in teamset_teams + if has_specific_team_access(request.user, team) + ] + if not teams_with_access: + return Response( + build_api_error(ugettext_noop("No teamset found in given course with given id")), + status=status.HTTP_404_NOT_FOUND + ) + team_ids = [team.team_id for team in teams_with_access] if 'username' in request.query_params: specified_username_or_team = True @@ -1160,7 +1206,7 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): if not specified_username_or_team: return Response( - build_api_error(ugettext_noop("username or team_id must be specified.")), + build_api_error(ugettext_noop("username or (team_id or teamset_id) must be specified.")), status=status.HTTP_400_BAD_REQUEST ) @@ -1170,7 +1216,8 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): elif accessible_course_ids is not None: course_keys = accessible_course_ids - queryset = CourseTeamMembership.get_memberships(username, course_keys, team_id) + queryset = CourseTeamMembership.get_memberships(username, course_keys, team_ids) + page = self.paginate_queryset(queryset) serializer = self.get_serializer(page, many=True) return self.get_paginated_response(serializer.data)