fix: make page a nullable property and specs updated#3426
Open
marslanabdulrauf wants to merge 4 commits intomainfrom
Open
fix: make page a nullable property and specs updated#3426marslanabdulrauf wants to merge 4 commits intomainfrom
marslanabdulrauf wants to merge 4 commits intomainfrom
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
from
7746b42 to
283396f
Compare
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
2 times, most recently
from
fc9abea to
1a2f449
Compare
Contributor
Author
|
@ChristopherChudzicki should we bump the version for breaking change which is causing |
Contributor
Sorry I didn't have time to review this more carefully today, but IMO this isn't a breaking change—it's just making the spec accurate. The API can return null now, but the spec doesn't reflect that. We aren't changing the API, we are fixing the spec. |
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
from
1a2f449 to
fd56e0e
Compare
Contributor
Author
|
Oh, I checked the V2Course serializer didn't had Also had similar requirement for Programs as well:
So made changes around that as well and updated the v2 specs. |
Contributor
👍 That sounds good to me. Again, not a breaking change—the serializer already returned null for page, e.g., https://mitxonline.mit.edu/api/v2/courses/course-v1:UAI_NTUA_SOURCE+NTUA.UAI.VT.3/ (since DRF serializers don't validate responses, only requests). My suggestion is:
|
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
2 times, most recently
from
41ac17a to
7fa7635
Compare
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
from
7fa7635 to
00f3ead
Compare
marslanabdulrauf
force-pushed
the
marslan/10652-page-nullable
branch
from
ee7a32c to
271506c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the v2 course/program serializers and OpenAPI specs so the page field can be null when no CMS page is associated, and adds test coverage for that behavior.
Changes:
- Allow
pageto serialize asNonefor courses/programs without an associated CMS page and add serializer tests for this case. - Update OpenAPI spec files (v0/v1/v2) to mark
pageasnullable: true. - Add an
oasdiffignore file and wire it into the OpenAPI diff workflow; adjust spec-check script to ignore the new.txtfile.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test/openapi_spec_check.sh | Exclude .txt files from spec directory diffs (but current invocation is broken; see comment). |
| openapi/specs/v0.yaml | Mark course/program page schemas as nullable. |
| openapi/specs/v1.yaml | Mark course/program page schemas as nullable. |
| openapi/specs/v2.yaml | Mark course/program page schemas as nullable. |
| openapi/specs/oasdiff-err-ignore.txt | New ignore list for oasdiff breaking checks related to page becoming nullable. |
| drf_lint_baseline.json | Update baseline line references due to code shifts. |
| courses/serializers/v2/programs.py | Return None for program page when absent; avoid repeated .all() calls. |
| courses/serializers/v2/programs_test.py | Add test asserting page is None when program has no CMS page. |
| courses/serializers/v2/courses.py | Allow nested course page field to be null. |
| courses/serializers/v2/courses_test.py | Add test asserting page is None when course has no CMS page. |
| .github/workflows/openapi-diff.yml | Pass --err-ignore file to oasdiff breaking when present. |
Comments suppressed due to low confidence (1)
scripts/test/openapi_spec_check.sh:15
diffoptions need to come before the directory operands. As written,--exclude="*.txt"will be treated as a third path argument on many systems and cause the script to fail. Also, withset -e, a non-zerodiff(when specs are out of date) will exit immediately before the followingif [ $? -eq 0 ]block runs; wrap the diff in theif diff ...; then ...form (or otherwise capture the exit status) so the custom message executes reliably.
diff $TMPDIR $SPECS_DIR --exclude="*.txt"
if [ $? -eq 0 ]; then
echo "OpenAPI spec is up to date!"
exit 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ChristopherChudzicki
approved these changes
May 8, 2026
Contributor
|
👍 Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/10652
Description (What does it do?)
This pull request updates the course and program serializers to properly handle cases where a course or program does not have an associated CMS page. The serializers now return
nullfor thepagefield instead of a default object, and the OpenAPI specs are updated to reflect this. Corresponding tests are added to ensure this behavior.Serializer improvements:
CourseSerializerandProgramSerializerto allow thepagefield to benullif there is no associated CMS page, returningNoneinstead of a default object. (courses/serializers/v2/courses.py[1]courses/serializers/v2/programs.py[2]programs.pyby removing unusedget_thumbnail_url. (courses/serializers/v2/programs.pycourses/serializers/v2/programs.pyL15-R15)OpenAPI specification updates:
nullable: trueto thepagefields for courses and programs in all OpenAPI spec versions (v0.yaml,v1.yaml,v2.yaml) to indicate that these fields can benull.Testing:
page: Nonewhen a course or program does not have a CMS page. (courses/serializers/v2/courses_test.py[1]courses/serializers/v2/programs_test.py[2]How can this be tested?