-
Notifications
You must be signed in to change notification settings - Fork 23
feat!: Collection.key -> Collection.collection_code #542
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
Changes from all commits
39ce524
c0a0fbc
200f68b
1b5b8cd
9f08503
5e472a2
b7b9a4a
d55764c
97f850b
0614fcb
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 |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| .claude | ||
| *.py[cod] | ||
| __pycache__ | ||
| .pytest_cache | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| """ | ||
| Rename Collection.key -> Collection.collection_code and change from key_field to code_field. | ||
| """ | ||
| import re | ||
|
|
||
| import django.core.validators | ||
| import django.db.models.lookups | ||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
| import openedx_django_lib.fields | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('openedx_content', '0007_publishlogrecord_direct'), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
|
Contributor
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. This might be another Claude artifact. This migration does not actually depend on the
Member
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 presume we only need it for migrations which touch an FK to User?
Contributor
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. Yes, I looked and every migration in core that has this dependency has a reference to
Member
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. Noted on #322 for followup |
||
| ] | ||
|
|
||
| operations = [ | ||
| # Drop old constraint (references the old field name). | ||
| migrations.RemoveConstraint( | ||
| model_name='collection', | ||
| name='oel_coll_uniq_lp_key', | ||
| ), | ||
| # Rename the column. | ||
| migrations.RenameField( | ||
| model_name='collection', | ||
| old_name='key', | ||
| new_name='collection_code', | ||
| ), | ||
| # Change from key_field (max_length=500, no validator) to code_field | ||
| # (max_length=255, with regex validator). | ||
| migrations.AlterField( | ||
| model_name='collection', | ||
| name='collection_code', | ||
| field=openedx_django_lib.fields.MultiCollationCharField( | ||
| db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, | ||
| max_length=255, | ||
| validators=[ | ||
| django.core.validators.RegexValidator( | ||
| re.compile('^[a-zA-Z0-9_.-]+\\Z'), | ||
| 'Enter a valid "code name" consisting of letters, numbers, underscores, hyphens, or periods.', | ||
| 'invalid', | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| # Re-add uniqueness constraint with the new field name. | ||
| migrations.AddConstraint( | ||
| model_name='collection', | ||
| constraint=models.UniqueConstraint( | ||
| fields=('learning_package', 'collection_code'), | ||
| name='oel_coll_uniq_lp_key', | ||
| ), | ||
| ), | ||
| # DB-level regex check constraint. | ||
| migrations.AddConstraint( | ||
| model_name='collection', | ||
| constraint=models.CheckConstraint( | ||
| condition=django.db.models.lookups.Regex( | ||
| models.F('collection_code'), | ||
| '^[a-zA-Z0-9_.-]+\\Z', | ||
| ), | ||
| name='oel_coll_collection_code_regex', | ||
| violation_error_message=( | ||
| 'Enter a valid "code name" consisting of letters, numbers,' | ||
| ' underscores, hyphens, or periods.' | ||
| ), | ||
| ), | ||
| ), | ||
| ] | ||
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.
Not at all a blocker, but something to consider for the future: our APIs are a bit inconsistent about how we identify the "thing" we're modifying in each API call.
My preference is this style where you can pass in an instance or a primary key:
openedx-core/src/openedx_catalog/api_impl.py
Lines 63 to 64 in 10eae2d
openedx-core/src/openedx_content/applets/containers/api.py
Lines 454 to 455 in 10eae2d
But this collection API is now using a composite (
lp_id,collection_code) pattern:openedx-core/src/openedx_content/applets/collections/api.py
Lines 67 to 69 in 0614fcb
And other APIs accept only an ID:
openedx-core/src/openedx_content/applets/components/api.py
Lines 155 to 156 in 10eae2d
And other APIs accept only an instance:
openedx-core/src/openedx_tagging/api.py
Lines 436 to 437 in 10eae2d
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.
I agree, this would be great to fix soon