Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion courses/serializers/v2/programs.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@
get_thumbnail_url,
)
from courses.serializers.utils import get_unique_topics_from_courses
from courses.serializers.v1.base import EnrollmentModeSerializer, ProductRelatedField
from courses.serializers.v1.base import (
BaseProgramSerializer,
EnrollmentModeSerializer,
ProductRelatedField,
)
from courses.serializers.v1.departments import DepartmentSerializer
from courses.serializers.v2.courses import CourseRunEnrollmentSerializer
from main.serializers import StrictFieldsSerializer
Expand Down Expand Up @@ -132,6 +136,7 @@ class ProgramSerializer(serializers.ModelSerializer):

courses = serializers.SerializerMethodField()
collections = serializers.SerializerMethodField()
programs = serializers.SerializerMethodField()
requirements = serializers.SerializerMethodField()
req_tree = serializers.SerializerMethodField()
page = serializers.SerializerMethodField()
Expand All @@ -150,6 +155,38 @@ class ProgramSerializer(serializers.ModelSerializer):
start_date = serializers.SerializerMethodField()
enrollment_modes = EnrollmentModeSerializer(many=True, read_only=True)

@extend_schema_field(BaseProgramSerializer(many=True, allow_null=True))
def get_programs(self, instance):
"""Include parent programs for this program when requested.

This mirrors the behavior of the CourseSerializer.get_programs method,
but uses the ProgramRequirement.required_program reverse relation
("required_by") instead of Course.in_programs.
"""
if not self.context.get("include_programs", False):
return None

programs_qs = instance.required_by

if self.context.get("org_id"):
programs_qs = programs_qs.filter(
program__contract_memberships__contract__organization__pk=self.context.get(
"org_id"
)
)
elif self.context.get("contract_id"):
programs_qs = programs_qs.filter(
program__contract_memberships__contract__pk=self.context.get(
"contract_id"
)
Comment on lines +172 to +181
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The get_programs method can return duplicate parent programs because the underlying queryset is not deduplicated with .distinct() after joining on one-to-many relationships.
Severity: MEDIUM

Suggested Fix

Add a .distinct() call to the queryset before it is evaluated to ensure each parent program is included only once. For example, programs_qs.select_related("program").distinct().all() would prevent duplicate program objects from being added to the list.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: courses/serializers/v2/programs.py#L172-L181

Potential issue: The `get_programs` method in the `ProgramSerializer` can return
duplicate parent programs in the API response. This happens when filtering by `org_id`
or `contract_id`. The filtering involves joining across one-to-many relationships like
`program__contract_memberships`. If a program is part of multiple contracts within the
same organization, the underlying queryset will contain duplicate rows. The code then
constructs a list from this queryset without applying `.distinct()`, preserving the
duplicates in the final serialized output.

Did we get this right? 👍 / 👎 to inform future reviews.

)
else:
programs_qs = programs_qs.filter(program__b2b_only=False)

programs = [req.program for req in programs_qs.select_related("program").all()]

return BaseProgramSerializer(programs, many=True).data

def get_courses(self, instance) -> list[int]:
return [course[0].id for course in instance.courses if course[0].live]

