Refactors serializers/views for speed in enrollment APIs, adds flag t…

…o designate requirements as electives or not (#1736)

- Adds a flag to the ProgramRequirement model to signify if a node contains electives or not (only valid for depth=2)
- Some updates to use said flag rather than rely on the node title
- Refactored some computed properties that were using recursion to walk the requirements tree to instead pull flat lists
- Refactored the Program.courses computed property to use the new node flag

courses/factories.py

@@ -251,6 +251,7 @@ def program_with_empty_requirements():
         operator=ProgramRequirement.Operator.MIN_NUMBER_OF,
         operator_value=1,
         title="Elective Courses",
+        elective_flag=True,
     )
     return program
 
@@ -280,6 +281,7 @@ def program_with_requirements():
         operator=ProgramRequirement.Operator.MIN_NUMBER_OF,
         operator_value=2,
         title="Elective Courses",
+        elective_flag=True,
     )
     for course in elective_courses:
         elective_courses_node.add_child(

courses/forms.py

@@ -145,6 +145,16 @@ def program_requirements_schema():
                                     },
                                 },
                             },
+                            "elective_flag": {
+                                "type": "boolean",
+                                "title": "Contains Electives",
+                                "default": False,
+                                "options": {
+                                    "dependencies": {
+                                        "node_type": ProgramRequirementNodeType.OPERATOR.value,
+                                    },
+                                },
+                            },
                             # course fields
                             "course": {
                                 "type": "number",
@@ -200,6 +210,7 @@ def __init__(self, *args, **kwargs):
                         "title": "Elective Courses",
                         "operator": ProgramRequirement.Operator.MIN_NUMBER_OF.value,
                         "operator_value": 1,
+                        "elective_flag": True,
                     },
                     "children": [],
                 },

courses/migrations/0037_update_programrequirements_add_elective_flag.py

@@ -0,0 +1,43 @@
+# Generated by Django 3.2.18 on 2023-07-10 15:41
+
+from django.db import migrations, models
+
+
+def forwards_func(apps, schema_editor):
+    ProgramRequirement = apps.get_model("courses", "ProgramRequirement")
+    db_alias = schema_editor.connection.alias
+    update_reqs = (
+        ProgramRequirement.objects.using(db_alias)
+        .filter(depth=2, title="Elective Courses")
+        .all()
+    )
+
+    for requirement in update_reqs:
+        requirement.elective_flag = True
+
+    ProgramRequirement.objects.using(db_alias).bulk_update(
+        update_reqs, ["elective_flag"]
+    )
+
+
+def reverse_func(apps, schema_editor):
+    # just here to make this reversible - the field goes away on down so nothing to do here
+    return
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("courses", "0036_update_letter_grades_for_dedp"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="programrequirement",
+            name="elective_flag",
+            field=models.BooleanField(blank=True, default=False, null=True),
+        ),
+        migrations.RunPython(
+            forwards_func,
+            reverse_func,
+        ),
+    ]

courses/models.py

@@ -113,12 +113,12 @@ class Program(TimestampedModel, ValidateOnSaveMixin):
     )
     live = models.BooleanField(default=False)
 
-    @property
+    @cached_property
     def page(self):
         """Gets the associated ProgramPage"""
         return getattr(self, "programpage", None)
 
-    @property
+    @cached_property
     def num_courses(self):
         """Gets the number of courses in this program"""
         return len(self.courses)
@@ -237,6 +237,7 @@ def _add_course_node(self, node_type, min_courses=1):
                     operator=node_type,
                     title="Elective Courses",
                     operator_value=min_courses,
+                    elective_flag=True,
                 )
             else:
                 node = self.get_requirements_root().add_child(
@@ -279,19 +280,7 @@ def save(self, *args, **kwargs):
                 program=self, node_type=ProgramRequirementNodeType.PROGRAM_ROOT.value
             )
 
-    def _req_course_walk(self, node, heap):
-        courses = []
-
-        if node.node_type == ProgramRequirementNodeType.COURSE:
-            return [(node.course, node.get_parent().title)]
-
-        for child in node.get_children().all():
-            courses.extend(self._req_course_walk(child, heap))
-
-        courses.extend(heap)
-        return courses
-
-    @cached_property
+    @property
     def courses(self):
         """
         Returns the courses associated with this program via the requirements
@@ -301,7 +290,33 @@ def courses(self):
         - list of tuple (Course, string): courses that are either requirements or electives, plus the requirement type
         """
 
-        return self._req_course_walk(self.requirements_root, [])
+        heap = []
+        main_ops = ProgramRequirement.objects.filter(program=self, depth=2).all()
+
+        for op in main_ops:
+            reqs = (
+                ProgramRequirement.objects.filter(
+                    program__id=self.id,
+                    path__startswith=op.path,
+                    node_type=ProgramRequirementNodeType.COURSE,
+                )
+                .select_related("course", "course__page")
+                .all()
+            )
+
+            heap.extend(
+                [
+                    (
+                        req.course,
+                        "Required Courses"
+                        if not op.elective_flag
+                        else "Elective Courses",
+                    )
+                    for req in reqs
+                ]
+            )
+
+        return heap
 
     @cached_property
     def required_courses(self):
@@ -435,12 +450,12 @@ def course_number(self):
         """
         return self.readable_id.split("+")[-1]
 
-    @property
+    @cached_property
     def page(self):
         """Gets the associated CoursePage"""
         return getattr(self, "coursepage", None)
 
-    @property
+    @cached_property
     def active_products(self):
         """
         Gets active products for the first unexpired courserun for this course
@@ -512,22 +527,21 @@ def programs(self):
         Returns:
             list: List of Programs this Course is a requirement or elective for.
         """
-        programs_containing_course = []
-
-        def _program_root_contains_course(node: MP_Node):
-            if node.is_course and node.course == self:
-                return True
-            elif node.get_children():
-                for child in node.get_children():
-                    if _program_root_contains_course(child):
-                        return True
-            return False
-
-        for program_root_node in ProgramRequirement.get_root_nodes():
-            if _program_root_contains_course(program_root_node):
-                programs_containing_course.append(program_root_node.program)
+        programs_containing_course = (
+            ProgramRequirement.objects.filter(
+                node_type=ProgramRequirementNodeType.COURSE, course=self
+            )
+            .distinct("program_id")
+            .order_by("program_id")
+            .values_list("program_id", flat=True)
+        )
 
-        return programs_containing_course
+        return [
+            program
+            for program in Program.objects.filter(
+                pk__in=[id for id in programs_containing_course]
+            ).all()
+        ]
 
     def is_country_blocked(self, user):
         """
@@ -1383,6 +1397,7 @@ class Operator(models.TextChoices):
     )
 
     title = models.TextField(null=True, blank=True, default="")
+    elective_flag = models.BooleanField(null=True, blank=True, default=False)
 
     @property
     def is_all_of_operator(self):

courses/models_test.py

@@ -585,6 +585,7 @@ def test_program_requirements(program_with_requirements):
         "operator_value": None,
         "title": "",
         "program": program_with_requirements.program.id,
