From a0a937842585a5bd02463b05746f82c82b32b911 Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Mon, 11 Sep 2023 11:50:12 -0400 Subject: [PATCH 1/3] Remove duplicate catalog URL path (#1871) --- main/urls.py | 1 - 1 file changed, 1 deletion(-) diff --git a/main/urls.py b/main/urls.py index b2765d0a79..df06b0ab53 100644 --- a/main/urls.py +++ b/main/urls.py @@ -87,7 +87,6 @@ re_path(r"^documents/", include(wagtaildocs_urls)), path("", include(wagtail_urls)), path("", include("cms.urls")), - path("catalog/", index, name="catalog"), # Example view path("", index, name="main-index"), ] + ( From f16b217ed4dee3a4a0f9b473197839dabddff7d1 Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Mon, 11 Sep 2023 14:49:24 -0400 Subject: [PATCH 2/3] 1869: Course program api performance improvements (#1872) * This works but is just POC * Add department serializer and prefetch * improvement for courses API * program improvements * Fix course run serializer, migration, enrollment api * Add live filter to front end API call * Fix tests, format, add filtering to front end query * select related course on course_run API * Javascript formatting * Fix serializers * Fix Course serializer * Format * py test passing * Format and repair tests * Clean up department serializer * Add class doc * Cleanup department serializer --- cms/models.py | 3 +- cms/serializers.py | 24 +-- courses/api.py | 2 - .../0043_course_program_live_index.py | 22 +++ courses/models.py | 17 +- courses/serializers.py | 151 +++++------------- courses/serializers_test.py | 34 ++-- courses/views/v1/__init__.py | 37 +++-- courses/views_test.py | 19 ++- .../public/src/components/EnrolledItemCard.js | 13 +- .../src/containers/ProductDetailEnrollApp.js | 2 +- .../containers/ProductDetailEnrollApp_test.js | 5 + .../public/src/containers/UpsellCardApp.js | 2 +- .../src/containers/pages/CatalogPage.js | 11 +- .../src/containers/pages/CatalogPage_test.js | 22 +-- frontend/public/src/factories/course.js | 13 +- frontend/public/src/lib/courseApi.js | 4 +- frontend/public/src/lib/courseApi_test.js | 2 +- frontend/public/src/lib/queries/courseRuns.js | 2 +- frontend/public/src/lib/queries/courses.js | 2 +- frontend/public/src/lib/queries/programs.js | 2 +- 21 files changed, 172 insertions(+), 217 deletions(-) create mode 100644 courses/migrations/0043_course_program_live_index.py diff --git a/cms/models.py b/cms/models.py index c1b2804d30..6a20db0b6f 100644 --- a/cms/models.py +++ b/cms/models.py @@ -12,6 +12,7 @@ from django.core.exceptions import ValidationError from django.core.serializers.json import DjangoJSONEncoder from django.db import models +from django.utils.functional import cached_property from django.forms import ChoiceField, DecimalField from django.http import Http404 from django.template.response import TemplateResponse @@ -1084,7 +1085,7 @@ class CoursePage(ProductPage): ) ] - @property + @cached_property def product(self): """Gets the product associated with this page""" return self.course diff --git a/cms/serializers.py b/cms/serializers.py index 52209cf52e..81b67f3de9 100644 --- a/cms/serializers.py +++ b/cms/serializers.py @@ -20,7 +20,6 @@ class CoursePageSerializer(serializers.ModelSerializer): description = serializers.SerializerMethodField() current_price = serializers.SerializerMethodField() instructors = serializers.SerializerMethodField() - live = serializers.SerializerMethodField() effort = serializers.SerializerMethodField() length = serializers.SerializerMethodField() @@ -76,28 +75,14 @@ def get_financial_assistance_form_url(self, instance): else "" ) - def get_next_run_id(self, instance): - """Get next run id""" - run = instance.course.first_unexpired_run - return run.id if run is not None else None - def get_description(self, instance): return bleach.clean(instance.description, tags=[], strip=True) def get_current_price(self, instance): - next_run = self.get_next_run_id(instance) - - if next_run is None: - return None - - course_ct = ContentType.objects.get(app_label="courses", model="courserun") - relevant_product = ( - Product.objects.filter( - content_type=course_ct, object_id=next_run, is_active=True - ) - .order_by("-price") - .first() + instance.product.active_products.filter().order_by("-price").first() + if instance.product.active_products + else None ) return relevant_product.price if relevant_product else None @@ -120,9 +105,6 @@ def get_instructors(self, instance): return returnable_members - def get_live(self, instance): - return instance.live - def get_effort(self, instance): return ( bleach.clean(instance.effort, tags=[], strip=True) diff --git a/courses/api.py b/courses/api.py index 13d7e8bc59..6645a62c39 100644 --- a/courses/api.py +++ b/courses/api.py @@ -31,11 +31,9 @@ CourseRunCertificate, CourseRunEnrollment, CourseRunGrade, - Program, ProgramCertificate, ProgramEnrollment, ProgramRequirement, - ProgramRequirementNodeType, ) from courses.tasks import subscribe_edx_course_emails from courses.utils import ( diff --git a/courses/migrations/0043_course_program_live_index.py b/courses/migrations/0043_course_program_live_index.py new file mode 100644 index 0000000000..7caa2c6249 --- /dev/null +++ b/courses/migrations/0043_course_program_live_index.py @@ -0,0 +1,22 @@ +# Generated by Django 3.2.20 on 2023-09-10 11:40 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("courses", "0042_add_ordering_to_course_and_program"), + ] + + operations = [ + migrations.AlterField( + model_name="course", + name="live", + field=models.BooleanField(db_index=True, default=False), + ), + migrations.AlterField( + model_name="program", + name="live", + field=models.BooleanField(db_index=True, default=False), + ), + ] diff --git a/courses/models.py b/courses/models.py index 9f0ccd3a76..f0eb360633 100644 --- a/courses/models.py +++ b/courses/models.py @@ -125,7 +125,7 @@ class Meta: readable_id = models.CharField( max_length=255, unique=True, validators=[validate_url_path_field] ) - live = models.BooleanField(default=False) + live = models.BooleanField(default=False, db_index=True) program_type = models.CharField( max_length=255, default="Series", @@ -295,7 +295,7 @@ def save(self, *args, **kwargs): program=self, node_type=ProgramRequirementNodeType.PROGRAM_ROOT.value ) - @property + @cached_property def courses(self): """ Returns the courses associated with this program via the requirements @@ -315,7 +315,7 @@ def courses(self): path__startswith=op.path, node_type=ProgramRequirementNodeType.COURSE, ) - .select_related("course", "course__page") + .select_related("course") .all() ) @@ -436,7 +436,7 @@ class Meta: readable_id = models.CharField( max_length=255, unique=True, validators=[validate_url_path_field] ) - live = models.BooleanField(default=False) + live = models.BooleanField(default=False, db_index=True) departments = models.ManyToManyField(Department, blank=True) flexible_prices = GenericRelation( "flexiblepricing.FlexiblePrice", @@ -449,7 +449,7 @@ class Meta: content_type_field="courseware_content_type", ) - @property + @cached_property def course_number(self): """ Returns: @@ -523,6 +523,7 @@ def programs(self): ProgramRequirement.objects.filter( node_type=ProgramRequirementNodeType.COURSE, course=self ) + .all() .distinct("program_id") .order_by("program_id") .values_list("program_id", flat=True) @@ -575,7 +576,7 @@ class CourseRun(TimestampedModel): objects = CourseRunQuerySet.as_manager() course = models.ForeignKey( - Course, on_delete=models.CASCADE, related_name="courseruns" + Course, on_delete=models.CASCADE, related_name="courseruns", db_index=True ) # product = GenericRelation(Product, related_query_name="course_run") title = models.CharField( @@ -633,7 +634,9 @@ class CourseRun(TimestampedModel): live = models.BooleanField(default=False) is_self_paced = models.BooleanField(default=False) - products = GenericRelation("ecommerce.Product", related_query_name="courseruns") + products = GenericRelation( + "ecommerce.Product", related_query_name="courserunproducts" + ) class Meta: unique_together = ("course", "run_tag") diff --git a/courses/serializers.py b/courses/serializers.py index 5445567e29..a275b3a23a 100644 --- a/courses/serializers.py +++ b/courses/serializers.py @@ -6,18 +6,16 @@ from django.conf import settings from django.contrib.auth.models import AnonymousUser -from django.core.exceptions import ObjectDoesNotExist from django.templatetags.static import static from rest_framework import serializers from rest_framework.exceptions import ValidationError +from django.db.models import CharField -from cms.models import CoursePage, ProgramPage from cms.serializers import CoursePageSerializer, ProgramPageSerializer from courses import models from courses.api import create_run_enrollments from courses.constants import CONTENT_TYPE_MODEL_COURSE, CONTENT_TYPE_MODEL_PROGRAM -from ecommerce.models import Product -from ecommerce.serializers import BaseProductSerializer, ProductFlexibilePriceSerializer +from ecommerce.serializers import ProductFlexibilePriceSerializer from flexiblepricing.api import is_courseware_flexible_price_approved from main import features from main.serializers import StrictFieldsSerializer @@ -98,27 +96,27 @@ class Meta: "expiration_date", "courseware_url", "courseware_id", + "certificate_available_date", "upgrade_deadline", "is_upgradable", "is_self_paced", "run_tag", "id", "live", + "course_number", ] class CourseRunSerializer(BaseCourseRunSerializer): """CourseRun model serializer""" - products = ProductRelatedField(many=True, queryset=Product.objects.all()) - page = serializers.SerializerMethodField() + products = ProductRelatedField(many=True, read_only=True) approved_flexible_price_exists = serializers.SerializerMethodField() class Meta: model = models.CourseRun fields = BaseCourseRunSerializer.Meta.fields + [ "products", - "page", "approved_flexible_price_exists", ] @@ -134,14 +132,6 @@ def to_representation(self, instance): } return data - def get_page(self, instance): - try: - return CoursePageSerializer( - instance=CoursePage.objects.filter(course=instance.course).get() - ).data - except ObjectDoesNotExist: - return None - def get_approved_flexible_price_exists(self, instance): # Get the User object if it exists. user = self.context["request"].user if "request" in self.context else None @@ -159,13 +149,20 @@ def get_approved_flexible_price_exists(self, instance): return flexible_price_exists +class DepartmentSerializer(serializers.ModelSerializer): + """Department model serializer""" + + class Meta: + model = models.Department + fields = ["name"] + + class CourseSerializer(BaseCourseSerializer): - """Course model serializer - also serializes child course runs""" + """Course model serializer""" - courseruns = serializers.SerializerMethodField() + departments = DepartmentSerializer(many=True, read_only=True) next_run_id = serializers.SerializerMethodField() - departments = serializers.SerializerMethodField() - page = serializers.SerializerMethodField() + page = CoursePageSerializer(read_only=True) programs = serializers.SerializerMethodField() def get_next_run_id(self, instance): @@ -173,45 +170,6 @@ def get_next_run_id(self, instance): run = instance.first_unexpired_run return run.id if run is not None else None - def get_courseruns(self, instance): - """Returns all course runs related to the course.""" - if features.is_enabled(features.ENABLE_NEW_DESIGN): - return [ - CourseRunSerializer(instance=run, context=self.context).data - for run in instance.courseruns.all().order_by("id") - ] - all_runs = self.context.get("all_runs", False) - if all_runs: - active_runs = instance.unexpired_runs - else: - user = self.context["request"].user if "request" in self.context else None - active_runs = ( - instance.available_runs(user) - if user and user.is_authenticated - else instance.unexpired_runs - ) - return [ - CourseRunSerializer(instance=run, context=self.context).data - for run in active_runs - if run.live - ] - - def get_departments(self, instance): - """List departments of a course""" - return sorted( - [{"name": department.name} for department in instance.departments.all()], - key=lambda department: department["name"], - ) - - def get_page(self, instance): - return ( - CoursePageSerializer( - instance=CoursePage.objects.filter(course=instance).get() - ).data - if CoursePage.objects.filter(course=instance).exists() - else None - ) - def get_programs(self, instance): if self.context.get("all_runs", False): from courses.serializers import BaseProgramSerializer @@ -226,7 +184,6 @@ class Meta: "id", "title", "readable_id", - "courseruns", "next_run_id", "departments", "page", @@ -234,43 +191,29 @@ class Meta: ] -class CourseRunDetailSerializer(serializers.ModelSerializer): +class CourseWithCourseRunsSerializer(CourseSerializer): + """Course model serializer - also serializes child course runs""" + + courseruns = CourseRunSerializer(many=True, read_only=True) + + class Meta: + model = models.Course + fields = CourseSerializer.Meta.fields + [ + "courseruns", + ] + + +class CourseRunWithCourseSerializer(CourseRunSerializer): """ - CourseRun model serializer - also serializes the parent Course - Includes the relevant Page (if there is one) and Products (if they exist, - just the base product data) + CourseRun model serializer - also serializes the parent Course. """ - course = BaseCourseSerializer(read_only=True, context={"include_page_fields": True}) - products = BaseProductSerializer(read_only=True, many=True) - page = serializers.SerializerMethodField() - - def get_page(self, instance): - try: - return CoursePageSerializer(instance=instance.course.page).data - except ObjectDoesNotExist: - return None + course = CourseSerializer(read_only=True, context={"include_page_fields": True}) class Meta: model = models.CourseRun - fields = [ - "course_number", + fields = CourseRunSerializer.Meta.fields + [ "course", - "title", - "start_date", - "end_date", - "enrollment_start", - "enrollment_end", - "expiration_date", - "certificate_available_date", - "courseware_url", - "courseware_id", - "upgrade_deadline", - "is_upgradable", - "is_self_paced", - "id", - "products", - "page", ] @@ -295,11 +238,11 @@ class ProgramSerializer(serializers.ModelSerializer): requirements = serializers.SerializerMethodField() req_tree = serializers.SerializerMethodField() page = serializers.SerializerMethodField() - departments = serializers.SerializerMethodField() + departments = DepartmentSerializer(many=True, read_only=True) def get_courses(self, instance): """Serializer for courses""" - return CourseSerializer( + return CourseWithCourseRunsSerializer( [course[0] for course in instance.courses if course[0].live], many=True, context={"include_page_fields": True}, @@ -320,20 +263,11 @@ def get_req_tree(self, instance): return ProgramRequirementTreeSerializer(instance=req_root).data def get_page(self, instance): - if ProgramPage.objects.filter(program=instance).exists(): - return ProgramPageSerializer( - instance=ProgramPage.objects.filter(program=instance).get() - ).data + if hasattr(instance, "page"): + return ProgramPageSerializer(instance.page).data else: return {"feature_image_src": _get_thumbnail_url(None)} - def get_departments(self, instance): - """List departments of a course""" - return sorted( - [{"name": department.name} for department in instance.departments.all()], - key=lambda department: department["name"], - ) - class Meta: model = models.Program fields = [ @@ -356,7 +290,6 @@ class FullProgramSerializer(ProgramSerializer): start_date = serializers.SerializerMethodField() end_date = serializers.SerializerMethodField() enrollment_start = serializers.SerializerMethodField() - departments = serializers.SerializerMethodField() def get_start_date(self, instance): """ @@ -400,16 +333,6 @@ def get_enrollment_start(self, instance): .first() ) - def get_departments(self, instance): - """List all departments in all courses in the program""" - courses_in_program = [course[0] for course in instance.courses] - departments = ( - models.Department.objects.filter(course__in=courses_in_program) - .values("name") - .distinct("name") - ) - return list(departments) - class Meta(ProgramSerializer.Meta): fields = ProgramSerializer.Meta.fields + [ "title", @@ -517,7 +440,7 @@ class Meta: class CourseRunEnrollmentSerializer(BaseCourseRunEnrollmentSerializer): """CourseRunEnrollment model serializer""" - run = CourseRunDetailSerializer(read_only=True) + run = CourseRunWithCourseSerializer(read_only=True) run_id = serializers.IntegerField(write_only=True) certificate = serializers.SerializerMethodField(read_only=True) enrollment_mode = serializers.ChoiceField( diff --git a/courses/serializers_test.py b/courses/serializers_test.py index c73fccb7e8..2aee0260b8 100644 --- a/courses/serializers_test.py +++ b/courses/serializers_test.py @@ -26,13 +26,14 @@ ProgramRequirementNodeType, ) from courses.serializers import ( + CourseSerializer, BaseCourseSerializer, BaseProgramSerializer, - CourseRunDetailSerializer, + CourseRunWithCourseSerializer, CourseRunEnrollmentSerializer, CourseRunGradeSerializer, CourseRunSerializer, - CourseSerializer, + CourseWithCourseRunsSerializer, LearnerRecordSerializer, ProgramRequirementSerializer, ProgramRequirementTreeSerializer, @@ -120,7 +121,9 @@ def test_serialize_program(mock_context, remove_tree, program_with_empty_require "readable_id": program_with_empty_requirements.readable_id, "id": program_with_empty_requirements.id, "courses": [ - CourseSerializer(instance=course, context={**mock_context}).data + CourseWithCourseRunsSerializer( + instance=course, context={**mock_context} + ).data for course in [course1, course2] ] if not remove_tree @@ -169,7 +172,7 @@ def test_serialize_course(mocker, mock_context, is_anonymous, all_runs, settings run=courseRun1, **({} if is_anonymous else {"user": user}) ) - data = CourseSerializer(instance=course, context=mock_context).data + data = CourseWithCourseRunsSerializer(instance=course, context=mock_context).data assert_drf_json_equal( data, @@ -260,21 +263,24 @@ def test_serialize_course_run(): "is_upgradable": course_run.is_upgradable, "id": course_run.id, "products": [], - "page": None, "approved_flexible_price_exists": False, "live": True, "is_self_paced": course_run.is_self_paced, + "certificate_available_date": drf_datetime( + course_run.certificate_available_date + ), + "course_number": course_run.course_number, }, ) -def test_serialize_course_run_detail(): - """Test CourseRunDetailSerializer serialization""" +def test_serialize_course_run_with_course(): + """Test CoursePageDepartmentsSerializer serialization""" course_run = CourseRunFactory.create(course__page=None) - data = CourseRunDetailSerializer(course_run).data + data = CourseRunWithCourseSerializer(course_run).data assert data == { - "course": BaseCourseSerializer(course_run.course).data, + "course": CourseSerializer(course_run.course).data, "course_number": course_run.course_number, "title": course_run.title, "courseware_id": course_run.courseware_id, @@ -292,7 +298,9 @@ def test_serialize_course_run_detail(): "is_self_paced": False, "id": course_run.id, "products": BaseProductSerializer(course_run.products, many=True).data, - "page": None, + "approved_flexible_price_exists": False, + "live": True, + "run_tag": course_run.run_tag, } @@ -303,7 +311,7 @@ def test_serialize_course_run_enrollments(settings, receipts_enabled): course_run_enrollment = CourseRunEnrollmentFactory.create() serialized_data = CourseRunEnrollmentSerializer(course_run_enrollment).data assert serialized_data == { - "run": CourseRunDetailSerializer(course_run_enrollment.run).data, + "run": CourseRunWithCourseSerializer(course_run_enrollment.run).data, "id": course_run_enrollment.id, "edx_emails_subscription": True, "enrollment_mode": "audit", @@ -331,7 +339,7 @@ def test_serialize_course_run_enrollments_with_flexible_pricing( ) serialized_data = CourseRunEnrollmentSerializer(course_run_enrollment).data assert serialized_data == { - "run": CourseRunDetailSerializer(course_run_enrollment.run).data, + "run": CourseRunWithCourseSerializer(course_run_enrollment.run).data, "id": course_run_enrollment.id, "edx_emails_subscription": True, "enrollment_mode": "audit", @@ -351,7 +359,7 @@ def test_serialize_course_run_enrollments_with_grades(): serialized_data = CourseRunEnrollmentSerializer(course_run_enrollment).data assert serialized_data == { - "run": CourseRunDetailSerializer(course_run_enrollment.run).data, + "run": CourseRunWithCourseSerializer(course_run_enrollment.run).data, "id": course_run_enrollment.id, "edx_emails_subscription": True, "enrollment_mode": "audit", diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index e94ee96d1d..5e86d7df07 100644 --- a/courses/views/v1/__init__.py +++ b/courses/views/v1/__init__.py @@ -8,6 +8,7 @@ from django.db.models import Q from django.http import HttpResponse, HttpResponseRedirect from django.urls import reverse +from django_filters.rest_framework import DjangoFilterBackend from requests import ConnectionError as RequestsConnectionError from requests.exceptions import HTTPError from rest_framework import mixins, status, viewsets @@ -35,12 +36,12 @@ ) from courses.serializers import ( CourseRunEnrollmentSerializer, - CourseRunSerializer, - CourseSerializer, + CourseRunWithCourseSerializer, LearnerRecordSerializer, PartnerSchoolSerializer, ProgramSerializer, UserProgramEnrollmentDetailSerializer, + CourseWithCourseRunsSerializer, ) from courses.tasks import send_partner_school_email from courses.utils import get_program_certificate_by_enrollment @@ -84,7 +85,9 @@ class ProgramViewSet(viewsets.ReadOnlyModelViewSet): permission_classes = [] serializer_class = ProgramSerializer - queryset = Program.objects.filter(live=True) + filter_backends = [DjangoFilterBackend] + filterset_fields = ["id", "live"] + queryset = Program.objects.filter().prefetch_related("departments") pagination_class = Pagination def paginate_queryset(self, queryset): @@ -102,15 +105,17 @@ class CourseViewSet(viewsets.ReadOnlyModelViewSet): pagination_class = Pagination permission_classes = [] - - serializer_class = CourseSerializer + filter_backends = [DjangoFilterBackend] + serializer_class = CourseWithCourseRunsSerializer + filterset_fields = ["id", "live", "readable_id"] def get_queryset(self): - readable_id = self.request.query_params.get("readable_id", None) - if readable_id: - return Course.objects.filter(live=True, readable_id=readable_id) - - return Course.objects.filter(live=True) + return ( + Course.objects.filter() + .select_related("page") + .prefetch_related("courseruns", "departments") + .all() + ) def get_serializer_context(self): added_context = {} @@ -132,8 +137,10 @@ def paginate_queryset(self, queryset): class CourseRunViewSet(viewsets.ReadOnlyModelViewSet): """API view set for CourseRuns""" - serializer_class = CourseRunSerializer + serializer_class = CourseRunWithCourseSerializer permission_classes = [] + filter_backends = [DjangoFilterBackend] + filterset_fields = ["id", "live"] def get_queryset(self): relevant_to = self.request.query_params.get("relevant_to", None) @@ -144,7 +151,11 @@ def get_queryset(self): else: return CourseRun.objects.none() else: - return CourseRun.objects.all() + return ( + CourseRun.objects.select_related("course") + .prefetch_related("course__departments", "course__page") + .all() + ) def get_serializer_context(self): added_context = {} @@ -274,7 +285,7 @@ class UserEnrollmentsApiViewSet( def get_queryset(self): return ( CourseRunEnrollment.objects.filter(user=self.request.user) - .select_related("run__course__page") + .select_related("run__course__page", "user", "run") .all() ) diff --git a/courses/views_test.py b/courses/views_test.py index 459dcf1baa..c16a01d65d 100644 --- a/courses/views_test.py +++ b/courses/views_test.py @@ -26,8 +26,9 @@ from courses.serializers import ( CourseRunEnrollmentSerializer, CourseRunSerializer, - CourseSerializer, + CourseWithCourseRunsSerializer, ProgramSerializer, + CourseRunWithCourseSerializer, ) from courses.views.v1 import UserEnrollmentsApiViewSet from ecommerce.factories import LineFactory, OrderFactory, ProductFactory @@ -121,7 +122,8 @@ def test_get_courses(user_drf_client, courses, mock_context): assert len(courses_data) == len(courses) for course in courses: assert ( - CourseSerializer(instance=course, context=mock_context).data in courses_data + CourseWithCourseRunsSerializer(instance=course, context=mock_context).data + in courses_data ) @@ -130,13 +132,18 @@ def test_get_course(user_drf_client, courses, mock_context): course = courses[0] resp = user_drf_client.get(reverse("courses_api-detail", kwargs={"pk": course.id})) course_data = resp.json() - assert course_data == CourseSerializer(instance=course, context=mock_context).data + assert ( + course_data + == CourseWithCourseRunsSerializer(instance=course, context=mock_context).data + ) def test_create_course(user_drf_client, courses, mock_context): """Test the view that handles a request to create a Course""" course = courses[0] - course_data = CourseSerializer(instance=course, context=mock_context).data + course_data = CourseWithCourseRunsSerializer( + instance=course, context=mock_context + ).data del course_data["id"] course_data["title"] = "New Course Title" request_url = reverse("courses_api-list") @@ -169,7 +176,7 @@ def test_get_course_runs(user_drf_client, course_runs): # Force sorting by run id since this test has been flaky course_runs_data = sorted(course_runs_data, key=op.itemgetter("id")) for course_run, course_run_data in zip(course_runs, course_runs_data): - assert course_run_data == CourseRunSerializer(course_run).data + assert course_run_data == CourseRunWithCourseSerializer(course_run).data @pytest.mark.parametrize("is_enrolled", [True, False]) @@ -221,7 +228,7 @@ def test_get_course_run(user_drf_client, course_runs): reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) ) course_run_data = resp.json() - assert course_run_data == CourseRunSerializer(course_run).data + assert course_run_data == CourseRunWithCourseSerializer(course_run).data def test_create_course_run(user_drf_client, course_runs): diff --git a/frontend/public/src/components/EnrolledItemCard.js b/frontend/public/src/components/EnrolledItemCard.js index 7388a7e449..0f7750caa7 100644 --- a/frontend/public/src/components/EnrolledItemCard.js +++ b/frontend/public/src/components/EnrolledItemCard.js @@ -426,7 +426,7 @@ export class EnrolledItemCard extends React.Component< const financialAssistanceLink = isFinancialAssistanceAvailable(enrollment.run) && !enrollment.approved_flexible_price_exists ? ( - + Financial assistance? ) : null @@ -462,7 +462,7 @@ export class EnrolledItemCard extends React.Component< const startDateDescription = generateStartDateText(enrollment.run) const onUnenrollClick = partial(this.onDeactivate.bind(this), [enrollment]) const courseId = enrollment.run.course_number - const pageLocation = enrollment.run.page + const pageLocation = enrollment.run.course.page const menuTitle = `Course options for ${enrollment.run.course.title}` const courseRunStatusMessageText = courseRunStatusMessage(enrollment.run) @@ -483,11 +483,14 @@ export class EnrolledItemCard extends React.Component< )} {!enrollment.run.course.feature_image_src && - enrollment.run.page && - enrollment.run.page.feature_image_src && ( + enrollment.run.course.page && + enrollment.run.course.page.feature_image_src && (
- +
)} diff --git a/frontend/public/src/containers/ProductDetailEnrollApp.js b/frontend/public/src/containers/ProductDetailEnrollApp.js index 119678e0fc..788d4f88f5 100644 --- a/frontend/public/src/containers/ProductDetailEnrollApp.js +++ b/frontend/public/src/containers/ProductDetailEnrollApp.js @@ -175,7 +175,7 @@ export class ProductDetailEnrollApp extends React.Component< isFinancialAssistanceAvailable(run) && !run.approved_flexible_price_exists ? (

- + Need financial assistance?

diff --git a/frontend/public/src/containers/ProductDetailEnrollApp_test.js b/frontend/public/src/containers/ProductDetailEnrollApp_test.js index 0e15e04fc0..14691494e0 100644 --- a/frontend/public/src/containers/ProductDetailEnrollApp_test.js +++ b/frontend/public/src/containers/ProductDetailEnrollApp_test.js @@ -404,6 +404,11 @@ describe("ProductDetailEnrollApp", () => { ;[[true], [false]].forEach(([flexPriceApproved]) => { it(`shows the flexible pricing available link if the user does not have approved flexible pricing for the course run`, async () => { courseRun["approved_flexible_price_exists"] = flexPriceApproved + courseRun["course"] = { + page: { + financial_assistance_form_url: "google.com" + } + } isWithinEnrollmentPeriodStub.returns(true) isFinancialAssistanceAvailableStub.returns(true) const { inner } = await renderPage() diff --git a/frontend/public/src/containers/UpsellCardApp.js b/frontend/public/src/containers/UpsellCardApp.js index 8f478a57c8..0e2636ea6b 100644 --- a/frontend/public/src/containers/UpsellCardApp.js +++ b/frontend/public/src/containers/UpsellCardApp.js @@ -57,7 +57,7 @@ export class UpsellCardApp extends React.Component { isFinancialAssistanceAvailable(run) && !run.approved_flexible_price_exists ? (

- + Need financial assistance?

diff --git a/frontend/public/src/containers/pages/CatalogPage.js b/frontend/public/src/containers/pages/CatalogPage.js index d3cccf8ae3..268b0c3dac 100644 --- a/frontend/public/src/containers/pages/CatalogPage.js +++ b/frontend/public/src/containers/pages/CatalogPage.js @@ -376,7 +376,7 @@ export class CatalogPage extends React.Component { } /** - * Returns an array of Programs which have live = true, page.live = true, and a department name which + * Returns an array of Programs which have page.live = true and a department name which * matches the currently selected department. * @param {Array} programs An array of Programs which will be filtered by Department and other criteria. * @param {string} selectedDepartment The Department name used to compare against the courses in the array. @@ -387,11 +387,10 @@ export class CatalogPage extends React.Component { ) { return programs.filter( program => - (selectedDepartment === ALL_DEPARTMENTS || - program.departments - .map(department => department.name) - .includes(selectedDepartment)) && - program.live + selectedDepartment === ALL_DEPARTMENTS || + program.departments + .map(department => department.name) + .includes(selectedDepartment) ) } diff --git a/frontend/public/src/containers/pages/CatalogPage_test.js b/frontend/public/src/containers/pages/CatalogPage_test.js index 27345e673f..9fcc74d21f 100644 --- a/frontend/public/src/containers/pages/CatalogPage_test.js +++ b/frontend/public/src/containers/pages/CatalogPage_test.js @@ -412,24 +412,6 @@ describe("CatalogPage", function() { expect(programsFilteredByCriteriaAndDepartment.length).equals(3) }) - it("renders catalog programs only if the program is live", async () => { - const program1 = JSON.parse(JSON.stringify(displayedProgram)) - program1.live = true - const program2 = JSON.parse(JSON.stringify(displayedProgram)) - program2.live = true - const { inner } = await renderPage() - programs = [program1, program2] - let programsFilteredByCriteriaAndDepartment = inner - .instance() - .filteredProgramsByDepartmentAndCriteria("All Departments", programs) - expect(programsFilteredByCriteriaAndDepartment.length).equals(2) - program2.live = false - programsFilteredByCriteriaAndDepartment = inner - .instance() - .filteredProgramsByDepartmentAndCriteria("All Departments", programs) - expect(programsFilteredByCriteriaAndDepartment.length).equals(1) - }) - it("renders no catalog courses if the course's associated course run is not live", async () => { const courseRuns = JSON.parse(JSON.stringify(displayedCourse.courseruns)) const { inner } = await renderPage() @@ -748,7 +730,7 @@ describe("CatalogPage", function() { sinon.assert.calledWith( helper.handleRequestStub, - "/api/courses/?page=2", + "/api/courses/?page=2&live=true", "GET" ) @@ -929,7 +911,7 @@ describe("CatalogPage", function() { sinon.assert.calledWith( helper.handleRequestStub, - "/api/programs/?page=2", + "/api/programs/?page=2&live=true", "GET" ) diff --git a/frontend/public/src/factories/course.js b/frontend/public/src/factories/course.js index c875f2440a..b8c273473f 100644 --- a/frontend/public/src/factories/course.js +++ b/frontend/public/src/factories/course.js @@ -65,7 +65,6 @@ export const makeCourseRunWithProduct = (): CourseRun => ({ // $FlowFixMe id: genCourseRunId.next().value, course_number: casual.word, - page: { financial_assistance_form_url: casual.url }, is_upgradable: true, products: [ { @@ -95,7 +94,17 @@ const makeCourseDetail = (): CourseDetail => ({ readable_id: casual.word, feature_image_src: casual.url, departments: [makeDepartment()], - live: true + live: true, + page: { + financial_assistance_form_url: casual.url, + feature_image_src: casual.url, + live: true, + description: casual.text, + current_price: casual.integer(), + instructors: [], + length: casual.text, + effort: casual.text + } }) const makeRequirementRootNode = ( diff --git a/frontend/public/src/lib/courseApi.js b/frontend/public/src/lib/courseApi.js index 6b65752be3..e33cf015ea 100644 --- a/frontend/public/src/lib/courseApi.js +++ b/frontend/public/src/lib/courseApi.js @@ -102,7 +102,9 @@ export const generateStartDateText = (run: CourseRunDetail) => { } export const isFinancialAssistanceAvailable = (run: CourseRunDetail) => { - return run.page ? !!run.page.financial_assistance_form_url : false + return run.course.page + ? !!run.course.page.financial_assistance_form_url + : false } export const enrollmentHasPassingGrade = (enrollment: RunEnrollment) => { diff --git a/frontend/public/src/lib/courseApi_test.js b/frontend/public/src/lib/courseApi_test.js index f82b5ca44a..575700cddc 100644 --- a/frontend/public/src/lib/courseApi_test.js +++ b/frontend/public/src/lib/courseApi_test.js @@ -147,7 +147,7 @@ describe("Course API", () => { ["/courses/course-v1:MITx+14.310x/financial-assistance-request/", true] ].forEach(([url, expResult]) => { it(`returns ${String(expResult)}`, () => { - courseRun["page"] = { financial_assistance_form_url: url } + courseRun["course"]["page"] = { financial_assistance_form_url: url } assert.equal(isFinancialAssistanceAvailable(courseRun), expResult) }) }) diff --git a/frontend/public/src/lib/queries/courseRuns.js b/frontend/public/src/lib/queries/courseRuns.js index 013a5d5b9d..865f892fcc 100644 --- a/frontend/public/src/lib/queries/courseRuns.js +++ b/frontend/public/src/lib/queries/courseRuns.js @@ -21,7 +21,7 @@ export const courseRunsQuery = (courseKey: string = "") => ({ export const coursesQuery = (courseKey: string = "") => ({ queryKey: coursesQueryKey, - url: `/api/courses/?readable_id=${encodeURIComponent(courseKey)}`, + url: `/api/courses/?readable_id=${encodeURIComponent(courseKey)}&live=true`, transform: json => ({ courses: json }), diff --git a/frontend/public/src/lib/queries/courses.js b/frontend/public/src/lib/queries/courses.js index a79fcc6dfb..e3f8ee21ca 100644 --- a/frontend/public/src/lib/queries/courses.js +++ b/frontend/public/src/lib/queries/courses.js @@ -14,7 +14,7 @@ export const coursesQueryKey = "courses" export const coursesQuery = page => ({ queryKey: coursesQueryKey, - url: `/api/courses/?page=${page}`, + url: `/api/courses/?page=${page}&live=true`, transform: json => ({ courses: json }), diff --git a/frontend/public/src/lib/queries/programs.js b/frontend/public/src/lib/queries/programs.js index e74ed14a40..ef92e63cf8 100644 --- a/frontend/public/src/lib/queries/programs.js +++ b/frontend/public/src/lib/queries/programs.js @@ -18,7 +18,7 @@ export const programsQueryKey = "programs" export const programsQuery = page => ({ queryKey: programsQueryKey, - url: `/api/programs/?page=${page}`, + url: `/api/programs/?page=${page}&live=true`, transform: json => ({ programs: json }), From 9116738b95008441dfb046a090edbe65c576206f Mon Sep 17 00:00:00 2001 From: Doof Date: Mon, 11 Sep 2023 19:55:25 +0000 Subject: [PATCH 3/3] Release 0.73.1 --- RELEASE.rst | 6 ++++++ main/settings.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index e3e101400d..0ea3fc149b 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,12 @@ Release Notes ============= +Version 0.73.1 +-------------- + +- 1869: Course program api performance improvements (#1872) +- Remove duplicate catalog URL path (#1871) + Version 0.73.0 (Released September 11, 2023) -------------- diff --git a/main/settings.py b/main/settings.py index 8f2c438ad5..e4b2fc30a9 100644 --- a/main/settings.py +++ b/main/settings.py @@ -29,7 +29,7 @@ from main.celery_utils import OffsettingSchedule from main.sentry import init_sentry -VERSION = "0.73.0" +VERSION = "0.73.1" log = logging.getLogger()