diff --git a/RELEASE.rst b/RELEASE.rst index fb13a57717..cba39c06ad 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,13 @@ Release Notes ============= +Version 0.140.0 +--------------- + +- Fix performance for enrollments API w/ 0 results (#3360) +- Add command to re-run an existing course run (#3352) +- Adding prefetch for products in courses api (#3331) + Version 0.139.4 (Released March 05, 2026) --------------- diff --git a/b2b/api.py b/b2b/api.py index 5c38eefd34..cf913875c5 100644 --- a/b2b/api.py +++ b/b2b/api.py @@ -33,7 +33,7 @@ ) from cms.api import get_home_page from courses.constants import UAI_COURSEWARE_ID_PREFIX -from courses.models import Course, CourseRun, Department +from courses.models import Course, CourseRun, Department, EnrollmentMode from ecommerce.constants import ( DISCOUNT_TYPE_FIXED_PRICE, PAYMENT_TYPE_SALES, @@ -50,6 +50,7 @@ ) from main import constants as main_constants from main.utils import date_to_datetime +from openedx.constants import EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE from openedx.tasks import clone_courserun log = logging.getLogger(__name__) @@ -274,12 +275,10 @@ def create_contract_run( # noqa: PLR0913 the current year, and the contract ID. This means that the source course will have runs that have readable IDs that do not match the course ID. - The course key is generated according to a set algorithm. The new course key - will have the organization part set to "UAI_orgkey" and the run tag set to - "year_Cid" where orgkey is the organization key (set in the organization - record), year is the current year, and id is the ID of the contract. For more - information on the key format, see this discussion post: - https://github.com/mitodl/hq/discussions/7525 + The course key is generated according to a set algorithm, which is implemented + in "create_course_run_key". Generally these now look like regular semester + run keys, but with extra data in them. For more information on the key format, + see this discussion post: https://github.com/mitodl/hq/discussions/7525 A product will be created for the new contract course run, and its price will either be zero or the amount specified by the contract. (Free courses still @@ -362,8 +361,14 @@ def create_contract_run( # noqa: PLR0913 ) course_run.save() + required_modes = EnrollmentMode.objects.filter( + mode_slug__in=[EDX_ENROLLMENT_VERIFIED_MODE, EDX_ENROLLMENT_AUDIT_MODE] + ).all() + + [course_run.enrollment_modes.add(mode) for mode in required_modes] + if not skip_edx: - clone_courserun.delay(course_run.id, base_id=clone_course_run.courseware_id) + clone_courserun.delay(course_run.id, clone_course_run.courseware_id) log.debug( "Created run %s for course %s in contract %s from course run %s", diff --git a/courses/api.py b/courses/api.py index c48fc8aaeb..3234a57ad4 100644 --- a/courses/api.py +++ b/courses/api.py @@ -5,7 +5,7 @@ import logging import re from collections import namedtuple -from datetime import timedelta +from datetime import datetime, timedelta from decimal import Decimal from traceback import format_exc from typing import TYPE_CHECKING @@ -74,6 +74,7 @@ get_edx_api_course_list_client, get_edx_course_modes, get_edx_grades_with_users, + process_course_run_clone, unenroll_edx_course_run, ) from openedx.constants import ( @@ -1594,3 +1595,96 @@ def create_verifiable_credential(certificate: BaseCertificate, *, raise_on_error ) if raise_on_error: raise + + +def rerun_course_run( # noqa: PLR0913 + base_run: CourseRun, + run_tag: str, + *, + courseware_id: str | None = None, + organization: str | None = None, + title: str | None = None, + start_date: datetime | None = None, + end_date: datetime | None = None, + enrollment_start: datetime | None = None, + enrollment_end: datetime | None = None, + is_self_paced: bool | None = None, + live: bool | None = None, + enrollment_modes: list[EnrollmentMode] | None = None, +) -> CourseRun: + """ + Re-run an existing course run. + + Takes the specified base run and re-runs it in edX with the specified run tag. + The resulting run must not exist in edX. + + By default, the created run will share all the same parameters as the base + run, other than the run tag. + + When specifying start and end dates, be aware that course start and end must + be set to set enrollment start and end, and the enrollment start and end dates + must be before or equal to the course start and end dates respectively. + + The organization option has nothing to do with B2B organizations. This is the + first part of the courseware ID after the `course-v1:` part. + + Specifying a custom courseware ID will override the run tag and organization + settings passed in. + + Args: + - base_run (CourseRun): the course to re-run + - run_tag (str): the new run tag to use + Keyword Args: + - courseware_id (str): new courseware/readable ID to use + - organization (str): new organization to use + - title (str): new course title + - start_date (DateTime): course start date + - end_date (DateTime): course end date + - enrollment_start (DateTime): enrollment start date + - enrollment_end (DateTime): enrollment end date + - is_self_paced (bool): course is self-paced + - live (bool): course is live (in MITx Online) + - enrollment_modes (list[EnrollmentMode]): modes to add to the new run + Returns: + - CourseRun, the created run + """ + + if courseware_id: + target_run_id = courseware_id + else: + base_key = CourseKey.from_string(base_run.courseware_id) + + base_key = base_key.replace(run=run_tag) + if organization: + base_key = base_key.replace(org=organization) + + target_run_id = str(base_key) + + with transaction.atomic(): + new_run = CourseRun.objects.create( + course=base_run.course, + title=title if title else base_run.title, + courseware_id=target_run_id, + run_tag=run_tag, + start_date=start_date if start_date else base_run.start_date, + end_date=end_date if end_date else base_run.end_date, + enrollment_start=enrollment_start + if enrollment_start + else base_run.enrollment_start, + enrollment_end=enrollment_end + if enrollment_end + else base_run.enrollment_end, + is_self_paced=is_self_paced + if is_self_paced is not None + else base_run.is_self_paced, + live=live if live is not None else base_run.live, + ) + + for mode in ( + enrollment_modes if enrollment_modes else base_run.enrollment_modes.all() + ): + new_run.enrollment_modes.add(mode) + + process_course_run_clone(new_run, base_run.courseware_id) + + return new_run diff --git a/courses/factories.py b/courses/factories.py index 85d84e273c..8cedd8dd79 100644 --- a/courses/factories.py +++ b/courses/factories.py @@ -23,6 +23,7 @@ CourseRunGrade, CoursesTopic, Department, + EnrollmentMode, LearnerProgramRecordShare, PartnerSchool, Program, @@ -32,6 +33,7 @@ ProgramRequirementNodeType, ProgramRun, ) +from openedx.constants import EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE from users.factories import UserFactory FAKE = faker.Factory.create() @@ -41,6 +43,10 @@ dept["slug"]: dept["name"] for dept in json.loads(Path("courses/test_data/departments.json").read_text()) } +MODES = [ + EDX_ENROLLMENT_AUDIT_MODE, + EDX_ENROLLMENT_VERIFIED_MODE, +] class DepartmentFactory(DjangoModelFactory): @@ -53,6 +59,17 @@ class Meta: django_get_or_create = ("slug",) +class EnrollmentModeFactory(DjangoModelFactory): + """Factory for EnrollmentModes.""" + + mode_slug = factory.Iterator(MODES) + mode_display_name = factory.LazyAttribute(lambda emode: emode.mode_slug) + + class Meta: + model = EnrollmentMode + django_get_or_create = ("mode_slug",) + + class ProgramFactory(DjangoModelFactory): """Factory for Programs""" diff --git a/courses/management/commands/rerun_courserun.py b/courses/management/commands/rerun_courserun.py new file mode 100644 index 0000000000..f0926db019 --- /dev/null +++ b/courses/management/commands/rerun_courserun.py @@ -0,0 +1,162 @@ +"""Re-run a course.""" + +import logging +from argparse import RawTextHelpFormatter +from decimal import Decimal + +import reversion +from django.core.management import BaseCommand, CommandError +from opaque_keys.edx.keys import CourseKey + +from courses.api import rerun_course_run, resolve_courseware_object_from_id +from ecommerce.models import Product + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """Re-run a course.""" + + help = """Re-run an existing course or course run. + +Re-running a course will find the source course run for the specified course and then request a re-run of it in edX, with the specified run tag and any optional settings. A record in MITx Online will be created for the new run. The course must have a source run, however; if there's not one, you will get an error message. + +Re-running a specific course run will request a re-run of the specified run from edX, regardless of whether it's a source run, with the specified run tag and any optional settings. + +In either case, the command will try to create a new run. If it exists in edX already, then this will fail. If you want to pull in a run that exists, you probably want the import_courserun command. + """ + + def add_arguments(self, parser): + """Add command line arguments.""" + + parser.formatter_class = RawTextHelpFormatter + + parser.add_argument( + "course", + help="The course or course run (readable ID or numeric ID) to re-run. If specifying a course, the course must have a source run set up.", + type=str, + ) + + parser.add_argument( + "--run-tag", type=str, help="The run tag to use for the new run." + ) + + parser.add_argument( + "--organization", + "--org", + type=str, + help='The organization key to use (the first bit after "course-v1:"). Optional; you probably don\'t need to change this.', + ) + + parser.add_argument( + "--change-course-org", + action="store_true", + help="If set, change the upstream course's org key to what's set for --organization. Will not change existing runs; requires --organization.", + ) + + parser.add_argument( + "--keep-product", + action="store_true", + help="If set, create a new product for the new run that mirrors the prior run's product. Overrides --price.", + ) + + parser.add_argument( + "--price", + type=Decimal, + help="If set, create a new product for the new run with the specified price.", + ) + + def handle(self, *args, **kwargs): # noqa: ARG002 + """Create the re-run according to the options passed.""" + + course_opt = kwargs.pop("course", None) + + run_to_clone = resolve_courseware_object_from_id(course_opt) + + if not run_to_clone: + msg = f"Course/run {course_opt} not found." + raise CommandError(msg) + + if not run_to_clone.is_run: + source = run_to_clone.courseruns.filter(is_source_run=True) + + if source.count() > 1: + msg = f"Course {course_opt} has more than one source run" + raise CommandError(msg) + + if source.count() < 1: + msg = f"Course {course_opt} has no source run" + raise CommandError(msg) + + run_to_clone = source.first() + + run_tag = kwargs.pop("run_tag", None) + org = kwargs.pop("organization", None) + + if not run_tag: + msg = "Run tag is required." + raise CommandError(msg) + + # next steps for this: + # - look through create_contract_run and either adapt or something to create the run + # - make new key for the new run + # - grab products and recreate where necessary + # - update upstream course where necessary + + self.stdout.write(f"Rerunning {run_to_clone.courseware_id}...") + + new_course_run = rerun_course_run(run_to_clone, run_tag, organization=org) + + self.stdout.write(self.style.SUCCESS(f"Created new run {new_course_run}")) + + if org and kwargs.pop("change_course_org", False): + self.stdout.write( + f"Updating the org key on course {run_to_clone.course} to {org}..." + ) + + course_key = CourseKey.from_string( + f"{run_to_clone.course.readable_id}+RunTag" + ) + course_key = course_key.replace(org=org) + + run_to_clone.course.readable_id = ( + f"{course_key.CANONICAL_NAMESPACE}:{course_key.org}+{course_key.course}" + ) + run_to_clone.course.save() + + self.stdout.write( + self.style.SUCCESS( + f"Updated course key to {run_to_clone.course.readable_id}" + ) + ) + + keeping_product = kwargs.pop("keep_product", False) + + if keeping_product: + self.stdout.write( + f"Copying products for {run_to_clone} to {new_course_run}" + ) + + with reversion.create_revision(): + for original_product in run_to_clone.products.all(): + new_product = Product.objects.create( + purchasable_object=new_course_run, + price=original_product.price, + is_active=original_product.is_active, + description=new_course_run.courseware_id, + ) + self.stdout.write(f"Created product {new_product}") + + price = kwargs.pop("price", None) + + if price and not keeping_product: + self.stdout.write(f"Creating product for {new_course_run}") + + with reversion.create_revision(): + new_product = Product.objects.create( + purchasable_object=new_course_run, + price=price, + is_active=True, + description=new_course_run.courseware_id, + ) + self.stdout.write(f"Created product {new_product}") diff --git a/courses/models.py b/courses/models.py index 59f76a0063..102edca39b 100644 --- a/courses/models.py +++ b/courses/models.py @@ -1297,6 +1297,13 @@ def is_run(self): """Flag to indicate if this is a run""" return True + @cached_property + def edx_modes(self): + """Get the edX course modes for this course.""" + from openedx.api import get_edx_course_modes # noqa: PLC0415 + + return get_edx_course_modes(self.courseware_id) + def __str__(self): title = f"{self.courseware_id} | {self.title}" return title if len(title) <= 100 else title[:97] + "..." # noqa: PLR2004 @@ -1666,6 +1673,9 @@ def mapper(course_run_enrollment): @staticmethod def filter(course_run_and_user_ids): + if not course_run_and_user_ids: + return CourseRunCertificate.objects.none() + id_filters = Q() # django 5.1 supports this via @@ -1694,6 +1704,9 @@ def mapper(course_run_enrollment): @staticmethod def filter(course_run_and_user_ids): + if not course_run_and_user_ids: + return CourseRunGrade.objects.none() + id_filters = Q() # django 5.1 supports this via diff --git a/courses/serializers/v2/courses.py b/courses/serializers/v2/courses.py index e382db96d8..284e0a8b3d 100644 --- a/courses/serializers/v2/courses.py +++ b/courses/serializers/v2/courses.py @@ -18,10 +18,10 @@ BaseCourseRunSerializer, BaseCourseSerializer, BaseProgramSerializer, - ProductRelatedField, ) from courses.serializers.v1.departments import DepartmentSerializer from courses.utils import get_approved_flexible_price_exists, get_dated_courseruns +from ecommerce.serializers.v0 import BaseProductSerializer from main import features from openedx.constants import EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE @@ -233,7 +233,7 @@ class Meta: class CourseRunSerializer(BaseCourseRunSerializer): """CourseRun model serializer""" - products = ProductRelatedField(many=True, read_only=True) + products = serializers.SerializerMethodField(method_name="get_products") approved_flexible_price_exists = serializers.SerializerMethodField() class Meta: @@ -258,6 +258,17 @@ def to_representation(self, instance): def get_approved_flexible_price_exists(self, instance): return get_approved_flexible_price_exists(instance, self.context) + @extend_schema_field(list) + def get_products(self, obj): + # Use prefetched products if available to avoid N+1 queries + products = ( + obj.prefetched_products + if hasattr(obj, "prefetched_products") + else obj.products.all() + ) + + return BaseProductSerializer(products, many=True, context=self.context).data + @extend_schema_serializer(component_name="CourseWithCourseRunsSerializerV2") class CourseWithCourseRunsSerializer(CourseSerializer): @@ -267,22 +278,27 @@ class CourseWithCourseRunsSerializer(CourseSerializer): @extend_schema_field(CourseRunSerializer(many=True)) def get_courseruns(self, instance): - """Get the course runs for the given instance.""" - courseruns = instance.courseruns.prefetch_related("enrollment_modes").order_by( - "id" - ) - + # Use prefetched course runs to preserve prefetched products + courseruns = instance.courseruns.all() if "org_id" in self.context: - courseruns = courseruns.filter( - b2b_contract__organization_id=self.context["org_id"] - ) + courseruns = [ + run + for run in courseruns + if getattr(run.b2b_contract, "organization_id", None) + == int(self.context["org_id"]) + ] if "contract_id" in self.context: - courseruns = courseruns.filter(b2b_contract_id=self.context["contract_id"]) - + courseruns = [ + run + for run in courseruns + if getattr(run.b2b_contract, "id", None) + == int(self.context["contract_id"]) + ] if "org_id" not in self.context and "contract_id" not in self.context: - courseruns = courseruns.filter(b2b_contract_id=None) - - return CourseRunSerializer(courseruns, many=True, read_only=True).data + courseruns = [run for run in courseruns if run.b2b_contract_id is None] + return CourseRunSerializer( + courseruns, many=True, read_only=True, context=self.context + ).data class Meta: model = models.Course diff --git a/courses/utils.py b/courses/utils.py index 85fb699738..f78388b9e2 100644 --- a/courses/utils.py +++ b/courses/utils.py @@ -113,15 +113,9 @@ def get_enrollable_courses(queryset, enrollment_end_date=None): """ enrollable_courseruns_qs = CourseRun.objects.enrollable(enrollment_end_date) - return ( - queryset.prefetch_related( - Prefetch("courseruns", queryset=enrollable_courseruns_qs) - ) - .filter( - courseruns__id__in=enrollable_courseruns_qs.values_list("id", flat=True) - ) - .distinct() - ) + return queryset.filter( + courseruns__id__in=enrollable_courseruns_qs.values_list("id", flat=True) + ).distinct() def get_unenrollable_courses(queryset): diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index 5f2c282ca6..2a76d6118d 100644 --- a/courses/views/v1/__init__.py +++ b/courses/views/v1/__init__.py @@ -191,7 +191,9 @@ def get_queryset(self): Course.objects.filter() .select_related("page") .prefetch_related( - "courseruns", "departments", "courseruns__enrollment_modes" + "courseruns", + "departments", + "courseruns__enrollment_modes", ) .all() ) diff --git a/courses/views/v2/__init__.py b/courses/views/v2/__init__.py index 8f76d46267..dcec06fe3f 100644 --- a/courses/views/v2/__init__.py +++ b/courses/views/v2/__init__.py @@ -6,6 +6,7 @@ import django_filters from django.conf import settings +from django.contrib.contenttypes.models import ContentType from django.db.models import Count, Prefetch, Q from django.http import JsonResponse from django.shortcuts import get_object_or_404 @@ -66,6 +67,7 @@ get_unenrollable_courses, ) from ecommerce.api import create_verified_program_course_run_enrollment +from ecommerce.models import Product from main import features from openapi.utils import extend_schema_get_queryset from openedx.api import sync_enrollments_with_edx @@ -325,19 +327,6 @@ def filter_courserun_is_enrollable(self, queryset, _, value): else get_unenrollable_courses(queryset) ) - def filter_queryset(self, queryset): - queryset = super().filter_queryset(queryset) - # perform additional filtering - - filter_keys = self.form.cleaned_data.keys() - - if "courserun_is_enrollable" not in filter_keys: - queryset = queryset.prefetch_related( - Prefetch("courseruns", queryset=CourseRun.objects.order_by("id")), - ) - - return queryset - class CourseViewSet(ReadableIdLookupMixin, viewsets.ReadOnlyModelViewSet): """API view set for Courses""" @@ -352,14 +341,35 @@ class CourseViewSet(ReadableIdLookupMixin, viewsets.ReadOnlyModelViewSet): def get_queryset(self): """Get the queryset for the viewset.""" - return ( - Course.objects.select_related("page") - .prefetch_related("departments", "in_programs") - .annotate(count_b2b_courseruns=Count("courseruns__b2b_contract__id")) - .annotate(count_courseruns=Count("courseruns")) - .order_by("title") - .distinct() + queryset = Course.objects.select_related("page") + # Use Prefetch for reverse GenericRelation (products on CourseRun) + # 1. Get the ContentType object for the CourseRun model + courserun_content_type = ContentType.objects.get_for_model(CourseRun) + # 2. Create a Prefetch object to specify the queryset for the 'tags' relation + # This internal queryset only fetches products related to the CourseRun content type + courserun_product_queryset = Product.objects.filter( + content_type=courserun_content_type + ) + # 3. Use prefetch_related on main queryset, referencing the reverse relation's name (e.g., 'products') + products_prefetch = Prefetch( + "products", + queryset=courserun_product_queryset, + to_attr="prefetched_products", + ) + course_runs_prefetch = Prefetch( + "courseruns", + queryset=CourseRun.objects.order_by("id") + .select_related("b2b_contract") + .prefetch_related("enrollment_modes", products_prefetch), + ) + queryset = queryset.prefetch_related( + "departments", "in_programs", course_runs_prefetch + ) + queryset = queryset.annotate( + count_b2b_courseruns=Count("courseruns__b2b_contract__id") ) + queryset = queryset.annotate(count_courseruns=Count("courseruns")) + return queryset.order_by("title").distinct() def get_serializer_context(self): added_context = {} diff --git a/main/settings.py b/main/settings.py index 003bc95024..b8e1c77193 100644 --- a/main/settings.py +++ b/main/settings.py @@ -37,7 +37,7 @@ from main.sentry import init_sentry from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.139.4" +VERSION = "0.140.0" log = logging.getLogger() diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index d77bb9582b..41067aad7c 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -3220,23 +3220,6 @@ components: - description - id - price - BaseProductRequest: - type: object - description: Simple serializer for Product without related purchasable objects - properties: - price: - type: string - format: decimal - pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ - description: - type: string - minLength: 1 - is_active: - type: boolean - description: Controls visibility of the product in the app. - required: - - description - - price BaseProgram: type: object description: Basic program model serializer @@ -4178,9 +4161,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean @@ -7662,9 +7643,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index c5e7e2c9a4..9bcd12e4d1 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -3220,23 +3220,6 @@ components: - description - id - price - BaseProductRequest: - type: object - description: Simple serializer for Product without related purchasable objects - properties: - price: - type: string - format: decimal - pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ - description: - type: string - minLength: 1 - is_active: - type: boolean - description: Controls visibility of the product in the app. - required: - - description - - price BaseProgram: type: object description: Basic program model serializer @@ -4178,9 +4161,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean @@ -7662,9 +7643,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean diff --git a/openapi/specs/v2.yaml b/openapi/specs/v2.yaml index dafe8a0c9d..37072e1eb3 100644 --- a/openapi/specs/v2.yaml +++ b/openapi/specs/v2.yaml @@ -3220,23 +3220,6 @@ components: - description - id - price - BaseProductRequest: - type: object - description: Simple serializer for Product without related purchasable objects - properties: - price: - type: string - format: decimal - pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ - description: - type: string - minLength: 1 - is_active: - type: boolean - description: Controls visibility of the product in the app. - required: - - description - - price BaseProgram: type: object description: Basic program model serializer @@ -4178,9 +4161,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean @@ -7662,9 +7643,7 @@ components: readOnly: true products: type: array - items: - allOf: - - $ref: '#/components/schemas/BaseProduct' + items: {} readOnly: true approved_flexible_price_exists: type: boolean diff --git a/openedx/api.py b/openedx/api.py index 55039f8652..94400563c7 100644 --- a/openedx/api.py +++ b/openedx/api.py @@ -3,6 +3,7 @@ import logging import random from datetime import datetime, timedelta +from functools import partial from urllib.parse import parse_qs, urljoin, urlparse import requests @@ -1542,29 +1543,59 @@ def update_edx_course( # noqa: PLR0913 ) -def process_course_run_clone(target_id: int, *, base_id: int | str | None = None): +def fix_cloned_run_data(target_course: CourseRun, edx_client) -> CourseRun: + """Fix the title, pacing, and dates for a newly cloned run.""" + + course_params = [ + target_course.readable_id, + target_course.title, + "self_paced" if target_course.is_self_paced else "instructor_paced", + ] + + if target_course.start_date and target_course.end_date: + course_params.append(target_course.start_date) + course_params.append(target_course.end_date) + + if target_course.enrollment_start and target_course.enrollment_end: + course_params.append(target_course.enrollment_start) + course_params.append(target_course.enrollment_end) + + return update_edx_course( + *course_params, + client=edx_client, + ) + + +def process_course_run_clone(target_course: CourseRun, base_course_key: str): """ - Clone a course run, using details from CourseRun objects in MITx Online. + Clone a course run in edX, using details from a set of MITx Online runs. + + In edX, re-running a course copies course content into the new run, and allows + you to change certain parameters of the resulting run. Depending on final setup, + the new run gets content updates from the parent course run. + + Re-running a course will instruct edX to re-run the specified base course, + using the parameters of the specified target course for the new run, including + the course run key (readable ID/courseware ID). Therefore, to use this, the + target_course must be set up properly before passing it in. Ensure that the + courseware ID is set up properly - including any run tag or organization field + changes - and that the run dates are set if necessary. Note that enrollment + dates can only be set if the course has a start and end date. - When we need a new course run (as a re-run or a special B2B course run), we - create the CourseRun object in the system, and then we need to make the run - in edX. This will assume there's a base course run that uses the readable ID - of the underlying _Course_ record, and will clone that. Some of the data from - the CourseRun will then be copied over into the newly cloned edX run (dates, - etc.) and the URL will be backfilled into the CourseRun. + We'll kick off the re-run process, then update the new run with the data from + the target_course and ensure that the course modes are in as specified in the + target_course. - If a different course run needs to be used as the base course, you can - set that. Pass either the PK of the course or the readable ID. If a readable - ID is passed, it will be supplied verbatim to the clone API. (In other words, - it can be a course that MITx Online doesn't know about.) + The base course can just be a course run key (as a string); it does not need + to exist in MITx Online. + + The target course must _not_ exist in edX. Args: - - target_id (int): The PK of the target course (i.e. the one we're creating) - - base_id (int|str): The PK or readable ID of the base course to clone. - Returns: - bool, whether or not it worked + - target_course (CourseRun): The CourseRun to create in edX. + Kwargs: + - base_course_key (str): The key of the course to re-run. """ - from courses.api import check_course_modes # noqa: PLC0415 edx_client = get_edx_api_jwt_client( settings.OPENEDX_COURSES_SERVICE_WORKER_CLIENT_ID, @@ -1572,24 +1603,9 @@ def process_course_run_clone(target_id: int, *, base_id: int | str | None = None use_studio=True, ) - target_course = courses.models.CourseRun.objects.get(pk=target_id) - base_course = target_course.course.readable_id - - if base_id: - if base_id is int: - if courses.models.Course.objects.filter(pk=base_id).exists(): - base_course = courses.models.Course.objects.get(pk=base_id).readable_id - elif courses.models.CourseRun.objects.filter(pk=base_id).exists(): - base_course = courses.models.CourseRun.objects.get( - pk=base_id - ).readable_id - else: - msg = f"Specified base course {base_id} doesn't exist." - raise ValueError(msg) - else: - base_course = str(base_id) - - get_edx_course(base_course, client=edx_client) + # Ensure the base course exists on the edX side. You may pass a key in that + # does not exist in MITx Online, so we can't just check locally. + get_edx_course(base_course_key, client=edx_client) try: get_edx_course(target_course.readable_id, client=edx_client) @@ -1600,53 +1616,25 @@ def process_course_run_clone(target_id: int, *, base_id: int | str | None = None # An HTTP error is good in this case. We don't want the target course to exist. pass - resp = clone_edx_course(base_course, target_course.readable_id, client=edx_client) + resp = clone_edx_course( + base_course_key, target_course.readable_id, client=edx_client + ) if not resp: - msg = f"Couldn't clone {base_course} to {target_course.readable_id}." + msg = f"Couldn't clone {base_course_key} to {target_course.readable_id}." raise ValueError(msg) # We should have the target course in edX now. We need to update it with the # data from our course run. - - course_params = [ - target_course.readable_id, - target_course.title, - "self_paced" if target_course.is_self_paced else "instructor_paced", - ] - - # We can only specify the start and end dates if there are both of them. - # And, we can only specify enrollment start/stop if there are both of them - # and the course has start/end dates. - - if target_course.start_date and target_course.end_date: - course_params.append(target_course.start_date) - course_params.append(target_course.end_date) - - if target_course.enrollment_start and target_course.enrollment_end: - course_params.append(target_course.enrollment_start) - course_params.append(target_course.enrollment_end) - - resp = update_edx_course( - *course_params, - client=edx_client, + # Run these after the transaction that this will most likely be + transaction.on_commit( + partial(fix_cloned_run_data, target_course=target_course, edx_client=edx_client) ) - - # Ensure the new course has the proper modes in it. - check_course_modes(target_course) - - # Set the ingestion flag on the course run to True - # All B2B courses should be flagged for content file ingestion - we can - # toggle it off manually if necessary. - # In the odd chance we don't have a page, trigger a warning. - if target_course.course.page: - target_course.course.page.ingest_content_files_for_ai = True - target_course.course.save() - else: - log.warning( - "Warning: processed course run clone for %s but can't set the ingestion flag because there's no CoursePage", - target_course, + transaction.on_commit( + partial( + push_edx_modes_from_run, course_run=target_course, edx_client=edx_client ) + ) def get_edx_course_modes( @@ -1753,3 +1741,27 @@ def delete_edx_course_mode( course_id=course_id, mode_slug=mode_slug, ) + + +def push_edx_modes_from_run(course_run: CourseRun, *, edx_client=None) -> int: + """Push course modes from the run into edX. Returns the number of modes created.""" + + existing_mode_slugs = [edx_mode.mode_slug for edx_mode in course_run.edx_modes] + created_count = 0 + + for mode in course_run.enrollment_modes.all(): + if mode.mode_slug not in existing_mode_slugs: + # If the mode requires payment, the price gets set to a nominal amount + # because it just needs to be non-zero. edX doesn't need to know what + # the price actually is, which is sometimes ambiguous anyway. + + create_edx_course_mode( + course_id=course_run.courseware_id, + mode_slug=mode.mode_slug, + mode_display_name=mode.mode_display_name, + min_price=(1 if mode.requires_payment else 0), + client=edx_client, + ) + created_count += 1 + + return created_count diff --git a/openedx/api_test.py b/openedx/api_test.py index 2d3077dad9..e9c16f9391 100644 --- a/openedx/api_test.py +++ b/openedx/api_test.py @@ -3,13 +3,14 @@ # pylint: disable=redefined-outer-name import itertools from datetime import timedelta -from unittest.mock import patch +from unittest.mock import ANY, call, patch from urllib.parse import parse_qsl import factory import pytest import responses from django.contrib.auth import get_user_model +from edx_api.course_runs.exceptions import CourseRunAPIError from freezegun import freeze_time from mitol.common.utils.datetime import now_in_utc from mitol.common.utils.user import _reformat_for_username, usernameify @@ -18,7 +19,11 @@ from requests.exceptions import HTTPError from rest_framework import status -from courses.factories import CourseRunEnrollmentFactory, CourseRunFactory +from courses.factories import ( + CourseRunEnrollmentFactory, + CourseRunFactory, + EnrollmentModeFactory, +) from main.test_utils import MockHttpError, MockResponse from openedx.api import ( ACCESS_TOKEN_HEADER_NAME, @@ -34,6 +39,8 @@ get_edx_api_client, get_edx_retirement_service_client, get_valid_edx_api_auth, + process_course_run_clone, + push_edx_modes_from_run, reconcile_edx_username, repair_all_faulty_openedx_users, repair_faulty_edx_user, @@ -1410,3 +1417,77 @@ def test_update_edx_user_profile_error(mock_session, mock_get_auth, mocker, user with pytest.raises(EdxApiUserUpdateError) as exc: update_edx_user_profile(user) assert "Error updating Open edX user" in str(exc.value) + + +def test_push_edx_modes_from_run(mocker): + """Test that we can push modes into edX from a course""" + + mocked_edx_modes = mocker.patch("openedx.api.get_edx_course_modes", return_value=[]) + mocked_create_edx_mode = mocker.patch("openedx.api.create_edx_course_mode") + + course_run = CourseRunFactory.create() + mxo_modes = EnrollmentModeFactory.create_batch(2) + calls = [] + + for mode in mxo_modes: + course_run.enrollment_modes.add(mode) + calls.append( + call( + course_id=course_run.courseware_id, + mode_slug=mode.mode_slug, + mode_display_name=mode.mode_display_name, + min_price=(1 if mode.requires_payment else 0), + client=ANY, + ) + ) + + assert course_run.edx_modes == [] + assert course_run.enrollment_modes.count() == 2 + assert mocked_edx_modes.called + + created = push_edx_modes_from_run(course_run) + + assert created == course_run.enrollment_modes.count() + assert mocked_create_edx_mode.called + mocked_create_edx_mode.assert_has_calls(calls, any_order=True) + + +def test_process_course_run_clone(mocker): + """Test that the course run clone calls the edX APIs properly.""" + + mocker.patch("openedx.api.get_edx_api_jwt_client") + mocker.patch( + "openedx.api.get_edx_course", + side_effect=[True, CourseRunAPIError("fake value error")], + ) + mocker.patch("openedx.api.get_edx_course_modes", return_value=[]) + mocker.patch("openedx.api.fix_cloned_run_data") + mocker.patch("openedx.api.create_edx_course_mode") + + mocked_clone_course = mocker.patch( + "openedx.api.clone_edx_course", return_value={"result": "success"} + ) + + cloneable_key = "course-v1:PyT+TestCourse+9T3036" + calls = [] + + course_run = CourseRunFactory.create() + mxo_modes = EnrollmentModeFactory.create_batch(2) + + for mode in mxo_modes: + course_run.enrollment_modes.add(mode) + calls.append( + call( + course_id=course_run.courseware_id, + mode_slug=mode.mode_slug, + mode_display_name=mode.mode_display_name, + min_price=(1 if mode.requires_payment else 0), + client=ANY, + ) + ) + + process_course_run_clone(course_run, cloneable_key) + + mocked_clone_course.assert_called_with( + cloneable_key, course_run.courseware_id, client=ANY + ) diff --git a/openedx/factories.py b/openedx/factories.py index 08e98cb0a7..aa2086e259 100644 --- a/openedx/factories.py +++ b/openedx/factories.py @@ -4,7 +4,8 @@ from zoneinfo import ZoneInfo import faker -from factory import Faker, LazyAttribute, SelfAttribute, SubFactory, Trait +from edx_api.course_detail.models import CourseMode +from factory import Factory, Faker, LazyAttribute, SelfAttribute, SubFactory, Trait from factory.django import DjangoModelFactory from factory.fuzzy import FuzzyText from mitol.common.utils import now_in_utc @@ -47,3 +48,38 @@ class Params: lambda _: now_in_utc() - timedelta(days=1) ) ) + + +class CourseModeProxy: + """Proxy for CourseMode, which doesn't instantiate in a way that works with Factory""" + + coursemode = None + + def __init__(self, mode_slug, mode_display_name, course_id): + """Init a CourseMode and return that instead""" + + self.coursemode = CourseMode( + { + "mode_slug": mode_slug, + "mode_disply_name": mode_display_name, + "course_id": course_id, + } + ) + + +class CourseModeFactory(Factory): + """Factory for edX Course Modes""" + + mode_slug = FAKE.slug() + mode_display_name = FAKE.catch_phrase() + course_id = FAKE.slug() + + class Meta: + model = CourseModeProxy + + @classmethod + def create(cls, *args, **kwargs): + """Generate, but return the proxy's CourseMode.""" + + proxy_obj = super().create(*args, **kwargs) + return proxy_obj.coursemode diff --git a/openedx/tasks.py b/openedx/tasks.py index fe54c64d2e..ea656608f3 100644 --- a/openedx/tasks.py +++ b/openedx/tasks.py @@ -104,7 +104,11 @@ def update_edx_user_profile(user_id): @app.task -def clone_courserun(target_id: int, *, base_id: int | str | None = None): +def clone_courserun(target_id: int, base_key: str): """Queue call to clone an existing course run.""" - api.process_course_run_clone(target_id, base_id=base_id) + from courses.models import CourseRun # noqa: PLC0415 + + target_course = CourseRun.objects.get(pk=target_id) + + api.process_course_run_clone(target_course, base_key)