Merge pull request #5463 from edx/gprice/cohort-events-take2

Cohort events

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,19 +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
     )
-
-
-class CohortConflict(Exception):
-    """
-    Raised when user to be added is already in another cohort in same course.
-    """
-    pass
-
+    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):
     """
@@ -288,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,
@@ -302,22 +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)
-
-
-def delete_empty_cohort(course_key, name):
-    """
-    Remove an empty cohort.  Raise ValueError if cohort is not empty.
-    """
-    cohort = get_cohort_by_name(course_key, name)
-    if cohort.users.exists():
-        raise ValueError(_("You cannot delete non-empty cohort {cohort_name} in course {course_key}").format(
-            cohort_name=name,
-            course_key=course_key
-        ))
-
-    cohort.delete()
+    return (user, previous_cohort_name)

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

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(
@@ -408,34 +531,3 @@ class TestCohorts(django.test.TestCase):
             User.DoesNotExist,
             lambda: cohorts.add_user_to_cohort(first_cohort, "non_existent_username")
         )
-
-    def test_delete_empty_cohort(self):
-        """
-        Make sure that cohorts.delete_empty_cohort() properly removes an empty cohort
-        for a given course.
-        """
-        course = modulestore().get_course(self.toy_course_key)
-        user = UserFactory(username="Username", email="a@b.com")
-        empty_cohort = CohortFactory(course_id=course.id, name="EmptyCohort")
-        nonempty_cohort = CohortFactory(course_id=course.id, name="NonemptyCohort")
-        nonempty_cohort.users.add(user)
-
-        cohorts.delete_empty_cohort(course.id, "EmptyCohort")
-
-        # Make sure we cannot access the deleted cohort
-        self.assertRaises(
-            CourseUserGroup.DoesNotExist,
-            lambda: cohorts.get_cohort_by_id(course.id, empty_cohort.id)
-        )
-        self.assertRaises(
-            ValueError,
-            lambda: cohorts.delete_empty_cohort(course.id, "NonemptyCohort")
-        )
-        self.assertRaises(
-            CourseUserGroup.DoesNotExist,
-            lambda: cohorts.delete_empty_cohort(SlashSeparatedCourseKey('course', 'does_not', 'exist'), "EmptyCohort")
-        )
-        self.assertRaises(
-            CourseUserGroup.DoesNotExist,
-            lambda: cohorts.delete_empty_cohort(course.id, "NonExistentCohort")
-        )

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
+        )

docs/en_us/data/source/internal_data_formats/.#tracking_logs.rst

+gprice@gprice-mbp.local.3450
\ No newline at end of file

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",

lms/lib/comment_client/thread.py

@@ -62,6 +62,7 @@ class Thread(models.Model):
         if query_params.get('text'):
             search_query = query_params['text']
             course_id = query_params['course_id']
+            group_id = query_params['group_id'] if 'group_id' in query_params else None
             requested_page = params['page']
             total_results = response.get('total_results')
             corrected_text = response.get('corrected_text')
@@ -72,15 +73,17 @@ class Thread(models.Model):
                 {
                     'query': search_query,
                     'corrected_text': corrected_text,
+                    'group_id': group_id,
                     'page': requested_page,
                     'total_results': total_results,
                 }
             )
             log.info(
-                'forum_text_search query="{search_query}" corrected_text="{corrected_text}" course_id={course_id} page={requested_page} total_results={total_results}'.format(
+                u'forum_text_search query="{search_query}" corrected_text="{corrected_text}" course_id={course_id} group_id={group_id} page={requested_page} total_results={total_results}'.format(
                     search_query=search_query,
                     corrected_text=corrected_text,
                     course_id=course_id,
+                    group_id=group_id,
                     requested_page=requested_page,
                     total_results=total_results
                 )