Commit 65cbab73 by Eric Fischer

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.
parent 6edadc2a
...@@ -1134,6 +1134,10 @@ class DeleteTeamTest(TeamFormActions): ...@@ -1134,6 +1134,10 @@ class DeleteTeamTest(TeamFormActions):
self.team = self.create_teams(self.topic, num_teams=1)[0] self.team = self.create_teams(self.topic, num_teams=1)[0]
self.team_page = TeamPage(self.browser, self.course_id, team=self.team) 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() self.team_page.visit()
def test_cancel_delete(self): def test_cancel_delete(self):
...@@ -1187,11 +1191,42 @@ class DeleteTeamTest(TeamFormActions): ...@@ -1187,11 +1191,42 @@ class DeleteTeamTest(TeamFormActions):
self.assertNotIn(self.team['name'], browse_teams_page.team_names) self.assertNotIn(self.team['name'], browse_teams_page.team_names)
def delete_team(self, **kwargs): 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_page.click_edit_team_button()
self.team_management_page.wait_for_page() self.team_management_page.wait_for_page()
self.team_management_page.delete_team_button.click() 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): def test_delete_team_updates_topics(self):
""" """
...@@ -1448,7 +1483,11 @@ class EditMembershipTest(TeamFormActions): ...@@ -1448,7 +1483,11 @@ class EditMembershipTest(TeamFormActions):
self.team_page = TeamPage(self.browser, self.course_id, team=self.team) self.team_page = TeamPage(self.browser, self.course_id, team=self.team)
def edit_membership_helper(self, role, cancel=False): 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: if role is not None:
AutoAuthPage( AutoAuthPage(
self.browser, self.browser,
...@@ -1472,7 +1511,21 @@ class EditMembershipTest(TeamFormActions): ...@@ -1472,7 +1511,21 @@ class EditMembershipTest(TeamFormActions):
self.edit_membership_page.cancel_delete_membership_dialog() self.edit_membership_page.cancel_delete_membership_dialog()
self.assertEqual(self.edit_membership_page.team_members, 1) self.assertEqual(self.edit_membership_page.team_members, 1)
else: 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.assertEqual(self.edit_membership_page.team_members, 0)
self.assertTrue(self.edit_membership_page.is_browser_on_page) self.assertTrue(self.edit_membership_page.is_browser_on_page)
......
...@@ -90,7 +90,11 @@ define([ ...@@ -90,7 +90,11 @@ define([
verifyTeamMembersView(view); verifyTeamMembersView(view);
deleteTeamMemember(view, true); 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); AjaxHelpers.respondWithNoContent(requests);
expect(view.teamEvents.trigger).toHaveBeenCalledWith( expect(view.teamEvents.trigger).toHaveBeenCalledWith(
'teams:update', { 'teams:update', {
...@@ -112,7 +116,11 @@ define([ ...@@ -112,7 +116,11 @@ define([
verifyTeamMembersView(view); verifyTeamMembersView(view);
deleteTeamMemember(view, true); 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); AjaxHelpers.respondWithError(requests);
expect(TeamUtils.showMessage).toHaveBeenCalledWith( expect(TeamUtils.showMessage).toHaveBeenCalledWith(
'An error occurred while removing the member from the team. Try again.', 'An error occurred while removing the member from the team. Try again.',
......
...@@ -145,7 +145,7 @@ define([ ...@@ -145,7 +145,7 @@ define([
} }
], ],
'fires a page view event for the edit team page': [ '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', page_name: 'edit-team',
topic_id: TeamSpecHelpers.testTopicID, topic_id: TeamSpecHelpers.testTopicID,
...@@ -154,7 +154,9 @@ define([ ...@@ -154,7 +154,9 @@ define([
] ]
}, function (url, expectedEvent) { }, function (url, expectedEvent) {
var requests = AjaxHelpers.requests(this), var requests = AjaxHelpers.requests(this),
teamsTabView = createTeamsTabView(); teamsTabView = createTeamsTabView({
userInfo: TeamSpecHelpers.createMockUserInfo({ staff: true })
});
teamsTabView.router.navigate(url, {trigger: true}); teamsTabView.router.navigate(url, {trigger: true});
if (requests.length) { if (requests.length) {
AjaxHelpers.respondWithJson(requests, {}); AjaxHelpers.respondWithJson(requests, {});
......
...@@ -85,7 +85,7 @@ ...@@ -85,7 +85,7 @@
function () { function () {
$.ajax({ $.ajax({
type: 'DELETE', type: 'DELETE',
url: self.teamMembershipDetailUrl + username url: self.teamMembershipDetailUrl.concat(username, '?admin=true')
}).done(function () { }).done(function () {
self.teamEvents.trigger('teams:update', { self.teamEvents.trigger('teams:update', {
action: 'leave', action: 'leave',
......
...@@ -279,6 +279,7 @@ ...@@ -279,6 +279,7 @@
}); });
self.mainView = editViewWithHeader; self.mainView = editViewWithHeader;
self.render(); self.render();
TeamAnalytics.emitPageViewed('edit-team', topicID, teamID);
}); });
}, },
...@@ -303,7 +304,7 @@ ...@@ -303,7 +304,7 @@
} }
); );
self.render(); self.render();
TeamAnalytics.emitPageViewed('edit-team', topicID, teamID); TeamAnalytics.emitPageViewed('edit-team-members', topicID, teamID);
}); });
}, },
......
...@@ -389,12 +389,8 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): ...@@ -389,12 +389,8 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase):
def delete_membership(self, team_id, username, expected_status=200, **kwargs): def delete_membership(self, team_id, username, expected_status=200, **kwargs):
"""Deletes an individual membership record. Verifies expected_status.""" """Deletes an individual membership record. Verifies expected_status."""
return self.make_call( url = reverse('team_membership_detail', args=[team_id, username]) + '?admin=true'
reverse('team_membership_detail', args=[team_id, username]), return self.make_call(url, expected_status, 'delete', **kwargs)
expected_status,
'delete',
**kwargs
)
def verify_expanded_public_user(self, user): def verify_expanded_public_user(self, user):
"""Verifies that fields exist on the returned user json indicating that it is expanded.""" """Verifies that fields exist on the returned user json indicating that it is expanded."""
...@@ -803,9 +799,12 @@ class TestDetailTeamAPI(TeamAPITestCase): ...@@ -803,9 +799,12 @@ class TestDetailTeamAPI(TeamAPITestCase):
@ddt.ddt @ddt.ddt
class TestDeleteTeamAPI(TeamAPITestCase): class TestDeleteTeamAPI(EventTestMixin, TeamAPITestCase):
"""Test cases for the team delete endpoint.""" """Test cases for the team delete endpoint."""
def setUp(self): # pylint: disable=arguments-differ
super(TestDeleteTeamAPI, self).setUp('teams.views.tracker')
@ddt.data( @ddt.data(
(None, 401), (None, 401),
('student_inactive', 401), ('student_inactive', 401),
...@@ -818,6 +817,19 @@ class TestDeleteTeamAPI(TeamAPITestCase): ...@@ -818,6 +817,19 @@ class TestDeleteTeamAPI(TeamAPITestCase):
@ddt.unpack @ddt.unpack
def test_access(self, user, status): def test_access(self, user, status):
self.delete_team(self.solar_team.team_id, status, user=user) 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): def test_does_not_exist(self):
self.delete_team('nonexistent', 404) self.delete_team('nonexistent', 404)
...@@ -825,6 +837,18 @@ class TestDeleteTeamAPI(TeamAPITestCase): ...@@ -825,6 +837,18 @@ class TestDeleteTeamAPI(TeamAPITestCase):
def test_memberships_deleted(self): def test_memberships_deleted(self):
self.assertEqual(CourseTeamMembership.objects.filter(team=self.solar_team).count(), 1) self.assertEqual(CourseTeamMembership.objects.filter(team=self.solar_team).count(), 1)
self.delete_team(self.solar_team.team_id, 204, user='staff') 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) self.assertEqual(CourseTeamMembership.objects.filter(team=self.solar_team).count(), 0)
...@@ -1351,17 +1375,32 @@ class TestDeleteMembershipAPI(EventTestMixin, TeamAPITestCase): ...@@ -1351,17 +1375,32 @@ class TestDeleteMembershipAPI(EventTestMixin, TeamAPITestCase):
) )
if status == 204: if status == 204:
remove_method = 'self_removal' if user == 'student_enrolled' else 'removed_by_admin'
self.assert_event_emitted( self.assert_event_emitted(
'edx.team.learner_removed', 'edx.team.learner_removed',
team_id=self.solar_team.team_id, team_id=self.solar_team.team_id,
course_id=unicode(self.solar_team.course_id), course_id=unicode(self.solar_team.course_id),
user_id=self.users['student_enrolled'].id, user_id=self.users['student_enrolled'].id,
remove_method=remove_method remove_method='removed_by_admin'
) )
else: else:
self.assert_no_events_were_emitted() 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): def test_bad_team(self):
self.delete_membership('no_such_team', self.users['student_enrolled'].username, 404) self.delete_membership('no_such_team', self.users['student_enrolled'].username, 404)
......
...@@ -612,9 +612,23 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): ...@@ -612,9 +612,23 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView):
"""DELETE /api/team/v0/teams/{team_id}""" """DELETE /api/team/v0/teams/{team_id}"""
team = get_object_or_404(CourseTeam, team_id=team_id) team = get_object_or_404(CourseTeam, team_id=team_id)
self.check_object_permissions(request, team) 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 # Note: also deletes all team memberships associated with this team
team.delete() team.delete()
log.info('user %d deleted team %s', request.user.id, team_id) 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) return Response(status=status.HTTP_204_NO_CONTENT)
...@@ -1184,6 +1198,9 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): ...@@ -1184,6 +1198,9 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView):
team = self.get_team(team_id) team = self.get_team(team_id)
if has_team_api_access(request.user, team.course_id, access_username=username): if has_team_api_access(request.user, team.course_id, access_username=username):
membership = self.get_membership(username, team) membership = self.get_membership(username, team)
removal_method = 'self_removal'
if 'admin' in request.QUERY_PARAMS:
removal_method = 'removed_by_admin'
membership.delete() membership.delete()
tracker.emit( tracker.emit(
'edx.team.learner_removed', 'edx.team.learner_removed',
...@@ -1191,7 +1208,7 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): ...@@ -1191,7 +1208,7 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView):
'team_id': team.team_id, 'team_id': team.team_id,
'course_id': unicode(team.course_id), 'course_id': unicode(team.course_id),
'user_id': membership.user.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) return Response(status=status.HTTP_204_NO_CONTENT)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment