From 182bd7ac64f3ecdcaccb2d589cf1562c7372952d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 5 Dec 2024 15:52:57 +0100 Subject: [PATCH] fix: return empty list when no courses are found for request (#35942) 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. --- .../rest_api/v1/views/tests/test_home.py | 86 +++++++++++++++- .../rest_api/v2/views/tests/test_home.py | 98 +++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 21 +++- 3 files changed, 199 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index 69eee524373c..525cd0b88949 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -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 @@ 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. @@ -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""" diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py index c0ffa50903cd..d2422267cc4d 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -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. @@ -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) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 9f6cfb7c430e..244804c3062b 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -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 @@ -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( @@ -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. @@ -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: