Create a RESTful handle_cohort view method for GET and PUT/POST.

lms/static/js/models/cohort.js

@@ -3,7 +3,21 @@
         idAttribute: 'id',
         defaults: {
             name: '',
-            user_count: 0
+            user_count: 0,
+            /**
+             * Indicates how students are added to the cohort. Will be "none" (signifying manual assignment) or
+             * "random" (indicating students are randomly assigned).
+             */
+            assignment_type: '',
+            /**
+             * If this cohort is associated with a user partition group, the ID of the user partition.
+             */
+            user_partition_id: null,
+            /**
+             * If this cohort is associated with a user partition group, the ID of the group within the
+             * partition associated with user_partition_id.
+             */
+            group_id: null
         }
     });
 

lms/urls.py

@@ -345,11 +345,8 @@ if settings.COURSEWARE_ENABLED:
             'open_ended_grading.views.take_action_on_flags', name='open_ended_flagged_problems_take_action'),
 
         # Cohorts management
-        url(r'^courses/{}/cohorts$'.format(settings.COURSE_KEY_PATTERN),
-            'openedx.core.djangoapps.course_groups.views.list_cohorts', name="cohorts"),
-        url(r'^courses/{}/cohorts/add$'.format(settings.COURSE_KEY_PATTERN),
-            'openedx.core.djangoapps.course_groups.views.add_cohort',
-            name="add_cohort"),
+        url(r'^courses/{}/cohorts/(?P<cohort_id>[0-9]+)?$'.format(settings.COURSE_KEY_PATTERN),
+            'openedx.core.djangoapps.course_groups.views.cohort_handler', name="cohorts"),
         url(r'^courses/{}/cohorts/(?P<cohort_id>[0-9]+)$'.format(settings.COURSE_KEY_PATTERN),
             'openedx.core.djangoapps.course_groups.views.users_in_cohort',
             name="list_cohort"),

openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py

@@ -21,6 +21,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme
 from ..partition_scheme import CohortPartitionScheme, get_cohorted_user_partition
 from ..models import CourseUserGroupPartitionGroup
+from ..views import link_cohort_to_partition_group, unlink_cohort_partition_group
 from ..cohorts import add_user_to_cohort, get_course_cohorts
 from .helpers import CohortFactory, config_course_cohorts
 
@@ -55,22 +56,6 @@ class TestCohortPartitionScheme(django.test.TestCase):
         )
         self.student = UserFactory.create()
 
