Clarify enrollable run (#2285)

* Clarify enrollable run * refactoring is_enrollable * removing old tests * [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>
mitodl · Jul 16, 2024 · 50e4711 · 50e4711
1 parent 504312c
commit 50e4711
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 208 deletions.
diff --git a/cms/models.py b/cms/models.py
@@ -56,7 +56,7 @@
     SIGNATORY_INDEX_SLUG,
 )
 from cms.forms import CertificatePageForm
-from courses.api import get_relevant_course_run, get_relevant_course_run_qset
+from courses.api import get_relevant_course_run_qset
 from courses.models import (
     Course,
     CourseRun,
@@ -1203,8 +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_relevant_course_run(course=self.product)
         relevant_runs = list(get_relevant_course_run_qset(course=self.product))
+        relevant_run = relevant_runs[0] if relevant_runs else None
         sign_in_url = (
             None
             if request.user.is_authenticated

diff --git a/cms/models_test.py b/cms/models_test.py
@@ -214,8 +214,8 @@ def test_course_page_context_edx_access(  # noqa: PLR0913
             **(dict(in_progress=True) if is_in_progress else dict(in_future=True)),  # noqa: C408
         )
     )
-    patched_get_relevant_run = mocker.patch(
-        "cms.models.get_relevant_course_run", return_value=run
+    patched_get_relevant_run_qset = mocker.patch(
+        "cms.models.get_relevant_course_run_qset", return_value=[run if run else None]
     )
     if not is_authed:  # noqa: SIM108
         request_user = AnonymousUser()
@@ -230,7 +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)
+    patched_get_relevant_run_qset.assert_called_once_with(course=course_page.course)
 
 
 def generate_flexible_pricing_response(request_user, flexible_pricing_form):

diff --git a/courses/api.py b/courses/api.py
@@ -3,9 +3,8 @@
 import itertools
 import logging
 from collections import namedtuple
-from datetime import datetime, timedelta
+from datetime import timedelta
 from traceback import format_exc
-from typing import List, Optional  # noqa: UP035
 
 from django.conf import settings
 from django.contrib.contenttypes.models import ContentType
@@ -40,6 +39,7 @@
 from courses.tasks import subscribe_edx_course_emails
 from courses.utils import (
     exception_logging_generator,
+    get_enrollable_courseruns_qs,
     is_grade_valid,
     is_letter_grade_valid,
 )
@@ -74,46 +74,26 @@
 )
 
 
-def _relevant_course_qset_filter(
-    run_qset: QuerySet,
-    now: Optional[datetime] = None,  # noqa: FA100
-) -> QuerySet:
-    """
-    Does the actual filtering for user_relevant_course_run_qset and
-    user_relevant_program_course_run_qset.
-    """
-
-    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_relevant_course_run_qset(
     course: Course,
-    now: Optional[datetime] = None,  # noqa: FA100
 ) -> QuerySet:
     """
     Returns a QuerySet of relevant course runs
     """
-    now = now or now_in_utc()
-    run_qset = course.courseruns
-    return _relevant_course_qset_filter(run_qset, now)
+    enrollable_run_qset = get_enrollable_courseruns_qs(valid_courses=[course])
+    return enrollable_run_qset.order_by("enrollment_start")
 
 
 def get_user_relevant_program_course_run_qset(
     program: Program,
-    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())
-    return _relevant_course_qset_filter(run_qset, now)
+    enrollable_run_qset = get_enrollable_courseruns_qs(
+        valid_courses=program.courses_qset.all()
+    )
+    return enrollable_run_qset.order_by("enrollment_start")
 
 
 def get_relevant_course_run(
@@ -666,7 +646,7 @@ def sync_course_runs(runs):
     return success_count, failure_count
 
 
-def sync_course_mode(runs: List[CourseRun]) -> List[str]:  # noqa: UP006
+def sync_course_mode(runs: list[CourseRun]) -> list[str]:
     """
     Updates course run upgrade expiration dates from Open edX
 

diff --git a/courses/api_test.py b/courses/api_test.py
@@ -134,10 +134,10 @@ def test_get_relevant_course_run(user, dates, course, is_live, is_enrolled):
         ),
         end_date=factory.Iterator([None, dates.future_10_days, dates.future_60_days]),
         enrollment_start=factory.Iterator(
-            [dates.future_10_days, dates.past_60_days, dates.future_30_days]
+            [dates.past_60_days, dates.past_10_days, dates.past_30_days]
         ),
         enrollment_end=factory.Iterator(
-            [None, dates.past_30_days, dates.future_60_days]
+            [None, dates.future_30_days, dates.future_60_days]
         ),
     )
     if is_enrolled:

diff --git a/courses/models.py b/courses/models.py
@@ -3,7 +3,6 @@
 """
 
 import logging
-import operator as op
 import uuid
 from decimal import ROUND_HALF_EVEN, Decimal
 
@@ -350,7 +349,7 @@ def courses(self):
         heap = []
         main_ops = ProgramRequirement.objects.filter(program=self, depth=2).all()
 
-        for op in main_ops:  # noqa: F402
+        for op in main_ops:
             reqs = (
                 ProgramRequirement.objects.filter(
                     program__id=self.id,
@@ -567,7 +566,7 @@ def active_products(self):
             relevant_run.products.filter(is_active=True).all() if relevant_run else None
         )
 
-    @property
+    @cached_property
     def first_unexpired_run(self):
         """
         Gets the first unexpired CourseRun associated with this Course
@@ -581,27 +580,13 @@ def first_unexpired_run(self):
         """
         course_runs = self.courseruns.all()
         eligible_course_runs = [
-            course_run
-            for course_run in course_runs
-            if course_run.live and course_run.start_date and course_run.is_unexpired
+            course_run for course_run in course_runs if course_run.is_enrollable
         ]
         return first_matching_item(
             sorted(eligible_course_runs, key=lambda course_run: course_run.start_date),
             lambda course_run: True,  # noqa: ARG005
         )
 
-    @property
-    def unexpired_runs(self):
-        """
-        Gets all the unexpired CourseRuns associated with this Course
-        """
-        return list(
-            filter(
-                op.attrgetter("is_unexpired"),
-                self.courseruns.filter(live=True).order_by("start_date"),
-            )
-        )
-
     @cached_property
     def programs(self):
         """
@@ -641,22 +626,6 @@ def is_country_blocked(self, user):
             country=user.legal_address.country
         ).exists()
 
-    def available_runs(self, user):
-        """
-        Get all enrollable runs for a Course that a user has not already enrolled in.
-
-        Args:
-            user (users.models.User): The user to check available runs for.
-
-        Returns:
-            list of CourseRun: Unexpired and unenrolled Course runs
-
-        """
-        enrolled_runs = user.courserunenrollment_set.filter(
-            run__course=self
-        ).values_list("run__id", flat=True)
-        return [run for run in self.unexpired_runs if run.id not in enrolled_runs]
-
     def __str__(self):
         title = f"{self.readable_id} | {self.title}"
         return title if len(title) <= 100 else title[:97] + "..."  # noqa: PLR2004
@@ -745,30 +714,6 @@ def is_past(self):
             return False
         return self.end_date < now_in_utc()
 
-    @property
-    def is_enrollable(self):
-        """
-        Checks if the course is not beyond its enrollment period
-
-
-        Returns:
-            boolean: True if enrollment period has begun but not ended
-        """
-        now = now_in_utc()
-        return (self.enrollment_end is None or self.enrollment_end > now) and (
-            self.enrollment_start is None or self.enrollment_start <= now
-        )
-
-    @property
-    def is_unexpired(self):
-        """
-        Checks if the course is not expired
-
-        Returns:
-            boolean: True if course is not expired
-        """
-        return not self.is_past and self.is_enrollable
-
     @property
     def is_in_progress(self) -> bool:
         """
@@ -789,6 +734,20 @@ def is_upgradable(self):
         """
         return self.upgrade_deadline is None or (self.upgrade_deadline > now_in_utc())
 
+    @cached_property
+    def is_enrollable(self):
+        """
+        Determines if a run is enrollable
+        """
+        now = now_in_utc()
+        return (
+            (self.enrollment_end is None or self.enrollment_end > now)
+            and self.enrollment_start is not None
+            and self.enrollment_start <= now
+            and self.live is True
+            and self.start_date is not None
+        )
+
     @property
     def courseware_url(self):
         """

diff --git a/courses/models_test.py b/courses/models_test.py
@@ -2,7 +2,6 @@
 
 from datetime import timedelta
 
-import factory
 import pytest
 from django.core.exceptions import ValidationError
 from mitol.common.utils.datetime import now_in_utc
@@ -141,44 +140,6 @@ def test_course_run_invalid_expiration_date(start_delta, end_delta, expiration_d
         )
 
 
-@pytest.mark.parametrize(
-    "end_days, enroll_start_days, enroll_end_days, expected",  # noqa: PT006
-    [
-        [None, None, None, True],  # noqa: PT007
-        [None, None, 1, True],  # noqa: PT007
-        [None, None, -1, False],  # noqa: PT007
-        [1, None, None, True],  # noqa: PT007
-        [-1, None, None, True],  # noqa: PT007
-        [1, None, -1, False],  # noqa: PT007
-        [None, 1, None, False],  # noqa: PT007
-        [None, -1, None, True],  # noqa: PT007
-    ],
-)
-def test_course_run_is_enrollable(
-    end_days, enroll_start_days, enroll_end_days, expected
-):
-    """
-    Test that CourseRun.is_beyond_enrollment returns the expected boolean value
-    """
-    now = now_in_utc()
-    end_date = None if end_days is None else now + timedelta(days=end_days)
-    enr_end_date = (
-        None if enroll_end_days is None else now + timedelta(days=enroll_end_days)
-    )
-    enr_start_date = (
-        None if enroll_start_days is None else now + timedelta(days=enroll_start_days)
-    )
-
-    assert (
-        CourseRunFactory.create(
-            end_date=end_date,
-            enrollment_end=enr_end_date,
-            enrollment_start=enr_start_date,
-        ).is_enrollable
-        is expected
-    )
-
-
 @pytest.mark.parametrize(
     "start_delta, end_delta, expected_result",  # noqa: PT006
     [
@@ -205,25 +166,6 @@ def test_course_run_in_progress(start_delta, end_delta, expected_result):
     )
 
 
-@pytest.mark.parametrize(
-    "end_days,enroll_days,expected",  # noqa: PT006
-    [[-1, 1, False], [1, -1, False], [1, 1, True]],  # noqa: PT007
-)
-def test_course_run_unexpired(end_days, enroll_days, expected):
-    """
-    Test that CourseRun.is_unexpired returns the expected boolean value
-    """
-    now = now_in_utc()
-    end_date = now + timedelta(days=end_days)
-    enr_end_date = now + timedelta(days=enroll_days)
-    assert (
-        CourseRunFactory.create(
-            end_date=end_date, enrollment_end=enr_end_date
-        ).is_unexpired
-        is expected
-    )
-
-
 def test_course_first_unexpired_run():
     """
     Test that the first unexpired run of a course is returned
@@ -291,42 +233,6 @@ def test_program_first_unexpired_run():
     assert program.first_unexpired_run == first_run
 
 
-def test_course_unexpired_runs():
-    """unexpired_runs should return expected value"""
-    course = CourseFactory.create()
-    now = now_in_utc()
-    start_dates = [now, now + timedelta(days=-3)]
-    end_dates = [now + timedelta(hours=1), now + timedelta(days=-2)]
-    CourseRunFactory.create_batch(
-        2,
-        course=course,
-        start_date=factory.Iterator(start_dates),
-        end_date=factory.Iterator(end_dates),
-        live=True,
-    )
-
-    # Add a run that is not live and shouldn't show up in unexpired list
-    CourseRunFactory.create(
-        course=course, start_date=start_dates[0], end_date=end_dates[0], live=False
-    )
-
-    assert len(course.unexpired_runs) == 1
-    course_run = course.unexpired_runs[0]
-    assert course_run.start_date == start_dates[0]
-    assert course_run.end_date == end_dates[0]
-
-
-def test_course_available_runs():
-    """Enrolled runs for a user should not be in the list of available runs"""
-    user = UserFactory.create()
-    course = CourseFactory.create()
-    runs = CourseRunFactory.create_batch(2, course=course, live=True)
-    runs.sort(key=lambda run: run.start_date)
-    CourseRunEnrollmentFactory.create(run=runs[0], user=user)
-    assert course.available_runs(user) == [runs[1]]
-    assert course.available_runs(UserFactory.create()) == runs
-
-
 def test_reactivate_and_save():
     """Test that the reactivate_and_save method in enrollment models sets properties and saves"""
     course_run_enrollment = CourseRunEnrollmentFactory.create(