Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion openedx/core/djangoapps/course_groups/cohorts.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,15 @@ def migrate_cohort_settings(course):
return cohort_settings


def get_course_cohorts(course=None, course_id=None, assignment_type=None):
def get_course_cohorts(course=None, course_id=None, assignment_type=None, ordering='asc'):
"""
Get a list of all the cohorts in the given course. This will include auto cohorts,
regardless of whether or not the auto cohorts include any users.

Arguments:
course: the course for which cohorts should be returned
assignment_type: cohort assignment type
ordering: sort direction for results by name ('asc' or 'desc'), defaults to 'asc'

Returns:
A list of CourseUserGroup objects. Empty if there are no cohorts. Does
Expand All @@ -343,6 +344,10 @@ def get_course_cohorts(course=None, course_id=None, assignment_type=None):
group_type=CourseUserGroup.COHORT
)
query_set = query_set.filter(cohort__assignment_type=assignment_type) if assignment_type else query_set
sort_field = 'name'
if ordering == 'desc':
sort_field = '-name'
query_set = query_set.order_by(sort_field)
Comment on lines 346 to +350
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says ordering must be 'asc' or 'desc', but the implementation silently treats any other value as ascending. To avoid surprising callers, either validate/normalize ordering in get_course_cohorts (e.g., raise ValueError for invalid values) or adjust the docstring to reflect the fallback behavior.

Copilot uses AI. Check for mistakes.
return list(query_set)


Expand Down
42 changes: 42 additions & 0 deletions openedx/core/djangoapps/course_groups/tests/test_api_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,48 @@ def test_patch_cohort_with_group_id_missing_partition_id(self):
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_get_cohorts_default_ordering(self):
"""
Test that cohorts are returned in ascending alphabetical order by default.
"""
cohorts.add_cohort(self.course_key, "Zebra", "manual")
cohorts.add_cohort(self.course_key, "Alpha", "manual")
cohorts.add_cohort(self.course_key, "Mango", "manual")

path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str})
self.client.login(username=self.staff_user.username, password=self.password)
response = self.client.get(path=path)

assert response.status_code == 200
names = [c['name'] for c in response.json()]
assert names == ['Alpha', 'Mango', 'Zebra']

def test_get_cohorts_desc_ordering(self):
"""
Test that cohorts are returned in descending alphabetical order when ordering=desc.
"""
cohorts.add_cohort(self.course_key, "Zebra", "manual")
cohorts.add_cohort(self.course_key, "Alpha", "manual")
cohorts.add_cohort(self.course_key, "Mango", "manual")

path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str})
self.client.login(username=self.staff_user.username, password=self.password)
response = self.client.get(path=path, data={'ordering': 'desc'})

assert response.status_code == 200
names = [c['name'] for c in response.json()]
assert names == ['Zebra', 'Mango', 'Alpha']

def test_get_cohorts_invalid_ordering(self):
"""
Test that an invalid ordering value returns a 400 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)
response = self.client.get(path=path, data={'ordering': 'invalid'})

assert response.status_code == 400
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_get_cohorts_invalid_ordering only asserts the 400 status. Since this change introduces a specific error payload (developer_message/error_code), it would be more robust to assert at least the error_code (e.g., invalid-ordering-value) so the test doesn’t pass for an unrelated 400 response.

Suggested change
assert response.status_code == 400
assert response.status_code == 400
data = response.json()
assert data.get('error_code') == 'invalid-ordering-value'

Copilot uses AI. Check for mistakes.

def test_patch_cohort_with_name_only(self):
"""
Test that PATCH with only name is now valid (previously required assignment_type too).
Expand Down
9 changes: 8 additions & 1 deletion openedx/core/djangoapps/course_groups/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,14 @@ def get(self, request, course_key_string, cohort_id=None):
"""
course_key, course = _get_course_with_access(request, course_key_string, 'load')
if not cohort_id:
all_cohorts = cohorts.get_course_cohorts(course)
ordering = request.query_params.get('ordering', 'asc').lower()
if ordering not in ('asc', 'desc'):
raise self.api_error(
status.HTTP_400_BAD_REQUEST,
'Invalid ordering value. Must be "asc" or "desc".',
'invalid-ordering-value'
)
all_cohorts = cohorts.get_course_cohorts(course, ordering=ordering)
paginator = NamespacedPageNumberPagination()
paginator.max_page_size = MAX_PAGE_SIZE
page = paginator.paginate_queryset(all_cohorts, request)
Expand Down
Loading