Skip to content

Commit

Permalink
refactor!: remove ENABLE_HOME_PAGE_COURSE_API_V2 feature toggle (#3…
Browse files Browse the repository at this point in the history
  • Loading branch information
BryanttV authored Feb 24, 2025
1 parent 2798f28 commit a2bb8c9
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 127 deletions.
13 changes: 1 addition & 12 deletions cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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"),
Expand All @@ -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"),
Expand All @@ -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)
Expand Down
9 changes: 1 addition & 8 deletions cms/djangoapps/contentstore/rest_api/v2/views/home.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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(
Expand Down
23 changes: 1 addition & 22 deletions cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py
Original file line number Diff line number Diff line change
@@ -1,26 +1,21 @@
"""
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

from cms.djangoapps.contentstore.tests.utils import CourseTestCase
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):
"""
Expand Down Expand Up @@ -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"),
Expand All @@ -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),
Expand Down
45 changes: 10 additions & 35 deletions cms/djangoapps/contentstore/tests/test_course_listing.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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):
"""
Expand All @@ -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)
Expand Down
26 changes: 9 additions & 17 deletions cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
38 changes: 15 additions & 23 deletions cms/djangoapps/contentstore/views/tests/test_course_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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.
Expand All @@ -458,27 +452,25 @@ 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(
self,
separate_archived_courses,
username,
org,
mongo_queries,
sql_queries
):
"""
Expand All @@ -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)


Expand Down
10 changes: 0 additions & 10 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a2bb8c9

Please sign in to comment.