From 183d688129a0703a4cc6de1dccbb900907810fac Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 1 Oct 2014 12:06:08 -0400 Subject: [PATCH] Add events for cohort modifications This includes cohort creation/deletion and membership changes. --- common/djangoapps/course_groups/cohorts.py | 82 ++++++++++- .../djangoapps/course_groups/tests/helpers.py | 3 +- .../course_groups/tests/test_cohorts.py | 129 +++++++++++++++++- .../discussion/test_cohort_management.py | 64 +++++++++ .../internal_data_formats/.#tracking_logs.rst | 1 + lms/envs/bok_choy.auth.json | 9 ++ 6 files changed, 278 insertions(+), 10 deletions(-) create mode 120000 docs/en_us/data/source/internal_data_formats/.#tracking_logs.rst diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index 12a54e0673..f691ef5191 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -6,16 +6,69 @@ forums, and to the cohort admin views. import logging import random +from django.db.models.signals import post_save, m2m_changed +from django.dispatch import receiver from django.http import Http404 from django.utils.translation import ugettext as _ from courseware import courses +from eventtracking import tracker from student.models import get_user_by_username_or_email from .models import CourseUserGroup log = logging.getLogger(__name__) +@receiver(post_save, sender=CourseUserGroup) +def _cohort_added(sender, **kwargs): + """Emits a tracking log event each time a cohort is created""" + instance = kwargs["instance"] + if kwargs["created"] and instance.group_type == CourseUserGroup.COHORT: + tracker.emit( + "edx.cohort.created", + {"cohort_id": instance.id, "cohort_name": instance.name} + ) + + +@receiver(m2m_changed, sender=CourseUserGroup.users.through) +def _cohort_membership_changed(sender, **kwargs): + """Emits a tracking log event each time cohort membership is modified""" + def get_event_iter(user_id_iter, cohort_iter): + return ( + {"cohort_id": cohort.id, "cohort_name": cohort.name, "user_id": user_id} + for user_id in user_id_iter + for cohort in cohort_iter + ) + + action = kwargs["action"] + instance = kwargs["instance"] + pk_set = kwargs["pk_set"] + reverse = kwargs["reverse"] + + if action == "post_add": + event_name = "edx.cohort.user_added" + elif action in ["post_remove", "pre_clear"]: + event_name = "edx.cohort.user_removed" + else: + return + + if reverse: + user_id_iter = [instance.id] + if action == "pre_clear": + cohort_iter = instance.course_groups.filter(group_type=CourseUserGroup.COHORT) + else: + cohort_iter = CourseUserGroup.objects.filter(pk__in=pk_set, group_type=CourseUserGroup.COHORT) + else: + cohort_iter = [instance] if instance.group_type == CourseUserGroup.COHORT else [] + if action == "pre_clear": + user_id_iter = (user.id for user in instance.users.all()) + else: + user_id_iter = pk_set + + for event in get_event_iter(user_id_iter, cohort_iter): + tracker.emit(event_name, event) + + # A 'default cohort' is an auto-cohort that is automatically created for a course if no auto_cohort_groups have been # specified. It is intended to be used in a cohorted-course for users who have yet to be assigned to a cohort. # Note 1: If an administrator chooses to configure a cohort with the same name, the said cohort will be used as @@ -258,12 +311,16 @@ def add_cohort(course_key, name): except Http404: raise ValueError("Invalid course_key") - return CourseUserGroup.objects.create( + cohort = CourseUserGroup.objects.create( course_id=course.id, group_type=CourseUserGroup.COHORT, name=name ) - + tracker.emit( + "edx.cohort.creation_requested", + {"cohort_name": cohort.name, "cohort_id": cohort.id} + ) + return cohort def add_user_to_cohort(cohort, username_or_email): """ @@ -281,7 +338,8 @@ def add_user_to_cohort(cohort, username_or_email): ValueError if user already present in this cohort. """ user = get_user_by_username_or_email(username_or_email) - previous_cohort = None + previous_cohort_name = None + previous_cohort_id = None course_cohorts = CourseUserGroup.objects.filter( course_id=cohort.course_id, @@ -295,8 +353,20 @@ def add_user_to_cohort(cohort, username_or_email): cohort_name=cohort.name )) else: - previous_cohort = course_cohorts[0].name - course_cohorts[0].users.remove(user) + previous_cohort = course_cohorts[0] + previous_cohort.users.remove(user) + previous_cohort_name = previous_cohort.name + previous_cohort_id = previous_cohort.id + tracker.emit( + "edx.cohort.user_add_requested", + { + "user_id": user.id, + "cohort_id": cohort.id, + "cohort_name": cohort.name, + "previous_cohort_id": previous_cohort_id, + "previous_cohort_name": previous_cohort_name, + } + ) cohort.users.add(user) - return (user, previous_cohort) + return (user, previous_cohort_name) diff --git a/common/djangoapps/course_groups/tests/helpers.py b/common/djangoapps/course_groups/tests/helpers.py index 8b19efa379..94925132e3 100644 --- a/common/djangoapps/course_groups/tests/helpers.py +++ b/common/djangoapps/course_groups/tests/helpers.py @@ -4,6 +4,7 @@ Helper methods for testing cohorts. from factory import post_generation, Sequence from factory.django import DjangoModelFactory from course_groups.models import CourseUserGroup +from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum @@ -15,7 +16,7 @@ class CohortFactory(DjangoModelFactory): FACTORY_FOR = CourseUserGroup name = Sequence("cohort{}".format) - course_id = "dummy_id" + course_id = SlashSeparatedCourseKey("dummy", "dummy", "dummy") group_type = CourseUserGroup.COHORT @post_generation diff --git a/common/djangoapps/course_groups/tests/test_cohorts.py b/common/djangoapps/course_groups/tests/test_cohorts.py index f92333d51c..7e87ee4ba9 100644 --- a/common/djangoapps/course_groups/tests/test_cohorts.py +++ b/common/djangoapps/course_groups/tests/test_cohorts.py @@ -4,6 +4,7 @@ from django.conf import settings from django.http import Http404 from django.test.utils import override_settings +from mock import call, patch from student.models import CourseEnrollment from student.tests.factories import UserFactory @@ -25,6 +26,103 @@ TEST_MAPPING = {'edX/toy/2012_Fall': 'xml'} TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING) +@patch("course_groups.cohorts.tracker") +class TestCohortSignals(django.test.TestCase): + def setUp(self): + self.course_key = SlashSeparatedCourseKey("dummy", "dummy", "dummy") + + def test_cohort_added(self, mock_tracker): + # Add cohort + cohort = CourseUserGroup.objects.create( + name="TestCohort", + course_id=self.course_key, + group_type=CourseUserGroup.COHORT + ) + mock_tracker.emit.assert_called_with( + "edx.cohort.created", + {"cohort_id": cohort.id, "cohort_name": cohort.name} + ) + mock_tracker.reset_mock() + + # Modify existing cohort + cohort.name = "NewName" + cohort.save() + self.assertFalse(mock_tracker.called) + + # Add non-cohort group + CourseUserGroup.objects.create( + name="TestOtherGroupType", + course_id=self.course_key, + group_type="dummy" + ) + self.assertFalse(mock_tracker.called) + + def test_cohort_membership_changed(self, mock_tracker): + cohort_list = [CohortFactory() for _ in range(2)] + non_cohort = CourseUserGroup.objects.create( + name="dummy", + course_id=self.course_key, + group_type="dummy" + ) + user_list = [UserFactory() for _ in range(2)] + mock_tracker.reset_mock() + + def assert_events(event_name_suffix, user_list, cohort_list): + mock_tracker.emit.assert_has_calls([ + call( + "edx.cohort.user_" + event_name_suffix, + { + "user_id": user.id, + "cohort_id": cohort.id, + "cohort_name": cohort.name, + } + ) + for user in user_list for cohort in cohort_list + ]) + + # Add users to cohort + cohort_list[0].users.add(*user_list) + assert_events("added", user_list, cohort_list[:1]) + mock_tracker.reset_mock() + + # Remove users from cohort + cohort_list[0].users.remove(*user_list) + assert_events("removed", user_list, cohort_list[:1]) + mock_tracker.reset_mock() + + # Clear users from cohort + cohort_list[0].users.add(*user_list) + cohort_list[0].users.clear() + assert_events("removed", user_list, cohort_list[:1]) + mock_tracker.reset_mock() + + # Clear users from non-cohort group + non_cohort.users.add(*user_list) + non_cohort.users.clear() + self.assertFalse(mock_tracker.emit.called) + + # Add cohorts to user + user_list[0].course_groups.add(*cohort_list) + assert_events("added", user_list[:1], cohort_list) + mock_tracker.reset_mock() + + # Remove cohorts from user + user_list[0].course_groups.remove(*cohort_list) + assert_events("removed", user_list[:1], cohort_list) + mock_tracker.reset_mock() + + # Clear cohorts from user + user_list[0].course_groups.add(*cohort_list) + user_list[0].course_groups.clear() + assert_events("removed", user_list[:1], cohort_list) + mock_tracker.reset_mock() + + # Clear non-cohort groups from user + user_list[0].course_groups.add(non_cohort) + user_list[0].course_groups.clear() + self.assertFalse(mock_tracker.emit.called) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestCohorts(django.test.TestCase): @@ -354,13 +452,18 @@ class TestCohorts(django.test.TestCase): lambda: cohorts.get_cohort_by_id(course.id, cohort.id) ) - def test_add_cohort(self): + @patch("course_groups.cohorts.tracker") + def test_add_cohort(self, mock_tracker): """ Make sure cohorts.add_cohort() properly adds a cohort to a course and handles errors. """ course = modulestore().get_course(self.toy_course_key) added_cohort = cohorts.add_cohort(course.id, "My Cohort") + mock_tracker.emit.assert_any_call( + "edx.cohort.creation_requested", + {"cohort_name": added_cohort.name, "cohort_id": added_cohort.id} + ) self.assertEqual(added_cohort.name, "My Cohort") self.assertRaises( @@ -372,7 +475,8 @@ class TestCohorts(django.test.TestCase): lambda: cohorts.add_cohort(SlashSeparatedCourseKey("course", "does_not", "exist"), "My Cohort") ) - def test_add_user_to_cohort(self): + @patch("course_groups.cohorts.tracker") + def test_add_user_to_cohort(self, mock_tracker): """ Make sure cohorts.add_user_to_cohort() properly adds a user to a cohort and handles errors. @@ -390,13 +494,32 @@ class TestCohorts(django.test.TestCase): cohorts.add_user_to_cohort(first_cohort, "Username"), (course_user, None) ) + mock_tracker.emit.assert_any_call( + "edx.cohort.user_add_requested", + { + "user_id": course_user.id, + "cohort_id": first_cohort.id, + "cohort_name": first_cohort.name, + "previous_cohort_id": None, + "previous_cohort_name": None, + } + ) # Should get (user, previous_cohort_name) when moved from one cohort to # another self.assertEqual( cohorts.add_user_to_cohort(second_cohort, "Username"), (course_user, "FirstCohort") ) - + mock_tracker.emit.assert_any_call( + "edx.cohort.user_add_requested", + { + "user_id": course_user.id, + "cohort_id": second_cohort.id, + "cohort_name": second_cohort.name, + "previous_cohort_id": first_cohort.id, + "previous_cohort_name": first_cohort.name, + } + ) # Error cases # Should get ValueError if user already in cohort self.assertRaises( diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index f67af694dd..882fadd7f8 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -3,6 +3,10 @@ End-to-end tests related to the cohort management on the LMS Instructor Dashboard """ +from datetime import datetime + +from pymongo import MongoClient + from bok_choy.promise import EmptyPromise from .helpers import CohortTestMixin from ..helpers import UniqueCourseTest @@ -25,6 +29,8 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): """ super(CohortConfigurationTest, self).setUp() + self.event_collection = MongoClient()["test"]["events"] + # create course with cohorts self.manual_cohort_name = "ManualCohort1" self.auto_cohort_name = "AutoCohort1" @@ -106,7 +112,9 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): And I get a notification that 2 users have been added to the cohort And I get a notification that 1 user was moved from the other cohort And the user input field is empty + And appropriate events have been emitted """ + start_time = datetime.now() self.membership_page.select_cohort(self.auto_cohort_name) self.assertEqual(0, self.membership_page.get_selected_cohort_count()) self.membership_page.add_students_to_selected_cohort([self.student_name, self.instructor_name]) @@ -119,6 +127,44 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.assertEqual("2 students have been added to this cohort group", confirmation_messages[0]) self.assertEqual("1 student was removed from " + self.manual_cohort_name, confirmation_messages[1]) self.assertEqual("", self.membership_page.get_cohort_student_input_field_value()) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.instructor_id), int(self.student_id)]}, + "event.cohort_name": self.auto_cohort_name, + }).count(), + 2 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_removed", + "time": {"$gt": start_time}, + "event.user_id": int(self.student_id), + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_add_requested", + "time": {"$gt": start_time}, + "event.user_id": int(self.instructor_id), + "event.cohort_name": self.auto_cohort_name, + "event.previous_cohort_name": None, + }).count(), + 1 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_add_requested", + "time": {"$gt": start_time}, + "event.user_id": int(self.student_id), + "event.cohort_name": self.auto_cohort_name, + "event.previous_cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) def test_add_students_to_cohort_failure(self): """ @@ -164,7 +210,9 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): Then the new cohort is displayed and has no users in it And when I add the user to the new cohort Then the cohort has 1 user + And appropriate events have been emitted """ + start_time = datetime.now() new_cohort = str(uuid.uuid4().get_hex()[0:20]) self.assertFalse(new_cohort in self.membership_page.get_cohorts()) self.membership_page.add_cohort(new_cohort) @@ -178,3 +226,19 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): EmptyPromise( lambda: 1 == self.membership_page.get_selected_cohort_count(), 'Waiting for student to be added' ).fulfill() + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.created", + "time": {"$gt": start_time}, + "event.cohort_name": new_cohort, + }).count(), + 1 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.creation_requested", + "time": {"$gt": start_time}, + "event.cohort_name": new_cohort, + }).count(), + 1 + ) diff --git a/docs/en_us/data/source/internal_data_formats/.#tracking_logs.rst b/docs/en_us/data/source/internal_data_formats/.#tracking_logs.rst new file mode 120000 index 0000000000..b017637239 --- /dev/null +++ b/docs/en_us/data/source/internal_data_formats/.#tracking_logs.rst @@ -0,0 +1 @@ +gprice@gprice-mbp.local.3450 \ No newline at end of file diff --git a/lms/envs/bok_choy.auth.json b/lms/envs/bok_choy.auth.json index 66ca730f5e..86d08d6029 100644 --- a/lms/envs/bok_choy.auth.json +++ b/lms/envs/bok_choy.auth.json @@ -46,6 +46,15 @@ "port": 27017, "user": "edxapp" }, + "EVENT_TRACKING_BACKENDS": { + "mongo": { + "ENGINE": "eventtracking.backends.mongodb.MongoBackend", + "OPTIONS": { + "database": "test", + "collection": "events" + } + } + }, "MODULESTORE": { "default": { "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",