Courses appearing in catalogs must be available

A course is considered to be "available" if it contains at least one course run that can be enrolled in immediately, is ongoing or yet to start, and appears on the marketing site. The catalog API should only list courses which are available. An earlier attempt at this fell short because it filtered courses to those which contained at least one active *or* marketable run.

ECOM-6473

course_discovery/apps/api/serializers.py

@@ -477,8 +477,12 @@ class MinimalCourseSerializer(TimestampModelSerializer):
     image = ImageField(read_only=True, source='card_image_url')
 
     @classmethod
-    def prefetch_queryset(cls):
-        return Course.objects.all().select_related('partner').prefetch_related(
+    def prefetch_queryset(cls, queryset=None):
+        # Explicitly check for None to avoid returning all Courses when the
+        # queryset passed in happens to be empty.
+        queryset = queryset if queryset is not None else Course.objects.all()
+
+        return queryset.select_related('partner').prefetch_related(
             'authoring_organizations',
             Prefetch('course_runs', queryset=MinimalCourseRunSerializer.prefetch_queryset()),
         )
@@ -501,8 +505,12 @@ class CourseSerializer(MinimalCourseSerializer):
     marketing_url = serializers.SerializerMethodField()
 
     @classmethod
-    def prefetch_queryset(cls):
-        return Course.objects.all().select_related('level_type', 'video', 'partner').prefetch_related(
+    def prefetch_queryset(cls, queryset=None):
+        # Explicitly check for None to avoid returning all Courses when the
+        # queryset passed in happens to be empty.
+        queryset = queryset if queryset is not None else Course.objects.all()
+
+        return queryset.select_related('level_type', 'video', 'partner').prefetch_related(
             'expected_learning_items',
             'prerequisites',
             'subjects',
@@ -581,12 +589,23 @@ class CourseWithProgramsSerializer(CourseSerializer):
         fields = CourseSerializer.Meta.fields + ('programs',)
 
 
-class CourseSerializerExcludingClosedRuns(CourseSerializer):
-    """A ``CourseSerializer`` which only includes active course runs, as determined by ``CourseQuerySet``."""
+class CatalogCourseSerializer(CourseSerializer):
+    """
+    A CourseSerializer which only includes course runs that can be enrolled in
+    immediately, are ongoing or yet to start, and appear on the marketing site
+    (i.e., sellable runs that should appear in a catalog distributed to affiliates).
+    """
     course_runs = serializers.SerializerMethodField()
 
     def get_course_runs(self, course):
-        return CourseRunSerializer(course.course_runs.active().marketable(), many=True, context=self.context).data
+        return CourseRunSerializer(
+            # TODO: These queryset methods chain filter() and exclude() calls,
+            # causing prefetched results to be discarded. They should be replaced
+            # with Python-based filtering that preserves the prefetched data.
+            course.course_runs.active().enrollable().marketable(),
+            many=True,
+            context=self.context
+        ).data
 
 
 class ContainedCoursesSerializer(serializers.Serializer):

course_discovery/apps/api/v1/tests/test_views/mixins.py

@@ -7,7 +7,7 @@ from django.conf import settings
 from rest_framework.test import APIRequestFactory
 
 from course_discovery.apps.api.serializers import (
-    CatalogSerializer, CourseRunWithProgramsSerializer, CourseSerializerExcludingClosedRuns,
+    CatalogCourseSerializer, CatalogSerializer, CourseRunWithProgramsSerializer,
     CourseWithProgramsSerializer, FlattenedCourseRunWithCourseSerializer, MinimalProgramSerializer,
     OrganizationSerializer, PersonSerializer, ProgramSerializer, ProgramTypeSerializer
 )
@@ -57,7 +57,7 @@ class SerializationMixin(object):
         return self._serialize_object(ProgramTypeSerializer, program_type, many, format, extra_context)
 
     def serialize_catalog_course(self, course, many=False, format=None, extra_context=None):
-        return self._serialize_object(CourseSerializerExcludingClosedRuns, course, many, format, extra_context)
+        return self._serialize_object(CatalogCourseSerializer, course, many, format, extra_context)
 
     def serialize_catalog_flat_course_run(self, course_run, many=False, format=None, extra_context=None):
         return self._serialize_object(FlattenedCourseRunWithCourseSerializer, course_run, many, format, extra_context)

course_discovery/apps/api/v1/tests/test_views/test_catalogs.py

@@ -3,6 +3,7 @@ import datetime
 import urllib
 
 import ddt
+import pytest
 import pytz
 import responses
 from django.contrib.auth import get_user_model
@@ -15,12 +16,14 @@ from course_discovery.apps.catalogs.models import Catalog
 from course_discovery.apps.catalogs.tests.factories import CatalogFactory
 from course_discovery.apps.core.tests.factories import UserFactory
 from course_discovery.apps.core.tests.mixins import ElasticsearchTestMixin
+from course_discovery.apps.course_metadata.models import Course
 from course_discovery.apps.course_metadata.tests.factories import CourseRunFactory, SeatFactory
 
 User = get_user_model()
 
 
 @ddt.ddt
+@pytest.mark.usefixtures('course_run_states')
 class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixin, APITestCase):
     """ Tests for the catalog resource. """
     catalog_list_url = reverse('api:v1:catalog-list')
@@ -130,8 +133,8 @@ class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixi
 
         catalog = Catalog.objects.latest()
         latest_user = User.objects.latest()
-        self.assertEqual(latest_user.username, new_viewer_username)
-        self.assertListEqual(list(catalog.viewers), [existing_viewer, latest_user])
+        assert latest_user.username == new_viewer_username
+        assert set(catalog.viewers) == {existing_viewer, latest_user}
 
     def test_create_with_new_user_error(self):
         """ Verify no users are created if an error occurs while processing a create request. """
@@ -146,44 +149,43 @@ class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixi
         self.assertEqual(User.objects.count(), original_user_count)
 
     def test_courses(self):
-        """ Verify the endpoint returns the list of courses contained in the catalog. """
+        """
+        Verify the endpoint returns the list of available courses contained in
+        the catalog, and that courses appearing in the response always have at
+        least one serialized run.
+        """
         url = reverse('api:v1:catalog-courses', kwargs={'id': self.catalog.id})
 
-        # This run is published, has seats, and has a valid slug. We expect its
-        # parent course to be included in the response.
-        SeatFactory(course_run=self.course_run)
-        included_courses = [self.course]
-
-        # These courses/course runs should not be returned because they are no longer open for enrollment.
-        past = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30)
-        excluded_runs = [
-            # Despite the enrollment end date for this run being in the past, we
-            # still expect the parent course to be in the response. It had an active
-            # run associated with it above.
-            CourseRunFactory(enrollment_end=past, course=self.course),
-            # The course associated with this run is included in the catalog query,
-            # but the run is not active, so we don't expect to see the associated
-            # course in the response.
-            CourseRunFactory(enrollment_end=past, course__title='ABC Test Course 2'),
-        ]
-        for course_run in excluded_runs:
-            SeatFactory(course_run=course_run)
-
-        # The course associated with this run is included in the catalog query,
-        # but the run is not marketable (no seats), so we don't expect to see the
-        # associated course in the response.
-        future = datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=30)
-        CourseRunFactory(
-            enrollment_end=future,
-            end=future,
-            course__title='ABC Test Course 3'
-        )
+        for state in self.states():
+            Course.objects.all().delete()
 
