Merge pull request #3228 from edx/gprice/FOR-542

Change behavior of cohort addition interface

common/djangoapps/course_groups/cohorts.py

@@ -226,18 +226,16 @@ def add_user_to_cohort(cohort, username_or_email):
         username_or_email: string.  Treated as email if has '@'
 
     Returns:
-        User object.
+        Tuple of User object and string (or None) indicating previous cohort
 
     Raises:
         User.DoesNotExist if can't find user.
 
         ValueError if user already present in this cohort.
-
-        CohortConflict if user already in another cohort.
     """
     user = get_user_by_username_or_email(username_or_email)
+    previous_cohort = None
 
-    # If user in any cohorts in this course already, complain
     course_cohorts = CourseUserGroup.objects.filter(
         course_id=cohort.course_id,
         users__id=user.id,
@@ -248,12 +246,11 @@ def add_user_to_cohort(cohort, username_or_email):
                 user.username,
                 cohort.name))
         else:
-            raise CohortConflict("User {0} is in another cohort {1} in course"
-                                 .format(user.username,
-                                         course_cohorts[0].name))
+            previous_cohort = course_cohorts[0].name
+            course_cohorts[0].users.remove(user)
 
     cohort.users.add(user)
-    return user
+    return (user, previous_cohort)
 
 
 def get_course_cohort_names(course_id):

common/djangoapps/course_groups/tests/test_views.py

@@ -0,0 +1,187 @@
+import json
+
+from django.test.client import RequestFactory
+from django.test.utils import override_settings
+from factory import post_generation, Sequence
+from factory.django import DjangoModelFactory
+
+from course_groups.models import CourseUserGroup
+from course_groups.views import add_users_to_cohort
+from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE
+from student.tests.factories import UserFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+
+class CohortFactory(DjangoModelFactory):
+    FACTORY_FOR = CourseUserGroup
+
+    name = Sequence("cohort{}".format)
+    course_id = "dummy_id"
+    group_type = CourseUserGroup.COHORT
+
+    @post_generation
+    def users(self, create, extracted, **kwargs):  # pylint: disable=W0613
+        self.users.add(*extracted)
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class AddUsersToCohortTestCase(ModuleStoreTestCase):
+    def setUp(self):
+        self.course = CourseFactory.create()
+        self.staff_user = UserFactory.create(is_staff=True)
+        self.cohort1_users = [UserFactory.create() for _ in range(3)]
+        self.cohort2_users = [UserFactory.create() for _ in range(2)]
+        self.cohort3_users = [UserFactory.create() for _ in range(2)]
+        self.cohortless_users = [UserFactory.create() for _ in range(3)]
+        self.cohort1 = CohortFactory.create(course_id=self.course.id, users=self.cohort1_users)
+        self.cohort2 = CohortFactory.create(course_id=self.course.id, users=self.cohort2_users)
+        self.cohort3 = CohortFactory.create(course_id=self.course.id, users=self.cohort3_users)
+
+    def check_request(
+            self,
+            users_string,
+            expected_added=None,
+            expected_changed=None,
+            expected_present=None,
+            expected_unknown=None
+    ):
+        """
+        Check that add_users_to_cohort returns the expected result and has the
+        expected side effects. The given users will be added to cohort1.
+
+        users_string is the string input entered by the client
+
+        expected_added is a list of users
+
+        expected_changed is a list of (user, previous_cohort) tuples
+
+        expected_present is a list of (user, email/username) tuples where
+        email/username corresponds to the input
+
+        expected_unknown is a list of strings corresponding to the input
+        """
+        expected_added = expected_added or []
+        expected_changed = expected_changed or []
+        expected_present = expected_present or []
+        expected_unknown = expected_unknown or []
+        request = RequestFactory().post("dummy_url", {"users": users_string})
+        request.user = self.staff_user
+        response = add_users_to_cohort(request, self.course.id, self.cohort1.id)
+        self.assertEqual(response.status_code, 200)
+        result = json.loads(response.content)
+        self.assertEqual(result.get("success"), True)
+        self.assertItemsEqual(
+            result.get("added"),
+            [
+                {"username": user.username, "name": user.profile.name, "email": user.email}
+                for user in expected_added
+            ]
+        )
+        self.assertItemsEqual(
+            result.get("changed"),
+            [
+                {
+                    "username": user.username,
+                    "name": user.profile.name,
+                    "email": user.email,
+                    "previous_cohort": previous_cohort
+                }
+                for (user, previous_cohort) in expected_changed
+            ]
+        )
+        self.assertItemsEqual(
+            result.get("present"),
+            [username_or_email for (_, username_or_email) in expected_present]
+        )
+        self.assertItemsEqual(result.get("unknown"), expected_unknown)
+        for user in expected_added + [user for (user, _) in expected_changed + expected_present]:
+            self.assertEqual(
+                CourseUserGroup.objects.get(
+                    course_id=self.course.id,
+                    group_type=CourseUserGroup.COHORT,
+                    users__id=user.id
+                ),
+                self.cohort1
+            )
+
+    def test_empty(self):
+        self.check_request("")
+
+    def test_only_added(self):
+        self.check_request(
+            ",".join([user.username for user in self.cohortless_users]),
+            expected_added=self.cohortless_users
+        )
+
+    def test_only_changed(self):
+        self.check_request(
+            ",".join([user.username for user in self.cohort2_users + self.cohort3_users]),
+            expected_changed=(
+                [(user, self.cohort2.name) for user in self.cohort2_users] +
+                [(user, self.cohort3.name) for user in self.cohort3_users]
+            )
+        )
+
+    def test_only_present(self):
+        usernames = [user.username for user in self.cohort1_users]
+        self.check_request(
+            ",".join(usernames),
+            expected_present=[(user, user.username) for user in self.cohort1_users]
+        )
+
+    def test_only_unknown(self):
+        usernames = ["unknown_user{}".format(i) for i in range(3)]
+        self.check_request(
+            ",".join(usernames),
+            expected_unknown=usernames
+        )
+
+    def test_all(self):
+        unknowns = ["unknown_user{}".format(i) for i in range(3)]
+        self.check_request(
+            ",".join(
+                unknowns +
+                [
+                    user.username
+                    for user in self.cohortless_users + self.cohort1_users + self.cohort2_users + self.cohort3_users
+                ]
+            ),
+            self.cohortless_users,
+            (
+                [(user, self.cohort2.name) for user in self.cohort2_users] +
+                [(user, self.cohort3.name) for user in self.cohort3_users]
+            ),
+            [(user, user.username) for user in self.cohort1_users],
+            unknowns
+        )
+
+    def test_emails(self):
+        unknown = "unknown_user@example.com"
+        self.check_request(
+            ",".join([
+                self.cohort1_users[0].email,
+                self.cohort2_users[0].email,
+                self.cohortless_users[0].email,
+                unknown
+            ]),
+            [self.cohortless_users[0]],
+            [(self.cohort2_users[0], self.cohort2.name)],
+            [(self.cohort1_users[0], self.cohort1_users[0].email)],
+            [unknown]
+        )
+
+    def test_delimiters(self):
+        unknown = "unknown_user"
+        self.check_request(
+            " {} {}\t{}, \r\n{}".format(
+                unknown,
+                self.cohort1_users[0].username,
+                self.cohort2_users[0].username,
+                self.cohortless_users[0].username
+            ),
+            [self.cohortless_users[0]],
+            [(self.cohort2_users[0], self.cohort2.name)],
+            [(self.cohort1_users[0], self.cohort1_users[0].username)],
+            [unknown]
+        )

common/djangoapps/course_groups/views.py

@@ -134,11 +134,13 @@ def add_users_to_cohort(request, course_id, cohort_id):
     Return json dict of:
 
     {'success': True,
-     'added': [{'username': username,
-                'name': name,
-                'email': email}, ...],
-     'conflict': [{'username_or_email': ...,
-                    'msg': ...}],  # in another cohort
+     'added': [{'username': ...,
+                'name': ...,
+                'email': ...}, ...],
+     'changed': [{'username': ...,
+                  'name': ...,
+                  'email': ...,
+                  'previous_cohort': ...}, ...],
      'present': [str1, str2, ...],    # already there
      'unknown': [str1, str2, ...]}
     """