-    def link_cohort_partition_group(self, cohort, partition, group):
-        """
-        Utility for creating cohort -> partition group links
-        """
-        CourseUserGroupPartitionGroup(
-            course_user_group=cohort,
-            partition_id=partition.id,
-            group_id=group.id,
-        ).save()
-
-    def unlink_cohort_partition_group(self, cohort):
-        """
-        Utility for removing cohort -> partition group links
-        """
-        CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete()
-
     def assert_student_in_group(self, group, partition=None):
         """
         Utility for checking that our test student comes up as assigned to the
@@ -99,16 +84,16 @@ class TestCohortPartitionScheme(django.test.TestCase):
         self.assert_student_in_group(None)
 
         # link first cohort to group 0 in the partition
-        self.link_cohort_partition_group(
+        link_cohort_to_partition_group(
             first_cohort,
-            self.user_partition,
-            self.groups[0],
+            self.user_partition.id,
+            self.groups[0].id,
         )
         # link second cohort to to group 1 in the partition
-        self.link_cohort_partition_group(
+        link_cohort_to_partition_group(
             second_cohort,
-            self.user_partition,
-            self.groups[1],
+            self.user_partition.id,
+            self.groups[1].id,
         )
         self.assert_student_in_group(self.groups[0])
 
@@ -133,26 +118,26 @@ class TestCohortPartitionScheme(django.test.TestCase):
         self.assert_student_in_group(None)
 
         # link cohort to group 0
-        self.link_cohort_partition_group(
+        link_cohort_to_partition_group(
             test_cohort,
-            self.user_partition,
-            self.groups[0],
+            self.user_partition.id,
+            self.groups[0].id,
         )
         # now the scheme should find a link
         self.assert_student_in_group(self.groups[0])
 
         # link cohort to group 1 (first unlink it from group 0)
-        self.unlink_cohort_partition_group(test_cohort)
-        self.link_cohort_partition_group(
+        unlink_cohort_partition_group(test_cohort)
+        link_cohort_to_partition_group(
             test_cohort,
-            self.user_partition,
-            self.groups[1],
+            self.user_partition.id,
+            self.groups[1].id,
         )
         # scheme should pick up the link
         self.assert_student_in_group(self.groups[1])
 
         # unlink cohort from anywhere
-        self.unlink_cohort_partition_group(
+        unlink_cohort_partition_group(
             test_cohort,
         )
         # scheme should now return nothing
@@ -171,10 +156,10 @@ class TestCohortPartitionScheme(django.test.TestCase):
         cohort = get_course_cohorts(self.course)[0]
 
         # map that cohort to a group in our partition
-        self.link_cohort_partition_group(
+        link_cohort_to_partition_group(
             cohort,
-            self.user_partition,
-            self.groups[0],
+            self.user_partition.id,
+            self.groups[0].id,
         )
 
         # The student will be lazily assigned to the default cohort
@@ -190,10 +175,10 @@ class TestCohortPartitionScheme(django.test.TestCase):
         test_cohort = CohortFactory(course_id=self.course_key)
 
         # link cohort to group 0
-        self.link_cohort_partition_group(
+        link_cohort_to_partition_group(
             test_cohort,
-            self.user_partition,
-            self.groups[0],
+            self.user_partition.id,
+            self.groups[0].id,
         )
         # place student into cohort
         add_user_to_cohort(test_cohort, self.student.username)

openedx/core/djangoapps/course_groups/tests/test_views.py

@@ -15,11 +15,17 @@ from student.models import CourseEnrollment
 from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.django import modulestore
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from ..models import CourseUserGroup
-from ..views import list_cohorts, add_cohort, users_in_cohort, add_users_to_cohort, remove_user_from_cohort
-from ..cohorts import get_cohort, CohortAssignmentType, get_cohort_by_name, DEFAULT_COHORT_NAME
+from ..views import (
+    cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort, link_cohort_to_partition_group
+)
+from ..cohorts import (
+    get_cohort, CohortAssignmentType, get_cohort_by_name, get_cohort_by_id,
+    DEFAULT_COHORT_NAME, get_group_info_for_cohort
+)
 from .helpers import config_course_cohorts, CohortFactory
 
 
@@ -78,58 +84,82 @@ class CohortViewsTestCase(ModuleStoreTestCase):
         self.assertRaises(Http404, view, *view_args)
 
 
-class ListCohortsTestCase(CohortViewsTestCase):
+class CohortHandlerTestCase(CohortViewsTestCase):
     """
-    Tests the `list_cohorts` view.
+    Tests the `cohort_handler` view.
     """
-    def request_list_cohorts(self, course):
+    def get_cohort_handler(self, course, cohort=None):
         """
