Exclude courses without active, marketable runs from catalogs

The catalog API should only list courses which have at least one active and marketable course run.

ECOM-6473

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

@@ -149,22 +149,41 @@ class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixi
         """ Verify the endpoint returns the list of courses contained in the catalog. """
         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)
-        courses = [self.course]
+        included_courses = [self.course]
 
         # These courses/course runs should not be returned because they are no longer open for enrollment.
-        enrollment_end = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30)
+        past = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=30)
         excluded_runs = [
-            CourseRunFactory(enrollment_end=enrollment_end, course__title='ABC Test Course 2'),
-            CourseRunFactory(enrollment_end=enrollment_end, course=self.course),
+            # 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'
+        )
+
         with self.assertNumQueries(27):
             response = self.client.get(url)
+
         assert response.status_code == 200
-        assert response.data['results'] == self.serialize_catalog_course(courses, many=True)
+        assert response.data['results'] == self.serialize_catalog_course(included_courses, many=True)
 
     def test_contains_for_course_key(self):
         """

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

@@ -84,14 +84,12 @@ class CatalogViewSet(viewsets.ModelViewSet):
         """
         Retrieve the list of courses contained within this catalog.
 
-        Only courses with active course runs are returned. A course run is considered active if it is currently
-        open for enrollment, or will open in the future.
+        Only courses with at least one active and marketable course run are returned.
         ---
         serializer: serializers.CourseSerializerExcludingClosedRuns
         """
-
         catalog = self.get_object()
-        queryset = catalog.courses().active()
+        queryset = catalog.courses().active().marketable()
         queryset = prefetch_related_objects_for_courses(queryset)
 
         page = self.paginate_queryset(queryset)

course_discovery/apps/course_metadata/query.py

@@ -9,19 +9,41 @@ from course_discovery.apps.course_metadata.choices import CourseRunStatus, Progr
 
 class CourseQuerySet(models.QuerySet):
     def active(self):
-        """ Filters Courses to those with CourseRuns that are either currently open for enrollment,
-        or will be open for enrollment in the future. """
-
+        """
+        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.
+        """
         now = datetime.datetime.now(pytz.UTC)
         return self.filter(
             (
-                Q(course_runs__end__gt=now) | Q(course_runs__end__isnull=True)
+                Q(course_runs__end__gt=now) |
+                Q(course_runs__end__isnull=True)
             ) &
             (
-                Q(course_runs__enrollment_end__gt=now) | Q(course_runs__enrollment_end__isnull=True)
+                Q(course_runs__enrollment_end__gt=now) |
+                Q(course_runs__enrollment_end__isnull=True)
             )
         )
 
+    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.
+        """
+        # exclude() is intentionally avoided here. We want Courses to be included
+        # in the resulting queryset if only one of their runs matches our "marketable"
+        # 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()
+
 
 class CourseRunQuerySet(models.QuerySet):
     def active(self):

course_discovery/apps/course_metadata/tests/test_query.py

@@ -11,35 +11,67 @@ from course_discovery.apps.course_metadata.tests.factories import CourseRunFacto
 
 class CourseQuerySetTests(TestCase):
     def test_active(self):
-        """ Verify the method filters the Courses to those with active course runs. """
+        """
+        Verify the method filters Courses to those with active CourseRuns.
+        """
         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
+        # Create an active, enrollable course run
         active_course = CourseRunFactory(enrollment_end=open_enrollment_end, end=active_course_end).course
 
-        # Create an active unenrollable course
+        # Create an active, unenrollable course run
         CourseRunFactory(enrollment_end=closed_enrollment_end, end=active_course_end, course__title='ABC Test Course 2')
 
-        # Create an inactive unenrollable course
+        # Create an inactive, unenrollable course run
         CourseRunFactory(enrollment_end=closed_enrollment_end, end=inactive_course_end)
 
-        # Create an active course with unrestricted enrollment
+        # 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 with unrestricted enrollment
+        # Create an inactive course run with an unspecified enrollment end
         CourseRunFactory(enrollment_end=None, end=inactive_course_end)
 
-        # Create course with end date is NULL
+        # Create an enrollable course run with an unspecified end date
         course_without_end = CourseRunFactory(enrollment_end=open_enrollment_end, end=None).course
 
-        self.assertEqual(
-            set(Course.objects.active()),
-            {active_course, course_without_enrollment_end, course_without_end}
-        )
+        assert set(Course.objects.active()) == {
+            active_course, course_without_enrollment_end, course_without_end
+        }
+
+    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}
 
 
 @ddt.ddt