From 50e4711eba6a0f91b1cd97d15f20b323032cbf79 Mon Sep 17 00:00:00 2001 From: Anna Gavrilman Date: Tue, 16 Jul 2024 18:51:21 +0200 Subject: [PATCH] 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> --- cms/models.py | 4 +- cms/models_test.py | 6 +- courses/api.py | 38 +++-------- courses/api_test.py | 4 +- courses/models.py | 75 +++++---------------- courses/models_test.py | 94 --------------------------- courses/serializers/v2/departments.py | 9 +-- courses/utils.py | 33 ++++++---- 8 files changed, 55 insertions(+), 208 deletions(-) diff --git a/cms/models.py b/cms/models.py index ec47cf8fef..bcb2ea9041 100644 --- 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 index b517f4afbf..8b1c565aea 100644 --- 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 index 2cef959d04..dcbda04687 100644 --- 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 index a537fb23ab..b68c9d3ea1 100644 --- 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 index 346a4357bf..d3bad57126 100644 --- 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 index 04d8b3d052..6dfdabbe14 100644 --- 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( diff --git a/courses/serializers/v2/departments.py b/courses/serializers/v2/departments.py index 07ee25004e..9d2b4e86ce 100644 --- a/courses/serializers/v2/departments.py +++ b/courses/serializers/v2/departments.py @@ -1,8 +1,8 @@ from django.db.models import Q -from mitol.common.utils import now_in_utc from rest_framework import serializers from courses.models import CourseRun, Department +from courses.utils import get_enrollable_course_run_filter class DepartmentSerializer(serializers.ModelSerializer): @@ -32,14 +32,9 @@ def get_course_ids(self, instance): Returns: list: Course IDs associated with the Department. """ - now = now_in_utc() related_courses = instance.course_set.filter(live=True, page__live=True) relevant_courseruns = CourseRun.objects.filter( - Q(course__in=related_courses) - & Q(live=True) - & Q(start_date__isnull=False) - & Q(enrollment_start__lt=now) - & (Q(enrollment_end=None) | Q(enrollment_end__gt=now)) + get_enrollable_course_run_filter() & Q(course__in=related_courses) ).values_list("id", flat=True) return ( related_courses.filter(courseruns__id__in=relevant_courseruns) diff --git a/courses/utils.py b/courses/utils.py index 347d71eb46..8ea1f7d432 100644 --- a/courses/utils.py +++ b/courses/utils.py @@ -76,9 +76,9 @@ def get_program_certificate_by_enrollment(enrollment, program=None): return None -def get_enrollable_courseruns_qs(enrollment_end_date=None, valid_courses=None): +def get_enrollable_course_run_filter(enrollment_end_date=None, valid_courses=None): """ - Returns all course runs that are open for enrollment. + Returns a queryset of all course runs that are open for enrollment. args: enrollment_end_date: datetime, the date to check for enrollment end if a future date is needed @@ -88,7 +88,7 @@ def get_enrollable_courseruns_qs(enrollment_end_date=None, valid_courses=None): if enrollment_end_date is None: enrollment_end_date = now - valid_course_runs = CourseRun.objects.filter( + q_filters = ( Q(live=True) & Q(start_date__isnull=False) & Q(enrollment_start__lt=now) @@ -96,9 +96,22 @@ def get_enrollable_courseruns_qs(enrollment_end_date=None, valid_courses=None): ) if valid_courses: - return valid_course_runs.filter(course__in=valid_courses) + q_filters = q_filters & Q(course__in=valid_courses) + + return q_filters + + +def get_enrollable_courseruns_qs(enrollment_end_date=None, valid_courses=None): + """ + Returns all course runs that are open for enrollment. + + args: + enrollment_end_date: datetime, the date to check for enrollment end if a future date is needed + valid_courses: Queryset of Course objects, to filter the course runs by if needed + """ + q_filters = get_enrollable_course_run_filter(enrollment_end_date, valid_courses) - return valid_course_runs + return CourseRun.objects.filter(q_filters) def get_unenrollable_courseruns_qs(): @@ -117,14 +130,8 @@ def get_self_paced_courses(queryset, enrollment_end_date=None): if enrollment_end_date is None: enrollment_end_date = now course_ids = queryset.values_list("id", flat=True) - all_runs = CourseRun.objects.filter( - Q(live=True) - & Q(course_id__in=course_ids) - & Q(start_date__isnull=False) - & Q(enrollment_start__lt=now) - & (Q(enrollment_end=None) | Q(enrollment_end__gt=enrollment_end_date)) - ) - self_paced_runs = all_runs.filter(is_self_paced=True) + all_enrollable_runs = get_enrollable_courseruns_qs(valid_courses=course_ids) + self_paced_runs = all_enrollable_runs.filter(is_self_paced=True) return ( queryset.prefetch_related(Prefetch("courseruns", queryset=self_paced_runs)) .prefetch_related("courseruns__course")