-        Call `list_cohorts` for a given `course` and return its response as a
-        dict.
+        Call a GET on `cohort_handler` for a given `course` and return its response as a
+        dict. If `cohort` is specified, only information for that specific cohort is returned.
         """
         request = RequestFactory().get("dummy_url")
         request.user = self.staff_user
-        response = list_cohorts(request, course.id.to_deprecated_string())
+        if cohort:
+            response = cohort_handler(request, course.id.to_deprecated_string(), cohort.id)
+        else:
+            response = cohort_handler(request, course.id.to_deprecated_string())
         self.assertEqual(response.status_code, 200)
         return json.loads(response.content)
 
+    def put_cohort_handler(self, course, cohort=None, data=None, expected_response_code=200):
+        """
+        Call a PUT on `cohort_handler` for a given `course` and return its response as a
+        dict. If `cohort` is not specified, a new cohort is created. If `cohort` is specified,
+        the existing cohort is updated.
+        """
+        if not isinstance(data, basestring):
+            data = json.dumps(data or {})
+        request = RequestFactory().put(path="dummy path", data=data, content_type="application/json")
+        request.user = self.staff_user
+        if cohort:
+            response = cohort_handler(request, course.id.to_deprecated_string(), cohort.id)
+        else:
+            response = cohort_handler(request, course.id.to_deprecated_string())
+        self.assertEqual(response.status_code, expected_response_code)
+        return json.loads(response.content)
+
     def verify_lists_expected_cohorts(self, expected_cohorts, response_dict=None):
         """
         Verify that the server response contains the expected_cohorts.
         If response_dict is None, the list of cohorts is requested from the server.
         """
         if response_dict is None:
-            response_dict = self.request_list_cohorts(self.course)
+            response_dict = self.get_cohort_handler(self.course)
 
-        self.assertTrue(response_dict.get("success"))
-        self.assertItemsEqual(
+        self.assertEqual(
             response_dict.get("cohorts"),
             [
                 {
                     "name": cohort.name,
                     "id": cohort.id,
                     "user_count": cohort.user_count,
-                    "assignment_type": cohort.assignment_type
+                    "assignment_type": cohort.assignment_type,
+                    "user_partition_id": None,
+                    "group_id": None
                 }
                 for cohort in expected_cohorts
             ]
         )
 
     @staticmethod
-    def create_expected_cohort(cohort, user_count, assignment_type):
+    def create_expected_cohort(cohort, user_count, assignment_type, user_partition_id=None, group_id=None):
         """
         Create a tuple storing the expected cohort information.
         """
-        cohort_tuple = namedtuple("Cohort", "name id user_count assignment_type")
+        cohort_tuple = namedtuple("Cohort", "name id user_count assignment_type user_partition_id group_id")
         return cohort_tuple(
-            name=cohort.name, id=cohort.id, user_count=user_count, assignment_type=assignment_type
+            name=cohort.name, id=cohort.id, user_count=user_count, assignment_type=assignment_type,
+            user_partition_id=user_partition_id, group_id=group_id
         )
 
     def test_non_staff(self):
         """
-        Verify that we cannot access list_cohorts if we're a non-staff user.
+        Verify that we cannot access cohort_handler if we're a non-staff user.
         """
-        self._verify_non_staff_cannot_access(list_cohorts, "GET", [self.course.id.to_deprecated_string()])
+        self._verify_non_staff_cannot_access(cohort_handler, "GET", [self.course.id.to_deprecated_string()])
+        self._verify_non_staff_cannot_access(cohort_handler, "POST", [self.course.id.to_deprecated_string()])
+        self._verify_non_staff_cannot_access(cohort_handler, "PUT", [self.course.id.to_deprecated_string()])
 
     def test_no_cohorts(self):
         """
