diff --git a/docs/lms-openapi.yaml b/docs/lms-openapi.yaml index c7434f53b538..204c43ef4263 100644 --- a/docs/lms-openapi.yaml +++ b/docs/lms-openapi.yaml @@ -629,10 +629,9 @@ paths: parameters: [] responses: '200': - description: '' + description: Successful response with cohort details. schema: - type: object - properties: {} + $ref: '#/definitions/Cohort' tags: - cohorts post: @@ -643,32 +642,26 @@ paths: in: body required: true schema: - type: object - properties: {} + $ref: '#/definitions/CohortCreate' responses: - '201': - description: '' + '200': + description: Successful response with created cohort details. schema: - type: object - properties: {} + $ref: '#/definitions/Cohort' tags: - cohorts patch: operationId: cohorts_v1_courses_cohorts_partial_update - description: Endpoint to update a cohort name and/or assignment type. + description: Endpoint to update a cohort name, assignment type, and/or content group association. parameters: - name: data in: body required: true schema: - type: object - properties: {} + $ref: '#/definitions/CohortUpdate' responses: - '200': - description: '' - schema: - type: object - properties: {} + '204': + description: Successful update, no content returned. tags: - cohorts parameters: @@ -11040,6 +11033,91 @@ paths: required: true type: string definitions: + Cohort: + description: A cohort representation + type: object + properties: + id: + title: ID + description: The integer identifier for a cohort. + type: integer + name: + title: Name + description: The string identifier for a cohort. + type: string + user_count: + title: User Count + description: The number of students in the cohort. + type: integer + assignment_type: + title: Assignment Type + description: The assignment type ("manual" or "random"). + type: string + enum: + - manual + - random + user_partition_id: + title: User Partition ID + description: The integer identifier of the UserPartition (content group configuration). + type: integer + x-nullable: true + group_id: + title: Group ID + description: The integer identifier of the specific group in the partition. + type: integer + x-nullable: true + CohortCreate: + description: Request body for creating a new cohort + required: + - name + - assignment_type + type: object + properties: + name: + title: Name + description: The string identifier for a cohort. + type: string + minLength: 1 + assignment_type: + title: Assignment Type + description: The assignment type ("manual" or "random"). + type: string + enum: + - manual + - random + user_partition_id: + title: User Partition ID + description: The integer identifier of the UserPartition (content group configuration). Required if group_id is specified. + type: integer + group_id: + title: Group ID + description: The integer identifier of the specific group in the partition. + type: integer + CohortUpdate: + description: Request body for updating a cohort. At least one of name, assignment_type, or group_id must be provided. + type: object + properties: + name: + title: Name + description: The string identifier for a cohort. + type: string + minLength: 1 + assignment_type: + title: Assignment Type + description: The assignment type ("manual" or "random"). + type: string + enum: + - manual + - random + user_partition_id: + title: User Partition ID + description: The integer identifier of the UserPartition (content group configuration). Required if group_id is specified (non-null). + type: integer + group_id: + title: Group ID + description: The integer identifier of the specific group in the partition. Set to null to remove the content group association. + type: integer + x-nullable: true PermissionValidation: description: The permissions to validate required: diff --git a/openedx/core/djangoapps/course_groups/tests/test_api_views.py b/openedx/core/djangoapps/course_groups/tests/test_api_views.py index b87dc98fca59..94d3bb1d4d2c 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_api_views.py +++ b/openedx/core/djangoapps/course_groups/tests/test_api_views.py @@ -15,8 +15,9 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import ToyCourseFactory # lint-amnesty, pylint: disable=wrong-import-order -from .. import cohorts -from .helpers import CohortFactory +from openedx.core.djangoapps.course_groups import cohorts +from openedx.core.djangoapps.course_groups.views import link_cohort_to_partition_group +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory USERNAME = 'honor' USER_MAIL = 'honor@example.com' @@ -458,3 +459,151 @@ def test_add_users_csv(self, is_staff, payload, status): response = self.client.post(path=path, data={'uploaded-file': file_pointer}) assert response.status_code == status + + def test_post_cohort_with_group_id(self): + """ + Test creating a cohort with group_id and user_partition_id. + """ + path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str}) + self.client.login(username=self.staff_user.username, password=self.password) + + payload = json.dumps({ + 'name': 'TestCohort', + 'assignment_type': 'manual', + 'group_id': 1, + 'user_partition_id': 50 + }) + response = self.client.post(path=path, data=payload, content_type='application/json') + assert response.status_code == 200 + + data = json.loads(response.content.decode('utf-8')) + assert data['name'] == 'TestCohort' + assert data['assignment_type'] == 'manual' + assert data['group_id'] == 1 + assert data['user_partition_id'] == 50 + assert data['user_count'] == 0 + assert 'id' in data + + def test_post_cohort_with_group_id_missing_partition_id(self): + """ + Test that creating a cohort with group_id but without user_partition_id returns an error. + """ + path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str}) + self.client.login(username=self.staff_user.username, password=self.password) + + payload = json.dumps({ + 'name': 'TestCohort', + 'assignment_type': 'manual', + 'group_id': 1 + }) + response = self.client.post(path=path, data=payload, content_type='application/json') + assert response.status_code == 400 + + data = json.loads(response.content.decode('utf-8')) + assert data['developer_message'] == 'If group_id is specified, user_partition_id must also be specified.' + assert data['error_code'] == 'missing-user-partition-id' + + def test_patch_cohort_set_group_id(self): + """ + Test updating a cohort to set group_id and user_partition_id. + """ + cohort = cohorts.add_cohort(self.course_key, "TestCohort", "manual") + path = reverse( + 'api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id} + ) + self.client.login(username=self.staff_user.username, password=self.password) + + payload = json.dumps({ + 'group_id': 2, + 'user_partition_id': 50 + }) + response = self.client.patch(path=path, data=payload, content_type='application/json') + assert response.status_code == 204 + + # Verify by fetching the cohort + response = self.client.get(path=path) + data = json.loads(response.content.decode('utf-8')) + assert data['id'] == cohort.id + assert data['name'] == 'TestCohort' + assert data['assignment_type'] == 'manual' + assert data['group_id'] == 2 + assert data['user_partition_id'] == 50 + + def test_patch_cohort_remove_group_id(self): + """ + Test updating a cohort to remove the group_id association by setting it to null. + """ + cohort = cohorts.add_cohort(self.course_key, "TestCohort", "manual") + link_cohort_to_partition_group(cohort, 50, 1) + + path = reverse( + 'api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id} + ) + self.client.login(username=self.staff_user.username, password=self.password) + + # Verify the cohort has a group_id + response = self.client.get(path=path) + data = json.loads(response.content.decode('utf-8')) + assert data['id'] == cohort.id + assert data['name'] == 'TestCohort' + assert data['group_id'] == 1 + assert data['user_partition_id'] == 50 + + # Remove the group_id by setting it to null + payload = json.dumps({'group_id': None}) + response = self.client.patch(path=path, data=payload, content_type='application/json') + assert response.status_code == 204 + + # Verify the group_id was removed but other fields unchanged + response = self.client.get(path=path) + data = json.loads(response.content.decode('utf-8')) + assert data['id'] == cohort.id + assert data['name'] == 'TestCohort' + assert data['assignment_type'] == 'manual' + assert data['group_id'] is None + assert data['user_partition_id'] is None + + def test_patch_cohort_with_group_id_missing_partition_id(self): + """ + Test that updating a cohort with group_id but without user_partition_id returns an error. + """ + cohort = cohorts.add_cohort(self.course_key, "TestCohort", "manual") + path = reverse( + 'api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id} + ) + self.client.login(username=self.staff_user.username, password=self.password) + + payload = json.dumps({'group_id': 2}) + response = self.client.patch(path=path, data=payload, content_type='application/json') + assert response.status_code == 400 + + data = json.loads(response.content.decode('utf-8')) + assert data['developer_message'] == 'If group_id is specified, user_partition_id must also be specified.' + assert data['error_code'] == 'missing-user-partition-id' + + def test_patch_cohort_with_name_only(self): + """ + Test that PATCH with only name is now valid (previously required assignment_type too). + """ + cohort = cohorts.add_cohort(self.course_key, "OldName", "manual") + path = reverse( + 'api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id} + ) + self.client.login(username=self.staff_user.username, password=self.password) + + payload = json.dumps({'name': 'NewName'}) + response = self.client.patch(path=path, data=payload, content_type='application/json') + assert response.status_code == 204 + + # Verify the name was updated and other fields unchanged + response = self.client.get(path=path) + data = json.loads(response.content.decode('utf-8')) + assert data['id'] == cohort.id + assert data['name'] == 'NewName' + assert data['assignment_type'] == 'manual' + assert data['group_id'] is None + assert data['user_partition_id'] is None diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index da66240e74b9..c4775e4be6ca 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -424,6 +424,44 @@ def _get_cohort_response(cohort, course): return Response(_get_cohort_representation(cohort, course), status=status.HTTP_200_OK) +def _update_cohort_partition_group(cohort, group_id, user_partition_id, api_error_func): + """ + Helper method to update the partition group association for a cohort. + + Note: This logic is duplicated from the legacy cohort_handler function (lines 218-234). + We chose not to refactor cohort_handler to use this helper because: + 1. cohort_handler returns JsonResponse for errors, while this helper uses DRF's api_error pattern + 2. Unifying the error handling would require changing cohort_handler's interface or adding + try/except blocks, which could have downstream effects on existing callers + 3. The legacy endpoint may be deprecated in favor of the v1 API in the future + + Args: + cohort: The cohort to update + group_id: The group_id from the request (can be None to unlink, or an integer to link) + user_partition_id: The user_partition_id from the request + api_error_func: Function to call to raise API errors (e.g., self.api_error) + + Raises: + API error if group_id is provided without user_partition_id + """ + if group_id is not None: + if user_partition_id is None: + raise api_error_func( + status.HTTP_400_BAD_REQUEST, + 'If group_id is specified, user_partition_id must also be specified.', + 'missing-user-partition-id' + ) + 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 explicitly set to None, unlink the cohort from any partition group + existing_group_id, _ = cohorts.get_group_info_for_cohort(cohort) + if existing_group_id is not None: + unlink_cohort_partition_group(cohort) + + def _get_cohort_settings_response(course_key): """ Helper method to return a serialized response for the cohort settings. @@ -499,6 +537,25 @@ class CohortHandler(DeveloperErrorViewMixin, APIPermissions): GET /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id} PATCH /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id} + **POST Request Values** + + * name (required): The string identifier for a cohort. + * assignment_type (required): The string representing the assignment type ("manual" or "random"). + * user_partition_id (optional): The integer identifier of the UserPartition (content group configuration). + * group_id (optional): The integer identifier of the specific group in the partition. + If group_id is specified, user_partition_id must also be specified. + + **PATCH Request Values** + + * name (optional): The string identifier for a cohort. + * assignment_type (optional): The string representing the assignment type ("manual" or "random"). + * user_partition_id (optional): The integer identifier of the UserPartition (content group configuration). + * group_id (optional): The integer identifier of the specific group in the partition. + Set group_id to null to remove the content group association. + If group_id is specified (non-null), user_partition_id must also be specified. + + At least one of name, assignment_type, or group_id must be provided. + **Response Values** * cohorts: List of cohorts. @@ -507,8 +564,8 @@ class CohortHandler(DeveloperErrorViewMixin, APIPermissions): * id: The integer identifier for a cohort. * user_count: The number of students in the cohort. * assignment_type: The string representing the assignment type. - * user_partition_id: The integer identified of the UserPartition. - * group_id: The integer identified of the specific group in the partition. + * user_partition_id: The integer identifier of the UserPartition. + * group_id: The integer identifier of the specific group in the partition. """ queryset = [] @@ -547,12 +604,20 @@ def post(self, request, course_key_string, cohort_id=None): raise self.api_error(status.HTTP_400_BAD_REQUEST, '"assignment_type" must be specified to create cohort.', 'missing-assignment-type') - return _get_cohort_response( - cohorts.add_cohort(course_key, name, assignment_type), course) + + cohort = cohorts.add_cohort(course_key, name, assignment_type) + + # Handle optional group_id and user_partition_id for content group association + group_id = request.data.get('group_id') + user_partition_id = request.data.get('user_partition_id') + if group_id is not None or user_partition_id is not None: + _update_cohort_partition_group(cohort, group_id, user_partition_id, self.api_error) + + return _get_cohort_response(cohort, course) def patch(self, request, course_key_string, cohort_id=None): """ - Endpoint to update a cohort name and/or assignment type. + Endpoint to update a cohort name, assignment type, and/or content group association. """ if cohort_id is None: raise self.api_error(status.HTTP_405_METHOD_NOT_ALLOWED, @@ -560,9 +625,16 @@ def patch(self, request, course_key_string, cohort_id=None): 'missing-cohort-id') name = request.data.get('name') assignment_type = request.data.get('assignment_type') - if not any((name, assignment_type)): + # has_group_id checks key presence rather than truthiness because + # group_id=null is a valid request to unlink the content group association, + # which is distinct from group_id not being sent at all (no change). + has_group_id = 'group_id' in request.data + group_id = request.data.get('group_id') + user_partition_id = request.data.get('user_partition_id') + + if not any((name, assignment_type, has_group_id)): raise self.api_error(status.HTTP_400_BAD_REQUEST, - 'Request must include name and/or assignment type.', + 'Request must include name, assignment_type, and/or group_id.', 'missing-fields') course_key, _ = _get_course_with_access(request, course_key_string) cohort = cohorts.get_cohort_by_id(course_key, cohort_id) @@ -578,6 +650,11 @@ def patch(self, request, course_key_string, cohort_id=None): cohorts.set_assignment_type(cohort, assignment_type) except ValueError as e: raise self.api_error(status.HTTP_400_BAD_REQUEST, str(e), 'last-random-cohort') + + # Handle group_id and user_partition_id for content group association + if has_group_id: + _update_cohort_partition_group(cohort, group_id, user_partition_id, self.api_error) + return Response(status=status.HTTP_204_NO_CONTENT)