From 65cbab739a2283ec609177f3ad4a36f5d7d04ecc Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 3 Sep 2015 14:58:19 -0400 Subject: [PATCH] Team deleted and learner removed events Now emitting `edx.team.deleted` event where relevant. Includes tests. Also includes tests to cover learner_removed events that are fired as part of the deletion process, as well as learner_removed events fired from the edit membership view. Adding these deleted events has brought about a change in the teams API - the DELETE membership endpoint now takes an optional `admin` query parameter to differentiate between admin removal and self removal for analytics purposes. Also includes some changes needed to get page viewed events working with admin tools changes made in this PR. --- .../test/acceptance/tests/lms/test_teams.py | 61 +++++++++++++++++-- .../js/spec/views/edit_team_members_spec.js | 12 +++- .../teams/js/spec/views/teams_tab_spec.js | 6 +- .../teams/js/views/edit_team_members.js | 2 +- .../teams/static/teams/js/views/teams_tab.js | 3 +- lms/djangoapps/teams/tests/test_views.py | 57 ++++++++++++++--- lms/djangoapps/teams/views.py | 19 +++++- 7 files changed, 140 insertions(+), 20 deletions(-) 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)