@@ -143,9 +173,9 @@ class ListCohortsTestCase(CohortViewsTestCase):
         """
         self._create_cohorts()
         expected_cohorts = [
-            ListCohortsTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE),
-            ListCohortsTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE),
-            ListCohortsTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE),
         ]
         self.verify_lists_expected_cohorts(expected_cohorts)
 
@@ -159,16 +189,16 @@ class ListCohortsTestCase(CohortViewsTestCase):
         # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated.
         self._create_cohorts()
         # Get the cohorts from the course, which will cause auto cohorts to be created.
-        actual_cohorts = self.request_list_cohorts(self.course)
+        actual_cohorts = self.get_cohort_handler(self.course)
         # Get references to the created auto cohorts.
         auto_cohort_1 = get_cohort_by_name(self.course.id, "AutoGroup1")
         auto_cohort_2 = get_cohort_by_name(self.course.id, "AutoGroup2")
         expected_cohorts = [
-            ListCohortsTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE),
-            ListCohortsTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE),
-            ListCohortsTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE),
-            ListCohortsTestCase.create_expected_cohort(auto_cohort_1, 0, CohortAssignmentType.RANDOM),
-            ListCohortsTestCase.create_expected_cohort(auto_cohort_2, 0, CohortAssignmentType.RANDOM),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE),
+            CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CohortAssignmentType.RANDOM),
+            CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CohortAssignmentType.RANDOM),
         ]
         self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts)
 
@@ -195,94 +225,197 @@ class ListCohortsTestCase(CohortViewsTestCase):
 
         # verify the default cohort is automatically created
         default_cohort = get_cohort_by_name(self.course.id, DEFAULT_COHORT_NAME)
-        actual_cohorts = self.request_list_cohorts(self.course)
+        actual_cohorts = self.get_cohort_handler(self.course)
         self.verify_lists_expected_cohorts(
-            [ListCohortsTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.RANDOM)],
+            [CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.RANDOM)],
             actual_cohorts,
         )
 
         # set auto_cohort_groups and verify the default cohort is no longer listed as RANDOM
         config_course_cohorts(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"])
-        actual_cohorts = self.request_list_cohorts(self.course)
+        actual_cohorts = self.get_cohort_handler(self.course)
         auto_cohort = get_cohort_by_name(self.course.id, "AutoGroup")
         self.verify_lists_expected_cohorts(
             [
-                ListCohortsTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.NONE),
-                ListCohortsTestCase.create_expected_cohort(auto_cohort, 0, CohortAssignmentType.RANDOM),
+                CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.NONE),
+                CohortHandlerTestCase.create_expected_cohort(auto_cohort, 0, CohortAssignmentType.RANDOM),
             ],
             actual_cohorts,
         )
 
+    def test_get_single_cohort(self):
+        """
+        Tests that information for just a single cohort can be requested.
+        """
+        self._create_cohorts()
+        response_dict = self.get_cohort_handler(self.course, self.cohort2)
+        self.assertEqual(
+            response_dict,
+            {
+                "name": self.cohort2.name,
+                "id": self.cohort2.id,
+                "user_count": 2,
+                "assignment_type": "none",
+                "user_partition_id": None,
+                "group_id": None
+            }
+        )
 
-class AddCohortTestCase(CohortViewsTestCase):
+    ############### Tests of adding a new cohort ###############
+
+    def verify_contains_added_cohort(
+            self, response_dict, cohort_name, expected_user_partition_id=None, expected_group_id=None
+    ):
         """
-    Tests the `add_cohort` view.
+        Verifies that the cohort was created properly and the correct response was returned.
         """
-    def request_add_cohort(self, cohort_name, course):
+        created_cohort = get_cohort_by_name(self.course.id, cohort_name)
+        self.assertIsNotNone(created_cohort)
+        self.assertEqual(
+            response_dict,
+            {
+                "name": cohort_name,
+                "id": created_cohort.id,
+                "user_count": 0,
+                "assignment_type": CohortAssignmentType.NONE,
+                "user_partition_id": expected_user_partition_id,
+                "group_id": expected_group_id
+            }
+        )
+        self.assertEqual((expected_group_id, expected_user_partition_id), get_group_info_for_cohort(created_cohort))
+
+    def test_create_new_cohort(self):
         """