@@ -148,29 +150,34 @@ def add_users_to_cohort(request, course_id, cohort_id):
 
     users = request.POST.get('users', '')
     added = []
+    changed = []
     present = []
-    conflict = []
     unknown = []
     for username_or_email in split_by_comma_and_whitespace(users):
+        if not username_or_email:
+            continue
+
         try:
-            user = cohorts.add_user_to_cohort(cohort, username_or_email)
-            added.append({'username': user.username,
-                          'name': "{0} {1}".format(user.first_name, user.last_name),
-                          'email': user.email,
-                          })
+            (user, previous_cohort) = cohorts.add_user_to_cohort(cohort, username_or_email)
+            info = {
+                'username': user.username,
+                'name': user.profile.name,
+                'email': user.email,
+            }
+            if previous_cohort:
+                info['previous_cohort'] = previous_cohort
+                changed.append(info)
+            else:
+                added.append(info)
         except ValueError:
             present.append(username_or_email)
         except User.DoesNotExist:
             unknown.append(username_or_email)
-        except cohorts.CohortConflict as err:
-            conflict.append({'username_or_email': username_or_email,
-                              'msg': str(err)})
-
 
     return json_http_response({'success': True,
                             'added': added,
+                            'changed': changed,
                             'present': present,
-                            'conflict': conflict,
                             'unknown': unknown})
 
 

