Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: return empty list when no courses are found for request (#35942)

Browse Source 
This change addresses an issue reported while testing Sumac, where the API V2 is on by default in the authoring MFE: openedx/wg-build-test-release#428. It fails when retrieving an empty list of courses with the queryparams api/contentstore/v2/home/courses?page=1&order=display_name. When this was implemented, the course authoring MFE rendered the empty lists only with page=1 query param (didn't do any filtering/ordering by default), which was later changed to page=1&order=display_name which now ordered by default.

This issue occurs because all the filtering and ordering are done under the assumption that course_overviews is always a query set. However, that's only true when there are courses available and CourseOverview.get_all_courses is used. When not, an empty list is returned instead, raising a 500 error in Studio.
This commit is contained in:
Maria Grimaldi
2024-12-05 15:52:57 +01:00
committed by GitHub
parent 202d31b17e
commit 1c835eb643
3 changed files with 199 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

86
cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py
View File

@@ -2,7 +2,9 @@
Unit tests for home page view.
"""
import ddt
import pytz
from collections import OrderedDict
from datetime import datetime, timedelta
from django.conf import settings
from django.test import override_settings
from django.urls import reverse
@@ -100,12 +102,13 @@ class HomePageCoursesViewTest(CourseTestCase):
def setUp(self):
super().setUp()
self.url = reverse("cms.djangoapps.contentstore:v1:courses")
CourseOverviewFactory.create(
self.course_overview = CourseOverviewFactory.create(
id=self.course.id,
org=self.course.org,
display_name=self.course.display_name,
display_number_with_default=self.course.number,
)
self.non_staff_client, _ = self.create_non_staff_authed_user_client()
def test_home_page_response(self):
"""Check successful response content"""
@@ -131,6 +134,7 @@ class HomePageCoursesViewTest(CourseTestCase):
print(response.data)
self.assertDictEqual(expected_response, response.data)
@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API)
def test_home_page_response_with_api_v2(self):
"""Check successful response content with api v2 modifications.
@@ -155,12 +159,88 @@ class HomePageCoursesViewTest(CourseTestCase):
"in_process_course_actions": [],
}
with override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API):
response = self.client.get(self.url)
response = self.client.get(self.url)
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertDictEqual(expected_response, response.data)
@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API)
@ddt.data(
("active_only", "true", 2, 0),
("archived_only", "true", 0, 1),
("search", "sample", 1, 0),
("search", "demo", 0, 1),
("order", "org", 2, 1),
("order", "display_name", 2, 1),
("order", "number", 2, 1),
("order", "run", 2, 1)
)
@ddt.unpack
def test_filter_and_ordering_courses(
self,
filter_key,
filter_value,
expected_active_length,
expected_archived_length
):
"""Test home page with org filter and ordering for a staff user.
The test creates an active/archived course, and then filters/orders them using the query parameters.
"""
archived_course_key = self.store.make_course_key("demo-org", "demo-number", "demo-run")
CourseOverviewFactory.create(
display_name="Course (Demo)",
id=archived_course_key,
org=archived_course_key.org,
end=(datetime.now() - timedelta(days=365)).replace(tzinfo=pytz.UTC),
)
active_course_key = self.store.make_course_key("sample-org", "sample-number", "sample-run")
CourseOverviewFactory.create(
display_name="Course (Sample)",
id=active_course_key,
org=active_course_key.org,
)
response = self.client.get(self.url, {filter_key: filter_value})
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(len(response.data["archived_courses"]), expected_archived_length)
self.assertEqual(len(response.data["courses"]), expected_active_length)
@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API)
@ddt.data(
("active_only", "true"),
("archived_only", "true"),
("search", "sample"),
("order", "org"),
)
@ddt.unpack
def test_filter_and_ordering_no_courses_staff(self, filter_key, filter_value):
"""Test home page with org filter and ordering when there are no courses for a staff user."""
self.course_overview.delete()
response = self.client.get(self.url, {filter_key: filter_value})
self.assertEqual(len(response.data["courses"]), 0)
self.assertEqual(response.status_code, status.HTTP_200_OK)
@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API)
@ddt.data(
("active_only", "true"),
("archived_only", "true"),
("search", "sample"),
("order", "org"),
)
@ddt.unpack
def test_home_page_response_no_courses_non_staff(self, filter_key, filter_value):
"""Test home page with org filter and ordering when there are no courses for a non-staff user."""
self.course_overview.delete()
response = self.non_staff_client.get(self.url)
self.assertEqual(len(response.data["courses"]), 0)
self.assertEqual(response.status_code, status.HTTP_200_OK)
@override_waffle_switch(ENABLE_GLOBAL_STAFF_OPTIMIZATION, True)
def test_org_query_if_passed(self):
"""Test home page when org filter passed as a query param"""

98
cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py
View File