-        Call `add_cohort` and return its response as a dict.
+        Verify that a new cohort can be created, with and without user_partition_id/group_id information.
         """
-        request = RequestFactory().post("dummy_url", {"name": cohort_name})
-        request.user = self.staff_user
-        response = add_cohort(request, course.id.to_deprecated_string())
-        self.assertEqual(response.status_code, 200)
-        return json.loads(response.content)
+        new_cohort_name = "New cohort unassociated to content groups"
+        response_dict = self.put_cohort_handler(self.course, data={'name': new_cohort_name})
+        self.verify_contains_added_cohort(response_dict, new_cohort_name)
+
+        new_cohort_name = "New cohort linked to group"
+        response_dict = self.put_cohort_handler(
+            self.course, data={'name': new_cohort_name, 'user_partition_id': 1, 'group_id': 2}
+        )
+        self.verify_contains_added_cohort(response_dict, new_cohort_name, 1, 2)
 
-    def verify_contains_added_cohort(self, response_dict, cohort_name, expected_error_msg=None):
+    def test_create_new_cohort_missing_name(self):
         """
-        Check that `add_cohort`'s response correctly returns the newly added
-        cohort (or error) in the response.  Also verify that the cohort was
-        actually created/exists.
+        Verify that we cannot create a cohort without specifying a name.
         """
-        if expected_error_msg is not None:
-            self.assertFalse(response_dict.get("success"))
+        response_dict = self.put_cohort_handler(self.course, expected_response_code=400)
+        self.assertEqual("In order to create a cohort, a name must be specified.", response_dict.get("error"))
+
+    def test_create_new_cohort_existing_name(self):
+        """
+        Verify that we cannot add a cohort with the same name as an existing cohort.
+        """
+        self._create_cohorts()
+        response_dict = self.put_cohort_handler(
+            self.course, data={'name': self.cohort1.name}, expected_response_code=400
+        )
+        self.assertEqual("You cannot create two cohorts with the same name", response_dict.get("error"))
+
+    def test_create_new_cohort_missing_user_partition_id(self):
+        """
+        Verify that we cannot create a cohort with a group_id if the user_partition_id is not also specified.
+        """
+        response_dict = self.put_cohort_handler(
+            self.course, data={'name': "Cohort missing user_partition_id", 'group_id': 2}, expected_response_code=400
+        )
         self.assertEqual(
-                response_dict.get("msg"),
-                expected_error_msg
+            "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error")
+        )
+
+    ############### Tests of updating an existing cohort ###############
+
+    def test_update_manual_cohort_name(self):
+        """
+        Test that it is possible to update the name of an existing manual cohort.
+        """
+        self._create_cohorts()
+        updated_name = self.cohort1.name + "_updated"
+        response_dict = self.put_cohort_handler(self.course, self.cohort1, {'name': updated_name})
+        self.assertEqual(updated_name, get_cohort_by_id(self.course.id, self.cohort1.id).name)
+        self.assertEqual(updated_name, response_dict.get("name"))
+        self.assertEqual(CohortAssignmentType.NONE, response_dict.get("assignment_type"))
+        self.assertEqual(CohortAssignmentType.NONE, CohortAssignmentType.get(self.cohort1, self.course))
+
+    def test_update_random_cohort_name_not_supported(self):
+        """
+        Test that it is not possible to update the name of an existing random cohort.
+        """
+        random_cohort = CohortFactory(course_id=self.course.id)
+        random_cohort_name = random_cohort.name
+
+        # Update course cohort_config so random_cohort is in the list of auto cohorts.
+        self.course.cohort_config["auto_cohort_groups"] = [random_cohort_name]
+        modulestore().update_item(self.course, self.staff_user.id)
+
+        updated_name = random_cohort.name + "_updated"
+        response_dict = self.put_cohort_handler(
+            self.course, random_cohort, {'name': updated_name}, expected_response_code=400
         )
-        else:
-            self.assertTrue(response_dict.get("success"))
         self.assertEqual(
-                response_dict.get("cohort").get("name"),
-                cohort_name
+            "Renaming of random cohorts is not supported at this time.", response_dict.get("error")
         )
-        self.assertIsNotNone(get_cohort_by_name(self.course.id, cohort_name))
+        self.assertEqual(random_cohort_name, get_cohort_by_id(self.course.id, random_cohort.id).name)
+        self.assertEqual(CohortAssignmentType.RANDOM, CohortAssignmentType.get(random_cohort, self.course))
 
-    def test_non_staff(self):
+    def test_update_cohort_group_id(self):
         """
