From 218f898b6dd91fa4676f3dd0b3dd7d062adc8ada Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 2 Dec 2024 20:07:07 +0100 Subject: [PATCH 1/5] fix: return empty list when no courses are found for request --- .../rest_api/v2/views/tests/test_home.py | 22 +++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 3 +++ 2 files changed, 25 insertions(+) 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 6905de254f3e..a4dd0acafdcc 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 @@ -247,3 +247,25 @@ 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) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 9f6cfb7c430e..08fc3c2739ca 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -576,6 +576,9 @@ def filter_ccx(course_access): if course_keys: courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) + if not courses_list: + return [], [] + search_query, order, active_only, archived_only = get_query_params_if_present(request) courses_list = get_filtered_and_ordered_courses( courses_list, From 88f8ec69d4fb2003bb1268c10e70a8e398ef9630 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 2 Dec 2024 21:52:25 +0100 Subject: [PATCH 2/5] refactor: don't evaluate queryset before filtering/ordering --- cms/djangoapps/contentstore/views/course.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 08fc3c2739ca..b0899b60a953 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -575,9 +575,10 @@ def filter_ccx(course_access): if course_keys: courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) - - if not courses_list: - return [], [] + 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( From aba2b9fb03dcded92acb299bf1178773ba4e21bd Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 3 Dec 2024 11:31:21 +0100 Subject: [PATCH 3/5] test: add test cases for non-staff users --- .../rest_api/v1/views/tests/test_home.py | 86 ++++++++++++++++++- .../rest_api/v2/views/tests/test_home.py | 76 ++++++++++++++++ 2 files changed, 159 insertions(+), 3 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 73e3fe5ec3ad..b8892f0e59b4 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 a4dd0acafdcc..e773e7f213c6 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. @@ -269,3 +270,78 @@ def test_if_empty_list_of_courses(self, 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) From cf265ed45051f4daa2eeba5fce98526d14c74fdb Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 4 Dec 2024 13:26:54 +0100 Subject: [PATCH 4/5] refactor: add type hints to filter/order functions --- cms/djangoapps/contentstore/views/course.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b0899b60a953..405e1ef4ab4d 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 @@ -592,7 +593,8 @@ 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. @@ -606,7 +608,8 @@ 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: @@ -618,7 +621,8 @@ 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: From 4c6c4ab31bdbbe0fed8fc161f6aacd62a5825594 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 4 Dec 2024 14:04:35 +0100 Subject: [PATCH 5/5] refactor: fix quality issues with type hints --- cms/djangoapps/contentstore/views/course.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 405e1ef4ab4d..244804c3062b 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -593,8 +593,11 @@ def filter_ccx(course_access): return courses_list, [] -def get_courses_by_status(active_only: bool, archived_only: bool, course_overviews: QuerySet[CourseOverview]) \ --> QuerySet[CourseOverview]: +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. @@ -608,8 +611,10 @@ def get_courses_by_status(active_only: bool, archived_only: bool, course_overvie return CourseOverview.get_courses_by_status(active_only, archived_only, course_overviews) -def get_courses_by_search_query(search_query: str | None, course_overviews: QuerySet[CourseOverview]) \ --> QuerySet[CourseOverview]: +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: @@ -621,8 +626,10 @@ def get_courses_by_search_query(search_query: str | None, course_overviews: Quer return CourseOverview.get_courses_matching_query(search_query, course_overviews=course_overviews) -def get_courses_order_by(order_query: str | None, course_overviews: QuerySet[CourseOverview]) \ --> QuerySet[CourseOverview]: +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: