From a2bb8c94586ab15a28c5f6b360c8504d432bcbc4 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama <64033729+BryanttV@users.noreply.github.com> Date: Mon, 24 Feb 2025 10:42:06 -0500 Subject: [PATCH] refactor!: remove `ENABLE_HOME_PAGE_COURSE_API_V2` feature toggle (#36181) --- .../rest_api/v1/views/tests/test_home.py | 13 +----- .../contentstore/rest_api/v2/views/home.py | 9 +--- .../rest_api/v2/views/tests/test_home.py | 23 +--------- .../contentstore/tests/test_course_listing.py | 45 +++++-------------- cms/djangoapps/contentstore/views/course.py | 26 ++++------- .../views/tests/test_course_index.py | 38 +++++++--------- cms/envs/common.py | 10 ----- 7 files changed, 37 insertions(+), 127 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 8fe246cf23fd..3e88e0401060 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 @@ -15,12 +15,6 @@ from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory -FEATURES_WITH_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = True -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = False - - @ddt.ddt class HomePageViewTest(CourseTestCase): """ @@ -99,7 +93,6 @@ def test_taxonomy_list_link(self): ) -@override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API) @ddt.ddt class HomePageCoursesViewTest(CourseTestCase): """ @@ -141,7 +134,6 @@ 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. @@ -171,7 +163,6 @@ def test_home_page_response_with_api_v2(self): 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), @@ -214,7 +205,6 @@ def test_filter_and_ordering_courses( 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"), @@ -231,7 +221,6 @@ def test_filter_and_ordering_no_courses_staff(self, 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"), @@ -243,7 +232,7 @@ 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) + response = self.non_staff_client.get(self.url, {filter_key: filter_value}) self.assertEqual(len(response.data["courses"]), 0) self.assertEqual(response.status_code, status.HTTP_200_OK) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index c32741510157..6d2bd1dcc9be 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -1,8 +1,7 @@ """HomePageCoursesViewV2 APIView for getting content available to the logged in user.""" + import edx_api_doc_tools as apidocs from collections import OrderedDict -from django.conf import settings -from django.http import HttpResponseNotFound from rest_framework.response import Response from rest_framework.request import Request from rest_framework.views import APIView @@ -126,13 +125,7 @@ def get(self, request: Request): "in_process_course_actions": [], } ``` - - if the `ENABLE_HOME_PAGE_COURSE_API_V2` feature flag is not enabled, an HTTP 404 "Not Found" response - is returned. """ - if not settings.FEATURES.get('ENABLE_HOME_PAGE_COURSE_API_V2', False): - return HttpResponseNotFound() - courses, in_process_course_actions = get_course_context_v2(request) paginator = HomePageCoursesPaginator() courses_page = paginator.paginate_queryset( 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 e899019b4f17..6a51610ac9f2 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 @@ -1,14 +1,13 @@ """ Unit tests for home page view. """ + from collections import OrderedDict from datetime import datetime, timedelta -from unittest.mock import patch import ddt import pytz from django.conf import settings -from django.test import override_settings from django.urls import reverse from rest_framework import status @@ -16,11 +15,7 @@ from cms.djangoapps.contentstore.utils import reverse_course_url from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory -FEATURES_WITH_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = True - -@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API) @ddt.ddt class HomePageCoursesViewV2Test(CourseTestCase): """ @@ -208,21 +203,6 @@ def test_page_query_if_passed(self): self.assertEqual(response.data["count"], 2) self.assertEqual(response.status_code, status.HTTP_200_OK) - @patch("cms.djangoapps.contentstore.views.course.CourseOverview") - @patch("cms.djangoapps.contentstore.views.course.modulestore") - def test_api_v2_is_disabled(self, mock_modulestore, mock_course_overview): - """Get list of courses when home page course v2 API is disabled. - - Expected result: - - Courses are read from the modulestore. - """ - with override_settings(FEATURES={'ENABLE_HOME_PAGE_COURSE_API_V2': False}): - response = self.client.get(self.api_v1_url) - - 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"), @@ -245,7 +225,6 @@ 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), diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 44084e3595b8..7db6d078eaa8 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -3,14 +3,13 @@ by reversing group name formats. """ - import random from unittest.mock import Mock, patch import ddt from ccx_keys.locator import CCXLocator from django.conf import settings -from django.test import RequestFactory, override_settings +from django.test import RequestFactory from opaque_keys.edx.locations import CourseLocator from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient @@ -35,17 +34,12 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES -from xmodule.course_block import CourseSummary # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order TOTAL_COURSES_COUNT = 10 USER_COURSES_COUNT = 1 -FEATURES_WITH_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = True -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = False @ddt.ddt @@ -185,19 +179,11 @@ def test_staff_course_listing(self): courses_list_by_staff, __ = get_courses_accessible_to_user(self.request) self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) + self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_list_by_staff)) - with override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API): - # Verify fetched accessible courses list is a list of CourseOverview instances when home page course v2 - # api is enabled. - self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_list_by_staff)) - - with override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API): - # Verify fetched accessible courses list is a list of CourseSummery instances - self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list_by_staff)) - - # Now count the db queries for staff - with check_mongo_calls(2): - list(_accessible_courses_summary_iter(self.request)) + # Now count the db queries for staff + with self.assertNumQueries(2): + list(_accessible_courses_summary_iter(self.request)) def test_get_course_list_with_invalid_course_location(self): """ @@ -212,21 +198,10 @@ def test_get_course_list_with_invalid_course_location(self): courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) - with override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API): - # Verify fetched accessible courses list is a list of CourseOverview instances when home page course v2 - # api is enabled. - courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) - courses_summary_list = list(courses_summary_iter) - self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_summary_list)) - self.assertEqual(len(courses_summary_list), 1) - - with override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API): - # Verify fetched accessible courses list is a list of CourseSummery instances and only one course - # is returned - courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) - courses_summary_list = list(courses_summary_iter) - self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_summary_list)) - self.assertEqual(len(courses_summary_list), 1) + courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) + courses_summary_list = list(courses_summary_iter) + self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_summary_list)) + self.assertEqual(len(courses_summary_list), 1) # get courses by reversing group name formats courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 064cb1ad25e0..81ad1eb6ddde 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -405,23 +405,15 @@ def course_filter(course_summary): return has_studio_read_access(request.user, course_summary.id) - enable_home_page_api_v2 = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_API_V2"] - - if enable_home_page_api_v2: - # If the new home page API is enabled, we should use the Django ORM to filter and order the courses - courses_summary = CourseOverview.get_all_courses() - else: - courses_summary = modulestore().get_course_summaries() - - if enable_home_page_api_v2: - search_query, order, active_only, archived_only = get_query_params_if_present(request) - courses_summary = get_filtered_and_ordered_courses( - courses_summary, - active_only, - archived_only, - search_query, - order, - ) + courses_summary = CourseOverview.get_all_courses() + search_query, order, active_only, archived_only = get_query_params_if_present(request) + courses_summary = get_filtered_and_ordered_courses( + courses_summary, + active_only, + archived_only, + search_query, + order, + ) courses_summary = filter(course_filter, courses_summary) in_process_course_actions = get_in_process_course_actions(request) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index a49273928062..b121a490cd2b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -40,11 +40,6 @@ from ..course import _deprecated_blocks_info, course_outline_initial_state, reindex_course_and_check_access from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import VisibilityState, create_xblock_info -FEATURES_WITH_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = True -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() -FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = False - class TestCourseIndex(CourseTestCase): """ @@ -430,19 +425,18 @@ def check_index_page(self, separate_archived_courses, org): archived_course_tab = parsed_html.find_class('archived-courses') self.assertEqual(len(archived_course_tab), 1 if separate_archived_courses else 0) - @override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API) @ddt.data( # Staff user has course staff access - (True, 'staff', None, 0, 23), - (False, 'staff', None, 0, 23), + (True, 'staff', None, 23), + (False, 'staff', None, 23), # Base user has global staff access - (True, 'user', ORG, 2, 23), - (False, 'user', ORG, 2, 23), - (True, 'user', None, 2, 23), - (False, 'user', None, 2, 23), + (True, 'user', ORG, 23), + (False, 'user', ORG, 23), + (True, 'user', None, 23), + (False, 'user', None, 23), ) @ddt.unpack - def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): + def test_separate_archived_courses(self, separate_archived_courses, username, org, sql_queries): """ Ensure that archived courses are shown as expected for all user types, when the feature is enabled/disabled. Also ensure that enabling the feature does not adversely affect the database query count. @@ -458,19 +452,18 @@ def test_separate_archived_courses(self, separate_archived_courses, username, or with override_settings(FEATURES=features): self.check_index_page_with_query_count(separate_archived_courses=separate_archived_courses, org=org, - mongo_queries=mongo_queries, + mongo_queries=0, sql_queries=sql_queries) - @override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API) @ddt.data( # Staff user has course staff access - (True, 'staff', None, 0, 23), - (False, 'staff', None, 0, 23), + (True, 'staff', None, 23), + (False, 'staff', None, 23), # Base user has global staff access - (True, 'user', ORG, 0, 23), - (False, 'user', ORG, 0, 23), - (True, 'user', None, 0, 23), - (False, 'user', None, 0, 23), + (True, 'user', ORG, 23), + (False, 'user', ORG, 23), + (True, 'user', None, 23), + (False, 'user', None, 23), ) @ddt.unpack def test_separate_archived_courses_with_home_page_course_v2_api( @@ -478,7 +471,6 @@ def test_separate_archived_courses_with_home_page_course_v2_api( separate_archived_courses, username, org, - mongo_queries, sql_queries ): """ @@ -496,7 +488,7 @@ def test_separate_archived_courses_with_home_page_course_v2_api( with override_settings(FEATURES=features): self.check_index_page_with_query_count(separate_archived_courses=separate_archived_courses, org=org, - mongo_queries=mongo_queries, + mongo_queries=0, sql_queries=sql_queries) diff --git a/cms/envs/common.py b/cms/envs/common.py index 2f81a43d984b..b9f3ca8240b0 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -547,16 +547,6 @@ # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/33952 'ENABLE_HIDE_FROM_TOC_UI': False, - # .. toggle_name: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2'] - # .. toggle_implementation: DjangoSetting - # .. toggle_default: True - # .. toggle_description: Enables the new home page course v2 API, which is a new version of the home page course - # API with pagination, filter and ordering capabilities. - # .. toggle_use_cases: open_edx - # .. toggle_creation_date: 2024-03-14 - # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/34173 - 'ENABLE_HOME_PAGE_COURSE_API_V2': True, - # .. toggle_name: FEATURES['ENABLE_GRADING_METHOD_IN_PROBLEMS'] # .. toggle_implementation: DjangoSetting # .. toggle_default: False