-        Verify that non-staff users cannot access add_cohort.
+        Test that it is possible to update the user_partition_id/group_id of an existing cohort.
         """
-        self._verify_non_staff_cannot_access(add_cohort, "POST", [self.course.id.to_deprecated_string()])
+        self._create_cohorts()
+        self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1))
+        response_dict = self.put_cohort_handler(
+            self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3}
+        )
+        self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1))
+        self.assertEqual(2, response_dict.get("group_id"))
+        self.assertEqual(3, response_dict.get("user_partition_id"))
+        # Check that the name didn't change.
+        self.assertEqual(self.cohort1.name, response_dict.get("name"))
 
-    def test_new_cohort(self):
+    def test_update_cohort_remove_group_id(self):
         """
-        Verify that we can add a new cohort.
+        Test that it is possible to remove the user_partition_id/group_id linking of an existing cohort.
         """
-        cohort_name = "New Cohort"
-        self.verify_contains_added_cohort(
-            self.request_add_cohort(cohort_name, self.course),
-            cohort_name,
+        self._create_cohorts()
+        link_cohort_to_partition_group(self.cohort1, 5, 0)
+        self.assertEqual((0, 5), get_group_info_for_cohort(self.cohort1))
+        response_dict = self.put_cohort_handler(
+            self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': None}
         )
+        self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1))
+        self.assertEqual(None, response_dict.get("group_id"))
+        self.assertEqual(None, response_dict.get("user_partition_id"))
 
-    def test_no_cohort(self):
+    def test_change_cohort_group_id(self):
         """
-        Verify that we cannot explicitly add no cohort.
+        Test that it is possible to change the user_partition_id/group_id of an existing cohort to a
+        different group_id.
         """
-        response_dict = self.request_add_cohort("", self.course)
-        self.assertFalse(response_dict.get("success"))
-        self.assertEqual(response_dict.get("msg"), "No name specified")
+        self._create_cohorts()
+        self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1))
+        self.put_cohort_handler(
+            self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3}
+        )
+        self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1))
+        self.put_cohort_handler(
+            self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 1, 'user_partition_id': 3}
+        )
+        self.assertEqual((1, 3), get_group_info_for_cohort(self.cohort1))
 
-    def test_existing_cohort(self):
+    def test_update_cohort_missing_user_partition_id(self):
         """
-        Verify that we cannot add a cohort with the same name as an existing
-        cohort.
+        Verify that we cannot update a cohort with a group_id if the user_partition_id is not also specified.
         """
         self._create_cohorts()
-        cohort_name = self.cohort1.name
-        self.verify_contains_added_cohort(
-            self.request_add_cohort(cohort_name, self.course),
-            cohort_name,
-            expected_error_msg="You cannot create two cohorts with the same name"
+        response_dict = self.put_cohort_handler(
+            self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2}, expected_response_code=400
+        )
+        self.assertEqual(
+            "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error")
         )
 
 
@@ -457,14 +590,14 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
         `expected_unknown` is a list of strings corresponding to the input
         """
         self.assertTrue(response_dict.get("success"))
