diff --git a/cms/models.py b/cms/models.py index 1c617a8586..77b591fe70 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_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, diff --git a/cms/models_test.py b/cms/models_test.py index b1854b3093..b517f4afbf 100644 --- a/cms/models_test.py +++ b/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): diff --git a/courses/api.py b/courses/api.py index d20b24c8ff..2cef959d04 100644 --- a/courses/api.py +++ b/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,7 +76,6 @@ def _relevant_course_qset_filter( run_qset: QuerySet, - user: Optional[User], # noqa: FA100 now: Optional[datetime] = None, # noqa: FA100 ) -> QuerySet: """ @@ -85,83 +83,48 @@ def _relevant_course_qset_filter( 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 diff --git a/courses/api_test.py b/courses/api_test.py index eddb4a8c89..a537fb23ab 100644 --- a/courses/api_test.py +++ b/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) diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index 2ae2b1fe51..6428bb8ea4 100644 --- a/courses/views/v1/__init__.py +++ b/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,13 +193,11 @@ 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() ) @@ -207,7 +205,7 @@ def get_queryset(self): return ( CourseRun.objects.select_related("course") .prefetch_related("course__departments", "course__page") - .all() + .filter(live=True) ) def get_serializer_context(self): diff --git a/courses/views/v1/views_test.py b/courses/views/v1/views_test.py index 0b99c83f32..7003340432 100644 --- a/courses/views/v1/views_test.py +++ b/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