Merge pull request #7672 from specify/issue-7660#8125
Conversation
Fix create_cogtype_picklist with update_or_create for run_key_migrations
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCentralizes schema-config migration logic into update_schema_config, adds deduplication and component/agent migration helpers, refactors migrations 0039/0040 to call these helpers, and updates the run_key_migration_functions pipeline to run the new steps and a final ORM deduplication. ChangesSchema-config migration utility consolidation and pipeline update
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
specifyweb/specify/migration_utils/update_schema_config.py (3)
1591-1598: 💤 Low valueUnused loop variable
item.The loop body contains only
pass, making the iteration overitemspointless. If no revert logic is needed, consider removing the loop entirely or adding a TODO comment explaining why the revert is intentionally empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migration_utils/update_schema_config.py` around lines 1591 - 1598, The loop over items in update_schema_config.py is iterating Splocalecontaineritem objects (via items = Splocalecontaineritem.objects.filter(container=container, name=field_name.lower())) but the body only contains pass and the loop variable item is unused; remove the pointless inner loop entirely or replace it with a clear TODO/comment explaining why no revert logic is required, or implement the intended revert/reset logic for ishidden/text within the loop (referencing variables fields, field_name, container, and Splocalecontaineritem to locate the code).
1652-1652: ⚡ Quick winTypo in function name: "compoenents" should be "components".
The function is named
update_hidden_prop_for_compoenentsbut should beupdate_hidden_prop_for_components.Proposed fix
-def update_hidden_prop_for_compoenents(apps, schema_editor=None): +def update_hidden_prop_for_components(apps, schema_editor=None):Note: Also update the call site in
specifyweb/specify/migrations/0040_components.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migration_utils/update_schema_config.py` at line 1652, Rename the misspelled function update_hidden_prop_for_compoenents to update_hidden_prop_for_components and update any call sites to match (notably the call in specifyweb/specify/migrations/0040_components.py). Search for the symbol update_hidden_prop_for_compoenents and replace with update_hidden_prop_for_components in the function definition and every place it's invoked so imports/refs remain consistent; run tests/migrations to confirm no import errors.
426-428: ⚖️ Poor tradeoffPotential memory issue with large datasets.
This list comprehension iterates over all
ContainerItemobjects in Python to filter by the window function result. For databases with many schema config entries, this could consume significant memory.Consider using raw SQL for this operation (similar to
deduplicate_schema_config_sql) or batching the queryset evaluation. The SQL approach in the sibling function is more efficient for large datasets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migration_utils/update_schema_config.py` around lines 426 - 428, The current list comprehension building ids_to_delete by iterating qs can OOM for large tables; replace it with a streaming or SQL approach: either run the existing deduplicate_schema_config_sql (reuse that raw SQL) to delete duplicates in the DB directly, or change the Python path to query only ids (qs.filter(rn__gt=1).values_list('id', flat=True).iterator() or batch in chunks) and then delete in batches using ContainerItem.objects.filter(id__in=batch).delete(); target the ids_to_delete assignment and qs usage to implement one of these approaches.specifyweb/specify/management/commands/run_key_migration_functions.py (1)
52-52: 💤 Low valueConsider removing the commented-out code.
If
update_all_table_schema_config_with_defaultsis intentionally replaced by the targeted migration steps below, consider removing this commented line rather than leaving dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/management/commands/run_key_migration_functions.py` at line 52, Remove the dead commented call to update_all_table_schema_config_with_defaults from run_key_migration_functions (the line "# usc.update_all_table_schema_config_with_defaults,"); if the functionality is intentionally replaced by the targeted migration steps below, delete the commented line to avoid stale code, otherwise restore/replace it with the correct call to usc.update_all_table_schema_config_with_defaults or add a TODO explaining why it must remain commented.specifyweb/specify/migrations/0040_components.py (1)
20-20: ⚡ Quick winMatch helper name:
update_hidden_prop_for_compoenentsis spelled consistently across migration + utility.
specifyweb/specify/migration_utils/update_schema_config.pydefinesupdate_hidden_prop_for_compoenents(...), and the migration callsusc.update_hidden_prop_for_compoenents(...), so line 20’s spelling is correct as-is.revert_cosolidated_python_django_migration_operationsis consistently used bymigrations.RunPython(...); renaming it would be a readability-only change.- Forward vs revert operation counts differ (6 vs 3); confirm the omitted revert steps are intentionally non-reversible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migrations/0040_components.py` at line 20, The migration calls usc.update_hidden_prop_for_compoenents, which matches the helper name in specifyweb/specify/migration_utils/update_schema_config.py, but the unusual spelling should be verified and the migration's revert steps audited: confirm that update_hidden_prop_for_compoenents(...) is present and exported from update_schema_config.py and leave the call as-is if so; then check revert_cosolidated_python_django_migration_operations (the function passed to migrations.RunPython) and reconcile the forward vs revert operation counts (6 forward vs 3 revert) — either implement the missing revert steps in the revert function or annotate/approve the migration as intentionally non-reversible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 1704-1721: reverse_hide_component_fields currently re-applies
hiding by calling items.update(ishidden=True) instead of un-hiding; modify the
function reverse_hide_component_fields to set ishidden=False when updating
Splocalecontaineritem records found for each table/field from
MIGRATION_0040_HIDDEN_FIELDS (use the Splocalecontainer and
Splocalecontaineritem query logic already present) so the items.update(...) call
uses ishidden=False to properly reverse the hide operation.
- Around line 1723-1729: Rename the orchestration function
componets_schema_config_migrations to components_schema_config_migrations and
update its internal calls so it invokes the component-specific helpers: replace
update_schema_config_field_desc with
update_schema_config_field_desc_for_components, replace update_hidden_prop with
update_hidden_prop_for_compoenents, and replace
create_cotype_splocalecontaineritem with
create_cotype_splocalecontaineritem_for_components; also update any external
caller (e.g., the call in run_key_migration_functions.py that references
usc.componets_schema_config_migrations) to use the new
components_schema_config_migrations name so callers match the renamed function.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/run_key_migration_functions.py`:
- Line 52: Remove the dead commented call to
update_all_table_schema_config_with_defaults from run_key_migration_functions
(the line "# usc.update_all_table_schema_config_with_defaults,"); if the
functionality is intentionally replaced by the targeted migration steps below,
delete the commented line to avoid stale code, otherwise restore/replace it with
the correct call to usc.update_all_table_schema_config_with_defaults or add a
TODO explaining why it must remain commented.
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 1591-1598: The loop over items in update_schema_config.py is
iterating Splocalecontaineritem objects (via items =
Splocalecontaineritem.objects.filter(container=container,
name=field_name.lower())) but the body only contains pass and the loop variable
item is unused; remove the pointless inner loop entirely or replace it with a
clear TODO/comment explaining why no revert logic is required, or implement the
intended revert/reset logic for ishidden/text within the loop (referencing
variables fields, field_name, container, and Splocalecontaineritem to locate the
code).
- Line 1652: Rename the misspelled function update_hidden_prop_for_compoenents
to update_hidden_prop_for_components and update any call sites to match (notably
the call in specifyweb/specify/migrations/0040_components.py). Search for the
symbol update_hidden_prop_for_compoenents and replace with
update_hidden_prop_for_components in the function definition and every place
it's invoked so imports/refs remain consistent; run tests/migrations to confirm
no import errors.
- Around line 426-428: The current list comprehension building ids_to_delete by
iterating qs can OOM for large tables; replace it with a streaming or SQL
approach: either run the existing deduplicate_schema_config_sql (reuse that raw
SQL) to delete duplicates in the DB directly, or change the Python path to query
only ids (qs.filter(rn__gt=1).values_list('id', flat=True).iterator() or batch
in chunks) and then delete in batches using
ContainerItem.objects.filter(id__in=batch).delete(); target the ids_to_delete
assignment and qs usage to implement one of these approaches.
In `@specifyweb/specify/migrations/0040_components.py`:
- Line 20: The migration calls usc.update_hidden_prop_for_compoenents, which
matches the helper name in
specifyweb/specify/migration_utils/update_schema_config.py, but the unusual
spelling should be verified and the migration's revert steps audited: confirm
that update_hidden_prop_for_compoenents(...) is present and exported from
update_schema_config.py and leave the call as-is if so; then check
revert_cosolidated_python_django_migration_operations (the function passed to
migrations.RunPython) and reconcile the forward vs revert operation counts (6
forward vs 3 revert) — either implement the missing revert steps in the revert
function or annotate/approve the migration as intentionally non-reversible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b5f812c-151c-45f9-939b-df6f4478e648
📒 Files selected for processing (4)
specifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/migration_utils/update_schema_config.pyspecifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migrations/0040_components.py
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/specify/management/commands/run_key_migration_functions.py (1)
52-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlease keep the full-table schema-config backfill in this repair pipeline.
Commenting out
usc.update_all_table_schema_config_with_defaultsdrops the only sweep that repairs every table for every discipline. The new 0039/0040 helpers only cover the loan/gift-agent and component paths, so affected databases can still miss older schema-config defaults outside those scopes.Suggested change
def fix_schema_config(stdout: WriteToStdOut | None = None): funcs = [ - # usc.update_all_table_schema_config_with_defaults, + usc.update_all_table_schema_config_with_defaults, usc.create_geo_table_schema_config_with_defaults, # specify 0002 usc.create_cotype_splocalecontaineritem, # specify 0003 usc.create_strat_table_schema_config_with_defaults, # specify 0004 - getting skip warnings usc.create_agetype_picklist, # specify 0004🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/management/commands/run_key_migration_functions.py` around lines 52 - 83, The full-table schema-config backfill function usc.update_all_table_schema_config_with_defaults was commented out, which removes the only sweep that applies defaults to every table and risks missing older schema-config fixes; restore a call to usc.update_all_table_schema_config_with_defaults in this migration list (uncomment or re-add it) and place it before the discipline/feature-specific migrations (e.g., before usc.create_discipline_type_picklist / usc.components_schema_config_migrations) so the global defaults run prior to targeted helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 415-436: The current dedupe in the ContainerItem query collapses
rows across different schematype values and deletes attached strings instead of
re-pointing them; update the Window partition in the ContainerItem query to
include F('schematype') so dedupe is scoped to schema-config rows, compute the
survivor id per group (rn==1) and build a mapping from each deleted id to its
survivor id, then before deleting duplicates run updates on related string
tables (e.g., ItemStr and SpLocaleItemStr) to reassign itemname_id and
itemdesc_id from the deleted ids to the survivor id (use bulk update queries
like .filter(itemname_id__in=ids_to_delete).update(itemname_id=survivor_id) and
similarly for itemdesc_id and SpLocaleItemStr), and only after re-pointing
delete the duplicate ContainerItem rows; this change should be applied in the
same helper used by fix_schema_config() so run_key_migration_functions.py
preserves strings and keeps dedupe scoped to schematype.
- Around line 1581-1598: The revert_loan_and_gift_agents helper currently
no-ops; implement the inverse of the forward update by using the entries in
MIGRATION_0038_UPDATE_FIELDS: for each (field_name, ..., ...) tuple looked up in
the loop, load the original label/text and original hidden state from the tuple
(use the third element as the previous text and, if a fourth element exists, as
the previous ishidden boolean; otherwise default previous ishidden to False),
set Splocalecontaineritem.text to that original text and
Splocalecontaineritem.ishidden to the previous ishidden value, then save the
item (call item.save()); ensure you reference the revert_loan_and_gift_agents
function, the MIGRATION_0038_UPDATE_FIELDS mapping, and the Splocalecontainer /
Splocalecontaineritem models when making the change.
- Around line 1637-1650: The bulk updates over MIGRATION_0040_UPDATE_FIELDS are
overwriting all locale rows because Splocaleitemstr queries for itemdesc and
itemname lack a locale filter; update both querysets that reference
Splocaleitemstr (the two Splocaleitemstr.objects.filter(...) chains) to include
language="en" (or the intended locale) before calling .update(...) so only the
English captions/descriptions are modified.
- Around line 1537-1558: The upsert_single_str function is collapsing
translations across all locales because it only filters by
itemdesc_id/itemname_id and omits language/version when querying and creating
Splocaleitemstr; update upsert_single_str to accept language and version (or
obtain them from context), include language and version in the
Splocaleitemstr.objects.filter(...) used to build qs, include language and
version when calling Splocaleitemstr.objects.create(...), and ensure
qs.exclude(id=obj.id).delete() only removes duplicates within that same locale
so that only entries matching itemdesc_id/itemname_id/language/version are
collapsed; leave the obj.text update logic (obj.save(update_fields=["text"]))
as-is.
In `@specifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.py`:
- Around line 14-15: The migration currently calls
usc.revert_loan_and_gift_agents(apps) and
usc.revert_loan_and_gift_agent_fields(apps) but those helpers are no-ops,
leaving Splocalecontaineritem hides and Splocaleitemstr upserts in place; either
implement a true rollback in these helpers to un-hide any Splocalecontaineritem
rows and delete or restore the Splocaleitemstr caption/description rows created
by the forward migration (use the same keys/locales and reverse the upsert logic
in revert_loan_and_gift_agent_fields and undo any flag changes in
revert_loan_and_gift_agents), or mark this RunPython step as irreversible (set
reverse_code=migrations.RunPython.noop and add a clear comment) so the migration
cannot be partially rolled back. Ensure you reference and modify the functions
revert_loan_and_gift_agents and revert_loan_and_gift_agent_fields and operate on
Splocalecontaineritem and Splocaleitemstr using the apps.get_model(...)
querysets to keep migrations deterministic.
---
Outside diff comments:
In `@specifyweb/specify/management/commands/run_key_migration_functions.py`:
- Around line 52-83: The full-table schema-config backfill function
usc.update_all_table_schema_config_with_defaults was commented out, which
removes the only sweep that applies defaults to every table and risks missing
older schema-config fixes; restore a call to
usc.update_all_table_schema_config_with_defaults in this migration list
(uncomment or re-add it) and place it before the discipline/feature-specific
migrations (e.g., before usc.create_discipline_type_picklist /
usc.components_schema_config_migrations) so the global defaults run prior to
targeted helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56a9c1e9-531a-4624-bc87-c633867b7246
📒 Files selected for processing (4)
specifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/migration_utils/update_schema_config.pyspecifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migrations/0040_components.py
| def revert_loan_and_gift_agents(apps): | ||
| """ | ||
| Revert the field name/description updates. | ||
| """ | ||
| Splocalecontainer = apps.get_model('specify', 'Splocalecontainer') | ||
| Splocalecontaineritem = apps.get_model('specify', 'Splocalecontaineritem') | ||
|
|
||
| for table, fields in MIGRATION_0038_UPDATE_FIELDS.items(): | ||
| containers = Splocalecontainer.objects.filter(name=table.lower()) | ||
| for container in containers: | ||
| for (field_name, _, _) in fields: | ||
| items = Splocalecontaineritem.objects.filter( | ||
| container=container, | ||
| name=field_name.lower() | ||
| ) | ||
| for item in items: | ||
| # If needed, reset ishidden or revert text | ||
| pass |
There was a problem hiding this comment.
Rollback for 0039 is still unimplemented.
This helper is now the reverse path for the new loan/gift agent localization step, but the loop body only passes. A reverse migration will therefore leave the forward labels and hidden state in place. Please either implement the inverse here or make that migration explicitly irreversible instead of providing a no-op revert.
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 1596-1596: Loop control variable item not used within loop body
Rename unused item to _item
(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/specify/migration_utils/update_schema_config.py` around lines 1581
- 1598, The revert_loan_and_gift_agents helper currently no-ops; implement the
inverse of the forward update by using the entries in
MIGRATION_0038_UPDATE_FIELDS: for each (field_name, ..., ...) tuple looked up in
the loop, load the original label/text and original hidden state from the tuple
(use the third element as the previous text and, if a fourth element exists, as
the previous ishidden boolean; otherwise default previous ishidden to False),
set Splocalecontaineritem.text to that original text and
Splocalecontaineritem.ishidden to the previous ishidden value, then save the
item (call item.save()); ensure you reference the revert_loan_and_gift_agents
function, the MIGRATION_0038_UPDATE_FIELDS mapping, and the Splocalecontainer /
Splocalecontaineritem models when making the change.
| for table, fields in MIGRATION_0040_UPDATE_FIELDS.items(): | ||
| for field_name, new_name, new_desc in fields: | ||
|
|
||
| Splocaleitemstr.objects.filter( | ||
| itemdesc__container__name=table.lower(), | ||
| itemdesc__container__schematype=0, | ||
| itemdesc__name=field_name.lower() | ||
| ).update(text=new_desc) | ||
|
|
||
| Splocaleitemstr.objects.filter( | ||
| itemname__container__name=table.lower(), | ||
| itemname__container__schematype=0, | ||
| itemname__name=field_name.lower() | ||
| ).update(text=new_name) |
There was a problem hiding this comment.
Filter the component caption updates to the target locale.
Both bulk updates currently touch every Splocaleitemstr row for the matching item, so non-English captions/descriptions get overwritten with the new English text. Add language="en" (or the intended locale) to both querysets before calling update(...).
Proposed fix
Splocaleitemstr.objects.filter(
itemdesc__container__name=table.lower(),
itemdesc__container__schematype=0,
- itemdesc__name=field_name.lower()
+ itemdesc__name=field_name.lower(),
+ language="en",
).update(text=new_desc)
Splocaleitemstr.objects.filter(
itemname__container__name=table.lower(),
itemname__container__schematype=0,
- itemname__name=field_name.lower()
+ itemname__name=field_name.lower(),
+ language="en",
).update(text=new_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/specify/migration_utils/update_schema_config.py` around lines 1637
- 1650, The bulk updates over MIGRATION_0040_UPDATE_FIELDS are overwriting all
locale rows because Splocaleitemstr queries for itemdesc and itemname lack a
locale filter; update both querysets that reference Splocaleitemstr (the two
Splocaleitemstr.objects.filter(...) chains) to include language="en" (or the
intended locale) before calling .update(...) so only the English
captions/descriptions are modified.
| usc.revert_loan_and_gift_agents(apps) | ||
| usc.revert_loan_and_gift_agent_fields(apps) |
There was a problem hiding this comment.
Implement a real rollback for the localized loan/gift agent changes.
Line 14 now delegates to usc.revert_loan_and_gift_agents(apps), but this stack already describes that helper as a no-op placeholder. The forward path hides existing Splocalecontaineritem rows and upserts Splocaleitemstr captions/descriptions, so reversing 0039 will leave those mutations behind even after the fields are removed. Please either add a real revert here or make this RunPython step explicitly irreversible instead of exposing a partial rollback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.py` around
lines 14 - 15, The migration currently calls
usc.revert_loan_and_gift_agents(apps) and
usc.revert_loan_and_gift_agent_fields(apps) but those helpers are no-ops,
leaving Splocalecontaineritem hides and Splocaleitemstr upserts in place; either
implement a true rollback in these helpers to un-hide any Splocalecontaineritem
rows and delete or restore the Splocaleitemstr caption/description rows created
by the forward migration (use the same keys/locales and reverse the upsert logic
in revert_loan_and_gift_agent_fields and undo any flag changes in
revert_loan_and_gift_agents), or mark this RunPython step as irreversible (set
reverse_code=migrations.RunPython.noop and add a clear comment) so the migration
cannot be partially rolled back. Ensure you reference and modify the functions
revert_loan_and_gift_agents and revert_loan_and_gift_agent_fields and operate on
Splocalecontaineritem and Splocaleitemstr using the apps.get_model(...)
querysets to keep migrations deterministic.
|
Addressing code changes in #8142 |
Fix create_cogtype_picklist with update_or_create for run_key_migrations
Fixes #7660
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Summary by CodeRabbit
New Features
Bug Fixes