-        self.assertItemsEqual(
+        self.assertEqual(
             response_dict.get("added"),
             [
                 {"username": user.username, "name": user.profile.name, "email": user.email}
                 for user in expected_added
             ]
         )
-        self.assertItemsEqual(
+        self.assertEqual(
             response_dict.get("changed"),
             [
                 {
@@ -476,11 +609,11 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
                 for (user, previous_cohort) in expected_changed
             ]
         )
-        self.assertItemsEqual(
+        self.assertEqual(
             response_dict.get("present"),
             [username_or_email for (_, username_or_email) in expected_present]
         )
-        self.assertItemsEqual(response_dict.get("unknown"), expected_unknown)
+        self.assertEqual(response_dict.get("unknown"), expected_unknown)
         for user in expected_added + [user for (user, _) in expected_changed + expected_present]:
             self.assertEqual(
                 CourseUserGroup.objects.get(

openedx/core/djangoapps/course_groups/views.py

@@ -4,6 +4,9 @@ from django.contrib.auth.models import User
 from django.core.paginator import Paginator, EmptyPage
 from django.core.urlresolvers import reverse
 from django.http import Http404, HttpResponse, HttpResponseBadRequest
+from django.views.decorators.http import require_http_methods
+from util.json_request import expect_json, JsonResponse
+from django.contrib.auth.decorators import login_required
 import json
 import logging
 import re
@@ -12,9 +15,8 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from courseware.courses import get_course_with_access
 from edxmako.shortcuts import render_to_response
 
-from util.json_request import JsonResponse
 from . import cohorts
-from .models import CourseUserGroup
+from .models import CourseUserGroup, CourseUserGroupPartitionGroup
 
 log = logging.getLogger(__name__)
 
@@ -29,76 +31,119 @@ def json_http_response(data):
 
 def split_by_comma_and_whitespace(cstr):
     """
-    Split a string both by commas and whitespice.  Returns a list.
+    Split a string both by commas and whitespace.  Returns a list.
     """
     return re.split(r'[\s,]+', cstr)
 
 
-@ensure_csrf_cookie
-def list_cohorts(request, course_key_string):
+def link_cohort_to_partition_group(cohort, partition_id, group_id):
     """
-    Return json dump of dict:
-
-    {'success': True,
-     'cohorts': [{'name': name, 'id': id}, ...]}
+    Create cohort to partition_id/group_id link.
     """
+    CourseUserGroupPartitionGroup(
+        course_user_group=cohort,
+        partition_id=partition_id,
+        group_id=group_id,
+    ).save()
 
-    # this is a string when we get it here
-    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string)
 
-    course = get_course_with_access(request.user, 'staff', course_key)
+def unlink_cohort_partition_group(cohort):
+    """
+    Remove any existing cohort to partition_id/group_id link.
+    """
+    CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete()
 
-    all_cohorts = [
-        {
-            'name': c.name,
-            'id': c.id,
-            'user_count': c.users.count(),
-            'assignment_type': cohorts.CohortAssignmentType.get(c, course)
-        }
-        for c in cohorts.get_course_cohorts(course)
-    ]
 
-    return json_http_response({'success': True,
-                               'cohorts': all_cohorts})
+def _get_cohort_representation(cohort, course):
+    """
+    Returns a JSON representation of a cohort.
+    """
+    group_id, partition_id = cohorts.get_group_info_for_cohort(cohort)
+    return {
+        'name': cohort.name,
+        'id': cohort.id,
+        'user_count': cohort.users.count(),
+        'assignment_type': cohorts.CohortAssignmentType.get(cohort, course),
+        'user_partition_id': partition_id,
+        'group_id': group_id
+    }
 
 
+@require_http_methods(("GET", "PUT", "POST", "PATCH"))
 @ensure_csrf_cookie
-@require_POST
-def add_cohort(request, course_key_string):
+@expect_json
+@login_required
+def cohort_handler(request, course_key_string, cohort_id=None):
     """
-    Return json of dict:
-    {'success': True,
-     'cohort': {'id': id,
-                'name': name}}
-
-                or
-
-    {'success': False,
-     'msg': error_msg} if there's an error
+    The restful handler for cohort requests. Requires JSON.
+    GET
+        If a cohort ID is specified, returns a JSON representation of the cohort
+            (name, id, user_count, assignment_type, user_partition_id, group_id).
+        If no cohort ID is specified, returns the JSON representation of all cohorts.
+           This is returned as a dict with the list of cohort information stored under the
+           key `cohorts`.
+    PUT or POST or PATCH
+        If a cohort ID is specified, updates the cohort with the specified ID. Currently the only
+        properties that can be updated are `name`, `user_partition_id` and `group_id`.
+        Returns the JSON representation of the updated cohort.
+        If no cohort ID is specified, creates a new cohort and returns the JSON representation of the updated
+        cohort.
     """
-    # this is a string when we get it here
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string)
-
-    get_course_with_access(request.user, 'staff', course_key)
-
-    name = request.POST.get("name")
+    course = get_course_with_access(request.user, 'staff', course_key)
+    if request.method == 'GET':
+        if not cohort_id:
+            all_cohorts = [
+                _get_cohort_representation(c, course)
+                for c in cohorts.get_course_cohorts(course)
+            ]
+            # TODO: change to just directly returning the lists.
+            return JsonResponse({'cohorts': all_cohorts})
+        else:
+            cohort = cohorts.get_cohort_by_id(course_key, cohort_id)
+            return JsonResponse(_get_cohort_representation(cohort, course))
+    else:
+        # If cohort_id is specified, update the existing cohort. Otherwise, create a new cohort.
+        if cohort_id:
+            cohort = cohorts.get_cohort_by_id(course_key, cohort_id)
+            name = request.json.get('name')
+            if name != cohort.name:
+                if cohorts.CohortAssignmentType.get(cohort, course) == cohorts.CohortAssignmentType.RANDOM:
+                    return JsonResponse(
+                        # Note: error message not translated because it is not exposed to the user (UI prevents).
+                        {"error": "Renaming of random cohorts is not supported at this time."}, 400
+                    )
+                cohort.name = name
+                cohort.save()
+        else:
+            name = request.json.get('name')
             if not name:
-        return json_http_response({'success': False,
-                                   'msg': "No name specified"})
-
+                # Note: error message not translated because it is not exposed to the user (UI prevents this state).
+                return JsonResponse({"error": "In order to create a cohort, a name must be specified."}, 400)
             try:
                 cohort = cohorts.add_cohort(course_key, name)
             except ValueError as err:
-        return json_http_response({'success': False,
-                                   'msg': str(err)})
+                return JsonResponse({"error": unicode(err)}, 400)
+
+        group_id = request.json.get('group_id')
+        if group_id is not None:
+            user_partition_id = request.json.get('user_partition_id')
+            if user_partition_id is None:
+                # Note: error message not translated because it is not exposed to the user (UI prevents this state).
+                return JsonResponse(
+                    {"error": "If group_id is specified, user_partition_id must also be specified."}, 400
+                )
+            existing_group_id, existing_partition_id = cohorts.get_group_info_for_cohort(cohort)
+            if group_id != existing_group_id or user_partition_id != existing_partition_id:
+                unlink_cohort_partition_group(cohort)
+                link_cohort_to_partition_group(cohort, user_partition_id, group_id)
+        else:
+            # If group_id was specified as None, unlink the cohort if it previously was associated with a group.
+            existing_group_id, _ = cohorts.get_group_info_for_cohort(cohort)
+            if existing_group_id is not None:
+                unlink_cohort_partition_group(cohort)
 
-    return json_http_response({
-        'success': 'True',
-        'cohort': {
-            'id': cohort.id,
-            'name': cohort.name
-        }
-    })
+        return JsonResponse(_get_cohort_representation(cohort, course))
 
 
 @ensure_csrf_cookie
@@ -121,7 +166,7 @@ def users_in_cohort(request, course_key_string, cohort_id):
     get_course_with_access(request.user, 'staff', course_key)
 
     # this will error if called with a non-int cohort_id.  That's ok--it
-    # shoudn't happen for valid clients.
+    # shouldn't happen for valid clients.
     cohort = cohorts.get_cohort_by_id(course_key, int(cohort_id))
 
     paginator = Paginator(cohort.users.all(), 100)