From 5ea7ced9003b4f773bfe40d03f451d5ec2af948d Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 18 Dec 2014 17:09:16 -0500 Subject: [PATCH] Create a RESTful handle_cohort view method for GET and PUT/POST. --- lms/static/js/models/cohort.js | 16 +- lms/urls.py | 7 +- .../tests/test_partition_scheme.py | 57 ++- .../course_groups/tests/test_views.py | 337 ++++++++++++------ .../core/djangoapps/course_groups/views.py | 169 +++++---- 5 files changed, 380 insertions(+), 206 deletions(-) diff --git a/lms/static/js/models/cohort.js b/lms/static/js/models/cohort.js index 32d929027a..4696351428 100644 --- a/lms/static/js/models/cohort.js +++ b/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 } }); diff --git a/lms/urls.py b/lms/urls.py index aeac3f6ed2..3bf18ff8d3 100644 --- a/lms/urls.py +++ b/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[0-9]+)?$'.format(settings.COURSE_KEY_PATTERN), + 'openedx.core.djangoapps.course_groups.views.cohort_handler', name="cohorts"), url(r'^courses/{}/cohorts/(?P[0-9]+)$'.format(settings.COURSE_KEY_PATTERN), 'openedx.core.djangoapps.course_groups.views.users_in_cohort', name="list_cohort"), diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py index 7891be1844..a4fd039e8e 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py +++ b/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) diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py index 712c966ac8..4acde656c7 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_views.py +++ b/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, ) - -class AddCohortTestCase(CohortViewsTestCase): - """ - Tests the `add_cohort` view. - """ - def request_add_cohort(self, cohort_name, course): + def test_get_single_cohort(self): """ - Call `add_cohort` and return its response as a dict. - """ - 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) - - def verify_contains_added_cohort(self, response_dict, cohort_name, expected_error_msg=None): - """ - 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. - """ - if expected_error_msg is not None: - self.assertFalse(response_dict.get("success")) - self.assertEqual( - response_dict.get("msg"), - expected_error_msg - ) - else: - self.assertTrue(response_dict.get("success")) - self.assertEqual( - response_dict.get("cohort").get("name"), - cohort_name - ) - self.assertIsNotNone(get_cohort_by_name(self.course.id, cohort_name)) - - def test_non_staff(self): - """ - Verify that non-staff users cannot access add_cohort. - """ - self._verify_non_staff_cannot_access(add_cohort, "POST", [self.course.id.to_deprecated_string()]) - - def test_new_cohort(self): - """ - Verify that we can add a new cohort. - """ - cohort_name = "New Cohort" - self.verify_contains_added_cohort( - self.request_add_cohort(cohort_name, self.course), - cohort_name, - ) - - def test_no_cohort(self): - """ - Verify that we cannot explicitly add no cohort. - """ - response_dict = self.request_add_cohort("", self.course) - self.assertFalse(response_dict.get("success")) - self.assertEqual(response_dict.get("msg"), "No name specified") - - def test_existing_cohort(self): - """ - Verify that we cannot add a cohort with the same name as an existing - cohort. + Tests that information for just a single cohort can be requested. """ 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.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 + } + ) + + ############### 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 + ): + """ + Verifies that the cohort was created properly and the correct response was returned. + """ + 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): + """ + Verify that a new cohort can be created, with and without user_partition_id/group_id information. + """ + 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 test_create_new_cohort_missing_name(self): + """ + Verify that we cannot create a cohort without specifying a name. + """ + 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( + "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 + ) + self.assertEqual( + "Renaming of random cohorts is not supported at this time.", response_dict.get("error") + ) + 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_update_cohort_group_id(self): + """ + Test that it is possible to update the user_partition_id/group_id of an existing cohort. + """ + 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_update_cohort_remove_group_id(self): + """ + Test that it is possible to remove the user_partition_id/group_id linking of an existing cohort. + """ + 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_change_cohort_group_id(self): + """ + Test that it is possible to change the user_partition_id/group_id of an existing cohort to a + different group_id. + """ + 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_update_cohort_missing_user_partition_id(self): + """ + Verify that we cannot update a cohort with a group_id if the user_partition_id is not also specified. + """ + self._create_cohorts() + 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( diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index 37872e60bf..034a6a9231 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/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) +def link_cohort_to_partition_group(cohort, partition_id, group_id): + """ + Create cohort to partition_id/group_id link. + """ + CourseUserGroupPartitionGroup( + course_user_group=cohort, + partition_id=partition_id, + group_id=group_id, + ).save() + + +def unlink_cohort_partition_group(cohort): + """ + Remove any existing cohort to partition_id/group_id link. + """ + CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() + + +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 -def list_cohorts(request, course_key_string): +@expect_json +@login_required +def cohort_handler(request, course_key_string, cohort_id=None): """ - Return json dump of dict: - - {'success': True, - 'cohorts': [{'name': name, 'id': id}, ...]} + 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) - 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: + # 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 JsonResponse({"error": unicode(err)}, 400) - 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) - ] + 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, - 'cohorts': all_cohorts}) - - -@ensure_csrf_cookie -@require_POST -def add_cohort(request, course_key_string): - """ - Return json of dict: - {'success': True, - 'cohort': {'id': id, - 'name': name}} - - or - - {'success': False, - 'msg': error_msg} if there's an error - """ - # 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") - if not name: - return json_http_response({'success': False, - 'msg': "No name specified"}) - - try: - cohort = cohorts.add_cohort(course_key, name) - except ValueError as err: - return json_http_response({'success': False, - 'msg': str(err)}) - - 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)