Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion data_fixtures/fixtures/departments.json
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@
"model": "learning_resources.LearningResourceDepartment",
"fields": {
"department_id": "21M",
"name": "Music and Theater Arts",
"name": "Music",
"school_id": 3,
"created_on": "2024-04-17T00:00:00.000000+00:00",
"updated_on": "2024-04-17T00:00:00.000000+00:00"
Expand Down
47 changes: 47 additions & 0 deletions data_fixtures/migrations/0025_rename_department_21M_to_music.py

Copy link
Copy Markdown
Member

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:

  1. 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-utils to whatever version that gets released in.

  2. Old department name is cached in Course.course_numbers. etl/utils.py:1023 writes 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.

  3. /c/department/music-and-theater-arts will 404. next.config.js already 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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.
  3. Thanks for this. I have added the redirect.
  4. I intend on running recreate_index --all post migration. Please let me know if you think that may be a bit too much for us.
  5. 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 --courses Doesn'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?

Copy link
Copy Markdown
Member

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_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/

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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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),
]
6 changes: 3 additions & 3 deletions frontends/api/src/generated/v0/api.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading