Route drawer CTAs for MITxOnline programs-as-courses to correct product pages#3063
Route drawer CTAs for MITxOnline programs-as-courses to correct product pages#3063ChristopherChudzicki wants to merge 11 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the resource_type_group concept across the backend API, OpenAPI specs, and frontend, so clients can reliably treat it as a first-class enum and correctly route “program displayed as course” content on MITxOnline product pages.
Changes:
- Introduces reusable DRF fields for
resource_type_group, including a computed, read-only serializer field for learning resources. - Updates OpenAPI specs (v0/v1) to document
resource_type_groupmore clearly and represent it as an enum (including addingResourceTypeGroupEnumto v0). - Updates frontend routing + tests + factories to handle MITxOnline “program-as-course” links using
resource_type_group.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi/specs/v1.yaml | Updates descriptions and schema typing for resource_type_group across query params and component schemas. |
| openapi/specs/v0.yaml | Adds ResourceTypeGroupEnum and updates resource_type_group fields to reference it. |
| learning_resources/serializers.py | Adds ResourceTypeGroupChoiceField and ComputedResourceTypeGroupField; replaces SerializerMethodField usage. |
| learning_resources/constants.py | Adds RESOURCE_TYPE_GROUP_CHOICES derived from RESOURCE_TYPE_GROUP_VALUES for DRF choice fields. |
| learning_resources_search/serializers.py | Reuses the shared ResourceTypeGroupChoiceField for the search request serializer. |
| frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.tsx | Routes MITxOnline programs to course-style pages when resource_type_group indicates “course”. |
| frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.test.tsx | Adds coverage for the MITxOnline “program-as-course” routing behavior under the feature flag. |
| frontends/api/src/test-utils/factories/learningResources.ts | Ensures factory resources include resource_type_group to match the new generated typings. |
| frontends/api/src/generated/v1/api.ts | Updates generated types so resource_type_group is ResourceTypeGroupEnum instead of string. |
| frontends/api/src/generated/v0/api.ts | Adds ResourceTypeGroupEnum and updates generated types accordingly. |
You can also share your feedback on Copilot code review. Take the survey.
d90e4b3 to
0c07801
Compare
OpenAPI Changes624 changes: 0 error, 624 warning, 0 info Unexpected changes? Ensure your branch is up-to-date with |
| --volume ${{ github.workspace }}:${{ github.workspace }}:rw \ | ||
| -e GITHUB_WORKSPACE=${{ github.workspace }} \ | ||
| tufin/oasdiff changelog --composed \ | ||
| tufin/oasdiff changelog --composed --flatten-allof \ |
There was a problem hiding this comment.
See https://github.com/oasdiff/oasdiff/blob/main/docs/ALLOF.md, -flatten-allof helps reduce some false positives
| ``` | ||
| ${{ steps.oasdif_changelog.outputs.changelog }} | ||
| ``` | ||
| [View full changelog](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) |
There was a problem hiding this comment.
link to the details instead of posting all of the changelog... in this case, it was bigger than the comment could handle
|
Would this be a good PR to make the change to open the product page in the same tab, instead of opening a new one? |
a68c61d to
91d19dc
Compare
| readable_id: resource.readable_id, | ||
| display_mode: null, | ||
| display_mode: | ||
| resource.resource_type_group === ResourceTypeGroupEnum.Course |
There was a problem hiding this comment.
ResourceTypeGroupEnum.Course and DisplayModeEnum.Course both happen to be "course". The code works, but coupling two enums from different domains via string coincidence is fragile. If either value changes, this silently breaks.
Suggestion: Either pass resource.resource_type_group directly as display_mode (since the values align by design), or add a comment explicitly calling out that these values are intentionally the same. Could also be simplified by avoiding DisplayModeEnum entirely here since programPageView just checks for the string "course":
display_mode:
resource.resource_type_group === ResourceTypeGroupEnum.Course
? resource.resource_type_group // "course" satisfies BaseProgramDisplayMode
: undefined,There was a problem hiding this comment.
Added a comment about the data.
| { resource_type: ResourceTypeEnum.Program }, | ||
| { | ||
| resource_type: ResourceTypeEnum.Program, | ||
| resource_type_group: faker.helpers.arrayElement([ |
There was a problem hiding this comment.
resource_type_group: faker.helpers.arrayElement([
ResourceTypeGroupEnum.Program,
ResourceTypeGroupEnum.Course, // random!
]),Randomly picking between Program and Course is realistic but can cause flaky tests if any test renders a program CTA without explicitly overriding resource_type_group. Tests that care about program routing now need to pin this value.
Suggestion: This is probably fine given that the factory comment explains the rationale, but worth making sure all existing program-related tests that check CTA links explicitly set resource_type_group.
There was a problem hiding this comment.
Programs can have either resource_type_group, so our data factories should support both.
I think it's true that this could cause a flakey test. On the other hand, that's also true of how we create randomized test data for all our tests (on the frontend and backend) via faker (frontend) or faker+factory-boy (backend).
A similar example is learningResource(overrides), which creates a random learning resource of any type.
The idea is that if resource_type_group matters for a specific test, the test writer should pin the value
learningResource({ resource_type: "program", resource_type_group: "whatever-specific-value"})
learning_resources/serializers.py
Outdated
|
|
||
| def __init__(self, **kwargs): | ||
| kwargs.setdefault("read_only", True) | ||
| kwargs.setdefault("source", "*") |
There was a problem hiding this comment.
The base class ResourceTypeGroupChoiceField is exported and reused in the search serializer — good. The subclass ComputedResourceTypeGroupField cleanly separates the "what values are valid" concern from the "how to compute the value" concern. This is a nice design.
One minor nit: source="*" on ComputedResourceTypeGroupField is a somewhat obscure DRF pattern. A short comment explaining why it's needed (to pass the full instance to to_representation) would help future readers.
|
Overall Looks good to me. The only thing which I would suggest is the detail level testing which I am sure you would have done. |
91d19dc to
5d94db1
Compare
5d94db1 to
c3582c8
Compare
…API enum generation Extract ResourceTypeGroupChoiceField and ComputedResourceTypeGroupField from the inline SerializerMethodField so that drf-spectacular generates a proper ResourceTypeGroupEnum instead of a plain string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
resource_type_group is now typed as ResourceTypeGroupEnum instead of string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…urses/p/
When the MITxOnline product pages feature flag is enabled, programs with
resource_type_group="course" now link to /courses/p/{readable_id} instead of
/programs/{readable_id}. Also adds resource_type_group to test factories and
test coverage for the program-as-course routing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
c3582c8 to
84e23e0
Compare
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/10538
Note
The vast majority of the line changes here are auto-generated files (e.g., openapi docstring changes)
Description (What does it do?)
When the MITxOnline product pages feature flag is enabled, search drawer CTA links for MITxOnline resources now route to the correct product page based on
resource_type_group:/courses/{readable_id}/courses/p/{readable_id}/programs/{readable_id}Previously, all MITxOnline programs linked to
/programs/{readable_id}becausedisplay_modewas hardcoded tonull.Implementation overview
resource_type_groupfrom aSerializerMethodField(typed asstringin OpenAPI) to aComputedResourceTypeGroupFieldbacked byResourceTypeGroupChoiceField. This causes drf-spectacular to generate a properResourceTypeGroupEnuminstead of a plain string, giving the frontend typed values to branch on.getResourceUrlinCallToActionSection.tsxnow checksresource.resource_type_group === ResourceTypeGroupEnum.Courseto passDisplayModeEnum.CoursetoprogramPageView, routing program-as-course resources to/courses/p/.resource_type_groupto their appropriate enum value.How can this be tested?
Prerequisites:
./manage.py backpopulate_mitxonline_data). Ensure the MITxOnline environment has some courses, programs, and separate programs with display_mode="course"mitxonline-product-pagesfeature flag in PostHog turned on/searchpage, search for an MITxOnline program that hasdisplay_mode="course"; Open the drawer, click "Learn More". You should end up at/courses/p/{readable_id}./search?resource_type_group=course&resource_type=program&platform=mitxonlineshould do the trick/programs/{readable_id}/search?resource_type_group=program&platform=mitxonline/courses/{readable_id}./search?resource_type_group=course&resource_type=course&platform=mitxonline