Rename department 21M from Music and Theater Arts to Music - #3669
Rename department 21M from Music and Theater Arts to Music#3669zawan-ila wants to merge 5 commits into
Conversation
Update the DEPARTMENTS constant and the departments fixture, and add a data migration that renames the existing 21M department (and its channel title/slug) from "Music and Theater Arts" to "Music". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWziwfKikfpbH3DpfZUABM
Rename the 21M department display name to Music in the OpenAPI specs and the generated TypeScript client, regenerated against the current main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWziwfKikfpbH3DpfZUABM
6db86e0 to
ae0e7a6
Compare
OpenAPI Changes8 changes: 0 error, 1 warning, 7 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
Renames department 21M’s display name from “Music and Theater Arts” to “Music” across backend constants/fixtures and generated OpenAPI + TypeScript client artifacts, and includes a data migration to update existing DB rows (including the associated department channel title/slug).
Changes:
- Update
DEPARTMENTSconstant anddepartments.jsonfixture to use21M → Music. - Add a data migration to rename the existing
LearningResourceDepartment(21M)and update its department channel title/slug. - Regenerate OpenAPI specs (
v0.yaml,v1.yaml) and generated TS API clients to reflect the new display name.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi/specs/v1.yaml | Updates 21M enum documentation to “Music”. |
| openapi/specs/v0.yaml | Updates 21M enum documentation to “Music”. |
| learning_resources/constants.py | Updates DEPARTMENTS["21M"] display name. |
| frontends/api/src/generated/v1/api.ts | Regenerated client types/docs reflecting “Music”. |
| frontends/api/src/generated/v0/api.ts | Regenerated client types/docs reflecting “Music”. |
| data_fixtures/migrations/0025_rename_department_21M_to_music.py | Data migration to rename department + update channel title/slug. |
| data_fixtures/fixtures/departments.json | Fixture value updated for 21M name. |
| channel = Channel.objects.filter(department_detail__department=department).first() | ||
| if channel: | ||
| channel.title = name | ||
| channel.name = slugify(name) | ||
| channel.save() |
There was a problem hiding this comment.
I don't think we associated multiple channels with the same Department so this is a non-issue.
There was a problem hiding this comment.
Looks great overall! Noticed a few things:
-
Search facet still says "Music and Theater Arts." The department filter label comes from
@mitodl/course-search-utils, which has its own hardcoded department map. Rename department 21M to Music course-search-utils#213 fixes it — this PR needs to pin@mitodl/course-search-utilsto whatever version that gets released in. -
Old department name is cached in
Course.course_numbers.etl/utils.py:1023writes the department name into that JSON field, and it's served on the v1 API — so 21M courses will still report "Music and Theater Arts" there. The next ETL run overwrites it for courses still in the upstream feed, but archived ones keep the old name forever. Either update that JSON in the migration too, or say in the PR description that we're accepting the stale value on archived courses. -
/c/department/music-and-theater-artswill 404.next.config.jsalready has legacy redirects; this is a few lines in the same array.
Also, I think the update_index command will need to be run after migrations to get the new label in opensearch. Also maybe generate_embeddings --courses?
There was a problem hiding this comment.
- I don't really see a clear way to do this, and hence would appreciate some guidance. This PR needs to bump course-search-utils with the changes from Rename department 21M to Music course-search-utils#213 but Rename department 21M to Music course-search-utils#213 needs to bump its version of mit-learn-api-axios too, which won't be available until after this PR is on the release branch? So there is kind of a circular dependency here.
- I am thinking it is easiest to just trigger a run of update_course_number_departments post migration? That should update the course number json field as appropriate.
- Thanks for this. I have added the redirect.
- I intend on running
recreate_index --allpost migration. Please let me know if you think that may be a bit too much for us. - I am not really familiar with generate_embeddings at all, but I'll make sure we run that post migration too.
./manage.py generate_embeddings --coursesDoesn't look like it can cause any harm. On this note, it appears that vector search is gated behind a flag and not currently active? Are there any other uses of embeddings/qdrant stuff?
There was a problem hiding this comment.
1 - @ChristopherChudzicki might have some ideas on the best way to handle this. Maybe it's okay (and even unavoidable) to have a slight mismatch temporarily until everything is merged & released across the repos.
2 - Sounds good, I forgot that command exists
4 - That might be overkill, update_index is generally much faster. Probably worth trying it locally to verify if update_index --courses makes all necessary changes without the need for recreate_index
5 - I think vector search is used by default on the topics pages on production. For example https://learn.mit.edu/c/topic/ai makes a call to https://api.learn.mit.edu/api/v0/vector_learning_resources_search/
| @@ -0,0 +1,47 @@ | |||
| # Data migration: rename department 21M from "Music and Theater Arts" to "Music" | |||
There was a problem hiding this comment.
Bug: This PR's migration is numbered 0025, which conflicts with a migration in a companion PR. If the other PR merges first, this will cause a deployment-blocking migration conflict.
Severity: HIGH
Suggested Fix
Before merging, check if the companion 'add 21T' pull request has been merged. If it has, renumber this migration to the next available number (e.g., 0026) and update its dependencies to point to the other 0025 migration. This will ensure a linear, conflict-free migration history.
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: data_fixtures/migrations/0025_rename_department_21M_to_music.py#L1
Potential issue: This pull request introduces a Django migration file named
`0025_rename_department_21M_to_music.py`. A separate, companion pull request also
contains a migration with the same number, `0025`. If the companion pull request is
merged into the main branch first, merging this pull request afterward will introduce a
duplicate migration number. This will cause Django's migration system to fail, blocking
all subsequent deployments until the conflict is manually resolved.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Yup. This is a concern. I'll update the numbers before merge.
What are the relevant tickets?
Part of https://github.com/mitodl/hq/issues/5616
Description (What does it do?)
Renames department 21M from "Music and Theater Arts" to Music in mit-learn:
21M→Musicin theDEPARTMENTSconstant and thedepartmentsfixture.0025_rename_department_21M_to_musicthat renames the existing department row and updates its channel title/slug.v0.yaml,v1.yaml) and the generated TypeScript client for the renamed department display name.How can this be tested?
docker compose run --rm web python manage.py migrate— the 21M department name is Music and its channel title/slug are updated../scripts/generate_openapi.shproduces no diff against this branch.Additional Context
music-and-theater-arts→music), which changes the department's channel URL.0025_*; whichever merges second will need renumbering.