From 4e061cbf20608cf8b9dff23e9c0c154e1c152ccc Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Tue, 12 Sep 2023 15:57:29 -0400 Subject: [PATCH] Repair cart page including reinstated course api improvements (#1885) * Revert "Revert "Department REST API (#1877)" (#1882)" This reverts commit c87bdc15e75c6bae728749dedd5a91cae3755c8d. * Revert "Department REST API (#1877)" This reverts commit f49e9c6cfc5fe5b85218b41fca795192def17703. * format * satisfy flow * flow and me --- 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 | 165 ++++++------------ courses/serializers_test.py | 34 ++-- courses/urls.py | 1 + courses/views/v1/__init__.py | 53 ++++-- 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 | 126 ++++++++----- .../src/containers/pages/CatalogPage_test.js | 145 ++++++++++----- .../src/containers/pages/checkout/CartPage.js | 9 +- 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 +- .../public/src/lib/queries/departments.js | 18 ++ frontend/public/src/lib/queries/programs.js | 2 +- 24 files changed, 393 insertions(+), 292 deletions(-) create mode 100644 courses/migrations/0043_course_program_live_index.py create mode 100644 frontend/public/src/lib/queries/departments.js 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..f35a27f9ba 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,34 @@ 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 DepartmentWithCountSerializer(DepartmentSerializer): + """CourseRun model serializer that includes the number of courses and programs associated with each departments""" + + courses = serializers.IntegerField() + programs = serializers.IntegerField() + + class Meta: + model = models.Department + fields = DepartmentSerializer.Meta.fields + [ + "courses", + "programs", + ] + + 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 +184,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 +198,6 @@ class Meta: "id", "title", "readable_id", - "courseruns", "next_run_id", "departments", "page", @@ -234,43 +205,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 +252,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 +277,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 +304,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 +347,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 +454,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/urls.py b/courses/urls.py index d1e8ad7c32..7add658c2d 100644 --- a/courses/urls.py +++ b/courses/urls.py @@ -19,6 +19,7 @@ v1.UserProgramEnrollmentsViewSet, basename="user_program_enrollments_api", ) +router.register(r"departments", v1.DepartmentViewSet, basename="departments_api") urlpatterns = [ re_path(r"^api/v1/", include(router.urls)), diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index e94ee96d1d..a79b530b7c 100644 --- a/courses/views/v1/__init__.py +++ b/courses/views/v1/__init__.py @@ -5,9 +5,10 @@ from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.db import transaction -from django.db.models import Q +from django.db.models import Q, Count 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 @@ -32,15 +33,17 @@ PartnerSchool, Program, ProgramEnrollment, + Department, ) from courses.serializers import ( CourseRunEnrollmentSerializer, - CourseRunSerializer, - CourseSerializer, + CourseRunWithCourseSerializer, LearnerRecordSerializer, PartnerSchoolSerializer, ProgramSerializer, UserProgramEnrollmentDetailSerializer, + CourseWithCourseRunsSerializer, + DepartmentWithCountSerializer, ) from courses.tasks import send_partner_school_email from courses.utils import get_program_certificate_by_enrollment @@ -84,7 +87,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 +107,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 +139,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 +153,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 +287,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() ) @@ -496,3 +509,15 @@ def get_learner_record_from_uuid(request, uuid): record.program, context={"user": record.user, "anonymous_pull": True} ).data ) + + +@permission_classes([]) +class DepartmentViewSet(viewsets.ReadOnlyModelViewSet): + """API view set for Departments""" + + serializer_class = DepartmentWithCountSerializer + + def get_queryset(self): + return Department.objects.annotate( + courses=Count("course"), programs=Count("program") + ) 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..1ea3e6b1ba 100644 --- a/frontend/public/src/containers/pages/CatalogPage.js +++ b/frontend/public/src/containers/pages/CatalogPage.js @@ -17,6 +17,12 @@ import { programsQueryKey } from "../../lib/queries/programs" +import { + departmentsSelector, + departmentsQuery, + departmentsQueryKey +} from "../../lib/queries/departments" + import { createStructuredSelector } from "reselect" import { compose } from "redux" import { connect } from "react-redux" @@ -50,7 +56,8 @@ export class CatalogPage extends React.Component { filteredCourses: [], filteredPrograms: [], filterCoursesCalled: false, - departments: [], + filteredDepartments: [], + filterDepartmentsCalled: false, selectedDepartment: ALL_DEPARTMENTS, mobileFilterWindowExpanded: false, items_per_row: 3, @@ -154,9 +161,11 @@ export class CatalogPage extends React.Component { } /** - * Updates the filteredCourses and courseDepartments state variables - * once coursesIsLoading is False. Adds an observer to detect when + * Updates the filteredCourses state variable + * once coursesIsLoading is false.. Adds an observer to detect when * the learner has scrolled to the bottom of the visible catalog items. + * Updates the filteredDepartments state variable once departmentsIsLoading + * is false. */ componentDidUpdate = () => { const { courses, coursesIsLoading } = this.props @@ -168,9 +177,6 @@ export class CatalogPage extends React.Component { courses ) this.setState({ filteredCourses: filteredCourses }) - this.setState({ - departments: this.collectDepartmentsFromCatalogItems(filteredCourses) - }) // Detect when the bottom of the catalog page has been reached and display more catalog items. this.io = new window.IntersectionObserver( @@ -179,30 +185,54 @@ export class CatalogPage extends React.Component { ) this.io.observe(this.container.current) } + + if ( + !this.props.departmentsIsLoading && + !this.state.filterDepartmentsCalled + ) { + this.setState({ filterDepartmentsCalled: true }) + this.setState({ + filteredDepartments: this.filterDepartmentsByTabName( + this.state.tabSelected + ) + }) + } } /** - * Returns an array of unique Department names that are associated with the Courses or Programs in the - * parameter and also includes an entry for "All Departments". - * @param {Array} catalogItems Array of Courses or Programs to collect Department names from. + * Returns an array of departments names which have one or more course(s) or program(s) + * related to them depending on the currently selected tab. + * @param {string} selectedTabName the name of the currently selected tab. */ - collectDepartmentsFromCatalogItems( - catalogItems: Array - ) { - const departments = Array.from(catalogItems, item => item.departments) - return [ - ...new Set([ - ALL_DEPARTMENTS, - ...departments.flat().map(department => department.name) - ]) - ] + filterDepartmentsByTabName(selectedTabName: string) { + if (!this.props.departmentsIsLoading) { + const { departments } = this.props + if (selectedTabName === COURSES_TAB) { + return [ + ...new Set([ + ALL_DEPARTMENTS, + ...departments.flatMap(department => + department.courses > 0 ? department.name : [] + ) + ]) + ] + } else { + return [ + ...new Set([ + ALL_DEPARTMENTS, + ...departments.flatMap(department => + department.programs > 0 ? department.name : [] + ) + ]) + ] + } + } } /** * Updates this.state.selectedDepartment to {ALL_DEPARTMENTS}, * updates this.state.tabSelected to the parameter, - * updates this.state.numberCatalogRowsToDisplay to {DEFAULT_MIN_CATALOG_ROWS_RENDERED}, - * updates this.state.departments to equal the unique department + * updates this.state.filteredDepartments * names from the catalog items in the selected tab, * updates this.state.filteredPrograms to equal the programs * which meet the criteria to be displayed in the catalog. @@ -212,16 +242,7 @@ export class CatalogPage extends React.Component { this.setState({ tabSelected: selectTabName }) this.changeSelectedDepartment(ALL_DEPARTMENTS, selectTabName) - if (selectTabName === COURSES_TAB) { - const { coursesIsLoading } = this.props - if (!coursesIsLoading) { - this.setState({ - departments: this.collectDepartmentsFromCatalogItems( - this.state.allCoursesRetrieved - ) - }) - } - } else { + if (selectTabName === PROGRAMS_TAB) { const { programs, programsIsLoading } = this.props if (!programsIsLoading) { const programsToFilter = [] @@ -241,11 +262,11 @@ export class CatalogPage extends React.Component { this.setState({ filteredPrograms: filteredPrograms }) - this.setState({ - departments: this.collectDepartmentsFromCatalogItems(programsToFilter) - }) } } + this.setState({ + filteredDepartments: this.filterDepartmentsByTabName(selectTabName) + }) } /** @@ -376,7 +397,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 +408,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) ) } @@ -505,7 +525,7 @@ export class CatalogPage extends React.Component { */ renderDepartmentSideBarList() { const departmentSideBarListItems = [] - this.state.departments.forEach(department => + this.state.filteredDepartments.forEach(department => departmentSideBarListItems.push(
  • force: true }) -const mapPropsToConfig = () => [coursesQuery(1), programsQuery(1)] +const mapPropsToConfig = () => [ + coursesQuery(1), + programsQuery(1), + departmentsQuery() +] const mapDispatchToProps = { getNextCoursePage: getNextCoursePage, @@ -673,12 +697,18 @@ const mapDispatchToProps = { } const mapStateToProps = createStructuredSelector({ - courses: coursesSelector, - coursesNextPage: coursesNextPageSelector, - programs: programsSelector, - programsNextPage: programsNextPageSelector, - coursesIsLoading: pathOr(true, ["queries", coursesQueryKey, "isPending"]), - programsIsLoading: pathOr(true, ["queries", programsQueryKey, "isPending"]) + courses: coursesSelector, + coursesNextPage: coursesNextPageSelector, + programs: programsSelector, + programsNextPage: programsNextPageSelector, + departments: departmentsSelector, + coursesIsLoading: pathOr(true, ["queries", coursesQueryKey, "isPending"]), + programsIsLoading: pathOr(true, ["queries", programsQueryKey, "isPending"]), + departmentsIsLoading: pathOr(true, [ + "queries", + departmentsQueryKey, + "isPending" + ]) }) export default compose( diff --git a/frontend/public/src/containers/pages/CatalogPage_test.js b/frontend/public/src/containers/pages/CatalogPage_test.js index 27345e673f..611e7ccc90 100644 --- a/frontend/public/src/containers/pages/CatalogPage_test.js +++ b/frontend/public/src/containers/pages/CatalogPage_test.js @@ -279,6 +279,10 @@ describe("CatalogPage", function() { programs: { isPending: false, status: 200 + }, + departments: { + isPending: false, + status: 200 } }, entities: { @@ -287,7 +291,19 @@ describe("CatalogPage", function() { }, programs: { results: programs - } + }, + departments: [ + { + name: "History", + courses: 0, + programs: 5 + }, + { + name: "Science", + courses: 1, + programs: 0 + } + ] } }, {} @@ -305,34 +321,53 @@ describe("CatalogPage", function() { expect(inner.instance().renderNumberOfCatalogItems()).equals(5) }) - it("renders catalog department filter for courses and programs without duplicates", async () => { - const course1 = JSON.parse(JSON.stringify(displayedCourse)) - course1.departments = [{ name: "Math" }] - const course2 = JSON.parse(JSON.stringify(displayedCourse)) - course2.departments = [{ name: "Math" }, { name: "Science" }] - const course3 = JSON.parse(JSON.stringify(displayedCourse)) - course3.departments = [{ name: "Math" }, { name: "History" }] - courses = [course1, course2, course3] - - const program1 = JSON.parse(JSON.stringify(displayedProgram)) - program1.departments = [{ name: "Computer Science" }, { name: "Physics" }] - const program2 = JSON.parse(JSON.stringify(displayedProgram)) - program2.departments = [] - const program3 = JSON.parse(JSON.stringify(displayedProgram)) - program3.departments = [{ name: "Physics" }] - programs = [program1, program2, program3] - const { inner } = await renderPage() - let collectedDepartments = inner + it("renders catalog department filter for courses and programs tabs", async () => { + const { inner } = await renderPage( + { + queries: { + departments: { + isPending: false, + status: 200 + } + }, + entities: { + departments: [ + { + name: "department1", + courses: 1, + programs: 0 + }, + { + name: "department2", + courses: 1, + programs: 1 + }, + { + name: "department3", + courses: 0, + programs: 1 + }, + { + name: "department4", + courses: 0, + programs: 0 + } + ] + } + }, + {} + ) + let filteredDepartments = inner .instance() - .collectDepartmentsFromCatalogItems(courses) - expect(JSON.stringify(collectedDepartments)).equals( - JSON.stringify(["All Departments", "Math", "Science", "History"]) + .filterDepartmentsByTabName("courses") + expect(JSON.stringify(filteredDepartments)).equals( + JSON.stringify(["All Departments", "department1", "department2"]) ) - collectedDepartments = inner + filteredDepartments = inner .instance() - .collectDepartmentsFromCatalogItems(programs) - expect(JSON.stringify(collectedDepartments)).equals( - JSON.stringify(["All Departments", "Computer Science", "Physics"]) + .filterDepartmentsByTabName("programs") + expect(JSON.stringify(filteredDepartments)).equals( + JSON.stringify(["All Departments", "department2", "department3"]) ) }) @@ -412,24 +447,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() @@ -640,6 +657,10 @@ describe("CatalogPage", function() { programs: { isPending: false, status: 200 + }, + departments: { + isPending: false, + status: 200 } }, entities: { @@ -648,7 +669,24 @@ describe("CatalogPage", function() { }, programs: { results: [displayedProgram] - } + }, + departments: [ + { + name: "History", + courses: 2, + programs: 1 + }, + { + name: "Math", + courses: 3, + programs: 0 + }, + { + name: "department4", + courses: 0, + programs: 0 + } + ] } }, {} @@ -748,7 +786,7 @@ describe("CatalogPage", function() { sinon.assert.calledWith( helper.handleRequestStub, - "/api/courses/?page=2", + "/api/courses/?page=2&live=true", "GET" ) @@ -885,6 +923,10 @@ describe("CatalogPage", function() { programs: { isPending: false, status: 200 + }, + departments: { + isPending: false, + status: 200 } }, entities: { @@ -895,7 +937,14 @@ describe("CatalogPage", function() { programs: { results: programs, next: "http://fake.com/api/courses/?page=2" - } + }, + departments: [ + { + name: "History", + courses: 1, + programs: 1 + } + ] } }, {} @@ -929,7 +978,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/containers/pages/checkout/CartPage.js b/frontend/public/src/containers/pages/checkout/CartPage.js index dfd45864e6..c5e0058aad 100644 --- a/frontend/public/src/containers/pages/checkout/CartPage.js +++ b/frontend/public/src/containers/pages/checkout/CartPage.js @@ -27,7 +27,6 @@ import { import type { RouterHistory } from "react-router" import { isSuccessResponse } from "../../../lib/util" -import { isFinancialAssistanceAvailable } from "../../../lib/courseApi" import { addUserNotification } from "../../../actions" type Props = { @@ -137,9 +136,11 @@ export class CartPage extends React.Component { userFlexiblePriceExists === false ) { if ( - isFinancialAssistanceAvailable( - cartItems[0].product.purchasable_object.course - ) + cartItems[0].product && + cartItems[0].product.purchasable_object && + cartItems[0].product.purchasable_object.course.page && + cartItems[0].product.purchasable_object.course.page + .financial_assistance_form_url ) { return ( ({ // $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/departments.js b/frontend/public/src/lib/queries/departments.js new file mode 100644 index 0000000000..7a53da0ab5 --- /dev/null +++ b/frontend/public/src/lib/queries/departments.js @@ -0,0 +1,18 @@ +import { pathOr } from "ramda" + +import { nextState } from "./util" + +export const departmentsSelector = pathOr(null, ["entities", "departments"]) + +export const departmentsQueryKey = "departments" + +export const departmentsQuery = () => ({ + queryKey: departmentsQueryKey, + url: `/api/departments`, + transform: json => ({ + departments: json + }), + update: { + departments: nextState + } +}) 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 }),