Filter relevant course runs to be live (#2244)

* Filter relevant course runs to be live

* Removing unused user

* updating

* tests

* tests

* small fix

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

cms/models.py

@@ -56,7 +56,7 @@
     SIGNATORY_INDEX_SLUG,
 )
 from cms.forms import CertificatePageForm
-from courses.api import get_user_relevant_course_run, get_user_relevant_course_run_qset
+from courses.api import get_relevant_course_run, get_relevant_course_run_qset
 from courses.models import (
     Course,
     CourseRun,
@@ -1203,17 +1203,8 @@ def get_admin_display_title(self):
         return f"{self.course.readable_id} | {self.title}"
 
     def get_context(self, request, *args, **kwargs):
-        relevant_run = get_user_relevant_course_run(
-            course=self.product, user=request.user
-        )
-        relevant_runs = list(
-            get_user_relevant_course_run_qset(course=self.product, user=request.user)
-        )
-        is_enrolled = (
-            False
-            if (relevant_run is None or not request.user.is_authenticated)
-            else (relevant_run.enrollments.filter(user_id=request.user.id).exists())
-        )
+        relevant_run = get_relevant_course_run(course=self.product)
+        relevant_runs = list(get_relevant_course_run_qset(course=self.product))
         sign_in_url = (
             None
             if request.user.is_authenticated
@@ -1236,7 +1227,6 @@ def get_context(self, request, *args, **kwargs):
             **get_base_context(request),
             "run": relevant_run,
             "course_runs": relevant_runs,
-            "is_enrolled": is_enrolled,
             "sign_in_url": sign_in_url,
             "start_date": start_date,
             "can_access_edx_course": can_access_edx_course,

cms/models_test.py

@@ -159,7 +159,6 @@ def test_course_page_context(  # noqa: PLR0913
         "request": request,
         "run": run,
         "course_runs": relevant_runs,
-        "is_enrolled": exp_is_enrolled,
         "sign_in_url": f"/signin/?next={quote_plus(course_page.get_url())}"
         if exp_sign_in_url
         else None,
@@ -216,7 +215,7 @@ def test_course_page_context_edx_access(  # noqa: PLR0913
         )
     )
     patched_get_relevant_run = mocker.patch(
-        "cms.models.get_user_relevant_course_run", return_value=run
+        "cms.models.get_relevant_course_run", return_value=run
     )
     if not is_authed:  # noqa: SIM108
         request_user = AnonymousUser()
@@ -231,9 +230,7 @@ def test_course_page_context_edx_access(  # noqa: PLR0913
     request.user = request_user
     context = course_page.get_context(request=request)
     assert context["can_access_edx_course"] is exp_can_access
-    patched_get_relevant_run.assert_called_once_with(
-        course=course_page.course, user=request_user
-    )
+    patched_get_relevant_run.assert_called_once_with(course=course_page.course)
 
 
 def generate_flexible_pricing_response(request_user, flexible_pricing_form):

courses/api.py

@@ -11,7 +11,7 @@
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ValidationError
 from django.db import IntegrityError, transaction
-from django.db.models import Count, Q
+from django.db.models import Q
 from django.db.models.query import QuerySet
 from mitol.common.utils import now_in_utc
 from mitol.common.utils.collections import (
@@ -60,7 +60,6 @@
     NoEdxApiAuthError,
     UnknownEdxApiEnrollException,
 )
-from users.models import User
 
 log = logging.getLogger(__name__)
 UserEnrollments = namedtuple(  # noqa: PYI024
@@ -77,91 +76,55 @@
 
 def _relevant_course_qset_filter(
     run_qset: QuerySet,
-    user: Optional[User],  # noqa: FA100
     now: Optional[datetime] = None,  # noqa: FA100
 ) -> QuerySet:
     """
     Does the actual filtering for user_relevant_course_run_qset and
     user_relevant_program_course_run_qset.
     """
 
-    if user and user.is_authenticated:
-        user_enrollments = Count(
-            "enrollments",
-            filter=Q(
-                enrollments__user=user,
-                enrollments__active=True,
-                enrollments__edx_enrolled=True,
-            ),
-        )
-        run_qset = run_qset.annotate(user_enrollments=user_enrollments).order_by(
-            "-user_enrollments", "enrollment_start"
-        )
-
-        verified_enrollments = Count(
-            "enrollments",
-            filter=Q(
-                enrollments__user=user,
-                enrollments__active=True,
-                enrollments__edx_enrolled=True,
-                enrollments__enrollment_mode=EDX_ENROLLMENT_VERIFIED_MODE,
-            ),
-        )
-        run_qset = run_qset.annotate(verified_enrollments=verified_enrollments)
-
-        runs = run_qset.filter(
-            Q(user_enrollments__gt=0)
-            | Q(enrollment_end=None)
-            | Q(enrollment_end__gt=now)
-        )
-    else:
-        runs = run_qset.filter(
-            Q(enrollment_end=None) | Q(enrollment_end__gt=now)
-        ).order_by("enrollment_start")
-    return runs
+    return (
+        run_qset.filter(Q(enrollment_end=None) | Q(enrollment_end__gt=now))
+        .exclude(start_date=None)
+        .exclude(enrollment_start=None)
+        .filter(live=True)
+        .order_by("enrollment_start")
+    )
 
 
-def get_user_relevant_course_run_qset(
+def get_relevant_course_run_qset(
     course: Course,
-    user: Optional[User],  # noqa: FA100
     now: Optional[datetime] = None,  # noqa: FA100
 ) -> QuerySet:
     """
     Returns a QuerySet of relevant course runs
     """
     now = now or now_in_utc()
-    run_qset = course.courseruns.exclude(start_date=None).exclude(enrollment_start=None)
-    return _relevant_course_qset_filter(run_qset, user, now)
+    run_qset = course.courseruns
+    return _relevant_course_qset_filter(run_qset, now)
 
 
 def get_user_relevant_program_course_run_qset(
     program: Program,
-    user: Optional[User],  # noqa: FA100
     now: Optional[datetime] = None,  # noqa: FA100
 ) -> QuerySet:
     """
     Returns a QuerySet of relevant course runs
     """
     now = now or now_in_utc()
-    run_qset = (
-        CourseRun.objects.filter(course__in=program.courses_qset.all())
-        .exclude(start_date=None)
-        .exclude(enrollment_start=None)
-    )
-    return _relevant_course_qset_filter(run_qset, user, now)
+    run_qset = CourseRun.objects.filter(course__in=program.courses_qset.all())
+    return _relevant_course_qset_filter(run_qset, now)
 
 
-def get_user_relevant_course_run(
+def get_relevant_course_run(
     course: Course,
-    user: Optional[User],  # noqa: FA100
-    now: Optional[datetime] = None,  # noqa: FA100
 ) -> CourseRun:
     """
     For a given Course, finds the course run that is the most relevant to the user.
     For anonymous users, this means the soonest enrollable course run.
     For logged-in users, this means an active course run that they're enrolled in, or the soonest enrollable course run.
     """
-    runs = get_user_relevant_course_run_qset(course, user, now)
+    runs = get_relevant_course_run_qset(course)
     run = first_or_none(runs)
     return run  # noqa: RET504
 

courses/api_test.py

@@ -22,8 +22,8 @@
     defer_enrollment,
     generate_course_run_certificates,
     generate_program_certificate,
+    get_relevant_course_run,
     get_user_enrollments,
-    get_user_relevant_course_run,
     manage_course_run_certificate_access,
     manage_program_certificate_access,
     override_user_grade,
@@ -119,10 +119,11 @@ def courses_api_logs(mocker):
 
 
 @pytest.mark.parametrize("is_enrolled", [True, False])
-def test_get_user_relevant_course_run(client, user, dates, course, is_enrolled):
+@pytest.mark.parametrize("is_live", [True, False])
+def test_get_relevant_course_run(user, dates, course, is_live, is_enrolled):
     """
-    get_user_relevant_course_run should return an enrolled run or the soonest course
-    run that does not have a past enrollment end date.
+    get_relevant_course_run should return the soonest course
+    run that is enrollable, even if the user is already enrolled.
     """
     # One run in the near future, one run in progress with an expired enrollment period, and one run in the far future.
     course_runs = CourseRunFactory.create_batch(
@@ -144,15 +145,20 @@ def test_get_user_relevant_course_run(client, user, dates, course, is_enrolled):
         CourseRunEnrollmentFactory.create(
             run=course_runs[1], user=user, edx_enrolled=True, active=True
         )
-    returned_run = get_user_relevant_course_run(course=course, user=user)
-    assert returned_run == (course_runs[1] if is_enrolled else course_runs[0])
+    returned_run = get_relevant_course_run(course=course)
+    assert returned_run == course_runs[0]
+    course_runs[0].live = is_live
+    course_runs[0].save()
+    returned_run = get_relevant_course_run(course=course)
+    assert returned_run == (course_runs[0] if is_live else course_runs[2])
 
 
-def test_get_user_relevant_course_run_invalid_dates(user, dates, course):
+def test_get_relevant_course_run_invalid_dates(user, dates, course):
     """
-    get_user_relevant_course_run should ignore course runs with any of the following properties when the user is not enrolled:
+    get_relevant_course_run should ignore course runs with any of the following properties when the user is not enrolled:
     1) No start date or enrollment start date
     2) An enrollment end date in the past
+    3) The course run is not live
 
     """
     CourseRunFactory.create_batch(
@@ -167,44 +173,11 @@ def test_get_user_relevant_course_run_invalid_dates(user, dates, course):
             [dates.future_60_days, None, dates.past_10_days]
         ),
     )
-    returned_run = get_user_relevant_course_run(course=course, user=user)
+    CourseRunFactory.create(course=course, live=False)
+    returned_run = get_relevant_course_run(course=course)
     assert returned_run is None
 
 
-def test_get_user_relevant_course_run_ignore_enrolled(user, dates, course):
-    """
-    get_user_relevant_course_run return a future course run if an enrolled run's end date is in the past, or if an
-    enrollment for an open course is not flagged as edX-enrolled
-    """
-    course_runs = CourseRunFactory.create_batch(
-        3,
-        course=course,
-        start_date=factory.Iterator(
-            [dates.past_30_days, dates.now, dates.future_10_days]
-        ),
-        end_date=factory.Iterator(
-            [dates.past_10_days, dates.future_10_days, dates.future_30_days]
-        ),
-        enrollment_start=factory.Iterator(
-            [dates.past_30_days, dates.past_30_days, dates.future_10_days]
-        ),
-        enrollment_end=factory.Iterator(
-            [dates.past_10_days, dates.past_10_days, dates.future_30_days]
-        ),
-    )
-    # Enroll in a past course run
-    CourseRunEnrollmentFactory.create(
-        run=course_runs[0], user=user, edx_enrolled=True, active=True
-    )
-    # Enroll in a currently-open course run with a closed enrollment period, but set to inactive
-    CourseRunEnrollmentFactory.create(
-        run=course_runs[1], user=user, edx_enrolled=False, active=True
-    )
-    returned_run = get_user_relevant_course_run(course=course, user=user)
-    # Returned course run should be the one the user is enrolled in that is active in edx
-    assert returned_run == course_runs[0]
-
-
 def test_get_user_enrollments(user, program_with_empty_requirements):  # noqa: F811
     """Test that get_user_enrollments returns an object with a user's program and course enrollments"""
     past_date = now_in_utc() - timedelta(days=1)

courses/views/v1/__init__.py

@@ -25,7 +25,7 @@
 from courses.api import (
     create_run_enrollments,
     deactivate_run_enrollment,
-    get_user_relevant_course_run_qset,
+    get_relevant_course_run_qset,
     get_user_relevant_program_course_run_qset,
 )
 from courses.constants import ENROLL_CHANGE_STATUS_UNENROLLED
@@ -193,21 +193,19 @@ def get_queryset(self):
         if relevant_to:
             course = Course.objects.filter(readable_id=relevant_to).first()
             if course:
-                return get_user_relevant_course_run_qset(course, self.request.user)
+                return get_relevant_course_run_qset(course)
             else:
                 program = Program.objects.filter(readable_id=relevant_to).first()
                 return (
-                    get_user_relevant_program_course_run_qset(
-                        program, self.request.user
-                    )
+                    get_user_relevant_program_course_run_qset(program)
                     if program
                     else Program.objects.none()
                 )
         else:
             return (
                 CourseRun.objects.select_related("course")
                 .prefetch_related("course__departments", "course__page")
-                .all()
+                .filter(live=True)
             )
 
     def get_serializer_context(self):

courses/views/v1/views_test.py

@@ -332,7 +332,7 @@ def test_get_course_runs_relevant(  # noqa: PLR0913
         ),
     )
     patched_run_qset = mocker.patch(
-        "courses.views.v1.get_user_relevant_course_run_qset",
+        "courses.views.v1.get_relevant_course_run_qset",
         return_value=CourseRun.objects.filter(id=course_run.id)
         .annotate(user_enrollments=user_enrollments)
         .order_by("-user_enrollments", "enrollment_start"),
@@ -346,7 +346,7 @@ def test_get_course_runs_relevant(  # noqa: PLR0913
             f"{reverse('v1:course_runs_api-list')}?relevant_to={course_run.course.readable_id}"
         )
     duplicate_queries_check(context)
-    patched_run_qset.assert_called_once_with(course_run.course, user)
+    patched_run_qset.assert_called_once_with(course_run.course)
     course_run_data = resp.json()[0]
 
     assert course_run_data["is_enrolled"] == is_enrolled