Expand Down Expand Up @@ -525,6 +562,7 @@ class Meta:
"id",
"courses",
"collections",
"programs",
"requirements",
"req_tree",
"page",
Expand Down
1 change: 1 addition & 0 deletions courses/serializers/v2/programs_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ def test_serialize_program(
if not remove_tree
else [],
"collections": [program_collection.id],
"programs": None,
"requirements": formatted_reqs,
"req_tree": ProgramRequirementTreeSerializer(
program_with_empty_requirements.requirements_root
Expand Down
20 changes: 20 additions & 0 deletions courses/views/v2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,26 @@ class ProgramViewSet(ReadableIdLookupMixin, viewsets.ReadOnlyModelViewSet):
filterset_class = ProgramFilterSet
lookup_value_regex = "[^/]+" # Accept any non-slash character

def get_serializer_context(self):
"""Add context flags used by ProgramSerializer.

- include_programs: include parent programs when retrieving a single
program or when filtering by readable_id (parity with CourseViewSet).
- org_id / contract_id: used to filter parent programs for B2B.
"""
added_context = {}
qp = self.request.query_params

if self.action == "retrieve" or qp.get("readable_id"):
added_context["include_programs"] = True

if qp.get("org_id"):
added_context["org_id"] = qp.get("org_id")
if qp.get("contract_id"):
added_context["contract_id"] = qp.get("contract_id")

return {**super().get_serializer_context(), **added_context}

def get_queryset(self):
return (
Program.objects.filter()
Expand Down
47 changes: 46 additions & 1 deletion courses/views/v2/views_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ def test_get_program(
duplicate_queries_check(context)
program_data = resp.json()
assert_drf_json_equal(
program_data, ProgramDetailSerializer(program).data, ignore_order=True
program_data,
ProgramDetailSerializer(program, context={"include_programs": True}).data,
ignore_order=True,
)


Expand Down Expand Up @@ -367,6 +369,48 @@ def test_retrievinng_single_course_by_pk_or_readable_id_includes_programs(
assert resp2.json()["results"][0] == resp.json()


@pytest.mark.django_db
def test_retrieving_single_program_by_pk_or_readable_id_includes_parent_programs(
user_drf_client,
):
"""Programs retrieved via v2 API should include parent programs when applicable."""

# Create a child program that will be treated as a course in Learn
child_program = ProgramFactory.create(display_mode="course")

# Create a parent program and add the child as a required program
parent_program = ProgramFactory.create()
parent_program.add_requirement(child_program)
parent_program.refresh_from_db()

# Request the child program by primary key
resp = user_drf_client.get(
reverse("v2:programs_api-detail", kwargs={"pk": child_program.id}),
)

assert resp.status_code == status.HTTP_200_OK
program_data = resp.json()

# Verify that parent programs are included in the response
assert "programs" in program_data
assert program_data["programs"] is not None
assert len(program_data["programs"]) == 1
assert program_data["programs"][0]["id"] == parent_program.id
assert program_data["programs"][0]["readable_id"] == parent_program.readable_id
assert program_data["programs"][0]["title"] == parent_program.title

# Check result is same if retrieving by readable_id or via list filter
resp_by_readable = user_drf_client.get(
reverse("v2:programs_api-detail", kwargs={"pk": child_program.readable_id}),
)
resp_list = user_drf_client.get(
reverse("v2:programs_api-list"), {"readable_id": child_program.readable_id}
)

assert resp_by_readable.json() == program_data
assert resp_list.json()["results"][0] == program_data


@pytest.mark.django_db
def test_filter_with_org_id_anonymous():
org = OrganizationPageFactory(name="Test Org")
Expand Down Expand Up @@ -1514,6 +1558,7 @@ def _get_page_prop(program_enrollment, prop, default=None):
!= "",
"topics": [],
"display_mode": None,
"programs": None,
"start_date": ANY_STR,
"end_date": None,
"enrollment_end": None,
Expand Down
14 changes: 14 additions & 0 deletions openapi/specs/v0.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8157,6 +8157,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8336,6 +8342,7 @@ components:
- min_weekly_hours
- min_weeks
- page
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down Expand Up @@ -8442,6 +8449,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8628,6 +8641,7 @@ components:
- min_weeks
- page
- products
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down
14 changes: 14 additions & 0 deletions openapi/specs/v1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8157,6 +8157,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8336,6 +8342,7 @@ components:
- min_weekly_hours
- min_weeks
- page
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down Expand Up @@ -8442,6 +8449,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8628,6 +8641,7 @@ components:
- min_weeks
- page
- products
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down
14 changes: 14 additions & 0 deletions openapi/specs/v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8157,6 +8157,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8336,6 +8342,7 @@ components:
- min_weekly_hours
- min_weeks
- page
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down Expand Up @@ -8442,6 +8449,12 @@ components:
items:
type: integer
readOnly: true
programs:
type: array
items:
$ref: '#/components/schemas/BaseProgram'
nullable: true
readOnly: true
requirements:
type: object
properties:
Expand Down Expand Up @@ -8628,6 +8641,7 @@ components:
- min_weeks
- page
- products
- programs
- readable_id
- req_tree
- required_prerequisites
Expand Down
Loading