@@ -45,6 +45,7 @@ class HomePageCoursesViewV2Test(CourseTestCase):
org=archived_course_key.org,
end=(datetime.now() - timedelta(days=365)).replace(tzinfo=pytz.UTC),
)
self.non_staff_client, _ = self.create_non_staff_authed_user_client()
def test_home_page_response(self):
"""Get list of courses available to the logged in user.
@@ -247,3 +248,100 @@ class HomePageCoursesViewV2Test(CourseTestCase):
self.assertEqual(response.status_code, status.HTTP_200_OK)
mock_modulestore().get_course_summaries.assert_called_once()
mock_course_overview.get_all_courses.assert_not_called()
@ddt.data(
("active_only", "true"),
("archived_only", "true"),
("search", "sample"),
("order", "org"),
("page", 1),
)
@ddt.unpack
def test_if_empty_list_of_courses(self, query_param, value):
"""Get list of courses when no courses are available.
Expected result:
- An empty list of courses available to the logged in user.
"""
self.active_course.delete()
self.archived_course.delete()
response = self.client.get(self.api_v2_url, {query_param: value})
self.assertEqual(len(response.data['results']['courses']), 0)
self.assertEqual(response.status_code, status.HTTP_200_OK)
@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API)
@ddt.data(
("active_only", "true", 2, 0),
("archived_only", "true", 0, 1),
("search", "foo", 1, 0),
("search", "demo", 0, 1),
("order", "org", 2, 1),
("order", "display_name", 2, 1),
("order", "number", 2, 1),
("order", "run", 2, 1)
)
@ddt.unpack
def test_filter_and_ordering_courses(
self,
filter_key,
filter_value,
expected_active_length,
expected_archived_length
):
"""Get list of courses when filter and ordering are applied.
This test creates two courses besides the default courses created in the setUp method.
Then filters and orders them based on the filter_key and filter_value passed as query parameters.
Expected result:
- A list of courses available to the logged in user for the specified filter and order.
"""
archived_course_key = self.store.make_course_key("demo-org", "demo-number", "demo-run")
CourseOverviewFactory.create(
display_name="Course (Demo)",
id=archived_course_key,
org=archived_course_key.org,
end=(datetime.now() - timedelta(days=365)).replace(tzinfo=pytz.UTC),
)
active_course_key = self.store.make_course_key("foo-org", "foo-number", "foo-run")
CourseOverviewFactory.create(
display_name="Course (Foo)",
id=active_course_key,
org=active_course_key.org,
)
response = self.client.get(self.api_v2_url, {filter_key: filter_value})
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(
len([course for course in response.data["results"]["courses"] if course["is_active"]]),
expected_active_length
)
self.assertEqual(
len([course for course in response.data["results"]["courses"] if not course["is_active"]]),
expected_archived_length
)
@ddt.data(
("active_only", "true"),
("archived_only", "true"),
("search", "sample"),
("order", "org"),
("page", 1),
)
@ddt.unpack
def test_if_empty_list_of_courses_non_staff(self, query_param, value):
"""Get list of courses when no courses are available for non-staff users.
Expected result:
- An empty list of courses available to the logged in user.
"""
self.active_course.delete()
self.archived_course.delete()
response = self.non_staff_client.get(self.api_v2_url, {query_param: value})
self.assertEqual(len(response.data["results"]["courses"]), 0)
self.assertEqual(response.status_code, status.HTTP_200_OK)

21
cms/djangoapps/contentstore/views/course.py
View File

@@ -15,6 +15,7 @@ from django.conf import settings
from django.contrib.auth import get_user_model
from django.contrib.auth.decorators import login_required
from django.core.exceptions import FieldError, PermissionDenied, ValidationError as DjangoValidationError
from django.db.models import QuerySet
from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseNotFound
from django.shortcuts import redirect
from django.urls import reverse
@@ -575,6 +576,10 @@ def _accessible_courses_list_from_groups(request):
if course_keys:
courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys})
else:
# If no course keys are found for the current user, then return without filtering
# or ordering the courses list.
return courses_list, []
search_query, order, active_only, archived_only = get_query_params_if_present(request)
courses_list = get_filtered_and_ordered_courses(
@@ -588,7 +593,11 @@ def _accessible_courses_list_from_groups(request):
return courses_list, []
def get_courses_by_status(active_only, archived_only, course_overviews):
def get_courses_by_status(
active_only: bool,
archived_only: bool,
course_overviews: QuerySet[CourseOverview]
) -> QuerySet[CourseOverview]:
"""
Return course overviews based on a base queryset filtered by a status.
@@ -602,7 +611,10 @@ def get_courses_by_status(active_only, archived_only, course_overviews):
return CourseOverview.get_courses_by_status(active_only, archived_only, course_overviews)
def get_courses_by_search_query(search_query, course_overviews):
def get_courses_by_search_query(
search_query: str | None,
course_overviews: QuerySet[CourseOverview]
) -> QuerySet[CourseOverview]:
"""Return course overviews based on a base queryset filtered by a search query.
Args:
@@ -614,7 +626,10 @@ def get_courses_by_search_query(search_query, course_overviews):
return CourseOverview.get_courses_matching_query(search_query, course_overviews=course_overviews)
def get_courses_order_by(order_query, course_overviews):
def get_courses_order_by(
order_query: str | None,
course_overviews: QuerySet[CourseOverview]
) -> QuerySet[CourseOverview]:
"""Return course overviews based on a base queryset ordered by a query.
Args: