From 97aeed8d6a048ec7054e2257da47ddfdd37102d9 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 30 Jan 2024 16:28:36 -0400 Subject: [PATCH 01/38] feat: add CourseHomeCoursesV2 with filtering & ordering capabilities --- cms/djangoapps/contentstore/rest_api/urls.py | 2 + .../contentstore/rest_api/v2/__init__.py | 0 .../rest_api/v2/serializers/__init__.py | 1 + .../rest_api/v2/serializers/home.py | 62 +++++++ .../contentstore/rest_api/v2/urls.py | 15 ++ .../rest_api/v2/views/__init__.py | 1 + .../contentstore/rest_api/v2/views/home.py | 91 +++++++++++ cms/djangoapps/contentstore/utils.py | 40 +++++ cms/djangoapps/contentstore/views/course.py | 153 +++++++++++++++++- .../content/course_overviews/models.py | 41 +++++ 10 files changed, 405 insertions(+), 1 deletion(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/serializers/home.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/urls.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/home.py diff --git a/cms/djangoapps/contentstore/rest_api/urls.py b/cms/djangoapps/contentstore/rest_api/urls.py index 1d4ff8d4ab6d..7296f7bb9858 100644 --- a/cms/djangoapps/contentstore/rest_api/urls.py +++ b/cms/djangoapps/contentstore/rest_api/urls.py @@ -7,10 +7,12 @@ from .v0 import urls as v0_urls from .v1 import urls as v1_urls +from .v2 import urls as v2_urls app_name = 'cms.djangoapps.contentstore' urlpatterns = [ path('v0/', include(v0_urls)), path('v1/', include(v1_urls)), + path('v2/', include(v2_urls)) ] diff --git a/cms/djangoapps/contentstore/rest_api/v2/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py new file mode 100644 index 000000000000..36ec8bda5a0b --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py @@ -0,0 +1 @@ +from cms.djangoapps.contentstore.rest_api.v2.serializers.home import CourseHomeTabSerializerV2 diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py new file mode 100644 index 000000000000..7e66629ab16a --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -0,0 +1,62 @@ +""" +API Serializers for course home V2 API. +""" +from django.conf import settings +from rest_framework import serializers + +from cms.djangoapps.contentstore.rest_api.serializers.common import CourseCommonSerializer +from cms.djangoapps.contentstore.utils import get_lms_link_for_item, reverse_course_url +from cms.djangoapps.contentstore.views.course import _get_rerun_link_for_item +from openedx.core.lib.api.serializers import CourseKeyField + + +class UnsucceededCourseSerializerV2(serializers.Serializer): + """Serializer for unsucceeded course""" + display_name = serializers.CharField() + course_key = CourseKeyField() + org = serializers.CharField() + number = serializers.CharField() + run = serializers.CharField() + is_failed = serializers.BooleanField() + is_in_progress = serializers.BooleanField() + dismiss_link = serializers.CharField() + + +class CourseCommonSerializerV2(serializers.Serializer): + """Serializer for course common fields V2.""" + + course_key = CourseKeyField(source='id') + display_name = serializers.CharField() + lms_link = serializers.SerializerMethodField() + cms_link = serializers.SerializerMethodField() + number = serializers.CharField() + org = serializers.CharField() + rerun_link = serializers.SerializerMethodField() + run = serializers.CharField(source='id.run') + url = serializers.SerializerMethodField() + is_active = serializers.SerializerMethodField() + + def get_lms_link(self, obj): + """Get LMS link for course.""" + return get_lms_link_for_item(obj.location) + + def get_cms_link(self, obj): + """Get CMS link for course.""" + return f"//{settings.CMS_BASE}/{reverse_course_url('course_handler', obj.id)}" + + def get_rerun_link(self, obj): + """Get rerun link for course.""" + return _get_rerun_link_for_item(obj.id) + + def get_url(self, obj): + """Get URL from the course handler.""" + return reverse_course_url('course_handler', obj.id) + + def get_is_active(self, obj): + """Check if the course is active.""" + return not obj.has_ended() + +class CourseHomeTabSerializerV2(serializers.Serializer): + """Serializer for course home tab V2 with unsucceeded courses and in process course actions.""" + courses = CourseCommonSerializerV2(required=False, many=True) + in_process_course_actions = UnsucceededCourseSerializerV2(many=True, required=False, allow_null=True) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py new file mode 100644 index 000000000000..ad61cc937015 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -0,0 +1,15 @@ +"""Contenstore API v2 URLs.""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v2.views import HomePageCoursesViewV2 + +app_name = "v2" + +urlpatterns = [ + path( + "home/courses", + HomePageCoursesViewV2.as_view(), + name="courses", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py new file mode 100644 index 000000000000..bba39cf46279 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py @@ -0,0 +1 @@ +from .home import HomePageCoursesViewV2 diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py new file mode 100644 index 000000000000..2283bd917048 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -0,0 +1,91 @@ +"""HomePageCoursesViewV2 APIView for getting content available to the logged in user.""" +import edx_api_doc_tools as apidocs +from rest_framework.request import Request +from rest_framework.response import Response +from rest_framework.views import APIView +from openedx.core.lib.api.view_utils import view_auth_classes + +from cms.djangoapps.contentstore.utils import get_course_context_v2 +from cms.djangoapps.contentstore.rest_api.v2.serializers import CourseHomeTabSerializerV2 + + +@view_auth_classes(is_authenticated=True) +class HomePageCoursesViewV2(APIView): + """View for getting all courses available to the logged in user.""" + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "org", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course org", + ), + apidocs.string_parameter( + "search", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by course name, org, or number", + ), + apidocs.string_parameter( + "order", + apidocs.ParameterLocation.QUERY, + description="Query param to order by course name, org, or number", + ), + apidocs.string_parameter( + "active_only", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by active courses only", + ), + apidocs.string_parameter( + "archived_only", + apidocs.ParameterLocation.QUERY, + description="Query param to filter by archived courses only", + ), + ], + responses={ + 200: CourseHomeTabSerializerV2, + 401: "The requester is not authenticated.", + }, + ) + def get(self, request: Request): + """ + Get an object containing all courses. + + **Example Request** + + GET /api/contentstore/v2/home/courses + + **Response Values** + + If the request is successful, an HTTP 200 "OK" response is returned. + + The HTTP 200 response contains a single dict that contains keys that + are the course's home. + + **Example Response** + + ```json + { + "courses": [ + { + "course_key": "course-v1:edX+E2E-101+course", + "display_name": "E2E Test Course", + "lms_link": "//localhost:18000/courses/course-v1:edX+E2E-101+course", + "number": "E2E-101", + "org": "edX", + "rerun_link": "/course_rerun/course-v1:edX+E2E-101+course", + "run": "course", + "url": "/course/course-v1:edX+E2E-101+course" + }, + ], + "in_process_course_actions": [], + } + ``` + """ + + courses, in_process_course_actions = get_course_context_v2(request) + courses_context = { + "courses": courses, + "in_process_course_actions": in_process_course_actions, + } + serializer = CourseHomeTabSerializerV2(courses_context) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index cc2c7b4f330e..b7ef7f70d1df 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1580,6 +1580,46 @@ def format_in_process_course_view(uca): return active_courses, archived_courses, in_process_course_actions +def get_course_context_v2(request): + """Get context of the homepage course tab from the Studio Home.""" + + from cms.djangoapps.contentstore.views.course import ( + get_courses_accessible_to_user_v2, + ENABLE_GLOBAL_STAFF_OPTIMIZATION, + ) + + def format_in_process_course_view(uca): + """ + Return a dict of the data which the view requires for each unsucceeded course. + + Args: + uca: CourseRerunUIStateManager object. + """ + return { + 'display_name': uca.display_name, + 'course_key': str(uca.course_key), + 'org': uca.course_key.org, + 'number': uca.course_key.course, + 'run': uca.course_key.run, + 'is_failed': uca.state == CourseRerunUIStateManager.State.FAILED, + 'is_in_progress': uca.state == CourseRerunUIStateManager.State.IN_PROGRESS, + 'dismiss_link': reverse_course_url( + 'course_notifications_handler', + uca.course_key, + kwargs={ + 'action_state_id': uca.id, + }, + ) if uca.state == CourseRerunUIStateManager.State.FAILED else '' + } + + optimization_enabled = GlobalStaff().has_user(request.user) and ENABLE_GLOBAL_STAFF_OPTIMIZATION.is_enabled() + + org = request.GET.get('org', '') if optimization_enabled else None + courses_iter, in_process_course_actions = get_courses_accessible_to_user_v2(request, org) + in_process_course_actions = [format_in_process_course_view(uca) for uca in in_process_course_actions] + return courses_iter, in_process_course_actions + + def get_home_context(request, no_course=False): """ Utils is used to get context of course home. diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 053514123825..0a01ee869bd6 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -15,7 +15,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.contrib.auth.decorators import login_required -from django.core.exceptions import PermissionDenied, ValidationError as DjangoValidationError +from django.core.exceptions import FieldError, PermissionDenied, ValidationError as DjangoValidationError from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseNotFound from django.shortcuts import redirect from django.urls import reverse @@ -419,6 +419,42 @@ def course_filter(course_summary): return courses_summary, in_process_course_actions +def _accessible_courses_summary_iter_v2(request, org=None): + """ + List all courses available to the logged in user by iterating through all the courses. + + Args: + request: the request object + org (string): if not None, this value will limit the courses returned. An empty + string will result in no courses, and otherwise only courses with the + specified org will be returned. The default value is None. + """ + def course_filter(course_summary): + """ + Filter out inaccessible courses for the current logged in user. + + Args: + course_summary (CourseOverview): the course overview object. + """ + return has_studio_read_access(request.user, course_summary.id) + + if org is not None: + courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) + else: + courses_summary = CourseOverview.get_all_courses() + + search_query = request.GET.get('search') + order = request.GET.get('order') + active_only = request.GET.get('active_only') + archived_only = request.GET.get('archived_only') + courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) + courses_summary = get_courses_by_search_query(search_query, courses_summary) + courses_summary = get_courses_order_by(order, courses_summary) + courses_summary = filter(course_filter, courses_summary) + in_process_course_actions = get_in_process_course_actions(request) + return courses_summary, in_process_course_actions + + def _accessible_courses_iter(request): """ List all courses available to the logged in user by iterating through all the courses. @@ -512,6 +548,95 @@ def filter_ccx(course_access): return courses_list, [] +def _accessible_courses_list_from_groups_v2(request): + """ + List all courses available to the logged in user by reversing access group names. + + Args: + request: the request object. + """ + def filter_ccx(course_access): + """ CCXs cannot be edited in Studio and should not be shown in this dashboard """ + return not isinstance(course_access.course_id, CCXLocator) + + instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role() + staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() + all_courses = list(filter(filter_ccx, instructor_courses | staff_courses)) + courses_list = [] + course_keys = {} + + user_global_orgs = set() + for course_access in all_courses: + if course_access.course_id is not None: + course_keys[course_access.course_id] = course_access.course_id + elif course_access.org: + user_global_orgs.add(course_access.org) + else: + raise AccessListFallback + + if user_global_orgs: + # Getting courses from user global orgs + overviews = CourseOverview.get_all_courses(orgs=list(user_global_orgs)) + overviews_course_keys = {overview.id: overview.id for overview in overviews} + course_keys.update(overviews_course_keys) + + course_keys = list(course_keys.values()) + + if course_keys: + courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) + + search_query = request.GET.get('search') + order = request.GET.get('order') + active_only = request.GET.get('active_only') + archived_only = request.GET.get('archived_only') + courses_list = get_courses_by_status(active_only, archived_only, courses_list) + courses_list = get_courses_by_search_query(search_query, courses_list) + courses_list = get_courses_order_by(order, courses_list) + return courses_list, [] + + +def get_courses_by_status(active_only, archived_only, course_overviews): + """ + Return course overviews based on a base queryset filtered by a status. + + Args: + active_only (str): if not None, this value will limit the courses returned to active courses. + The default value is None. + archived_only (str): if not None, this value will limit the courses returned to archived courses. + The default value is None. + course_overviews (Course Overview objects): course overview queryset to be filtered. + """ + return CourseOverview.get_courses_by_status(active_only, archived_only, course_overviews) + + +def get_courses_by_search_query(search_query, course_overviews): + """Return course overviews based on a base queryset filtered by a search query. + + Args: + search_query (str): any string used to filter Course Overviews based on visible fields. + course_overviews (Course Overview objects): course overview queryset to be filtered. + """ + if not search_query: + return course_overviews + return CourseOverview.get_courses_matching_query(search_query, course_overviews=course_overviews) + + +def get_courses_order_by(order_query, course_overviews): + """Return course overviews based on a base queryset ordered by a query. + + Args: + order_query (str): any string used to order Course Overviews. + base_queryset (Course Overview objects): queryset to be ordered. + """ + if not order_query: + return course_overviews + try: + return course_overviews.order_by(order_query) + except FieldError as e: + log.exception(f"Error ordering courses by {order_query}: {e}") + return course_overviews + + @function_trace('_accessible_libraries_iter') def _accessible_libraries_iter(user, org=None): """ @@ -657,6 +782,32 @@ def get_courses_accessible_to_user(request, org=None): return courses, in_process_course_actions +@function_trace('get_courses_accessible_to_user') +def get_courses_accessible_to_user_v2(request, org=None): + """ + Try to get all courses by first reversing django groups and fallback to old method if it fails + Note: overhead of pymongo reads will increase if getting courses from django groups fails + + Args: + request: the request object + org (string): for global staff users ONLY, this value will be used to limit + the courses returned. A value of None will have no effect (all courses + returned), an empty string will result in no courses, and otherwise only courses with the + specified org will be returned. The default value is None. + """ + if GlobalStaff().has_user(request.user): + # user has global access so no need to get courses from django groups + courses, in_process_course_actions = _accessible_courses_summary_iter_v2(request, org) + else: + try: + courses, in_process_course_actions = _accessible_courses_list_from_groups_v2(request) + except AccessListFallback: + # user have some old groups or there was some error getting courses from django groups + # so fallback to iterating through all courses + courses, in_process_course_actions = _accessible_courses_summary_iter_v2(request) + return courses, in_process_course_actions + + def _process_courses_list(courses_iter, in_process_course_actions, split_archived=False): """ Iterates over the list of courses to be displayed to the user, and: diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 7684615f0fcb..35ed0c4ffcc0 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -700,6 +700,47 @@ def get_all_courses(cls, orgs=None, filter_=None, active_only=False, course_keys return course_overviews + @classmethod + def get_courses_matching_query(cls, query, course_overviews=None): + """ + Return a queryset of CourseOverview objects filtered bythe given query. + + Args: + query: required parameter that allows filtering based on the CourseOverview. + course_overviews: queryset of CourseOverview objects to filter on. If not provided, + all CourseOverview objects will be used. + """ + if not course_overviews: + course_overviews = CourseOverview.objects.all() + return course_overviews.filter( + Q(display_name__icontains=query) | + Q(org__icontains=query) | + Q(id__icontains=query) + ) + + @classmethod + def get_courses_by_status(cls, active_only, archived_only, course_overviews=None): + """ + Return a queryset of CourseOverview objects based on the given status. + + Args: + active_only: when True, only active courses will be returned. + archived_only: when True, only archived courses will be returned. + course_overviews: queryset of CourseOverview objects to filter on. If not provided, + all CourseOverview objects will be used. + """ + if not course_overviews: + course_overviews = CourseOverview.objects.all() + if active_only: + return course_overviews.filter( + Q(end__isnull=True) | Q(end__gte=datetime.now().replace(tzinfo=pytz.UTC)) + ) + if archived_only: + return course_overviews.filter( + end__lt=datetime.now().replace(tzinfo=pytz.UTC) + ) + return course_overviews + @classmethod def get_all_course_keys(cls): """ From 6d218c2658d6382dd68997ad0dba108924358abc Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 6 Feb 2024 08:57:34 -0400 Subject: [PATCH 02/38] refactor: address PR reviews --- .../contentstore/rest_api/v2/serializers/home.py | 1 + cms/djangoapps/contentstore/rest_api/v2/views/home.py | 9 ++++++++- cms/djangoapps/contentstore/views/course.py | 11 ++++++----- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index 7e66629ab16a..1db52fafff5b 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -56,6 +56,7 @@ def get_is_active(self, obj): """Check if the course is active.""" return not obj.has_ended() + class CourseHomeTabSerializerV2(serializers.Serializer): """Serializer for course home tab V2 with unsucceeded courses and in process course actions.""" courses = CourseCommonSerializerV2(required=False, many=True) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 2283bd917048..8f2759c537b8 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -53,6 +53,11 @@ def get(self, request: Request): **Example Request** GET /api/contentstore/v2/home/courses + GET /api/contentstore/v2/home/courses?org=edX + GET /api/contentstore/v2/home/courses?search=E2E + GET /api/contentstore/v2/home/courses?order=-org + GET /api/contentstore/v2/home/courses?active_only=true + GET /api/contentstore/v2/home/courses?archived_only=true **Response Values** @@ -70,11 +75,13 @@ def get(self, request: Request): "course_key": "course-v1:edX+E2E-101+course", "display_name": "E2E Test Course", "lms_link": "//localhost:18000/courses/course-v1:edX+E2E-101+course", + "cms_link": "//localhost:18010/course/course-v1:edX+E2E-101+course", "number": "E2E-101", "org": "edX", "rerun_link": "/course_rerun/course-v1:edX+E2E-101+course", "run": "course", - "url": "/course/course-v1:edX+E2E-101+course" + "url": "/course/course-v1:edX+E2E-101+course", + "is_active": true }, ], "in_process_course_actions": [], diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 0a01ee869bd6..307178c6554c 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -37,6 +37,7 @@ from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.djangoapps.models.settings.course_metadata import CourseMetadata from cms.djangoapps.models.settings.encoder import CourseSettingsEncoder +from cms.djangoapps.contentstore.api.views.utils import get_bool_param from common.djangoapps.course_action_state.managers import CourseActionStateItemNotFoundError from common.djangoapps.course_action_state.models import CourseRerunState, CourseRerunUIStateManager from common.djangoapps.edxmako.shortcuts import render_to_response @@ -445,8 +446,8 @@ def course_filter(course_summary): search_query = request.GET.get('search') order = request.GET.get('order') - active_only = request.GET.get('active_only') - archived_only = request.GET.get('archived_only') + active_only = get_bool_param(request, 'active_only', None) + archived_only = get_bool_param(request, 'archived_only', None) courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) courses_summary = get_courses_by_search_query(search_query, courses_summary) courses_summary = get_courses_order_by(order, courses_summary) @@ -587,8 +588,8 @@ def filter_ccx(course_access): search_query = request.GET.get('search') order = request.GET.get('order') - active_only = request.GET.get('active_only') - archived_only = request.GET.get('archived_only') + active_only = get_bool_param(request, 'active_only', None) + archived_only = get_bool_param(request, 'archived_only', None) courses_list = get_courses_by_status(active_only, archived_only, courses_list) courses_list = get_courses_by_search_query(search_query, courses_list) courses_list = get_courses_order_by(order, courses_list) @@ -626,7 +627,7 @@ def get_courses_order_by(order_query, course_overviews): Args: order_query (str): any string used to order Course Overviews. - base_queryset (Course Overview objects): queryset to be ordered. + course_overviews (Course Overview objects): queryset to be ordered. """ if not order_query: return course_overviews From 12cbacfff73c7ce65e758a0bf37dbff2b24b2dc9 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 6 Feb 2024 09:10:49 -0400 Subject: [PATCH 03/38] fix: add missing blank line --- cms/djangoapps/contentstore/rest_api/v2/serializers/home.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index 1db52fafff5b..d9c930bd4a69 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -59,5 +59,6 @@ def get_is_active(self, obj): class CourseHomeTabSerializerV2(serializers.Serializer): """Serializer for course home tab V2 with unsucceeded courses and in process course actions.""" + courses = CourseCommonSerializerV2(required=False, many=True) in_process_course_actions = UnsucceededCourseSerializerV2(many=True, required=False, allow_null=True) From 5471f2e6a624217af795baf5dc3231ed9fbb87f1 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 6 Feb 2024 09:29:15 -0400 Subject: [PATCH 04/38] fix: address PR quality issues --- .../contentstore/rest_api/v2/serializers/__init__.py | 2 ++ cms/djangoapps/contentstore/rest_api/v2/serializers/home.py | 1 - cms/djangoapps/contentstore/rest_api/v2/views/__init__.py | 4 +++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py index 36ec8bda5a0b..6e102bab44a1 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py @@ -1 +1,3 @@ +"""Module for v2 serializers.""" + from cms.djangoapps.contentstore.rest_api.v2.serializers.home import CourseHomeTabSerializerV2 diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index d9c930bd4a69..69c470f86363 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -4,7 +4,6 @@ from django.conf import settings from rest_framework import serializers -from cms.djangoapps.contentstore.rest_api.serializers.common import CourseCommonSerializer from cms.djangoapps.contentstore.utils import get_lms_link_for_item, reverse_course_url from cms.djangoapps.contentstore.views.course import _get_rerun_link_for_item from openedx.core.lib.api.serializers import CourseKeyField diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py index bba39cf46279..73ddde98440c 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/__init__.py @@ -1 +1,3 @@ -from .home import HomePageCoursesViewV2 +"""Module for v2 views.""" + +from cms.djangoapps.contentstore.rest_api.v2.views.home import HomePageCoursesViewV2 From 8b02daaa0c1355d7febdf3cf681ad558e9bb8df7 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 6 Feb 2024 10:04:59 -0400 Subject: [PATCH 05/38] fix: cast filter as sequence --- cms/djangoapps/contentstore/views/course.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 307178c6554c..16256375406e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -451,7 +451,7 @@ def course_filter(course_summary): courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) courses_summary = get_courses_by_search_query(search_query, courses_summary) courses_summary = get_courses_order_by(order, courses_summary) - courses_summary = filter(course_filter, courses_summary) + courses_summary = list(filter(course_filter, courses_summary)) in_process_course_actions = get_in_process_course_actions(request) return courses_summary, in_process_course_actions @@ -562,7 +562,7 @@ def filter_ccx(course_access): instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role() staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() - all_courses = list(filter(filter_ccx, instructor_courses | staff_courses)) + all_courses = filter(filter_ccx, instructor_courses | staff_courses) courses_list = [] course_keys = {} From e9bef802c86ffd344eea0abd9187eef9326a4cee Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 6 Feb 2024 13:57:43 -0400 Subject: [PATCH 06/38] fix: address PR reviews --- cms/djangoapps/contentstore/rest_api/v2/serializers/home.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index 69c470f86363..0c6a9f23cfce 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -41,7 +41,7 @@ def get_lms_link(self, obj): def get_cms_link(self, obj): """Get CMS link for course.""" - return f"//{settings.CMS_BASE}/{reverse_course_url('course_handler', obj.id)}" + return f"//{settings.CMS_BASE}{reverse_course_url('course_handler', obj.id)}" def get_rerun_link(self, obj): """Get rerun link for course.""" From eefd67ccdf7e64dbbea618b08fe88cf0b13c1e58 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 16 Feb 2024 10:12:31 -0400 Subject: [PATCH 07/38] refactor: remove list instantiation for course_filter --- cms/djangoapps/contentstore/views/course.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 16256375406e..a597475df938 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -451,7 +451,7 @@ def course_filter(course_summary): courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) courses_summary = get_courses_by_search_query(search_query, courses_summary) courses_summary = get_courses_order_by(order, courses_summary) - courses_summary = list(filter(course_filter, courses_summary)) + courses_summary = filter(course_filter, courses_summary) in_process_course_actions = get_in_process_course_actions(request) return courses_summary, in_process_course_actions From 108517fdc89262462a51a49f2727e9220f117818 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 19 Feb 2024 10:00:30 -0400 Subject: [PATCH 08/38] refactor: address PR reviews --- .../contentstore/rest_api/v2/serializers/home.py | 11 ++++++++--- cms/djangoapps/contentstore/utils.py | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index 0c6a9f23cfce..857291fe838b 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -10,7 +10,8 @@ class UnsucceededCourseSerializerV2(serializers.Serializer): - """Serializer for unsucceeded course""" + """Serializer for unsucceeded course.""" + display_name = serializers.CharField() course_key = CourseKeyField() org = serializers.CharField() @@ -52,7 +53,7 @@ def get_url(self, obj): return reverse_course_url('course_handler', obj.id) def get_is_active(self, obj): - """Check if the course is active.""" + """Get whether the course is active or not.""" return not obj.has_ended() @@ -60,4 +61,8 @@ class CourseHomeTabSerializerV2(serializers.Serializer): """Serializer for course home tab V2 with unsucceeded courses and in process course actions.""" courses = CourseCommonSerializerV2(required=False, many=True) - in_process_course_actions = UnsucceededCourseSerializerV2(many=True, required=False, allow_null=True) + in_process_course_actions = UnsucceededCourseSerializerV2( + many=True, + required=False, + allow_null=True + ) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index b7ef7f70d1df..a646ac9623f1 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1583,6 +1583,9 @@ def format_in_process_course_view(uca): def get_course_context_v2(request): """Get context of the homepage course tab from the Studio Home.""" + # Importing here to avoid circular imports: + # ImportError: cannot import name 'reverse_course_url' from partially initialized module + # 'cms.djangoapps.contentstore.utils' (most likely due to a circular import) from cms.djangoapps.contentstore.views.course import ( get_courses_accessible_to_user_v2, ENABLE_GLOBAL_STAFF_OPTIMIZATION, From 37acb54b579e97169266178b5ba26d13c2c0949d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 19 Feb 2024 13:13:36 -0400 Subject: [PATCH 09/38] refactor!: remove checking for queryset emptiness --- .../core/djangoapps/content/course_overviews/models.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 35ed0c4ffcc0..c07d9a4d4dea 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -701,7 +701,7 @@ def get_all_courses(cls, orgs=None, filter_=None, active_only=False, course_keys return course_overviews @classmethod - def get_courses_matching_query(cls, query, course_overviews=None): + def get_courses_matching_query(cls, query, course_overviews): """ Return a queryset of CourseOverview objects filtered bythe given query. @@ -710,8 +710,6 @@ def get_courses_matching_query(cls, query, course_overviews=None): course_overviews: queryset of CourseOverview objects to filter on. If not provided, all CourseOverview objects will be used. """ - if not course_overviews: - course_overviews = CourseOverview.objects.all() return course_overviews.filter( Q(display_name__icontains=query) | Q(org__icontains=query) | @@ -719,7 +717,7 @@ def get_courses_matching_query(cls, query, course_overviews=None): ) @classmethod - def get_courses_by_status(cls, active_only, archived_only, course_overviews=None): + def get_courses_by_status(cls, active_only, archived_only, course_overviews): """ Return a queryset of CourseOverview objects based on the given status. @@ -729,8 +727,6 @@ def get_courses_by_status(cls, active_only, archived_only, course_overviews=None course_overviews: queryset of CourseOverview objects to filter on. If not provided, all CourseOverview objects will be used. """ - if not course_overviews: - course_overviews = CourseOverview.objects.all() if active_only: return course_overviews.filter( Q(end__isnull=True) | Q(end__gte=datetime.now().replace(tzinfo=pytz.UTC)) From fc47dab3b8f9572430ce2d6ccae63f5d2334d70f Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 19 Feb 2024 15:20:16 -0400 Subject: [PATCH 10/38] refactor!: use existing functions for getting course objects --- cms/djangoapps/contentstore/utils.py | 4 +- cms/djangoapps/contentstore/views/course.py | 99 ++------------------- 2 files changed, 7 insertions(+), 96 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index a646ac9623f1..9d90b1c7eda3 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1587,7 +1587,7 @@ def get_course_context_v2(request): # ImportError: cannot import name 'reverse_course_url' from partially initialized module # 'cms.djangoapps.contentstore.utils' (most likely due to a circular import) from cms.djangoapps.contentstore.views.course import ( - get_courses_accessible_to_user_v2, + get_courses_accessible_to_user, ENABLE_GLOBAL_STAFF_OPTIMIZATION, ) @@ -1618,7 +1618,7 @@ def format_in_process_course_view(uca): optimization_enabled = GlobalStaff().has_user(request.user) and ENABLE_GLOBAL_STAFF_OPTIMIZATION.is_enabled() org = request.GET.get('org', '') if optimization_enabled else None - courses_iter, in_process_course_actions = get_courses_accessible_to_user_v2(request, org) + courses_iter, in_process_course_actions = get_courses_accessible_to_user(request, org) in_process_course_actions = [format_in_process_course_view(uca) for uca in in_process_course_actions] return courses_iter, in_process_course_actions diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index a597475df938..e246aa39d7dd 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -411,34 +411,6 @@ def course_filter(course_summary): return False return has_studio_read_access(request.user, course_summary.id) - if org is not None: - courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) - else: - courses_summary = modulestore().get_course_summaries() - courses_summary = filter(course_filter, courses_summary) - in_process_course_actions = get_in_process_course_actions(request) - return courses_summary, in_process_course_actions - - -def _accessible_courses_summary_iter_v2(request, org=None): - """ - List all courses available to the logged in user by iterating through all the courses. - - Args: - request: the request object - org (string): if not None, this value will limit the courses returned. An empty - string will result in no courses, and otherwise only courses with the - specified org will be returned. The default value is None. - """ - def course_filter(course_summary): - """ - Filter out inaccessible courses for the current logged in user. - - Args: - course_summary (CourseOverview): the course overview object. - """ - return has_studio_read_access(request.user, course_summary.id) - if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) else: @@ -448,11 +420,14 @@ def course_filter(course_summary): order = request.GET.get('order') active_only = get_bool_param(request, 'active_only', None) archived_only = get_bool_param(request, 'archived_only', None) + courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) courses_summary = get_courses_by_search_query(search_query, courses_summary) courses_summary = get_courses_order_by(order, courses_summary) courses_summary = filter(course_filter, courses_summary) + in_process_course_actions = get_in_process_course_actions(request) + return courses_summary, in_process_course_actions @@ -543,46 +518,6 @@ def filter_ccx(course_access): course_keys = list(course_keys.values()) - if course_keys: - courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) - - return courses_list, [] - - -def _accessible_courses_list_from_groups_v2(request): - """ - List all courses available to the logged in user by reversing access group names. - - Args: - request: the request object. - """ - def filter_ccx(course_access): - """ CCXs cannot be edited in Studio and should not be shown in this dashboard """ - return not isinstance(course_access.course_id, CCXLocator) - - instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role() - staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() - all_courses = filter(filter_ccx, instructor_courses | staff_courses) - courses_list = [] - course_keys = {} - - user_global_orgs = set() - for course_access in all_courses: - if course_access.course_id is not None: - course_keys[course_access.course_id] = course_access.course_id - elif course_access.org: - user_global_orgs.add(course_access.org) - else: - raise AccessListFallback - - if user_global_orgs: - # Getting courses from user global orgs - overviews = CourseOverview.get_all_courses(orgs=list(user_global_orgs)) - overviews_course_keys = {overview.id: overview.id for overview in overviews} - course_keys.update(overviews_course_keys) - - course_keys = list(course_keys.values()) - if course_keys: courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) @@ -590,9 +525,11 @@ def filter_ccx(course_access): order = request.GET.get('order') active_only = get_bool_param(request, 'active_only', None) archived_only = get_bool_param(request, 'archived_only', None) + courses_list = get_courses_by_status(active_only, archived_only, courses_list) courses_list = get_courses_by_search_query(search_query, courses_list) courses_list = get_courses_order_by(order, courses_list) + return courses_list, [] @@ -783,32 +720,6 @@ def get_courses_accessible_to_user(request, org=None): return courses, in_process_course_actions -@function_trace('get_courses_accessible_to_user') -def get_courses_accessible_to_user_v2(request, org=None): - """ - Try to get all courses by first reversing django groups and fallback to old method if it fails - Note: overhead of pymongo reads will increase if getting courses from django groups fails - - Args: - request: the request object - org (string): for global staff users ONLY, this value will be used to limit - the courses returned. A value of None will have no effect (all courses - returned), an empty string will result in no courses, and otherwise only courses with the - specified org will be returned. The default value is None. - """ - if GlobalStaff().has_user(request.user): - # user has global access so no need to get courses from django groups - courses, in_process_course_actions = _accessible_courses_summary_iter_v2(request, org) - else: - try: - courses, in_process_course_actions = _accessible_courses_list_from_groups_v2(request) - except AccessListFallback: - # user have some old groups or there was some error getting courses from django groups - # so fallback to iterating through all courses - courses, in_process_course_actions = _accessible_courses_summary_iter_v2(request) - return courses, in_process_course_actions - - def _process_courses_list(courses_iter, in_process_course_actions, split_archived=False): """ Iterates over the list of courses to be displayed to the user, and: From 2f3173534c01f5e0bb238099ebf3f1b0f25b590e Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 23 Feb 2024 09:51:52 -0400 Subject: [PATCH 11/38] refactor: move filter section to its own method --- cms/djangoapps/contentstore/views/course.py | 72 +++++++++++++++++---- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index e246aa39d7dd..f34ed3174fc7 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -416,20 +416,64 @@ def course_filter(course_summary): else: 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) + + return courses_summary, in_process_course_actions + +def get_query_params_if_present(request): + """ + Returns the query params from request if present. + + Arguments: + request: the request object + + Returns: + search_query (str): any string used to filter Course Overviews based on visible fields. + order (str): any string used to order Course Overviews. + active_only (str): if not None, this value will limit the courses returned to active courses. + The default value is None. + archived_only (str): if not None, this value will limit the courses returned to archived courses. + The default value is None. + """ + if not request.GET: + return None, None, None, None search_query = request.GET.get('search') order = request.GET.get('order') active_only = get_bool_param(request, 'active_only', None) archived_only = get_bool_param(request, 'archived_only', None) + return search_query, order, active_only, archived_only - courses_summary = get_courses_by_status(active_only, archived_only, courses_summary) - courses_summary = get_courses_by_search_query(search_query, courses_summary) - courses_summary = get_courses_order_by(order, courses_summary) - courses_summary = filter(course_filter, courses_summary) - in_process_course_actions = get_in_process_course_actions(request) +def get_filtered_and_ordered_courses(course_overviews, active_only, archived_only, search_query, order): + """ + Returns the filtered and ordered courses based on the query params. - return courses_summary, in_process_course_actions + Arguments: + courses_summary (Course Overview objects): course overview queryset to be filtered. + active_only (str): if not None, this value will limit the courses returned to active courses. + The default value is None. + archived_only (str): if not None, this value will limit the courses returned to archived courses. + The default value is None. + search_query (str): any string used to filter Course Overviews based on visible fields. + order (str): any string used to order Course Overviews. + Returns: + Course Overview objects: queryset filtered and ordered based on the query params. + """ + course_overviews = get_courses_by_status(active_only, archived_only, course_overviews) + course_overviews = get_courses_by_search_query(search_query, course_overviews) + course_overviews = get_courses_order_by(order, course_overviews) + return course_overviews def _accessible_courses_iter(request): """ @@ -521,14 +565,14 @@ def filter_ccx(course_access): if course_keys: courses_list = CourseOverview.get_all_courses(filter_={'id__in': course_keys}) - search_query = request.GET.get('search') - order = request.GET.get('order') - active_only = get_bool_param(request, 'active_only', None) - archived_only = get_bool_param(request, 'archived_only', None) - - courses_list = get_courses_by_status(active_only, archived_only, courses_list) - courses_list = get_courses_by_search_query(search_query, courses_list) - courses_list = get_courses_order_by(order, courses_list) + search_query, order, active_only, archived_only = get_query_params_if_present(request) + courses_list = get_filtered_and_ordered_courses( + courses_list, + active_only, + archived_only, + search_query, + order, + ) return courses_list, [] From ed21fe14610c08ff0ca1a11d5448279423e29287 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 23 Feb 2024 10:21:28 -0400 Subject: [PATCH 12/38] fix: address testing errors --- cms/djangoapps/contentstore/views/course.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index f34ed3174fc7..34d925273587 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -430,6 +430,7 @@ def course_filter(course_summary): return courses_summary, in_process_course_actions + def get_query_params_if_present(request): """ Returns the query params from request if present. @@ -475,6 +476,7 @@ def get_filtered_and_ordered_courses(course_overviews, active_only, archived_onl course_overviews = get_courses_order_by(order, course_overviews) return course_overviews + def _accessible_courses_iter(request): """ List all courses available to the logged in user by iterating through all the courses. From 4bf0a09e52ae70d1273a07dbb836bc97a39b2ea0 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 1 Mar 2024 15:57:41 -0400 Subject: [PATCH 13/38] feat: add pagination to the HomePageCoursesV2 API (#34175) --- .../contentstore/rest_api/v2/views/home.py | 56 ++++++++++++--- .../rest_api/v2/views/tests/__init__.py | 0 .../rest_api/v2/views/tests/test_home.py | 69 +++++++++++++++++++ 3 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 8f2759c537b8..4c7ee4151535 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -1,14 +1,44 @@ """HomePageCoursesViewV2 APIView for getting content available to the logged in user.""" import edx_api_doc_tools as apidocs -from rest_framework.request import Request +from collections import OrderedDict from rest_framework.response import Response +from rest_framework.request import Request from rest_framework.views import APIView +from rest_framework.pagination import PageNumberPagination + from openedx.core.lib.api.view_utils import view_auth_classes from cms.djangoapps.contentstore.utils import get_course_context_v2 from cms.djangoapps.contentstore.rest_api.v2.serializers import CourseHomeTabSerializerV2 +class HomePageCoursesPaginator(PageNumberPagination): + + def get_paginated_response(self, data): + """Return a paginated style `Response` object for the given output data.""" + return Response(OrderedDict([ + ('count', self.page.paginator.count), + ('num_pages', self.page.paginator.num_pages), + ('next', self.get_next_link()), + ('previous', self.get_previous_link()), + ('results', data), + ])) + + def paginate_queryset(self, queryset, request, view=None): + """ + Paginate a queryset if required, either returning a page object, + or `None` if pagination is not configured for this view. + + This method is a modified version of the original `paginate_queryset` method + from the `PageNumberPagination` class. The original method was modified to + handle the case where the `queryset` is a `filter` object. + """ + if isinstance(queryset, filter): + queryset = list(queryset) + + return super().paginate_queryset(queryset, request, view) + + @view_auth_classes(is_authenticated=True) class HomePageCoursesViewV2(APIView): """View for getting all courses available to the logged in user.""" @@ -40,6 +70,11 @@ class HomePageCoursesViewV2(APIView): apidocs.ParameterLocation.QUERY, description="Query param to filter by archived courses only", ), + apidocs.string_parameter( + "page", + apidocs.ParameterLocation.QUERY, + description="Query param to paginate the courses", + ), ], responses={ 200: CourseHomeTabSerializerV2, @@ -58,6 +93,7 @@ def get(self, request: Request): GET /api/contentstore/v2/home/courses?order=-org GET /api/contentstore/v2/home/courses?active_only=true GET /api/contentstore/v2/home/courses?archived_only=true + GET /api/contentstore/v2/home/courses?page=2 **Response Values** @@ -88,11 +124,15 @@ def get(self, request: Request): } ``` """ - courses, in_process_course_actions = get_course_context_v2(request) - courses_context = { - "courses": courses, - "in_process_course_actions": in_process_course_actions, - } - serializer = CourseHomeTabSerializerV2(courses_context) - return Response(serializer.data) + paginator = HomePageCoursesPaginator() + courses_page = paginator.paginate_queryset( + courses, + self.request, + view=self + ) + serializer = CourseHomeTabSerializerV2({ + 'courses': courses_page, + 'in_process_course_actions': in_process_course_actions, + }) + return paginator.get_paginated_response(serializer.data) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 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 new file mode 100644 index 000000000000..3e58b6d92491 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -0,0 +1,69 @@ +""" +Unit tests for home page view. +""" +import ddt +from django.conf import settings +from django.urls import reverse +from edx_toggles.toggles.testutils import ( + override_waffle_switch, +) +from rest_framework import status + +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.views.course import ENABLE_GLOBAL_STAFF_OPTIMIZATION +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory +from xmodule.modulestore.tests.factories import CourseFactory + + +@ddt.ddt +class HomePageCoursesViewTest(CourseTestCase): + """ + Tests for HomePageView. + """ + + def setUp(self): + super().setUp() + self.url = reverse("cms.djangoapps.contentstore:v2:courses") + + def test_home_page_response(self): + """Check successful response content""" + response = self.client.get(self.url) + course_id = str(self.course.id) + + expected_response = { + "courses": [{ + "course_key": course_id, + "display_name": self.course.display_name, + "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', + "number": self.course.number, + "org": self.course.org, + "rerun_link": f'/course_rerun/{course_id}', + "run": self.course.id.run, + "url": f'/course/{course_id}', + }], + "in_process_course_actions": [], + } + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertDictEqual(expected_response, response.data) + + @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""" + foo_course = self.store.make_course_key('foo-org', 'bar-number', 'baz-run') + test_course = CourseFactory.create( + org=foo_course.org, + number=foo_course.course, + run=foo_course.run + ) + CourseOverviewFactory.create(id=test_course.id, org='foo-org') + response = self.client.get(self.url, {"org": "foo-org"}) + self.assertEqual(len(response.data['courses']), 1) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + @override_waffle_switch(ENABLE_GLOBAL_STAFF_OPTIMIZATION, True) + def test_org_query_if_empty(self): + """Test home page with an empty org query param""" + response = self.client.get(self.url) + self.assertEqual(len(response.data['courses']), 0) + self.assertEqual(response.status_code, status.HTTP_200_OK) From b6621117725aae9171dfb1e3961afba71a65086f Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 1 Mar 2024 16:56:13 -0400 Subject: [PATCH 14/38] test: add same v1 test with adjustments for new output format --- .../rest_api/v2/views/tests/test_home.py | 75 ++++++++++++------- cms/djangoapps/contentstore/views/course.py | 1 + 2 files changed, 51 insertions(+), 25 deletions(-) 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 3e58b6d92491..fe3a35620627 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 @@ -4,6 +4,7 @@ import ddt from django.conf import settings from django.urls import reverse +from collections import OrderedDict from edx_toggles.toggles.testutils import ( override_waffle_switch, ) @@ -13,57 +14,81 @@ from cms.djangoapps.contentstore.views.course import ENABLE_GLOBAL_STAFF_OPTIMIZATION from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from xmodule.modulestore.tests.factories import CourseFactory +from cms.djangoapps.contentstore.utils import get_lms_link_for_item, reverse_course_url @ddt.ddt -class HomePageCoursesViewTest(CourseTestCase): +class HomePageCoursesViewV2Test(CourseTestCase): """ - Tests for HomePageView. + Tests for HomePageView view version 2. """ def setUp(self): super().setUp() self.url = reverse("cms.djangoapps.contentstore:v2:courses") + CourseOverviewFactory.create( + id=self.course.id, + org=self.course.org, + run=self.course.id.run, + display_name=self.course.display_name, + ) def test_home_page_response(self): - """Check successful response content""" + """Get list of courses available to the logged in user. + + Expected result: + - A list of courses available to the logged in user. + - A paginated response. + """ response = self.client.get(self.url) course_id = str(self.course.id) - expected_response = { - "courses": [{ - "course_key": course_id, - "display_name": self.course.display_name, - "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', - "number": self.course.number, - "org": self.course.org, - "rerun_link": f'/course_rerun/{course_id}', - "run": self.course.id.run, - "url": f'/course/{course_id}', - }], + expected_data = { + "courses": [ + OrderedDict([ + ("course_key", course_id), + ("display_name", self.course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f'/course_rerun/{course_id}'), + ("run", self.course.id.run), + ("url", f'/course/{course_id}'), + ("is_active", True), + ])], "in_process_course_actions": [], } + expected_response = OrderedDict([ + ('count', 1), + ('num_pages', 1), + ('next', None), + ('previous',None), + ('results', expected_data), + ]) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertDictEqual(expected_response, response.data) @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""" - foo_course = self.store.make_course_key('foo-org', 'bar-number', 'baz-run') - test_course = CourseFactory.create( - org=foo_course.org, - number=foo_course.course, - run=foo_course.run - ) - CourseOverviewFactory.create(id=test_course.id, org='foo-org') - response = self.client.get(self.url, {"org": "foo-org"}) - self.assertEqual(len(response.data['courses']), 1) + """Get list of courses when org filter passed as a query param. + + Expected result: + - A list of courses available to the logged in user for the specified org. + """ + demo_course_key = self.store.make_course_key('demo-org', 'demo-number', 'demo-run') + CourseOverviewFactory.create(id=demo_course_key, org=demo_course_key.org) + + response = self.client.get(self.url, {"org": "demo-org"}) + + self.assertEqual(len(response.data['results']['courses']), 1) self.assertEqual(response.status_code, status.HTTP_200_OK) @override_waffle_switch(ENABLE_GLOBAL_STAFF_OPTIMIZATION, True) def test_org_query_if_empty(self): """Test home page with an empty org query param""" response = self.client.get(self.url) - self.assertEqual(len(response.data['courses']), 0) + + 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 34d925273587..4432fe05a87d 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -411,6 +411,7 @@ def course_filter(course_summary): return False return has_studio_read_access(request.user, course_summary.id) + if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) else: From 2b54de756ec6a1b3a5b9d80e6c98cda864045e12 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 7 Mar 2024 14:32:13 -0400 Subject: [PATCH 15/38] test: add test suite for home page courses v2 --- .../rest_api/v2/views/tests/test_home.py | 159 +++++++++++++++--- 1 file changed, 137 insertions(+), 22 deletions(-) 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 fe3a35620627..5fa3f9ceec29 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,7 +1,9 @@ """ Unit tests for home page view. """ +from datetime import datetime, timedelta import ddt +import pytz from django.conf import settings from django.urls import reverse from collections import OrderedDict @@ -13,8 +15,7 @@ from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.views.course import ENABLE_GLOBAL_STAFF_OPTIMIZATION from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory -from xmodule.modulestore.tests.factories import CourseFactory -from cms.djangoapps.contentstore.utils import get_lms_link_for_item, reverse_course_url +from cms.djangoapps.contentstore.utils import reverse_course_url @ddt.ddt @@ -26,41 +27,60 @@ class HomePageCoursesViewV2Test(CourseTestCase): def setUp(self): super().setUp() self.url = reverse("cms.djangoapps.contentstore:v2:courses") - CourseOverviewFactory.create( + self.active_course = CourseOverviewFactory.create( id=self.course.id, org=self.course.org, - run=self.course.id.run, display_name=self.course.display_name, ) + archived_course_key = self.store.make_course_key('demo-org', 'demo-number', 'demo-run') + self.archived_course = CourseOverviewFactory.create( + display_name="Demo Course (Sample)", + id=archived_course_key, + org=archived_course_key.org, + end=(datetime.now() - timedelta(days=365)).replace(tzinfo=pytz.UTC), + ) def test_home_page_response(self): """Get list of courses available to the logged in user. Expected result: - - A list of courses available to the logged in user. - A paginated response. + - A list of courses available to the logged in user. """ response = self.client.get(self.url) course_id = str(self.course.id) expected_data = { "courses": [ - OrderedDict([ - ("course_key", course_id), - ("display_name", self.course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), - ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), - ("number", self.course.number), - ("org", self.course.org), - ("rerun_link", f'/course_rerun/{course_id}'), - ("run", self.course.id.run), - ("url", f'/course/{course_id}'), - ("is_active", True), - ])], + OrderedDict([ + ("course_key", course_id), + ("display_name", self.course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f'/course_rerun/{course_id}'), + ("run", self.course.id.run), + ("url", f'/course/{course_id}'), + ("is_active", True), + ]), + OrderedDict([ + ("course_key", str(self.archived_course.id)), + ("display_name", self.archived_course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), + ("number", self.archived_course.number), + ("org", self.archived_course.org), + ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), + ("run", self.archived_course.id.run), + ("url", f'/course/{str(self.archived_course.id)}'), + ("is_active", False), + ]), + ], "in_process_course_actions": [], } expected_response = OrderedDict([ - ('count', 1), + ('count', 2), ('num_pages', 1), ('next', None), ('previous',None), @@ -77,9 +97,6 @@ def test_org_query_if_passed(self): Expected result: - A list of courses available to the logged in user for the specified org. """ - demo_course_key = self.store.make_course_key('demo-org', 'demo-number', 'demo-run') - CourseOverviewFactory.create(id=demo_course_key, org=demo_course_key.org) - response = self.client.get(self.url, {"org": "demo-org"}) self.assertEqual(len(response.data['results']['courses']), 1) @@ -87,8 +104,106 @@ def test_org_query_if_passed(self): @override_waffle_switch(ENABLE_GLOBAL_STAFF_OPTIMIZATION, True) def test_org_query_if_empty(self): - """Test home page with an empty org query param""" + """Get home page with an empty org query param. + + Expected result: + - An empty list of courses available to the logged in user. + """ response = self.client.get(self.url) self.assertEqual(len(response.data['results']['courses']), 0) self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_active_only_query_if_passed(self): + """Get list of active courses only. + + Expected result: + - A list of active courses available to the logged in user. + """ + response = self.client.get(self.url, {"active_only": "true"}) + + self.assertEqual(len(response.data["results"]["courses"]), 1) + self.assertEqual(response.data["results"]["courses"], [OrderedDict([ + ("course_key", str(self.course.id)), + ("display_name", self.course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.course.id)}/jump_to/{self.course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f'/course_rerun/{str(self.course.id)}'), + ("run", self.course.id.run), + ("url", f'/course/{str(self.course.id)}'), + ("is_active", True), + ])]) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_archived_only_query_if_passed(self): + """Get list of archived courses only. + + Expected result: + - A list of archived courses available to the logged in user. + """ + response = self.client.get(self.url, {"archived_only": "true"}) + + self.assertEqual(len(response.data["results"]["courses"]), 1) + self.assertEqual(response.data["results"]["courses"], [OrderedDict([ + ("course_key", str(self.archived_course.id)), + ("display_name", self.archived_course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), + ("number", self.archived_course.number), + ("org", self.archived_course.org), + ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), + ("run", self.archived_course.id.run), + ("url", f'/course/{str(self.archived_course.id)}'), + ("is_active", False), + ])]) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_search_query_if_passed(self): + """Get list of courses when search filter passed as a query param. + + Expected result: + - A list of courses (active or inactive) available to the logged in user for the specified search. + """ + response = self.client.get(self.url, {"search": "sample"}) + + self.assertEqual(len(response.data["results"]["courses"]), 1) + self.assertEqual(response.data["results"]["courses"], [OrderedDict([ + ("course_key", str(self.archived_course.id)), + ("display_name", self.archived_course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), + ("number", self.archived_course.number), + ("org", self.archived_course.org), + ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), + ("run", self.archived_course.id.run), + ("url", f'/course/{str(self.archived_course.id)}'), + ("is_active", False), + ])]) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + @ddt.data(("org", "demo-org"), ("-org", "org.4")) + @ddt.unpack + def test_order_query_if_passed(self, order_query, expected_first_org): + """Get list of courses when order filter passed as a query param. + + Expected result: + - A list of courses (active or inactive) available to the logged in user for the specified order. + """ + response = self.client.get(self.url, {"order": order_query}) + + self.assertEqual(len(response.data["results"]["courses"]), 2) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["results"]["courses"][0]["org"], expected_first_org) + + def test_page_query_if_passed(self): + """Get list of courses when page filter passed as a query param. + + Expected result: + - A list of courses (active or inactive) available to the logged in user for the specified page. + """ + response = self.client.get(self.url, {"page": 1}) + + self.assertEqual(response.data["count"], 2) + self.assertEqual(response.status_code, status.HTTP_200_OK) From a298b3ffdd7c1216d57234240c4710416d49d3a5 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Mar 2024 12:16:25 -0400 Subject: [PATCH 16/38] fix: address test failures --- .../rest_api/v1/views/tests/test_home.py | 28 +++++++++++------ .../contentstore/rest_api/v2/views/home.py | 1 + .../rest_api/v2/views/tests/test_home.py | 31 +++++++++++++------ cms/djangoapps/contentstore/views/course.py | 5 +-- 4 files changed, 43 insertions(+), 22 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 5279af0b1297..8a502d3934e2 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,6 +2,7 @@ Unit tests for home page view. """ import ddt +from collections import OrderedDict from django.conf import settings from django.urls import reverse from edx_toggles.toggles.testutils import ( @@ -83,6 +84,11 @@ class HomePageCoursesViewTest(CourseTestCase): def setUp(self): super().setUp() self.url = reverse("cms.djangoapps.contentstore:v1:courses") + CourseOverviewFactory.create( + id=self.course.id, + org=self.course.org, + display_name=self.course.display_name, + ) def test_home_page_response(self): """Check successful response content""" @@ -91,16 +97,18 @@ def test_home_page_response(self): expected_response = { "archived_courses": [], - "courses": [{ - "course_key": course_id, - "display_name": self.course.display_name, - "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', - "number": self.course.number, - "org": self.course.org, - "rerun_link": f'/course_rerun/{course_id}', - "run": self.course.id.run, - "url": f'/course/{course_id}', - }], + "courses": [ + OrderedDict([ + ("course_key", course_id), + ("display_name", self.course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f'/course_rerun/{course_id}'), + ("run", self.course.id.run), + ("url", f'/course/{course_id}'), + ]), + ], "in_process_course_actions": [], } diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 4c7ee4151535..21ca90cd8de1 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -13,6 +13,7 @@ class HomePageCoursesPaginator(PageNumberPagination): + """Custom paginator for the home page courses view version 2.""" def get_paginated_response(self, data): """Return a paginated style `Response` object for the given output data.""" 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 5fa3f9ceec29..a92167987fe2 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 @@ -49,6 +49,7 @@ def test_home_page_response(self): """ response = self.client.get(self.url) course_id = str(self.course.id) + archived_course_id = str(self.archived_course.id) expected_data = { "courses": [ @@ -67,8 +68,14 @@ def test_home_page_response(self): OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), - ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' + ), + ( + "cms_link", + f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}', + ), ("number", self.archived_course.number), ("org", self.archived_course.org), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), @@ -83,7 +90,7 @@ def test_home_page_response(self): ('count', 2), ('num_pages', 1), ('next', None), - ('previous',None), + ('previous', None), ('results', expected_data), ]) @@ -149,7 +156,10 @@ def test_archived_only_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -172,7 +182,10 @@ def test_search_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -183,19 +196,17 @@ def test_search_query_if_passed(self): ])]) self.assertEqual(response.status_code, status.HTTP_200_OK) - @ddt.data(("org", "demo-org"), ("-org", "org.4")) - @ddt.unpack - def test_order_query_if_passed(self, order_query, expected_first_org): + def test_order_query_if_passed(self): """Get list of courses when order filter passed as a query param. Expected result: - A list of courses (active or inactive) available to the logged in user for the specified order. """ - response = self.client.get(self.url, {"order": order_query}) + response = self.client.get(self.url, {"order": "org"}) self.assertEqual(len(response.data["results"]["courses"]), 2) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data["results"]["courses"][0]["org"], expected_first_org) + self.assertEqual(response.data["results"]["courses"][0]["org"], "demo-org") def test_page_query_if_passed(self): """Get list of courses when page filter passed as a query param. diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4432fe05a87d..4d9160127722 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -415,7 +415,7 @@ def course_filter(course_summary): if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) else: - courses_summary = CourseOverview.get_all_courses() + courses_summary = courses_summary = modulestore().get_course_summaries() search_query, order, active_only, archived_only = get_query_params_if_present(request) courses_summary = get_filtered_and_ordered_courses( @@ -447,7 +447,8 @@ def get_query_params_if_present(request): archived_only (str): if not None, this value will limit the courses returned to archived courses. The default value is None. """ - if not request.GET: + allowed_query_params = ['search', 'order', 'active_only', 'archived_only'] + if not any(param in request.GET for param in allowed_query_params): return None, None, None, None search_query = request.GET.get('search') order = request.GET.get('order') From 06b6bf4a37f2ffffafd58da3134acde77094b2e1 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Mar 2024 13:00:01 -0400 Subject: [PATCH 17/38] fix: go back to course overview queryset filtering --- cms/djangoapps/contentstore/views/course.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4d9160127722..0a4dff688c63 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -415,7 +415,7 @@ def course_filter(course_summary): if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) else: - courses_summary = courses_summary = modulestore().get_course_summaries() + 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( From f295535770e2241d593e90bc460acb568c3a0a32 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Mar 2024 15:08:00 -0400 Subject: [PATCH 18/38] fix: address test failures --- .../rest_api/v1/views/tests/test_home.py | 1 + .../contentstore/tests/test_course_listing.py | 4 ++-- .../contentstore/views/tests/test_course_index.py | 13 +++++++++---- 3 files changed, 12 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 8a502d3934e2..f7ab53cd0276 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 @@ -88,6 +88,7 @@ def setUp(self): id=self.course.id, org=self.course.org, display_name=self.course.display_name, + display_number_with_default=self.course.number, ) def test_home_page_response(self): diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 66c16dc6dd8c..379bbe347b51 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -183,7 +183,7 @@ def test_staff_course_listing(self): self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) # Verify fetched accessible courses list is a list of CourseSummery instances - self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list_by_staff)) + self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_list_by_staff)) # Now count the db queries for staff with check_mongo_calls(2): @@ -207,7 +207,7 @@ def test_get_course_list_with_invalid_course_location(self): # Verify fetched accessible courses list is a list of CourseSummery instances and only one course # is returned - self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_summary_list)) + 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 diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index b30f8c95a631..5184dd6c977d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -59,6 +59,11 @@ def setUp(self): number='test-2.3_course', display_name='dotted.course.name-2', ) + CourseOverviewFactory.create( + id=self.odd_course.id, + org=self.odd_course.org, + display_name=self.odd_course.display_name, + ) def check_courses_on_index(self, authed_client, expected_course_tab_len): """ @@ -425,10 +430,10 @@ def check_index_page(self, separate_archived_courses, org): (True, 'staff', None, 0, 21), (False, 'staff', None, 0, 21), # Base user has global staff access - (True, 'user', ORG, 2, 21), - (False, 'user', ORG, 2, 21), - (True, 'user', None, 2, 21), - (False, 'user', None, 2, 21), + (True, 'user', ORG, 0, 21), + (False, 'user', ORG, 0, 21), + (True, 'user', None, 0, 21), + (False, 'user', None, 0, 21), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): From 70c92e219798913e0db13a496fe1570c9904191e Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Mar 2024 15:30:59 -0400 Subject: [PATCH 19/38] fix: address test failures --- cms/djangoapps/contentstore/tests/test_course_listing.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 379bbe347b51..6d2cfe6f3f1a 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -35,7 +35,6 @@ 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 @@ -185,10 +184,6 @@ def test_staff_course_listing(self): # Verify fetched accessible courses list is a list of CourseSummery instances self.assertTrue(all(isinstance(course, CourseOverview) 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)) - def test_get_course_list_with_invalid_course_location(self): """ Test getting courses with invalid course location (course deleted from modulestore). From a58b00a90e2ff8a8419945be08017608e8422287 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Mar 2024 15:49:22 -0400 Subject: [PATCH 20/38] fix: address test failures --- cms/djangoapps/contentstore/tests/test_course_listing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 6d2cfe6f3f1a..1fd5566faf4a 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -37,7 +37,7 @@ from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES 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 From d7f2f2f91af61519e07ff09f0e83f679e4566f94 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 21:09:03 -0400 Subject: [PATCH 21/38] refactor: read from mysql when homepage courses API is on --- cms/djangoapps/contentstore/views/course.py | 25 ++++++++++++--------- cms/envs/common.py | 11 +++++++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 0a4dff688c63..e78fae8b51fc 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -412,19 +412,24 @@ def course_filter(course_summary): return has_studio_read_access(request.user, course_summary.id) + enable_home_page_v2_api = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_V2_API"] + if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) - else: + elif enable_home_page_v2_api: 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, - ) + else: + courses_summary = modulestore().get_course_summaries() + + if enable_home_page_v2_api: + 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/envs/common.py b/cms/envs/common.py index 15e1c39d1951..bfe18153adeb 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -561,6 +561,17 @@ # .. toggle_creation_date: 2024-02-29 # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/33952 'ENABLE_HIDE_FROM_TOC_UI': False, + + # .. toggle_name: FEATURES['ENABLE_HOME_PAGE_COURSE_V2_API'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. 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_target_removal_date: None + # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/34173 + 'ENABLE_HOME_PAGE_COURSE_V2_API': False, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE From fbdeb85d61bf8f1bf83c8c79133e519f0d13321f Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 21:47:18 -0400 Subject: [PATCH 22/38] Revert "fix: address test failures" This reverts commit 94a9e671dfb360eca1d9dedf6e4718ac617bc340. --- .../rest_api/v1/views/tests/test_home.py | 1 - .../contentstore/tests/test_course_listing.py | 9 +++++++-- .../contentstore/views/tests/test_course_index.py | 13 ++++--------- 3 files changed, 11 insertions(+), 12 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 f7ab53cd0276..8a502d3934e2 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 @@ -88,7 +88,6 @@ def setUp(self): id=self.course.id, org=self.course.org, display_name=self.course.display_name, - display_number_with_default=self.course.number, ) def test_home_page_response(self): diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 1fd5566faf4a..dcc4c5da5794 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -35,6 +35,7 @@ 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 # lint-amnesty, pylint: disable=wrong-import-order @@ -182,7 +183,11 @@ def test_staff_course_listing(self): self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) # Verify fetched accessible courses list is a list of CourseSummery instances - self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_list_by_staff)) + 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)) def test_get_course_list_with_invalid_course_location(self): """ @@ -202,7 +207,7 @@ def test_get_course_list_with_invalid_course_location(self): # Verify fetched accessible courses list is a list of CourseSummery instances and only one course # is returned - self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_summary_list)) + self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_summary_list)) self.assertEqual(len(courses_summary_list), 1) # get courses by reversing group name formats diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 5184dd6c977d..b30f8c95a631 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -59,11 +59,6 @@ def setUp(self): number='test-2.3_course', display_name='dotted.course.name-2', ) - CourseOverviewFactory.create( - id=self.odd_course.id, - org=self.odd_course.org, - display_name=self.odd_course.display_name, - ) def check_courses_on_index(self, authed_client, expected_course_tab_len): """ @@ -430,10 +425,10 @@ def check_index_page(self, separate_archived_courses, org): (True, 'staff', None, 0, 21), (False, 'staff', None, 0, 21), # Base user has global staff access - (True, 'user', ORG, 0, 21), - (False, 'user', ORG, 0, 21), - (True, 'user', None, 0, 21), - (False, 'user', None, 0, 21), + (True, 'user', ORG, 2, 21), + (False, 'user', ORG, 2, 21), + (True, 'user', None, 2, 21), + (False, 'user', None, 2, 21), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): From 8b306dac7e47d5f369d4f1ee4b5d200c954674b1 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 21:48:40 -0400 Subject: [PATCH 23/38] Revert "fix: address test failures" This reverts commit 5d53a1e6633484ae435a124da0679420b23fa47a. --- .../rest_api/v1/views/tests/test_home.py | 28 ++++++----------- .../contentstore/rest_api/v2/views/home.py | 1 - .../rest_api/v2/views/tests/test_home.py | 31 ++++++------------- 3 files changed, 20 insertions(+), 40 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 8a502d3934e2..5279af0b1297 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,6 @@ Unit tests for home page view. """ import ddt -from collections import OrderedDict from django.conf import settings from django.urls import reverse from edx_toggles.toggles.testutils import ( @@ -84,11 +83,6 @@ class HomePageCoursesViewTest(CourseTestCase): def setUp(self): super().setUp() self.url = reverse("cms.djangoapps.contentstore:v1:courses") - CourseOverviewFactory.create( - id=self.course.id, - org=self.course.org, - display_name=self.course.display_name, - ) def test_home_page_response(self): """Check successful response content""" @@ -97,18 +91,16 @@ def test_home_page_response(self): expected_response = { "archived_courses": [], - "courses": [ - OrderedDict([ - ("course_key", course_id), - ("display_name", self.course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), - ("number", self.course.number), - ("org", self.course.org), - ("rerun_link", f'/course_rerun/{course_id}'), - ("run", self.course.id.run), - ("url", f'/course/{course_id}'), - ]), - ], + "courses": [{ + "course_key": course_id, + "display_name": self.course.display_name, + "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', + "number": self.course.number, + "org": self.course.org, + "rerun_link": f'/course_rerun/{course_id}', + "run": self.course.id.run, + "url": f'/course/{course_id}', + }], "in_process_course_actions": [], } diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 21ca90cd8de1..4c7ee4151535 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -13,7 +13,6 @@ class HomePageCoursesPaginator(PageNumberPagination): - """Custom paginator for the home page courses view version 2.""" def get_paginated_response(self, data): """Return a paginated style `Response` object for the given output data.""" 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 a92167987fe2..5fa3f9ceec29 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 @@ -49,7 +49,6 @@ def test_home_page_response(self): """ response = self.client.get(self.url) course_id = str(self.course.id) - archived_course_id = str(self.archived_course.id) expected_data = { "courses": [ @@ -68,14 +67,8 @@ def test_home_page_response(self): OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ( - "lms_link", - f'//{settings.LMS_BASE}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' - ), - ( - "cms_link", - f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}', - ), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), @@ -90,7 +83,7 @@ def test_home_page_response(self): ('count', 2), ('num_pages', 1), ('next', None), - ('previous', None), + ('previous',None), ('results', expected_data), ]) @@ -156,10 +149,7 @@ def test_archived_only_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ( - "lms_link", - f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', - ), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -182,10 +172,7 @@ def test_search_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ( - "lms_link", - f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', - ), + ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -196,17 +183,19 @@ def test_search_query_if_passed(self): ])]) self.assertEqual(response.status_code, status.HTTP_200_OK) - def test_order_query_if_passed(self): + @ddt.data(("org", "demo-org"), ("-org", "org.4")) + @ddt.unpack + def test_order_query_if_passed(self, order_query, expected_first_org): """Get list of courses when order filter passed as a query param. Expected result: - A list of courses (active or inactive) available to the logged in user for the specified order. """ - response = self.client.get(self.url, {"order": "org"}) + response = self.client.get(self.url, {"order": order_query}) self.assertEqual(len(response.data["results"]["courses"]), 2) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data["results"]["courses"][0]["org"], "demo-org") + self.assertEqual(response.data["results"]["courses"][0]["org"], expected_first_org) def test_page_query_if_passed(self): """Get list of courses when page filter passed as a query param. From b5a5f3607d286d8aef05cc597b0cb69cbe738935 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 21:49:57 -0400 Subject: [PATCH 24/38] fix: address test failures --- .../contentstore/rest_api/v2/views/home.py | 1 + .../rest_api/v2/views/tests/test_home.py | 31 +++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 4c7ee4151535..21ca90cd8de1 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -13,6 +13,7 @@ class HomePageCoursesPaginator(PageNumberPagination): + """Custom paginator for the home page courses view version 2.""" def get_paginated_response(self, data): """Return a paginated style `Response` object for the given output data.""" 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 5fa3f9ceec29..a92167987fe2 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 @@ -49,6 +49,7 @@ def test_home_page_response(self): """ response = self.client.get(self.url) course_id = str(self.course.id) + archived_course_id = str(self.archived_course.id) expected_data = { "courses": [ @@ -67,8 +68,14 @@ def test_home_page_response(self): OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), - ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' + ), + ( + "cms_link", + f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}', + ), ("number", self.archived_course.number), ("org", self.archived_course.org), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), @@ -83,7 +90,7 @@ def test_home_page_response(self): ('count', 2), ('num_pages', 1), ('next', None), - ('previous',None), + ('previous', None), ('results', expected_data), ]) @@ -149,7 +156,10 @@ def test_archived_only_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -172,7 +182,10 @@ def test_search_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.archived_course.id)), ("display_name", self.archived_course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}'), + ( + "lms_link", + f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), ("org", self.archived_course.org), @@ -183,19 +196,17 @@ def test_search_query_if_passed(self): ])]) self.assertEqual(response.status_code, status.HTTP_200_OK) - @ddt.data(("org", "demo-org"), ("-org", "org.4")) - @ddt.unpack - def test_order_query_if_passed(self, order_query, expected_first_org): + def test_order_query_if_passed(self): """Get list of courses when order filter passed as a query param. Expected result: - A list of courses (active or inactive) available to the logged in user for the specified order. """ - response = self.client.get(self.url, {"order": order_query}) + response = self.client.get(self.url, {"order": "org"}) self.assertEqual(len(response.data["results"]["courses"]), 2) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data["results"]["courses"][0]["org"], expected_first_org) + self.assertEqual(response.data["results"]["courses"][0]["org"], "demo-org") def test_page_query_if_passed(self): """Get list of courses when page filter passed as a query param. From 4aeee4e45cc9bf700fa050c917cd47387a6e1431 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 21:53:08 -0400 Subject: [PATCH 25/38] fix: import missing check_mongo_calls --- cms/djangoapps/contentstore/tests/test_course_listing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index dcc4c5da5794..66c16dc6dd8c 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -38,7 +38,7 @@ 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 # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order TOTAL_COURSES_COUNT = 10 USER_COURSES_COUNT = 1 From 46d9feb372247597f215a36b07e25a5503168174 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 22:36:00 -0400 Subject: [PATCH 26/38] refactor: modify tests to match latest changes in API helpers --- .../rest_api/v2/views/tests/test_home.py | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-) 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 a92167987fe2..fbe0ab3920f7 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,23 +1,28 @@ """ 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 collections import OrderedDict -from edx_toggles.toggles.testutils import ( - override_waffle_switch, -) +from edx_toggles.toggles.testutils import override_waffle_switch from rest_framework import status from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.utils import reverse_course_url from cms.djangoapps.contentstore.views.course import ENABLE_GLOBAL_STAFF_OPTIMIZATION from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory -from cms.djangoapps.contentstore.utils import reverse_course_url + +FEATURES_WITH_HOME_PAGE_COURSE_V2_API = settings.FEATURES.copy() +FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_V2_API'] = True +@override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API) @ddt.ddt class HomePageCoursesViewV2Test(CourseTestCase): """ @@ -26,7 +31,8 @@ class HomePageCoursesViewV2Test(CourseTestCase): def setUp(self): super().setUp() - self.url = reverse("cms.djangoapps.contentstore:v2:courses") + self.api_v2_url = reverse("cms.djangoapps.contentstore:v2:courses") + self.api_v1_url = reverse("cms.djangoapps.contentstore:v1:courses") self.active_course = CourseOverviewFactory.create( id=self.course.id, org=self.course.org, @@ -47,7 +53,7 @@ def test_home_page_response(self): - A paginated response. - A list of courses available to the logged in user. """ - response = self.client.get(self.url) + response = self.client.get(self.api_v2_url) course_id = str(self.course.id) archived_course_id = str(self.archived_course.id) @@ -104,7 +110,7 @@ def test_org_query_if_passed(self): Expected result: - A list of courses available to the logged in user for the specified org. """ - response = self.client.get(self.url, {"org": "demo-org"}) + response = self.client.get(self.api_v2_url, {"org": "demo-org"}) self.assertEqual(len(response.data['results']['courses']), 1) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -116,7 +122,7 @@ def test_org_query_if_empty(self): Expected result: - An empty list of courses available to the logged in user. """ - response = self.client.get(self.url) + response = self.client.get(self.api_v2_url) self.assertEqual(len(response.data['results']['courses']), 0) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -127,7 +133,7 @@ def test_active_only_query_if_passed(self): Expected result: - A list of active courses available to the logged in user. """ - response = self.client.get(self.url, {"active_only": "true"}) + response = self.client.get(self.api_v2_url, {"active_only": "true"}) self.assertEqual(len(response.data["results"]["courses"]), 1) self.assertEqual(response.data["results"]["courses"], [OrderedDict([ @@ -150,7 +156,7 @@ def test_archived_only_query_if_passed(self): Expected result: - A list of archived courses available to the logged in user. """ - response = self.client.get(self.url, {"archived_only": "true"}) + response = self.client.get(self.api_v2_url, {"archived_only": "true"}) self.assertEqual(len(response.data["results"]["courses"]), 1) self.assertEqual(response.data["results"]["courses"], [OrderedDict([ @@ -176,7 +182,7 @@ def test_search_query_if_passed(self): Expected result: - A list of courses (active or inactive) available to the logged in user for the specified search. """ - response = self.client.get(self.url, {"search": "sample"}) + response = self.client.get(self.api_v2_url, {"search": "sample"}) self.assertEqual(len(response.data["results"]["courses"]), 1) self.assertEqual(response.data["results"]["courses"], [OrderedDict([ @@ -202,7 +208,7 @@ def test_order_query_if_passed(self): Expected result: - A list of courses (active or inactive) available to the logged in user for the specified order. """ - response = self.client.get(self.url, {"order": "org"}) + response = self.client.get(self.api_v2_url, {"order": "org"}) self.assertEqual(len(response.data["results"]["courses"]), 2) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -214,7 +220,22 @@ def test_page_query_if_passed(self): Expected result: - A list of courses (active or inactive) available to the logged in user for the specified page. """ - response = self.client.get(self.url, {"page": 1}) + response = self.client.get(self.api_v2_url, {"page": 1}) 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_V2_API': 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() From 8fbd800b22062c70d727c7a2bd3643e179ec3886 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 22:37:52 -0400 Subject: [PATCH 27/38] docs: add in-line comment explaining why read from mysql --- cms/djangoapps/contentstore/views/course.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index e78fae8b51fc..43d8faa9fa7a 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -417,6 +417,7 @@ def course_filter(course_summary): if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) elif enable_home_page_v2_api: + # 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() From a9d13d50e83c58b06d050091bd1fc47e6216d8fd Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 23:08:56 -0400 Subject: [PATCH 28/38] refactor: register URL if and only if homepage course API v2 is enabled --- .../contentstore/rest_api/v2/urls.py | 19 +++++++++++-------- .../rest_api/v2/views/tests/test_home.py | 4 ++-- cms/djangoapps/contentstore/views/course.py | 2 +- cms/envs/common.py | 4 ++-- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index ad61cc937015..7ffcd70a212b 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -1,15 +1,18 @@ """Contenstore API v2 URLs.""" - +from django.conf import settings from django.urls import path from cms.djangoapps.contentstore.rest_api.v2.views import HomePageCoursesViewV2 app_name = "v2" -urlpatterns = [ - path( - "home/courses", - HomePageCoursesViewV2.as_view(), - name="courses", - ), -] +if settings.FEATURES.get('ENABLE_HOME_PAGE_COURSE_API_V2', False): + urlpatterns = [ + path( + "home/courses", + HomePageCoursesViewV2.as_view(), + name="courses", + ), + ] +else: + urlpatterns = [] 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 fbe0ab3920f7..c0ffa50903cd 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 @@ -19,7 +19,7 @@ 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_V2_API'] = True +FEATURES_WITH_HOME_PAGE_COURSE_V2_API['ENABLE_HOME_PAGE_COURSE_API_V2'] = True @override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API) @@ -233,7 +233,7 @@ def test_api_v2_is_disabled(self, mock_modulestore, mock_course_overview): Expected result: - Courses are read from the modulestore. """ - with override_settings(FEATURES={'ENABLE_HOME_PAGE_COURSE_V2_API': False}): + 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) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 43d8faa9fa7a..37c843d706fd 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -412,7 +412,7 @@ def course_filter(course_summary): return has_studio_read_access(request.user, course_summary.id) - enable_home_page_v2_api = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_V2_API"] + enable_home_page_v2_api = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_API_V2"] if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) diff --git a/cms/envs/common.py b/cms/envs/common.py index bfe18153adeb..9ec6dee81c71 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -562,7 +562,7 @@ # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/33952 'ENABLE_HIDE_FROM_TOC_UI': False, - # .. toggle_name: FEATURES['ENABLE_HOME_PAGE_COURSE_V2_API'] + # .. toggle_name: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2'] # .. toggle_implementation: DjangoSetting # .. toggle_default: False # .. toggle_description: Enables the new home page course v2 API, which is a new version of the home page course @@ -571,7 +571,7 @@ # .. toggle_creation_date: 2024-03-14 # .. toggle_target_removal_date: None # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/34173 - 'ENABLE_HOME_PAGE_COURSE_V2_API': False, + 'ENABLE_HOME_PAGE_COURSE_API_V2': False, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE From 73a95865d60efdf0d923edcfc115cab970019a6f Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 23:14:27 -0400 Subject: [PATCH 29/38] fix: correct variable name according feature toggle --- cms/djangoapps/contentstore/views/course.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 37c843d706fd..efb5b58bde3f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -412,17 +412,17 @@ def course_filter(course_summary): return has_studio_read_access(request.user, course_summary.id) - enable_home_page_v2_api = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_API_V2"] + enable_home_page_api_v2 = settings.FEATURES["ENABLE_HOME_PAGE_COURSE_API_V2"] if org is not None: courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org]) - elif enable_home_page_v2_api: + elif 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_v2_api: + 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, From 8e6b1f5e7d42634783056c91700a494b95fcfa0b Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 14 Mar 2024 23:35:41 -0400 Subject: [PATCH 30/38] refactor: return not found when feature not enabled --- .../contentstore/rest_api/v2/urls.py | 19 ++++++++----------- .../contentstore/rest_api/v2/views/home.py | 8 ++++++++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/urls.py b/cms/djangoapps/contentstore/rest_api/v2/urls.py index 7ffcd70a212b..ad61cc937015 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/urls.py @@ -1,18 +1,15 @@ """Contenstore API v2 URLs.""" -from django.conf import settings + from django.urls import path from cms.djangoapps.contentstore.rest_api.v2.views import HomePageCoursesViewV2 app_name = "v2" -if settings.FEATURES.get('ENABLE_HOME_PAGE_COURSE_API_V2', False): - urlpatterns = [ - path( - "home/courses", - HomePageCoursesViewV2.as_view(), - name="courses", - ), - ] -else: - urlpatterns = [] +urlpatterns = [ + path( + "home/courses", + HomePageCoursesViewV2.as_view(), + name="courses", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/home.py b/cms/djangoapps/contentstore/rest_api/v2/views/home.py index 21ca90cd8de1..c32741510157 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/home.py @@ -1,6 +1,8 @@ """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 @@ -124,7 +126,13 @@ 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( From fb848f9547c7cc043dbbbdeefa758ad66f602330 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 15 Mar 2024 08:15:57 -0400 Subject: [PATCH 31/38] docs: update method docstrings --- openedx/core/djangoapps/content/course_overviews/models.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index c07d9a4d4dea..10a56f0868fb 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -707,8 +707,7 @@ def get_courses_matching_query(cls, query, course_overviews): Args: query: required parameter that allows filtering based on the CourseOverview. - course_overviews: queryset of CourseOverview objects to filter on. If not provided, - all CourseOverview objects will be used. + course_overviews: queryset of CourseOverview objects to filter on. """ return course_overviews.filter( Q(display_name__icontains=query) | @@ -724,8 +723,7 @@ def get_courses_by_status(cls, active_only, archived_only, course_overviews): Args: active_only: when True, only active courses will be returned. archived_only: when True, only archived courses will be returned. - course_overviews: queryset of CourseOverview objects to filter on. If not provided, - all CourseOverview objects will be used. + course_overviews: queryset of CourseOverview objects to filter on. """ if active_only: return course_overviews.filter( From d1e5b99ce150f76e216c4baacb1704871d0875f4 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Sun, 17 Mar 2024 21:23:58 -0400 Subject: [PATCH 32/38] refactor: address PR reviews --- cms/envs/common.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 9ec6dee81c71..cf02df9093d1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -564,14 +564,13 @@ # .. toggle_name: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2'] # .. toggle_implementation: DjangoSetting - # .. toggle_default: False + # .. 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_target_removal_date: None # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/34173 - 'ENABLE_HOME_PAGE_COURSE_API_V2': False, + 'ENABLE_HOME_PAGE_COURSE_API_V2': True, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE From f808777e8ed56aa62b50b54e7cdfe7c469a886b4 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 12:19:43 -0400 Subject: [PATCH 33/38] Revert "Revert "fix: address test failures"" This reverts commit 5ff30de163bc2f24c6da691e997f530e957fbace. --- .../rest_api/v1/views/tests/test_home.py | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 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 5279af0b1297..8a502d3934e2 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,6 +2,7 @@ Unit tests for home page view. """ import ddt +from collections import OrderedDict from django.conf import settings from django.urls import reverse from edx_toggles.toggles.testutils import ( @@ -83,6 +84,11 @@ class HomePageCoursesViewTest(CourseTestCase): def setUp(self): super().setUp() self.url = reverse("cms.djangoapps.contentstore:v1:courses") + CourseOverviewFactory.create( + id=self.course.id, + org=self.course.org, + display_name=self.course.display_name, + ) def test_home_page_response(self): """Check successful response content""" @@ -91,16 +97,18 @@ def test_home_page_response(self): expected_response = { "archived_courses": [], - "courses": [{ - "course_key": course_id, - "display_name": self.course.display_name, - "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', - "number": self.course.number, - "org": self.course.org, - "rerun_link": f'/course_rerun/{course_id}', - "run": self.course.id.run, - "url": f'/course/{course_id}', - }], + "courses": [ + OrderedDict([ + ("course_key", course_id), + ("display_name", self.course.display_name), + ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("number", self.course.number), + ("org", self.course.org), + ("rerun_link", f'/course_rerun/{course_id}'), + ("run", self.course.id.run), + ("url", f'/course/{course_id}'), + ]), + ], "in_process_course_actions": [], } From 084adfadd83a95dfd4e8fdabfabe09b70c4b62af Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 12:52:11 -0400 Subject: [PATCH 34/38] refactor: turn on feature as default --- .../rest_api/v1/views/tests/test_home.py | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 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 8a502d3934e2..1d9032ef40e8 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 @@ -4,6 +4,7 @@ import ddt from collections import OrderedDict from django.conf import settings +from django.test import override_settings from django.urls import reverse from edx_toggles.toggles.testutils import ( override_waffle_switch, @@ -19,6 +20,13 @@ from xmodule.modulestore.tests.factories import CourseFactory +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): """ @@ -74,7 +82,7 @@ def test_taxonomy_list_link(self): f'{settings.COURSE_AUTHORING_MICROFRONTEND_URL}/taxonomies' ) - +@override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API) @ddt.ddt class HomePageCoursesViewTest(CourseTestCase): """ @@ -88,6 +96,7 @@ def setUp(self): id=self.course.id, org=self.course.org, display_name=self.course.display_name, + display_number_with_default=self.course.number, ) def test_home_page_response(self): @@ -95,6 +104,32 @@ def test_home_page_response(self): response = self.client.get(self.url) course_id = str(self.course.id) + expected_response = { + "archived_courses": [], + "courses": [{ + "course_key": course_id, + "display_name": self.course.display_name, + "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', + "number": self.course.number, + "org": self.course.org, + "rerun_link": f'/course_rerun/{course_id}', + "run": self.course.id.run, + "url": f'/course/{course_id}', + }], + "in_process_course_actions": [], + } + + self.assertEqual(response.status_code, status.HTTP_200_OK) + print(response.data) + self.assertDictEqual(expected_response, response.data) + + def test_home_page_response_with_api_v2(self): + """Check successful response content with api v2 modifications. + + When the feature flag is enabled, the courses are exclusively fetched from the CourseOverview model, so + the values in the courses' list are OrderedDicts instead of the default dictionaries. + """ + course_id = str(self.course.id) expected_response = { "archived_courses": [], "courses": [ @@ -112,8 +147,10 @@ def test_home_page_response(self): "in_process_course_actions": [], } + with override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API): + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) - print(response.data) self.assertDictEqual(expected_response, response.data) @override_waffle_switch(ENABLE_GLOBAL_STAFF_OPTIMIZATION, True) From 9666b1d8bb84db816ca8bb8563b6e236b39ff844 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 12:52:26 -0400 Subject: [PATCH 35/38] Revert "Revert "fix: address test failures"" This reverts commit 0638311dd9aa5139c6d8f7aa8267ac5611db4d1d. --- .../contentstore/tests/test_course_listing.py | 9 ++------- .../contentstore/views/tests/test_course_index.py | 13 +++++++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 66c16dc6dd8c..6d2cfe6f3f1a 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -35,7 +35,6 @@ 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 @@ -183,11 +182,7 @@ def test_staff_course_listing(self): self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) # 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)) + self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_list_by_staff)) def test_get_course_list_with_invalid_course_location(self): """ @@ -207,7 +202,7 @@ def test_get_course_list_with_invalid_course_location(self): # Verify fetched accessible courses list is a list of CourseSummery instances and only one course # is returned - self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_summary_list)) + 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 diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index b30f8c95a631..5184dd6c977d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -59,6 +59,11 @@ def setUp(self): number='test-2.3_course', display_name='dotted.course.name-2', ) + CourseOverviewFactory.create( + id=self.odd_course.id, + org=self.odd_course.org, + display_name=self.odd_course.display_name, + ) def check_courses_on_index(self, authed_client, expected_course_tab_len): """ @@ -425,10 +430,10 @@ def check_index_page(self, separate_archived_courses, org): (True, 'staff', None, 0, 21), (False, 'staff', None, 0, 21), # Base user has global staff access - (True, 'user', ORG, 2, 21), - (False, 'user', ORG, 2, 21), - (True, 'user', None, 2, 21), - (False, 'user', None, 2, 21), + (True, 'user', ORG, 0, 21), + (False, 'user', ORG, 0, 21), + (True, 'user', None, 0, 21), + (False, 'user', None, 0, 21), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): From 3fb4b670258e80292168df91cf10e7c261282dc5 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 13:11:11 -0400 Subject: [PATCH 36/38] fix: address test failures --- .../rest_api/v1/views/tests/test_home.py | 1 - .../contentstore/tests/test_course_listing.py | 36 +++++++++++++---- .../views/tests/test_course_index.py | 39 ++++++++++++++++++- 3 files changed, 67 insertions(+), 9 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 1d9032ef40e8..fcabe41564fd 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 @@ -22,7 +22,6 @@ 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 diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 6d2cfe6f3f1a..b620759b0ce7 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -10,7 +10,7 @@ import ddt from ccx_keys.locator import CCXLocator from django.conf import settings -from django.test import RequestFactory +from django.test import RequestFactory, override_settings from opaque_keys.edx.locations import CourseLocator from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient @@ -35,12 +35,17 @@ 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 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 @@ -181,8 +186,18 @@ def test_staff_course_listing(self): self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) - # Verify fetched accessible courses list is a list of CourseSummery instances - 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)) def test_get_course_list_with_invalid_course_location(self): """ @@ -200,10 +215,17 @@ def test_get_course_list_with_invalid_course_location(self): courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) courses_summary_list = list(courses_summary_iter) - # Verify fetched accessible courses list is a list of CourseSummery instances and only one course - # is returned - self.assertTrue(all(isinstance(course, CourseOverview) for course in courses_summary_list)) - self.assertEqual(len(courses_summary_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. + 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 + self.assertTrue(all(isinstance(course, CourseSummary) 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/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 5184dd6c977d..36d5e623260b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -40,6 +40,11 @@ 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): """ @@ -425,6 +430,38 @@ 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, 21), + (False, 'staff', None, 0, 21), + # Base user has global staff access + (True, 'user', ORG, 2, 21), + (False, 'user', ORG, 2, 21), + (True, 'user', None, 2, 21), + (False, 'user', None, 2, 21), + ) + @ddt.unpack + def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, 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. + """ + # Authenticate the requested user + user = getattr(self, username) + password = getattr(self, username + '_password') + self.client.login(username=user, password=password) + + # Enable/disable the feature before viewing the index page. + features = settings.FEATURES.copy() + features['ENABLE_SEPARATE_ARCHIVED_COURSES'] = separate_archived_courses + with override_settings(FEATURES=features): + self.check_index_page_with_query_count(separate_archived_courses=separate_archived_courses, + org=org, + mongo_queries=mongo_queries, + 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, 21), @@ -436,7 +473,7 @@ def check_index_page(self, separate_archived_courses, org): (False, 'user', None, 0, 21), ) @ddt.unpack - def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): + def test_separate_archived_courses_with_home_page_course_v2_api(self, separate_archived_courses, username, org, mongo_queries, 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. From 79239878b5e3d44e3212a9eb5019d67515138410 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 13:30:27 -0400 Subject: [PATCH 37/38] fix: address test (quality) failures --- .../contentstore/rest_api/v1/views/tests/test_home.py | 1 + .../contentstore/views/tests/test_course_index.py | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) 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 fcabe41564fd..a7e2e8f03aa8 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 @@ -81,6 +81,7 @@ def test_taxonomy_list_link(self): f'{settings.COURSE_AUTHORING_MICROFRONTEND_URL}/taxonomies' ) + @override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API) @ddt.ddt class HomePageCoursesViewTest(CourseTestCase): diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 36d5e623260b..952483893a74 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -473,7 +473,14 @@ def test_separate_archived_courses(self, separate_archived_courses, username, or (False, 'user', None, 0, 21), ) @ddt.unpack - def test_separate_archived_courses_with_home_page_course_v2_api(self, separate_archived_courses, username, org, mongo_queries, sql_queries): + def test_separate_archived_courses_with_home_page_course_v2_api( + self, + separate_archived_courses, + username, + org, + mongo_queries, + 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. From 93e55c842752ed0d7ccaa1ffbe184bb7bb889742 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 18 Mar 2024 17:39:02 -0400 Subject: [PATCH 38/38] fix: address test failures --- cms/djangoapps/contentstore/tests/test_course_listing.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index b620759b0ce7..44084e3595b8 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -212,18 +212,19 @@ def test_get_course_list_with_invalid_course_location(self): courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) - courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) - courses_summary_list = list(courses_summary_iter) - 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)