+        "elective_flag": False,
     }
 
     assert program_with_requirements.root_node.dump_bulk() == [
@@ -625,6 +626,7 @@ def test_program_requirements(program_with_requirements):
                         "operator_value": "2",
                         "title": "Elective Courses",
                         "node_type": ProgramRequirementNodeType.OPERATOR.value,
+                        "elective_flag": True,
                     },
                     "id": program_with_requirements.elective_courses_node.id,
                     "children": [

courses/serializers.py

@@ -230,9 +230,7 @@ class CourseRunDetailSerializer(serializers.ModelSerializer):
 
     def get_page(self, instance):
         try:
-            return CoursePageSerializer(
-                instance=CoursePage.objects.filter(course=instance.course).get()
-            ).data
+            return CoursePageSerializer(instance=instance.course.page).data
         except ObjectDoesNotExist:
             return None
 
@@ -284,25 +282,11 @@ class ProgramSerializer(serializers.ModelSerializer):
     def get_courses(self, instance):
         """Serializer for courses"""
         return CourseSerializer(
-            models.Course.objects.filter(live=True, in_programs__program=instance)
-            .order_by("id", "courseruns__start_date")
-            .distinct("id"),
+            [course[0] for course in instance.courses if course[0].live],
             many=True,
             context={"include_page_fields": True},
         ).data
 
-    def _get_nested_requirements(self, node):
-        ids = []
-
-        if node.get_children():
-            for child in node.get_children():
-                if child.node_type == models.ProgramRequirementNodeType.OPERATOR:
-                    ids += self._get_nested_requirements(child)
-                elif child.course.id is not None:
-                    ids.append(child.course.id)
-
-        return ids
-
     def get_requirements(self, instance):
         return {
             "required": [course.id for course in instance.required_courses],
@@ -427,32 +411,14 @@ class Meta:
         fields = ["grade", "letter_grade", "passed", "set_by_admin", "grade_percent"]
 
 
-class CourseRunEnrollmentSerializer(serializers.ModelSerializer):
-    """CourseRunEnrollment model serializer"""
-
-    run = CourseRunDetailSerializer(read_only=True)
-    run_id = serializers.IntegerField(write_only=True)
+class BaseCourseRunEnrollmentSerializer(serializers.ModelSerializer):
     certificate = serializers.SerializerMethodField(read_only=True)
     enrollment_mode = serializers.ChoiceField(
         (EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE), read_only=True
     )
     approved_flexible_price_exists = serializers.SerializerMethodField()
     grades = serializers.SerializerMethodField(read_only=True)
 
-    def create(self, validated_data):
-        user = self.context["user"]
-        run_id = validated_data["run_id"]
-        try:
-            run = models.CourseRun.objects.get(id=run_id)
-        except models.CourseRun.DoesNotExist:
-            raise ValidationError({"run_id": f"Invalid course run id: {run_id}"})
-        successful_enrollments, edx_request_success = create_run_enrollments(
-            user,
-            [run],
-            keep_failed_enrollments=features.is_enabled(features.IGNORE_EDX_FAILURES),
-        )
-        return successful_enrollments
-
     def get_certificate(self, enrollment):
         """
         Resolve a certificate for this enrollment if it exists
@@ -513,7 +479,6 @@ class Meta:
         fields = [
             "run",
             "id",
-            "run_id",
             "edx_emails_subscription",
             "certificate",
             "enrollment_mode",
@@ -522,6 +487,38 @@ class Meta:
         ]
 
 
+class CourseRunEnrollmentSerializer(BaseCourseRunEnrollmentSerializer):
+    """CourseRunEnrollment model serializer"""
+
+    run = CourseRunDetailSerializer(read_only=True)
+    run_id = serializers.IntegerField(write_only=True)
+    certificate = serializers.SerializerMethodField(read_only=True)
+    enrollment_mode = serializers.ChoiceField(
+        (EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE), read_only=True
+    )
+    approved_flexible_price_exists = serializers.SerializerMethodField()
+    grades = serializers.SerializerMethodField(read_only=True)
+
+    def create(self, validated_data):
+        user = self.context["user"]
+        run_id = validated_data["run_id"]
+        try:
+            run = models.CourseRun.objects.get(id=run_id)
+        except models.CourseRun.DoesNotExist:
+            raise ValidationError({"run_id": f"Invalid course run id: {run_id}"})
+        successful_enrollments, edx_request_success = create_run_enrollments(
+            user,
+            [run],
+            keep_failed_enrollments=features.is_enabled(features.IGNORE_EDX_FAILURES),
+        )
+        return successful_enrollments
+
+    class Meta(BaseCourseRunEnrollmentSerializer.Meta):
+        fields = BaseCourseRunEnrollmentSerializer.Meta.fields + [
+            "run_id",
+        ]
+
+
 class ProgramCertificateSerializer(serializers.ModelSerializer):
     """ProgramCertificate model serializer"""
 
@@ -557,6 +554,7 @@ class ProgramRequirementDataSerializer(StrictFieldsSerializer):
     title = serializers.CharField(allow_null=True, default=None)
     operator = serializers.CharField(allow_null=True, default=None)
     operator_value = serializers.CharField(allow_null=True, default=None)
+    elective_flag = serializers.BooleanField(allow_null=True, default=False)
 
 
 class ProgramRequirementSerializer(StrictFieldsSerializer):