Skip to content

Commit

Permalink
fix: return empty list when no courses are found for request (#35942)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mariajgrimaldi committed Dec 5, 2024
1 parent 1115edb commit 22ee97c
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 6 deletions.
86 changes: 83 additions & 3 deletions cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"""
Expand All @@ -131,6 +134,7 @@ def test_home_page_response(self):
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.
Expand All @@ -155,12 +159,88 @@ def test_home_page_response_with_api_v2(self):
"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"""
Expand Down
98 changes: 98 additions & 0 deletions cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def setUp(self):
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.
Expand Down Expand Up @@ -239,3 +240,100 @@ def test_api_v2_is_disabled(self, mock_modulestore, mock_course_overview):
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 changes: 18 additions & 3 deletions cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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
Expand Down Expand Up @@ -575,6 +576,10 @@ def filter_ccx(course_access):

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(
Expand All @@ -588,7 +593,11 @@ def filter_ccx(course_access):
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.
Expand All @@ -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:
Expand All @@ -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:
Expand Down

0 comments on commit 22ee97c

Please sign in to comment.