diff --git a/common/test/acceptance/tests/lms/test_teams.py b/common/test/acceptance/tests/lms/test_teams.py index 2897313cb6..d172b0267c 100644 --- a/common/test/acceptance/tests/lms/test_teams.py +++ b/common/test/acceptance/tests/lms/test_teams.py @@ -1134,6 +1134,10 @@ class DeleteTeamTest(TeamFormActions): self.team = self.create_teams(self.topic, num_teams=1)[0] self.team_page = TeamPage(self.browser, self.course_id, team=self.team) + + #need to have a membership to confirm it gets deleted as well + self.create_membership(self.user_info['username'], self.team['id']) + self.team_page.visit() def test_cancel_delete(self): @@ -1187,11 +1191,42 @@ class DeleteTeamTest(TeamFormActions): self.assertNotIn(self.team['name'], browse_teams_page.team_names) def delete_team(self, **kwargs): - """Delete a team. Passes `kwargs` to `confirm_prompt`.""" + """ + Delete a team. Passes `kwargs` to `confirm_prompt`. + Expects edx.team.deleted event to be emitted, with correct course_id. + Also expects edx.team.learner_removed event to be emitted for the + membership that is removed as a part of the delete operation. + """ + self.team_page.click_edit_team_button() self.team_management_page.wait_for_page() self.team_management_page.delete_team_button.click() - confirm_prompt(self.team_management_page, **kwargs) + + if 'cancel' in kwargs and kwargs['cancel'] is True: + confirm_prompt(self.team_management_page, **kwargs) + else: + expected_events = [ + { + 'event_type': 'edx.team.deleted', + 'event': { + 'course_id': self.course_id, + 'team_id': self.team['id'] + } + }, + { + 'event_type': 'edx.team.learner_removed', + 'event': { + 'course_id': self.course_id, + 'team_id': self.team['id'], + 'remove_method': 'team_deleted', + 'user_id': self.user_info['user_id'] + } + } + ] + with self.assert_events_match_during( + event_filter=self.only_team_events, expected_events=expected_events + ): + confirm_prompt(self.team_management_page, **kwargs) def test_delete_team_updates_topics(self): """ @@ -1448,7 +1483,11 @@ class EditMembershipTest(TeamFormActions): self.team_page = TeamPage(self.browser, self.course_id, team=self.team) def edit_membership_helper(self, role, cancel=False): - """ Helper for common functionality in edit membership tests """ + """ + Helper for common functionality in edit membership tests. + Checks for all relevant assertions about membership being removed, + including verify edx.team.learner_removed events are emitted. + """ if role is not None: AutoAuthPage( self.browser, @@ -1472,7 +1511,21 @@ class EditMembershipTest(TeamFormActions): self.edit_membership_page.cancel_delete_membership_dialog() self.assertEqual(self.edit_membership_page.team_members, 1) else: - self.edit_membership_page.confirm_delete_membership_dialog() + expected_events = [ + { + 'event_type': 'edx.team.learner_removed', + 'event': { + 'course_id': self.course_id, + 'team_id': self.team['id'], + 'remove_method': 'removed_by_admin', + 'user_id': self.user_info['user_id'] + } + } + ] + with self.assert_events_match_during( + event_filter=self.only_team_events, expected_events=expected_events + ): + self.edit_membership_page.confirm_delete_membership_dialog() self.assertEqual(self.edit_membership_page.team_members, 0) self.assertTrue(self.edit_membership_page.is_browser_on_page) diff --git a/lms/djangoapps/teams/static/teams/js/spec/views/edit_team_members_spec.js b/lms/djangoapps/teams/static/teams/js/spec/views/edit_team_members_spec.js index cb58c24730..aa241b6b5a 100644 --- a/lms/djangoapps/teams/static/teams/js/spec/views/edit_team_members_spec.js +++ b/lms/djangoapps/teams/static/teams/js/spec/views/edit_team_members_spec.js @@ -90,7 +90,11 @@ define([ verifyTeamMembersView(view); deleteTeamMemember(view, true); - AjaxHelpers.expectJsonRequest(requests, 'DELETE', '/api/team/v0/team_membership/av,frodo', null); + AjaxHelpers.expectJsonRequest( + requests, + 'DELETE', + '/api/team/v0/team_membership/av,frodo?admin=true' + ); AjaxHelpers.respondWithNoContent(requests); expect(view.teamEvents.trigger).toHaveBeenCalledWith( 'teams:update', { @@ -112,7 +116,11 @@ define([ verifyTeamMembersView(view); deleteTeamMemember(view, true); - AjaxHelpers.expectJsonRequest(requests, 'DELETE', '/api/team/v0/team_membership/av,frodo', null); + AjaxHelpers.expectJsonRequest( + requests, + 'DELETE', + '/api/team/v0/team_membership/av,frodo?admin=true' + ); AjaxHelpers.respondWithError(requests); expect(TeamUtils.showMessage).toHaveBeenCalledWith( 'An error occurred while removing the member from the team. Try again.', diff --git a/lms/djangoapps/teams/static/teams/js/spec/views/teams_tab_spec.js b/lms/djangoapps/teams/static/teams/js/spec/views/teams_tab_spec.js index e0959f78ee..744dbad877 100644 --- a/lms/djangoapps/teams/static/teams/js/spec/views/teams_tab_spec.js +++ b/lms/djangoapps/teams/static/teams/js/spec/views/teams_tab_spec.js @@ -145,7 +145,7 @@ define([ } ], 'fires a page view event for the edit team page': [ - 'topics/' + TeamSpecHelpers.testTopicID + '/' + 'test_team_id/edit-team', + 'teams/' + TeamSpecHelpers.testTopicID + '/' + 'test_team_id/edit-team', { page_name: 'edit-team', topic_id: TeamSpecHelpers.testTopicID, @@ -154,7 +154,9 @@ define([ ] }, function (url, expectedEvent) { var requests = AjaxHelpers.requests(this), - teamsTabView = createTeamsTabView(); + teamsTabView = createTeamsTabView({ + userInfo: TeamSpecHelpers.createMockUserInfo({ staff: true }) + }); teamsTabView.router.navigate(url, {trigger: true}); if (requests.length) { AjaxHelpers.respondWithJson(requests, {}); diff --git a/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js b/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js index 02d251d839..dc48ca96e3 100644 --- a/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js +++ b/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js @@ -85,7 +85,7 @@ function () { $.ajax({ type: 'DELETE', - url: self.teamMembershipDetailUrl + username + url: self.teamMembershipDetailUrl.concat(username, '?admin=true') }).done(function () { self.teamEvents.trigger('teams:update', { action: 'leave', diff --git a/lms/djangoapps/teams/static/teams/js/views/teams_tab.js b/lms/djangoapps/teams/static/teams/js/views/teams_tab.js index 186156077e..3f3831efaf 100644 --- a/lms/djangoapps/teams/static/teams/js/views/teams_tab.js +++ b/lms/djangoapps/teams/static/teams/js/views/teams_tab.js @@ -279,6 +279,7 @@ }); self.mainView = editViewWithHeader; self.render(); + TeamAnalytics.emitPageViewed('edit-team', topicID, teamID); }); }, @@ -303,7 +304,7 @@ } ); self.render(); - TeamAnalytics.emitPageViewed('edit-team', topicID, teamID); + TeamAnalytics.emitPageViewed('edit-team-members', topicID, teamID); }); }, diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 81bee98855..45fb2512b8 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -389,12 +389,8 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): def delete_membership(self, team_id, username, expected_status=200, **kwargs): """Deletes an individual membership record. Verifies expected_status.""" - return self.make_call( - reverse('team_membership_detail', args=[team_id, username]), - expected_status, - 'delete', - **kwargs - ) + url = reverse('team_membership_detail', args=[team_id, username]) + '?admin=true' + return self.make_call(url, expected_status, 'delete', **kwargs) def verify_expanded_public_user(self, user): """Verifies that fields exist on the returned user json indicating that it is expanded.""" @@ -803,9 +799,12 @@ class TestDetailTeamAPI(TeamAPITestCase): @ddt.ddt -class TestDeleteTeamAPI(TeamAPITestCase): +class TestDeleteTeamAPI(EventTestMixin, TeamAPITestCase): """Test cases for the team delete endpoint.""" + def setUp(self): # pylint: disable=arguments-differ + super(TestDeleteTeamAPI, self).setUp('teams.views.tracker') + @ddt.data( (None, 401), ('student_inactive', 401), @@ -818,6 +817,19 @@ class TestDeleteTeamAPI(TeamAPITestCase): @ddt.unpack def test_access(self, user, status): self.delete_team(self.solar_team.team_id, status, user=user) + if status == 204: + self.assert_event_emitted( + 'edx.team.deleted', + team_id=self.solar_team.team_id, + course_id=unicode(self.test_course_1.id) + ) + self.assert_event_emitted( + 'edx.team.learner_removed', + team_id=self.solar_team.team_id, + course_id=unicode(self.test_course_1.id), + remove_method='team_deleted', + user_id=self.users['student_enrolled'].id + ) def test_does_not_exist(self): self.delete_team('nonexistent', 404) @@ -825,6 +837,18 @@ class TestDeleteTeamAPI(TeamAPITestCase): def test_memberships_deleted(self): self.assertEqual(CourseTeamMembership.objects.filter(team=self.solar_team).count(), 1) self.delete_team(self.solar_team.team_id, 204, user='staff') + self.assert_event_emitted( + 'edx.team.deleted', + team_id=self.solar_team.team_id, + course_id=unicode(self.test_course_1.id) + ) + self.assert_event_emitted( + 'edx.team.learner_removed', + team_id=self.solar_team.team_id, + course_id=unicode(self.test_course_1.id), + remove_method='team_deleted', + user_id=self.users['student_enrolled'].id + ) self.assertEqual(CourseTeamMembership.objects.filter(team=self.solar_team).count(), 0) @@ -1351,17 +1375,32 @@ class TestDeleteMembershipAPI(EventTestMixin, TeamAPITestCase): ) if status == 204: - remove_method = 'self_removal' if user == 'student_enrolled' else 'removed_by_admin' self.assert_event_emitted( 'edx.team.learner_removed', team_id=self.solar_team.team_id, course_id=unicode(self.solar_team.course_id), user_id=self.users['student_enrolled'].id, - remove_method=remove_method + remove_method='removed_by_admin' ) else: self.assert_no_events_were_emitted() + def test_leave_team(self): + """ + The key difference between this test and test_access above is that + removal via "Edit Membership" and "Leave Team" emit different events + despite hitting the same API endpoint, due to the 'admin' query string. + """ + url = reverse('team_membership_detail', args=[self.solar_team.team_id, self.users['student_enrolled'].username]) + self.make_call(url, 204, 'delete', user='student_enrolled') + self.assert_event_emitted( + 'edx.team.learner_removed', + team_id=self.solar_team.team_id, + course_id=unicode(self.solar_team.course_id), + user_id=self.users['student_enrolled'].id, + remove_method='self_removal' + ) + def test_bad_team(self): self.delete_membership('no_such_team', self.users['student_enrolled'].username, 404) diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index eb9b7f3c62..ff9efc23a4 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -612,9 +612,23 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): """DELETE /api/team/v0/teams/{team_id}""" 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)) + # Note: also deletes all team memberships associated with this team team.delete() log.info('user %d deleted team %s', request.user.id, team_id) + tracker.emit('edx.team.deleted', { + 'team_id': team_id, + 'course_id': unicode(team.course_id), + }) + for member in memberships: + tracker.emit('edx.team.learner_removed', { + 'team_id': team_id, + 'course_id': unicode(team.course_id), + 'remove_method': 'team_deleted', + 'user_id': member.user_id + }) return Response(status=status.HTTP_204_NO_CONTENT) @@ -1184,6 +1198,9 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): team = self.get_team(team_id) if has_team_api_access(request.user, team.course_id, access_username=username): membership = self.get_membership(username, team) + removal_method = 'self_removal' + if 'admin' in request.QUERY_PARAMS: + removal_method = 'removed_by_admin' membership.delete() tracker.emit( 'edx.team.learner_removed', @@ -1191,7 +1208,7 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): 'team_id': team.team_id, 'course_id': unicode(team.course_id), 'user_id': membership.user.id, - 'remove_method': 'self_removal' if membership.user == request.user else 'removed_by_admin' + 'remove_method': removal_method } ) return Response(status=status.HTTP_204_NO_CONTENT)