fix: update B2C subscription help text and automate MySQL row format - #70
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the B2C Subscription Inclusion field metadata in course_metadata and adds a MySQL-specific migration step to proactively set affected tables to ROW_FORMAT=DYNAMIC to avoid InnoDB row-size limit issues during deployment.
Changes:
- Updates
b2c_subscription_inclusionhelp text onCourseandProgrammodels. - Adds a non-atomic migration that checks MySQL table row formats and alters them to
DYNAMICwhen needed. - Applies corresponding
AlterFieldoperations for Course/Program and their historical tables.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| course_discovery/apps/course_metadata/models.py | Updates b2c_subscription_inclusion field help text on Course and Program. |
| course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py | Adds a MySQL row-format adjustment step and updates AlterField help text for Course/Program and historical models. |
Suppressed comments (4)
course_discovery/apps/course_metadata/models.py:3491
- The updated help text changes “B2C” to “B2c”. Please keep the capitalization consistent with “B2C”.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this program is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:72
- This help text also uses “B2c” instead of “B2C”.
blank=True,
default=False,
help_text="Signifies whether this course is included in the B2c subscription catalog.",
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:82
- This help text uses “B2c” instead of “B2C”.
blank=True,
default=False,
help_text="Signifies whether this program is included in the B2c subscription catalog.",
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:92
- This help text uses “B2c” instead of “B2C”.
blank=True,
default=False,
help_text="Signifies whether this program is included in the B2c subscription catalog.",
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3e54664 to
dd1dafb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (6)
course_discovery/apps/course_metadata/models.py:1571
- The help text uses inconsistent capitalization (“B2c”). The rest of the codebase refers to this as “B2C”, so this reads like a typo and may confuse admins/users.
blank=True,
default=False,
help_text=_('Signifies whether this course is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/models.py:3491
- The help text uses inconsistent capitalization (“B2c”). Elsewhere this acronym is consistently “B2C”; please keep it consistent here as well.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this program is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:62
- Migration help_text string uses “B2c” (lowercase c). This should match the established “B2C” capitalization used throughout the codebase.
field=models.BooleanField(
blank=True,
default=False,
help_text="Signifies whether this course is included in the B2c subscription catalog.",
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:72
- Migration help_text string uses “B2c” (lowercase c). Use “B2C” for consistency with the rest of the repository.
field=models.BooleanField(
blank=True,
default=False,
help_text="Signifies whether this course is included in the B2c subscription catalog.",
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:82
- Migration help_text string uses “B2c” (lowercase c). Use the consistent “B2C” capitalization.
field=models.BooleanField(
blank=True,
default=False,
help_text="Signifies whether this program is included in the B2c subscription catalog.",
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:92
- Migration help_text string uses “B2c” (lowercase c). Use “B2C” to match existing wording elsewhere.
field=models.BooleanField(
blank=True,
default=False,
help_text="Signifies whether this program is included in the B2c subscription catalog.",
),
dd1dafb to
2919abe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
course_discovery/apps/course_metadata/models.py:1571
- Help text uses "B2c" (lowercase c), but the feature/branding elsewhere in this repo and the PR description refer to "B2C". This looks like a typo and will surface in admin/metadata UIs.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this course is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/migrations/0359_course_b2c_subscription_inclusion_and_more.py:41
- RunPython is currently irreversible (no reverse_code). Even if the reverse is a no-op, providing one keeps the migration reversible for test/dev workflows that migrate backwards.
operations = [
migrations.RunPython(update_row_format),
course_discovery/apps/course_metadata/migrations/0360_historicalprogram_b2c_subscription_inclusion_and_more.py:41
- RunPython is currently irreversible (no reverse_code). Consider adding reverse_code=migrations.RunPython.noop so the migration remains reversible in dev/test workflows.
operations = [
migrations.RunPython(update_row_format),
course_discovery/apps/course_metadata/models.py:3491
- Help text uses "B2c" (lowercase c). If the intended acronym is "B2C", this will display inconsistently across the UI/admin and translations.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this program is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:32
- This migration updates help_text to use "B2c" (lowercase c). If the intended acronym is "B2C", update all four AlterField statements to keep the DB-level field metadata consistent with the model help_text and PR description.
migrations.AlterField(
model_name='course',
name='b2c_subscription_inclusion',
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this course is included in the B2c subscription catalog.'),
),
2919abe to
6421bdc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (12)
course_discovery/apps/course_metadata/models.py:1570
- The help text uses inconsistent capitalization (“B2c”). Elsewhere in this feature/migrations it’s referred to as “B2C”, so this should be kept consistent to avoid confusing UI/admin strings.
help_text=_('Signifies whether this course is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:21
- This help text uses “B2c” which is inconsistent with the existing “B2C” capitalization used elsewhere in these migrations; consider standardizing to “B2C”.
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this course is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:26
- This help text uses “B2c” which is inconsistent with the existing “B2C” capitalization used elsewhere in these migrations; consider standardizing to “B2C”.
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this program is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:31
- This help text uses “B2c” which is inconsistent with the existing “B2C” capitalization used elsewhere in these migrations; consider standardizing to “B2C”.
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this program is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/migrations/0359_course_b2c_subscription_inclusion_and_more.py:45
- Adding the row-format logic by editing an existing migration can be a no-op for any environment that has already applied this migration, which undermines the goal of preventing deployment failures. Consider moving this to a new migration (that depends on the current latest) so it will always run on upgrade.
operations = [
migrations.RunPython(update_row_format),
migrations.AddField(
course_discovery/apps/course_metadata/migrations/0359_course_b2c_subscription_inclusion_and_more.py:43
- RunPython without a reverse_code makes the migration irreversible. Since changing ROW_FORMAT is operational and doesn’t need a rollback, it’s better to mark the reverse as a noop to keep migrations reversible.
migrations.RunPython(update_row_format),
course_discovery/apps/course_metadata/migrations/0360_historicalprogram_b2c_subscription_inclusion_and_more.py:45
- Adding the row-format logic by editing an existing migration can be a no-op for any environment that has already applied this migration, which undermines the goal of preventing deployment failures. Consider moving this to a new migration (that depends on the current latest) so it will always run on upgrade.
operations = [
migrations.RunPython(update_row_format),
migrations.AddField(
course_discovery/apps/course_metadata/migrations/0360_historicalprogram_b2c_subscription_inclusion_and_more.py:43
- RunPython without a reverse_code makes the migration irreversible. Since changing ROW_FORMAT is operational and doesn’t need a rollback, it’s better to mark the reverse as a noop to keep migrations reversible.
migrations.RunPython(update_row_format),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:16
- This help text uses “B2c” which is inconsistent with the existing “B2C” capitalization used elsewhere in these migrations; consider standardizing to “B2C”.
This issue also appears in the following locations of the same file:
- line 21
- line 26
- line 31
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this course is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/models.py:3490
- The help text uses inconsistent capitalization (“B2c”). Standardizing on “B2C” keeps admin/UI help text consistent across models and migrations.
help_text=_('Signifies whether this program is included in the B2c subscription catalog.'),
course_discovery/apps/course_metadata/migrations/0359_course_b2c_subscription_inclusion_and_more.py:33
- The migration opens a DB cursor but doesn’t close it. Using a context manager ensures the cursor is always cleaned up (even if an exception is raised).
This issue also appears in the following locations of the same file:
- line 42
- line 43
cursor = schema_editor.connection.cursor()
tables = [
"course_metadata_course",
"course_metadata_historicalcourse",
course_discovery/apps/course_metadata/migrations/0360_historicalprogram_b2c_subscription_inclusion_and_more.py:33
- The migration opens a DB cursor but doesn’t close it. Using a context manager ensures the cursor is always cleaned up (even if an exception is raised).
This issue also appears in the following locations of the same file:
- line 42
- line 43
cursor = schema_editor.connection.cursor()
tables = [
"course_metadata_program",
"course_metadata_historicalprogram",
6421bdc to
d98ae82
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (6)
course_discovery/apps/course_metadata/models.py:1571
- Help text uses "B2c" (lowercase “c”), which is inconsistent with the feature name/acronym "B2C" used elsewhere (including existing migrations). This looks like a typo in user-facing admin help text.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this course is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/models.py:3491
- Help text uses "B2c" (lowercase “c”), which is inconsistent with the "B2C" acronym used elsewhere. This is user-facing text in the admin and should likely be capitalized consistently.
b2c_subscription_inclusion = models.BooleanField(
blank=True,
default=False,
help_text=_('Signifies whether this program is included in the B2c subscription catalog.'),
)
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:22
- Migration help_text uses "B2c" (lowercase “c”). To keep admin help text consistent with the established acronym "B2C", update the capitalization here as well.
migrations.AlterField(
model_name='historicalcourse',
name='b2c_subscription_inclusion',
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this course is included in the B2c subscription catalog.'),
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:27
- Migration help_text uses "B2c" (lowercase “c”). To keep admin help text consistent with the established acronym "B2C", update the capitalization here as well.
migrations.AlterField(
model_name='historicalprogram',
name='b2c_subscription_inclusion',
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this program is included in the B2c subscription catalog.'),
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:32
- Migration help_text uses "B2c" (lowercase “c”). To keep admin help text consistent with the established acronym "B2C", update the capitalization here as well.
migrations.AlterField(
model_name='program',
name='b2c_subscription_inclusion',
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this program is included in the B2c subscription catalog.'),
),
course_discovery/apps/course_metadata/migrations/0362_alter_course_b2c_subscription_inclusion_and_more.py:17
- Migration help_text uses "B2c" (lowercase “c”). To keep admin help text consistent with the established acronym "B2C" (and with earlier migrations), this should be capitalized.
This issue also appears in the following locations of the same file:
- line 18
- line 23
- line 28
migrations.AlterField(
model_name='course',
name='b2c_subscription_inclusion',
field=models.BooleanField(blank=True, default=False, help_text='Signifies whether this course is included in the B2c subscription catalog.'),
),
This PR includes two improvements related to the B2C Subscription Inclusion feature: