Add department 21T (Theater Arts) - #3668
Conversation
f78ddea to
fed57e7
Compare
OpenAPI Changes21 changes: 0 error, 0 warning, 21 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
Adds support for MIT department 21T (Theater Arts) across the backend data model and the public API surface so ingestion and consumers can recognize the department and its enum value.
Changes:
- Added
21T → Theater Artsto the backendDEPARTMENTSconstant. - Added seed data for the department (fixture) and a data migration to create the department + its channel.
- Regenerated OpenAPI specs and the generated TypeScript API clients to include
21Tin department enums.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openapi/specs/v1.yaml | Adds 21T to department enum values/descriptions in the v1 OpenAPI spec. |
| openapi/specs/v0.yaml | Adds 21T to department enum values/descriptions in the v0 OpenAPI spec. |
| learning_resources/constants.py | Adds 21T to the server-side departments mapping. |
| frontends/api/src/generated/v1/api.ts | Regenerated client: adds 21T to v1 department enums/descriptions and request types. |
| frontends/api/src/generated/v0/api.ts | Regenerated client: adds 21T to v0 department enums/descriptions and request types. |
| data_fixtures/migrations/0025_add_department_21T.py | Data migration to create department 21T and its associated channel/detail record. |
| data_fixtures/fixtures/departments.json | Adds 21T department fixture entry linked to school id 3. |
Comments suppressed due to low confidence (1)
data_fixtures/migrations/0025_add_department_21T.py:58
ChannelDepartmentDetailuseschannelas a OneToOne primary key (channels/models.py:203-208). Usingget_or_create(channel=..., department=...)includesdepartmentin the lookup, so if a detail row already exists for the channel but with a different/NULL department, Django will attempt to create a second row with the same primary key and fail. Useupdate_or_createkeyed only onchannel, and setdepartmentviadefaults.
ChannelDepartmentDetail.objects.get_or_create(
channel=channel,
department=department,
defaults={"channel": channel, "department": department},
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
data_fixtures/migrations/0025_add_department_21T.py:42
add_theater_artsuses unconditionalobjects.create(...)for the department/channel. This makes the migration non-idempotent and it can fail with an IntegrityError if21T(or its channel) already exists (e.g., if data was loaded via fixtures in an existing environment). Also, if the school lookup returnsNone, this still creates a department channel even though the app’s channel upsert logic treats departments without a school as invalid.
Consider making this migration resilient by (1) ensuring the school exists, (2) using update_or_create for the department, (3) using update_or_create for the channel and department detail.
school = LearningResourceSchool.objects.filter(name=SCHOOL_NAME).first()
department = LearningResourceDepartment.objects.create(
department_id="21T",
name="Theater Arts",
|
|
||
| school = LearningResourceSchool.objects.filter(name=SCHOOL_NAME).first() | ||
|
|
||
| department = LearningResourceDepartment.objects.create( |
There was a problem hiding this comment.
Change to get_or_create here. On a fresh database, this will cause an error because the 21T department already gets created in the initial migration which loads data from the departments.json file, where 21T was also added in this PR.
department = LearningResourceDepartment.objects.get_or_create(
department_id="21T",
defaults={
"name": "Theater Arts",
"school": school,
}
)There was a problem hiding this comment.
Updated to incorporate suggestion
|
|
||
| school = LearningResourceSchool.objects.filter(name=SCHOOL_NAME).first() |
There was a problem hiding this comment.
Maybe change to school = LearningResourceSchool.objects.get(name=SCHOOL_NAME) - if this school does not exist, the department is created school-less, channels/plugins.py:99-102 deletes department channels with no school — you'd get a department with no channel and no error. So best to error out here in that case. Though this shouldn't happen in practice because the school exists (created in initial migration from fixtures).
| channel = Channel.objects.create( | ||
| search_filter=urlencode({"department": department.department_id}), | ||
| channel_type=ChannelType.department.name, | ||
| name=slugify(department.name), | ||
| title=department.name, | ||
| ) | ||
| ChannelDepartmentDetail.objects.create( | ||
| channel=channel, | ||
| department=department, | ||
| ) |
There was a problem hiding this comment.
Same thing here - get_or_create for both:
channel, _ = Channel.objects.get_or_create(
search_filter=urlencode({"department": department.department_id}),
channel_type=ChannelType.department.name,
defaults={
"name": slugify(department.name),
"title": department.name,
},
)
ChannelDepartmentDetail.objects.get_or_create(
channel=channel,
department=department,
)| "21M": "Music and Theater Arts", | ||
| "21T": "Theater Arts", |
There was a problem hiding this comment.
These two should go out together with #3669 - until the 21M rename lands, we'd have both "Music and Theater Arts" and "Theater Arts" showing up in the department facet, each with its own channel. Not a problem with this PR itself, just worth merging them back to back (and #3669 will need the migration renumbered to 0026, as you called out in the description).
There was a problem hiding this comment.
These two should go out together
I decided to send these out separately, though still as close to each other as possible.
The problem with sending them out together is that, for a while, until some of the existing 21M courses are moved to 21T (which is the plan) and the indexes updated, the "to be" Theater Arts courses would show up under the Music facet. The problem with sending them out separately, as you note, is that we have both the "Theater Arts" and "Music and Theater Arts" facets. To me, that is "more acceptable" than combining the changes in a single deployment (i.e "to be Theater Arts" courses showing up under "Music"). I also let Peter know about this (see item 1 below the divider here) and he seemed fine with it.
I do think that this is not ideal, and would love any suggestions on approaches that eliminate the negative aspects of both combined and separate deploys noted above.
| "21H": "History", | ||
| "21L": "Literature", | ||
| "21M": "Music and Theater Arts", | ||
| "21T": "Theater Arts", |
There was a problem hiding this comment.
One deploy-time thing to be aware of: extract_valid_department_from_id only returns a department if it's a key in this dict, so any edX/MITx Online course already ingested with a 21T prefix currently has no department attached. Those will pick it up on the next ETL run since load_departments re-set()s on every load, but the channel will look empty until then. If we want it immediate, ./manage.py update_course_number_departments followed by a reindex does it - same approach as learning_resources/migrations/0025_update_course_number_depts.py.
OCW is the opposite case and this PR is a straight fix there: OCW departments come from department_numbers in the metadata without going through DEPARTMENTS, and load_departments does an unguarded .get(department_id=...), so 21T courses were raising DoesNotExist before this.
There was a problem hiding this comment.
I don't fully understand this, but I do plan on running update_course_number_departments and update_index post migration, so I think we should be good here.
There was a problem hiding this comment.
Sounds good, maybe ./manage.py backpopulate_ocw_data too
Add 21T -> Theater Arts to the DEPARTMENTS constant, a departments fixture entry (SHASS, school_id 3), and an idempotent data migration (get_or_create) that creates the department and its channel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ktaufxca3bmtQ3L4qVwmsU
Regenerated against current main via ./scripts/generate_openapi.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ktaufxca3bmtQ3L4qVwmsU
0b6afe9 to
52bf987
Compare
What are the relevant tickets?
Part of https://github.com/mitodl/hq/issues/5616
Description (What does it do?)
Introduces department 21T (Theater Arts) in mit-learn:
21T→Theater Artsto theDEPARTMENTSconstant (learning_resources/constants.py).departmentsfixture entry linked to the School of Humanities, Arts, and Social Sciences (school_id3).0025_add_department_21T(modeled on0014_add_department_SP) that creates the department and its channel.openapi/specs/v0.yaml,v1.yaml) and the generated TypeScript client to include21Tin the department enum.How can this be tested?
This and mitodl/course-search-utils#212 should be tested together.
"@mitodl/course-search-utils": "https://github.com/mitodl/course-search-utils#f75060db9010d3ab108bfb9968382bdca3bef514"docker compose up s3. You may find https://github.com/mitodl/ocw-studio#local-s3-emulation-with-minio helpful for setting up minio. Now, visit the minio interface at localhost:9001. In minio, in some bucket, under some course path,courses/14-01sc-principles-of-microeconomics-fall-2011/data.json, add a data.json file that has the 21T department, and an appropriate course number21T.789(Just modify, for example, this data.json file to achieve the desired effect). Now, set up your learn instance to point to that minio instance. Set in your backend.local.envAlso set
AWS_ENDPOINT_URL_S3="http://host.docker.internal:9000"to allow learn to communicate with the minio container.Now, run a backpopulate like
./manage.py backpopulate_ocw_data --course-name <s3 path segment identifying the course>. Then, visit the search page and verify that the Theater Arts facet shows up under departments and clicking on it works as expected.Additional Context
0025_*. Whichever merges second will need renumbering (e.g.0026_*).