-        with self.assertNumQueries(27):
-            response = self.client.get(url)
+            course_run = CourseRunFactory(course__title='ABC Test Course')
+            for function in state:
+                function(course_run)
+
+            course_run.save()
+
+            if state in self.available_states:
+                course = course_run.course
+
+                # This run has no seats, but we still expect its parent course
+                # to be included.
+                CourseRunFactory(course=course)
+
+                with self.assertNumQueries(26):
+                    response = self.client.get(url)
+
+                assert response.status_code == 200
+                assert response.data['results'] == self.serialize_catalog_course([course], many=True)
+
+                # Any course appearing in the response must have at least one serialized run.
+                assert len(response.data['results'][0]['course_runs']) > 0
+            else:
+                with self.assertNumQueries(3):
+                    response = self.client.get(url)
 
-        assert response.status_code == 200
-        assert response.data['results'] == self.serialize_catalog_course(included_courses, many=True)
+                assert response.status_code == 200
+                assert response.data['results'] == []
 
     def test_contains_for_course_key(self):
         """

course_discovery/apps/api/v1/views/catalogs.py

@@ -10,7 +10,7 @@ from rest_framework.response import Response
 from course_discovery.apps.api import filters, serializers
 from course_discovery.apps.api.pagination import ProxiedPagination
 from course_discovery.apps.api.renderers import CourseRunCSVRenderer
-from course_discovery.apps.api.v1.views import User, prefetch_related_objects_for_courses
+from course_discovery.apps.api.v1.views import User
 from course_discovery.apps.catalogs.models import Catalog
 from course_discovery.apps.course_metadata.models import CourseRun
 
@@ -86,14 +86,14 @@ class CatalogViewSet(viewsets.ModelViewSet):
 
         Only courses with at least one active and marketable course run are returned.
         ---
-        serializer: serializers.CourseSerializerExcludingClosedRuns
+        serializer: serializers.CatalogCourseSerializer
         """
         catalog = self.get_object()
