-
Notifications
You must be signed in to change notification settings - Fork 4
Rename department 21M from Music and Theater Arts to Music #3669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
0b94703
ae0e7a6
8967ebb
3716c40
fbbf87e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # Data migration: rename department 21M from "Music and Theater Arts" to "Music" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: This PR's migration is numbered Suggested FixBefore 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., Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. This is a concern. I'll update the numbers before merge. |
||
|
|
||
| from django.db import migrations | ||
| from django.utils.text import slugify | ||
|
|
||
| OLD_NAME = "Music and Theater Arts" | ||
| NEW_NAME = "Music" | ||
|
|
||
|
|
||
| def _rename(apps, name): | ||
| LearningResourceDepartment = apps.get_model( | ||
| "learning_resources", "LearningResourceDepartment" | ||
| ) | ||
| Channel = apps.get_model("channels", "Channel") | ||
|
|
||
| department = LearningResourceDepartment.objects.filter(department_id="21M").first() | ||
| if not department: | ||
| return | ||
| department.name = name | ||
| department.save() | ||
|
|
||
| channel = Channel.objects.filter(department_detail__department=department).first() | ||
| if channel: | ||
| channel.title = name | ||
| channel.name = slugify(name) | ||
| channel.save() | ||
|
Comment on lines
+22
to
+26
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we associated multiple channels with the same Department so this is a non-issue. |
||
|
|
||
|
|
||
| def rename_to_music(apps, schema_editor): | ||
| _rename(apps, NEW_NAME) | ||
|
|
||
|
|
||
| def rename_to_music_and_theater_arts(apps, schema_editor): | ||
| _rename(apps, OLD_NAME) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ( | ||
| "data_fixtures", | ||
| "0024_add_ovs_platform_offeror", | ||
| ), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(rename_to_music, rename_to_music_and_theater_arts), | ||
| ] | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_indexcommand will need to be run after migrations to get the new label in opensearch. Also maybegenerate_embeddings --courses?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recreate_index --allpost migration. Please let me know if you think that may be a bit too much for us../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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_indexis generally much faster. Probably worth trying it locally to verify ifupdate_index --coursesmakes all necessary changes without the need forrecreate_index5 - 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/