common/static/js/course_groups/cohorts.js

@@ -160,32 +160,43 @@ var CohortManager = (function ($) {
                 log_error(response.msg || 
                           "There was an error loading users for " + cohort.title);
             }
+            op_results.empty();
             detail.show();
         }
 
 
         function added_users(response) {
-            function adder(note, color) {
-                return function(item) {
-                    var li = $('<li></li>')
-                    if (typeof item === "object" && item.username) {
-                        li.text(note + ' ' + item.name + ', ' + item.username + ', ' + item.email);
-                    } else if (typeof item === "object" && item.msg) {
-                        li.text(note + ' ' + item.username_or_email + ', ' + item.msg);
-                    } else {
-                        // string
-                        li.text(note + ' ' + item);
-                    }
-                    li.css('color', color);
-                    op_results.append(li);
-                }
-            }
             op_results.empty();
             if (response && response.success) {
-                response.added.forEach(adder("Added", "green"));
-                response.present.forEach(adder("Already present:", "black"));
-                response.conflict.forEach(adder("In another cohort:", "purple"));
-                response.unknown.forEach(adder("Unknown user:", "red"));
+                function add_to_list(text, color) {
+                    op_results.append($("<li/>").text(text).css("color", color));
+                }
+                response.added.forEach(
+                    function(item) {
+                        add_to_list(
+                            "Added: " + item.name + ", " + item.username + ", " + item.email,
+                            "green"
+                        );
+                    }
+                );
+                response.changed.forEach(
+                    function(item) {
+                        add_to_list(
+                            "Moved from cohort " + item.previous_cohort + ": " + item.name + ", " + item.username + ", " + item.email,
+                            "purple"
+                        )
+                    }
+                );
+                response.present.forEach(
+                    function(item) {
+                        add_to_list("Already present: " + item, "black");
+                    }
+                );
+                response.unknown.forEach(
+                    function(item) {
+                        add_to_list("Unknown user: " + item, "red")
+                    }
+                );
                 users_area.val('')
             } else {
                 log_error(response.msg || "There was an error adding users");