-        queryset = catalog.courses().active().marketable()
-        queryset = prefetch_related_objects_for_courses(queryset)
+        queryset = catalog.courses().available()
+        queryset = serializers.CatalogCourseSerializer.prefetch_queryset(queryset=queryset)
 
         page = self.paginate_queryset(queryset)
-        serializer = serializers.CourseSerializerExcludingClosedRuns(page, many=True, context={'request': request})
+        serializer = serializers.CatalogCourseSerializer(page, many=True, context={'request': request})
         return self.get_paginated_response(serializer.data)
 
     @detail_route()

course_discovery/apps/course_metadata/query.py

@@ -8,16 +8,20 @@ from course_discovery.apps.course_metadata.choices import CourseRunStatus, Progr
 
 
 class CourseQuerySet(models.QuerySet):
-    def active(self):
+    def available(self):
         """
-        Filter Courses to those with CourseRuns that have not yet ended and are
-        either open for enrollment or will be open for enrollment in the future.
+        A Course is considered to be "available" if it contains at least one CourseRun
+        that can be enrolled in immediately, is ongoing or yet to start, and appears
+        on the marketing site.
         """
         now = datetime.datetime.now(pytz.UTC)
-        return self.filter(
+
+        # A CourseRun is "enrollable" if its enrollment start date has passed,
+        # is now, or is None, and its enrollment end date is in the future or is None.
+        enrollable = (
             (
-                Q(course_runs__end__gt=now) |
-                Q(course_runs__end__isnull=True)
+                Q(course_runs__enrollment_start__lte=now) |
+                Q(course_runs__enrollment_start__isnull=True)
             ) &
             (
                 Q(course_runs__enrollment_end__gt=now) |
@@ -25,24 +29,28 @@ class CourseQuerySet(models.QuerySet):
             )
         )
 
-    def marketable(self):
-        """
-        Filter Courses to those with CourseRuns that can be marketed. A CourseRun
-        is deemed marketable if it has a defined slug, has seats, and has been published.
-        """
+        # A CourseRun is "not ended" if its end date is in the future or is None.
+        not_ended = (
+            Q(course_runs__end__gt=now) | Q(course_runs__end__isnull=True)
+        )
+
+        # A CourseRun is "marketable" if it has a non-empty slug, has seats, and
+        # is has a "published" status.
+        marketable = (
+            (
+                Q(course_runs__slug__isnull=False) & ~Q(course_runs__slug='')
+            ) &
+            Q(course_runs__seats__isnull=False) &
+            Q(course_runs__status=CourseRunStatus.Published)
+        )
+
         # exclude() is intentionally avoided here. We want Courses to be included
-        # in the resulting queryset if only one of their runs matches our "marketable"
+        # in the resulting queryset if at least one of their runs matches our availability
         # criteria. For example, consider a Course with two CourseRuns; one of the
         # runs is published while the other is not. If you used exclude(), the Course
         # would be dropped from the queryset even though it has one run which matches
-        # our criteria.
-        return self.filter(
-            Q(course_runs__slug__isnull=False) & ~Q(course_runs__slug='')
-        ).filter(
-            course_runs__seats__isnull=False
-        ).filter(
-            course_runs__status=CourseRunStatus.Published
-        ).distinct()
+        # our availability criteria.
+        return self.filter(enrollable & not_ended & marketable)
 
 
 class CourseRunQuerySet(models.QuerySet):

course_discovery/apps/course_metadata/tests/test_query.py

+# pylint: disable=no-member
 import datetime
 
 import ddt
+import pytest
 import pytz
 from django.test import TestCase
 
@@ -9,69 +11,33 @@ from course_discovery.apps.course_metadata.models import Course, CourseRun, Prog
 from course_discovery.apps.course_metadata.tests.factories import CourseRunFactory, ProgramFactory, SeatFactory
 
 
+@pytest.mark.usefixtures('course_run_states')
 class CourseQuerySetTests(TestCase):
-    def test_active(self):
+    def test_available(self):
         """
-        Verify the method filters Courses to those with active CourseRuns.
+        Verify the method filters Courses to those which contain at least one
+        CourseRun that can be enrolled in immediately, is ongoing or yet to start,
+        and appears on the marketing site.
         """
-        now = datetime.datetime.now(pytz.UTC)
-        active_course_end = now + datetime.timedelta(days=60)
-        inactive_course_end = now - datetime.timedelta(days=15)
-        open_enrollment_end = now + datetime.timedelta(days=30)
-        closed_enrollment_end = now - datetime.timedelta(days=30)
-
-        # Create an active, enrollable course run
-        active_course = CourseRunFactory(enrollment_end=open_enrollment_end, end=active_course_end).course
+        for state in self.states():
+            Course.objects.all().delete()
 
-        # Create an active, unenrollable course run
-        CourseRunFactory(enrollment_end=closed_enrollment_end, end=active_course_end, course__title='ABC Test Course 2')
+            course_run = CourseRunFactory()
+            for function in state:
+                function(course_run)
 
-        # Create an inactive, unenrollable course run
-        CourseRunFactory(enrollment_end=closed_enrollment_end, end=inactive_course_end)
-
-        # Create an active course run with an unspecified enrollment end
-        course_without_enrollment_end = CourseRunFactory(enrollment_end=None, end=active_course_end).course
-
-        # Create an inactive course run with an unspecified enrollment end
-        CourseRunFactory(enrollment_end=None, end=inactive_course_end)
+            course_run.save()
 
-        # Create an enrollable course run with an unspecified end date
-        course_without_end = CourseRunFactory(enrollment_end=open_enrollment_end, end=None).course
+            if state in self.available_states:
+                course = course_run.course
 
-        assert set(Course.objects.active()) == {
-            active_course, course_without_enrollment_end, course_without_end
-        }
+                # This run has no seats, but we still expect its parent course
+                # to be included.
+                CourseRunFactory(course=course)
 
-    def test_marketable(self):
-        """
-        Verify the method filters Courses to those with marketable CourseRuns.
-        """
-        # Courses whose runs have null or empty slugs are excluded, even if
-        # those runs are published and have seats.
-        for invalid_slug in (None, ''):
-            excluded_course_run = CourseRunFactory(slug=invalid_slug)
-            SeatFactory(course_run=excluded_course_run)
-
-        # Courses whose runs have no seats are excluded, even if those runs
-        # are published and have valid slugs.
-        CourseRunFactory()
-
-        # Courses whose runs are unpublished are excluded, even if those runs
-        # have seats and valid slugs.
-        excluded_course_run = CourseRunFactory(status=CourseRunStatus.Unpublished)
-        SeatFactory(course_run=excluded_course_run)
-
-        # Courses with at least one run that is published and has seats and a valid
-        # slug are included.
-        included_course_run = CourseRunFactory()
-        SeatFactory(course_run=included_course_run)
-
-        included_course = included_course_run.course
-        # This run has no seats and will be excluded, but we still expect its parent
-        # course to be included.
-        CourseRunFactory(course=included_course)
-
-        assert set(Course.objects.marketable()) == {included_course}
+                assert set(Course.objects.available()) == {course}
+            else:
+                assert set(Course.objects.available()) == set()
 
 
 @ddt.ddt

course_discovery/conftest.py

+import datetime
+from functools import partial
+from itertools import product
+
+import pytest
+import pytz
+
+from course_discovery.apps.course_metadata.choices import CourseRunStatus
+from course_discovery.apps.course_metadata.tests.factories import SeatFactory
+
+
+@pytest.fixture(scope='class')
+def course_run_states(request):
+    """
+    pytest fixture for providing test classes with attributes necessary to create
+    and test CourseRuns in all states affecting availability.
+    """
+    now = datetime.datetime.now(pytz.UTC)
+    past = now - datetime.timedelta(days=30)
+    future = now + datetime.timedelta(days=30)
+
+    def enrollment_start_null(course_run):
+        course_run.enrollment_start = None
+
+    def enrollment_start_past(course_run):
+        course_run.enrollment_start = past
+
+    def enrollment_start_future(course_run):
+        course_run.enrollment_start = future
+
+    def enrollment_end_null(course_run):
+        course_run.enrollment_end = None
+
+    def enrollment_end_past(course_run):
+        course_run.enrollment_end = past
+
+    def enrollment_end_future(course_run):
+        course_run.enrollment_end = future
+
+    def end_null(course_run):
+        course_run.end = None
+
+    def end_past(course_run):
+        course_run.end = past
+
+    def end_future(course_run):
+        course_run.end = future
+
+    def slug_null(course_run):
+        course_run.slug = None
+
+    def slug_blank(course_run):
+        course_run.slug = ''
+
+    def slug_valid(course_run):
+        course_run.slug = 'foo'
+
+    def seats_null(course_run):  # pylint: disable=unused-argument
+        pass
+
+    def seats_exist(course_run):
+        SeatFactory(course_run=course_run)
+
+    def published(course_run):
+        course_run.status = CourseRunStatus.Published
+
+    def unpublished(course_run):
+        course_run.status = CourseRunStatus.Unpublished
+
+    # The Cartesian product of these lists represents the 324 possible course
+    # run states that affect a parent course's availability.
+    states = [
+        [
+            enrollment_start_null,
+            enrollment_start_past,
+            enrollment_start_future
+        ],
+        [
+            enrollment_end_null,
+            enrollment_end_past,
+            enrollment_end_future
+        ],
+        [
+            end_null,
+            end_past,
+            end_future
+        ],
+        [
+            slug_null,
+            slug_blank,
+            slug_valid
+        ],
+        [
+            seats_null,
+            seats_exist
+        ],
+        [
+            published,
+            unpublished
+        ]
+    ]
+
+    # The Cartesian product of these lists represents the 8 possible course
+    # run states that yield an available parent course.
+    available_states = [
+        [
+            enrollment_start_null,
+            enrollment_start_past
+        ],
+        [
+            enrollment_end_null,
+            enrollment_end_future
+        ],
+        [
+            end_null,
+            end_future
+        ],
+        [
+            slug_valid
+        ],
+        [
+            seats_exist
+        ],
+        [
+            published
+        ]
+    ]
+
+    # Set class attributes on the invoking test context.
+    request.cls.states = partial(product, *states)
+    request.cls.available_states = list(product(*available_states))