Compartmentalize migration code#8223
Open
CarolineDenis wants to merge 48 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR splits the monolithic Changesmigration_utils Module Split
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 |
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/Deduplication.py`:
- Around line 83-92: The duplicate container detection in the annotated subquery
only matches containers with a strictly earlier timestampcreated value, which
allows containers with identical timestamps to evade deduplication. Modify the
filter conditions within the Exists subquery to also match containers where the
timestampcreated values are equal but the primary key (id) is smaller than the
outer reference's id, using OuterRef('id') as the tie-breaker. This can be
accomplished by adding an OR condition using Q objects or by adding an
additional filter clause that handles the equal timestamp case with id as the
deterministic ordering criterion.
In `@specifyweb/specify/migration_utils/MigrationHelpers.py`:
- Around line 1-16: The MigrationHelpers module is imported by
sync_schema_config_fields which calls find_missing_schema_config_fields() and
create_missing_schema_config_fields() as module attributes, but these functions
are not currently imported or re-exported in this file. Add imports for
find_missing_schema_config_fields and create_missing_schema_config_fields from
the appropriate source modules (SchemaReader and/or SchemaWriter) to the
existing import statements in MigrationHelpers.py so these functions become
accessible when the module is imported.
In `@specifyweb/specify/migration_utils/SchemaReader.py`:
- Line 18: Break the circular import between SchemaReader and SchemaWriter by
moving the FieldDefaults class definition from SchemaWriter to SchemaReader.
First, locate the FieldDefaults class in SchemaWriter (note there are duplicate
definitions at lines 24 and 145 that should be consolidated into a single
definition) and move it to SchemaReader where it is actually needed for the
TableDefaults TypedDict definition. Then update SchemaWriter to import
FieldDefaults from SchemaReader instead of defining it locally, and remove both
duplicate definitions from SchemaWriter.
In `@specifyweb/specify/migration_utils/SchemaWriter.py`:
- Around line 72-74: The revert helper functions filter using raw
caller-provided table_name values, but the forward helpers write schema
containers with lower-case names, causing revert filters to miss created rows.
At the anchor location in lines 72-74, the forward write uses
table_config.name.lower(). Apply the same normalization to revert filter
operations at lines 134-135 and 281-282 by changing filters that reference
table_name to use the lower-cased version instead. Additionally, add schematype:
0 filtering to all three revert locations (72-74, 134-135, and 281-282) since
forward helpers only create schematype=0 records, ensuring reverts only target
the correct records they created.
- Around line 313-331: The update_table_field_schema_config_params() function is
creating Splocalecontainer objects when they don't exist but then returns early
if the field item is missing, leaving incomplete containers without proper
labels/items that interfere with later sync operations. Remove the container
creation logic from this function (around the Splocalecontainer.objects.create
call) and instead return early when sp_local_container is None, or delegate
container creation to a full create helper that properly initializes all
container attributes. This applies to two locations in the file as noted in the
consolidated sites.
- Around line 216-227: The defaults for `ishidden`, `isrequired`, and
`picklistname` are only being applied during field creation, but existing/reused
schema items are not being updated with these values. Ensure that when
processing existing field items (not just newly created ones), these default
values from the defaults dictionary are explicitly applied to update the
corresponding field properties. This requires identifying where field items are
persisted back to the schema and ensuring that the values derived from
defaults.get('ishidden', field_hidden), defaults.get('isrequired',
field_required), and defaults.get('picklistname', picklist_name) are actually
assigned to the existing field object in addition to being applied during
creation.
🪄 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: 878aedba-067e-4017-a764-696da4e5dd9f
📒 Files selected for processing (31)
specifyweb/backend/setup_tool/schema_defaults.pyspecifyweb/backend/workbench/migrations/0007_spdatasetattachment.pyspecifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/management/commands/sync_schema_config_fields.pyspecifyweb/specify/migration_utils/Deduplication.pyspecifyweb/specify/migration_utils/MigrationHelpers.pyspecifyweb/specify/migration_utils/SchemaReader.pyspecifyweb/specify/migration_utils/SchemaWriter.pyspecifyweb/specify/migrations/0002_geo.pyspecifyweb/specify/migrations/0003_cotype_picklist.pyspecifyweb/specify/migrations/0004_stratigraphy_age.pyspecifyweb/specify/migrations/0007_schema_config_update.pyspecifyweb/specify/migrations/0008_ageCitations_fix.pyspecifyweb/specify/migrations/0012_add_cojo_to_schema_config.pyspecifyweb/specify/migrations/0013_collectionobjectgroup_parentcog.pyspecifyweb/specify/migrations/0015_add_version_to_ages.pyspecifyweb/specify/migrations/0017_schemaconfig_fixes.pyspecifyweb/specify/migrations/0018_cot_catnum_schema.pyspecifyweb/specify/migrations/0020_add_tectonicunit_to_pc_in_schema_config.pyspecifyweb/specify/migrations/0021_update_hidden_geo_tables.pyspecifyweb/specify/migrations/0023_update_schema_config_text.pyspecifyweb/specify/migrations/0024_add_uniqueIdentifier_storage.pyspecifyweb/specify/migrations/0027_CO_children.pyspecifyweb/specify/migrations/0029_remove_collectionobject_parentco.pyspecifyweb/specify/migrations/0032_add_quantities_gift.pyspecifyweb/specify/migrations/0033_update_paleo_desc.pyspecifyweb/specify/migrations/0034_accession_date_fields.pyspecifyweb/specify/migrations/0035_version_required.pyspecifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migrations/0040_components.pyspecifyweb/specify/migrations/0042_discipline_type_picklist.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
…after-review-2' into issue-8058
Contributor
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
specifyweb/specify/migrations/0007_schema_config_update.py (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the long import statement for better readability.
This import line is extremely long and difficult to read. Consider using parentheses to split it across multiple lines following PEP 8 style guidelines.
♻️ Proposed refactor to improve readability
-from specifyweb.specify.migration_utils.migration_helpers.helper_0007_schema_config_update import create_cogtype_picklist, revert_cog_type_fields, revert_cogtype_picklist, revert_cogtype_splocalecontaineritem, revert_cogtype_type_splocalecontaineritem, revert_systemcogtypes_picklist, update_cog_type_fields, update_cogtype_splocalecontaineritem, update_cogtype_type_splocalecontaineritem, update_systemcogtypes_picklist +from specifyweb.specify.migration_utils.migration_helpers.helper_0007_schema_config_update import ( + create_cogtype_picklist, + revert_cog_type_fields, + revert_cogtype_picklist, + revert_cogtype_splocalecontaineritem, + revert_cogtype_type_splocalecontaineritem, + revert_systemcogtypes_picklist, + update_cog_type_fields, + update_cogtype_splocalecontaineritem, + update_cogtype_type_splocalecontaineritem, + update_systemcogtypes_picklist, +)🤖 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/0007_schema_config_update.py` at line 19, The import statement that imports create_cogtype_picklist, revert_cog_type_fields, revert_cogtype_picklist, revert_cogtype_splocalecontaineritem, revert_cogtype_type_splocalecontaineritem, revert_systemcogtypes_picklist, update_cog_type_fields, update_cogtype_splocalecontaineritem, update_cogtype_type_splocalecontaineritem, and update_systemcogtypes_picklist from the migration helper module is too long and difficult to read. Refactor this import statement by wrapping it in parentheses and splitting it across multiple lines, placing each imported function on its own line or grouping related imports together, to comply with PEP 8 style guidelines for improved readability.specifyweb/specify/migrations/0042_discipline_type_picklist.py (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the import statement for consistency.
While not as long as some other imports in this refactoring, splitting this import across multiple lines would improve consistency and readability across the migration files.
♻️ Proposed refactor for consistency
-from specifyweb.specify.migration_utils.migration_helpers.helper_0042_discipline_type_picklist import create_discipline_type_picklist, revert_discipline_type_picklist, revert_discipline_type_splocalecontaineritem, update_discipline_type_splocalecontaineritem +from specifyweb.specify.migration_utils.migration_helpers.helper_0042_discipline_type_picklist import ( + create_discipline_type_picklist, + revert_discipline_type_picklist, + revert_discipline_type_splocalecontaineritem, + update_discipline_type_splocalecontaineritem, +)🤖 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/0042_discipline_type_picklist.py` at line 3, The import statement in the migration file is currently a single long line importing multiple functions (create_discipline_type_picklist, revert_discipline_type_picklist, revert_discipline_type_splocalecontaineritem, update_discipline_type_splocalecontaineritem) from the helper_0042_discipline_type_picklist module. Split this import across multiple lines for improved readability and consistency with other migration files. Each imported function or a logical grouping of functions should be on its own line.
🤖 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/backend/permissions/migrations/0008_attachment_import_role.py`:
- Around line 88-91: The migrations.RunPython operation for
add_attachment_import_role needs to be made reversible by providing a reverse
function. Create a new function that reverses the changes made by
add_attachment_import_role, which should delete the "Bulk Attachment Import"
LibraryRole and its associated policies. Then update the migrations.RunPython
call to pass both the forward function (add_attachment_import_role) and the new
reverse function as arguments, in that order, to make the migration reversible.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.py`:
- Line 22: In the UserRole tuple definition that contains the schema
description, there is a typo in the description string where "ecify users"
should be "Specify users". Locate the UserRole tuple entry and correct the
description text to read "Records roles associated with Specify users." instead
of "Records roles associated with ecify users."
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0008_schema_config_update.py`:
- Around line 20-24: The function `revert_relative_age_fields` has an
indentation error where the for loops on lines 22-24 are over-indented relative
to the function body. Fix this by reducing the indentation of both the outer for
loop (iterating over MIGRATION_0008_FIELDS.items()) and the inner for loop
(iterating over fields) to align with the comment above them. Both loops should
be indented exactly one level from the function definition to be at the proper
function body indentation level.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0017_schemaconfig_fixes.py`:
- Around line 38-43: The migration helper currently skips schema-config updates
when the table does not exist in the latest datamodel state (when
datamodel.get_table(table_name) returns None), but this approach is
non-deterministic for historical migrations with renamed or removed tables and
leaves stale entries uncorrected. Instead of checking table existence against
the current datamodel, refactor the logic to drive schema-config updates
directly from existing Splocalecontainer or Splocalecontaineritem rows in the
database (or use frozen migration metadata), ensuring that historical data is
processed regardless of whether the table still exists in the latest datamodel
state. Apply this refactoring to both code sections mentioned (lines 38-43 and
74-79).
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0021_update_hidden_geo_tables.py`:
- Around line 49-53: The reverse migration in the update_hidden_geo_tables
helper unconditionally sets ishidden=False on
Splocalecontaineritem.objects.filter(), which loses the original state of
records that were already hidden. Either store the original ishidden values for
each affected row during the forward migration and restore them in the reverse
migration, or make the reverse migration a no-op (empty operation) with a clear
comment explaining that this migration is irreversible and data about prior
hidden states cannot be recovered.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0023_update_schema_config_text.py`:
- Around line 191-193: The duplicate_items queryset selection is
non-deterministic because it uses .first() without any ordering, which means a
random duplicate item could be kept each time. Add an .order_by() clause to the
duplicate_items queryset before calling .first() on it to ensure the same item
is consistently selected. This will make the retention of item_to_keep
deterministic and ensure consistent metadata is preserved across migrations.
- Around line 206-208: The reverse migration query for
Splocalecontainer.objects.filter() is not matching the forward migration scope.
Add a schematype filter to the query on Splocalecontainer.objects.filter() to
include schematype=0, matching the constraint applied in the forward migration
path. This ensures the reverse migration only targets the same containers that
were modified by the forward migration, preventing unhiding of rows that were
never modified by this migration.
- Line 72: In the CollectionObjectGroup field list, remove the leading
whitespace from the field name ' text3' so it becomes 'text3' without the space.
The leading whitespace in the field name will prevent proper matching against
actual schema field names during the migration, causing the hidden-flag logic to
skip that field entirely.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0027_CO_children.py`:
- Around line 75-79: The for loop iterating over fields is receiving tuple
objects instead of unpacking them, causing the
Splocalecontaineritem.objects.filter call with name=field_name to compare
against a tuple value instead of the actual field name string. Modify the for
loop to properly unpack the tuple from fields, extracting only the field name
component that should be used in the filter's name parameter.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0033_update_paleo_desc.py`:
- Line 16: The update operation on
Splocaleitemstr.objects.filter(containerdesc__in=containers).update(text=table_desc)
is overwriting localized descriptions across all languages instead of updating
only the intended language. Add an additional filter condition to the queryset
to specify which language or locale should be updated, ensuring that
translations in other languages are not overwritten. This will require filtering
by a language identifier or locale field before calling the update method.
In
`@specifyweb/specify/migration_utils/migration_helpers/helper_0042_discipline_type_picklist.py`:
- Around line 59-63: The revert_discipline_type_picklist function currently
deletes all Picklist objects with the name matching
DISCIPLINE_TYPE_PICKLIST_NAME, which can inadvertently remove user-created
picklists with the same name. Modify the filter criteria in the
Picklist.objects.filter() call to include additional attributes that uniquely
identify picklists created by this migration (such as system ownership or
application-specific markers) to ensure only migration-owned picklists are
deleted during rollback.
- Around line 67-86: The rollback function
`revert_discipline_type_splocalecontaineritem` is incomplete because it only
resets the `picklistname` field to None but fails to reset the `isrequired`
field that was set to True in the forward migration. Update the `.update()` call
in `revert_discipline_type_splocalecontaineritem` to also include
`isrequired=None` alongside the existing `picklistname=None` to fully restore
the original state before the migration.
In `@specifyweb/specify/migration_utils/schema_writer.py`:
- Around line 40-94: The function get_or_create_splocalecontainer lowercases
resolved_container_attrs['name'] on line 57, and then passes this lowercased
value to camel_to_spaced_title_case(uncapitilize()) at lines 84 and 89 for label
generation. Since the camelCase detection regex only recognizes uppercase
letters, lowercased input like "collectionobjecttype" cannot be properly split
into words. Preserve the original casing of the name before the lowercasing
operation by storing it in a separate variable, then use that original-cased
value when calling camel_to_spaced_title_case() for the table_label and
table_description defaults, while keeping the lowercased version only for the
database operations (filter and create).
In `@specifyweb/specify/migrations/0029_remove_collectionobject_parentco.py`:
- Line 6: Remove the unused import alias from the migration_helpers import
statement. The alias usc is not referenced anywhere in the migration file and
only clutters the code. Simply remove the "as usc" portion from the import
statement so that migration_helpers is imported directly without an alias,
keeping the import clean and focused on what is actually being used.
---
Nitpick comments:
In `@specifyweb/specify/migrations/0007_schema_config_update.py`:
- Line 19: The import statement that imports create_cogtype_picklist,
revert_cog_type_fields, revert_cogtype_picklist,
revert_cogtype_splocalecontaineritem, revert_cogtype_type_splocalecontaineritem,
revert_systemcogtypes_picklist, update_cog_type_fields,
update_cogtype_splocalecontaineritem, update_cogtype_type_splocalecontaineritem,
and update_systemcogtypes_picklist from the migration helper module is too long
and difficult to read. Refactor this import statement by wrapping it in
parentheses and splitting it across multiple lines, placing each imported
function on its own line or grouping related imports together, to comply with
PEP 8 style guidelines for improved readability.
In `@specifyweb/specify/migrations/0042_discipline_type_picklist.py`:
- Line 3: The import statement in the migration file is currently a single long
line importing multiple functions (create_discipline_type_picklist,
revert_discipline_type_picklist, revert_discipline_type_splocalecontaineritem,
update_discipline_type_splocalecontaineritem) from the
helper_0042_discipline_type_picklist module. Split this import across multiple
lines for improved readability and consistency with other migration files. Each
imported function or a logical grouping of functions should be on its own line.
🪄 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: ba775b37-0d55-45b6-961a-e15dffed728a
📒 Files selected for processing (58)
specifyweb/backend/permissions/migrations/0008_attachment_import_role.pyspecifyweb/backend/setup_tool/schema_defaults.pyspecifyweb/backend/workbench/migrations/0007_spdatasetattachment.pyspecifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/management/commands/sync_schema_config_fields.pyspecifyweb/specify/migration_utils/deduplication.pyspecifyweb/specify/migration_utils/default_cots.pyspecifyweb/specify/migration_utils/migration_helpers/__init__.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0004_stratigraphy_age.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0007_schema_config_update.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0008_schema_config_update.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0012_add_cojo_to_schema_config.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0013_collectionobjectgroup_parentcog.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0015_add_version_to_ages.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0017_schemaconfig_fixes.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0018_cot_catnum_schema.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0020_add_tectonicunit_to_pc_in_schema_config.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0021_update_hidden_geo_tables.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0023_update_schema_config_text.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0024_add_uniqueIdentifier_storage.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0027_CO_children.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0029_remove_collectionobject_parentco.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0032_add_quantities_gift.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0033_update_paleo_desc.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0034_accession_date_fields.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0035_version_required.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0040_components.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0042_discipline_type_picklist.pyspecifyweb/specify/migration_utils/schema_reader.pyspecifyweb/specify/migration_utils/schema_writer.pyspecifyweb/specify/migration_utils/sp7_schemaconfig.pyspecifyweb/specify/migration_utils/utils.pyspecifyweb/specify/migrations/0002_geo.pyspecifyweb/specify/migrations/0003_cotype_picklist.pyspecifyweb/specify/migrations/0004_stratigraphy_age.pyspecifyweb/specify/migrations/0007_schema_config_update.pyspecifyweb/specify/migrations/0008_ageCitations_fix.pyspecifyweb/specify/migrations/0012_add_cojo_to_schema_config.pyspecifyweb/specify/migrations/0013_collectionobjectgroup_parentcog.pyspecifyweb/specify/migrations/0015_add_version_to_ages.pyspecifyweb/specify/migrations/0017_schemaconfig_fixes.pyspecifyweb/specify/migrations/0018_cot_catnum_schema.pyspecifyweb/specify/migrations/0020_add_tectonicunit_to_pc_in_schema_config.pyspecifyweb/specify/migrations/0021_update_hidden_geo_tables.pyspecifyweb/specify/migrations/0023_update_schema_config_text.pyspecifyweb/specify/migrations/0024_add_uniqueIdentifier_storage.pyspecifyweb/specify/migrations/0027_CO_children.pyspecifyweb/specify/migrations/0029_remove_collectionobject_parentco.pyspecifyweb/specify/migrations/0032_add_quantities_gift.pyspecifyweb/specify/migrations/0033_update_paleo_desc.pyspecifyweb/specify/migrations/0034_accession_date_fields.pyspecifyweb/specify/migrations/0035_version_required.pyspecifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migrations/0040_components.pyspecifyweb/specify/migrations/0042_discipline_type_picklist.py
💤 Files with no reviewable changes (1)
- specifyweb/specify/migration_utils/sp7_schemaconfig.py
✅ Files skipped from review due to trivial changes (4)
- specifyweb/specify/management/commands/sync_schema_config_fields.py
- specifyweb/backend/setup_tool/schema_defaults.py
- specifyweb/specify/migration_utils/migration_helpers/helper_0039_agent_fields_for_loan_and_gift.py
- specifyweb/backend/workbench/migrations/0007_spdatasetattachment.py
🚧 Files skipped from review as they are similar to previous changes (5)
- specifyweb/specify/migrations/0024_add_uniqueIdentifier_storage.py
- specifyweb/specify/migrations/0027_CO_children.py
- specifyweb/specify/migrations/0017_schemaconfig_fixes.py
- specifyweb/specify/migrations/0034_accession_date_fields.py
- specifyweb/specify/migrations/0015_add_version_to_ages.py
Comment on lines
+59
to
+63
| def revert_discipline_type_picklist(apps): | ||
| Picklist = apps.get_model('specify', 'Picklist') | ||
|
|
||
| Picklist.objects.filter(name=DISCIPLINE_TYPE_PICKLIST_NAME).delete() | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope rollback deletion to migration-owned picklists.
Line 62 currently deletes every Picklist named DisciplineType. That can remove user-created rows with the same name. Restrict the filter to the attributes this migration creates.
Proposed fix
def revert_discipline_type_picklist(apps):
Picklist = apps.get_model('specify', 'Picklist')
- Picklist.objects.filter(name=DISCIPLINE_TYPE_PICKLIST_NAME).delete()
+ Picklist.objects.filter(
+ name=DISCIPLINE_TYPE_PICKLIST_NAME,
+ type=0,
+ issystem=True,
+ readonly=True,
+ sizelimit=-1,
+ sorttype=1,
+ ).delete()🤖 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/migration_helpers/helper_0042_discipline_type_picklist.py`
around lines 59 - 63, The revert_discipline_type_picklist function currently
deletes all Picklist objects with the name matching
DISCIPLINE_TYPE_PICKLIST_NAME, which can inadvertently remove user-created
picklists with the same name. Modify the filter criteria in the
Picklist.objects.filter() call to include additional attributes that uniquely
identify picklists created by this migration (such as system ownership or
application-specific markers) to ensure only migration-owned picklists are
deleted during rollback.
Comment on lines
+67
to
+86
| Splocalecontaineritem.objects.filter( | ||
| container__name="discipline", | ||
| container__schematype=0, | ||
| name="type", | ||
| ).update( | ||
| picklistname=DISCIPLINE_TYPE_PICKLIST_NAME, | ||
| isrequired=True | ||
| ) | ||
|
|
||
| def revert_discipline_type_splocalecontaineritem(apps): | ||
| Splocalecontaineritem = apps.get_model("specify", "Splocalecontaineritem") | ||
|
|
||
| Splocalecontaineritem.objects.filter( | ||
| container__name="discipline", | ||
| picklistname=DISCIPLINE_TYPE_PICKLIST_NAME, | ||
| container__schematype=0, | ||
| name="type", | ||
| ).update( | ||
| picklistname=None | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect current helper behavior
cat -n specifyweb/specify/migration_utils/migration_helpers/helper_0042_discipline_type_picklist.py | sed -n '60,90p'
# 2) Inspect migration wiring for 0042
cat -n specifyweb/specify/migrations/0042_discipline_type_picklist.py | sed -n '1,80p'
# 3) Find prior writes to discipline/type schema-config requiredness to infer intended baseline
rg -n --type=py "container__name=['\"]discipline['\"].*name=['\"]type['\"]|isrequired" \
specifyweb/specify/migrations \
specifyweb/specify/migration_utils/migration_helpersRepository: specify/specify7
Length of output: 4844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find when discipline/type schema item was created and its initial isrequired state
# Search migration 0041 (the dependency) and earlier helpers for discipline/type
rg -n "discipline.*type|container.*name.*discipline" \
specifyweb/specify/migrations/0041_add_missing_schema_after_reorganization.py \
specifyweb/specify/migration_utils/migration_helpers/helper_0041*.py 2>/dev/null || echo "0041 files not found or no matches"
# Check initial schema creation (migration 0001) for discipline table/fields
rg -n -A 5 -B 5 "name='discipline'" specifyweb/specify/migrations/0001_initial.py | head -100
# Look for any earlier helpers that might have set discipline/type configuration
rg -n "discipline.*type.*isrequired|Splocalecontaineritem.*discipline.*type" \
specifyweb/specify/migration_utils/migration_helpers/ | head -30Repository: specify/specify7
Length of output: 5494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Splocalecontaineritem creation/update for discipline container and type field
rg -n "Splocalecontaineritem" specifyweb/specify/migration_utils/migration_helpers/ | grep -i "discipline\|type" | head -50
# Look for container creation for discipline
rg -n "container.*discipline\|name.*discipline" specifyweb/specify/migration_utils/migration_helpers/ | head -50
# Inspect helper_0001 or the earliest helpers that set up schema config
ls -la specifyweb/specify/migration_utils/migration_helpers/ | grep helper_000 | head -10Repository: specify/specify7
Length of output: 1470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all helpers to understand the sequence
ls -1 specifyweb/specify/migration_utils/migration_helpers/helper_*.py | sort
# Inspect the most relevant helpers (0017, 0018, 0035, 0040, 0041) for discipline/type config
for helper in helper_0017 helper_0018 helper_0035 helper_0040 helper_0041; do
file="specifyweb/specify/migration_utils/migration_helpers/${helper}_*.py"
echo "=== Searching in $helper ==="
rg -n "discipline.*type|container.*name.*discipline" $file 2>/dev/null || echo " (no matches)"
done
# Look for explicit creation of discipline container and its items
rg -n -B 3 -A 3 "container.*name.*=.*['\"]discipline['\"]" \
specifyweb/specify/migration_utils/migration_helpers/ | head -100Repository: specify/specify7
Length of output: 4319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if discipline/type Splocalecontaineritem was ever created before migration 0042
# by looking at all migrations and helpers for any mention of discipline type creation
rg -n "discipline.*type\|type.*discipline" specifyweb/specify/migrations/ | grep -v "0042" | head -20
# Look for schema initialization that might create discipline container items
rg -n -B 5 -A 5 "Splocalecontaineritem.*create\|\.create.*container" \
specifyweb/specify/migration_utils/migration_helpers/helper_0002*.py \
2>/dev/null | head -80
# Check if there's any fixture or initial data that sets up discipline/type
find specifyweb -name "*.json" -o -name "fixtures*" 2>/dev/null | head -10Repository: specify/specify7
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefcatollow pipefail
# Check if the discipline container itself is created in earlier helpers
rg -n "Splocalecontainer.*create\|container.*=.*create" \
specifyweb/specify/migration_utils/migration_helpers/ --type=py | grep -v "0042" | head -20
# Look for any tests related to discipline schema config
find specifyweb -path "*/test*" -name "*.py" -exec grep -l "discipline.*type\|Splocalecontaineritem" {} \; | head -10
# Check the complete helper_0002 which seems to be a schema config update
wc -l specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.py
# Look at the beginning of helper_0002 to see if it deals with discipline
head -100 specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.pyRepository: specify/specify7
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the discipline container itself is created in earlier helpers
rg -n "Splocalecontainer.*create\|container.*=.*create" \
specifyweb/specify/migration_utils/migration_helpers/ --type=py | grep -v "0042" | head -20
# Check the complete helper_0002 which seems to be a schema config update
wc -l specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.py
# Look at the beginning of helper_0002 to see if it deals with discipline
head -150 specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.pyRepository: specify/specify7
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Simple direct check - let's look at helper_0002
cat specifyweb/specify/migration_utils/migration_helpers/helper_0002_schema_config_update.py | head -200Repository: specify/specify7
Length of output: 5368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where discipline/type Splocalecontaineritem gets its initial value
# by looking at raw SQL or fixture data
find specifyweb -type f \( -name "*.sql" -o -name "*.json" -o -name "fixtures*" \) 2>/dev/null | head -20
# Check if there's a schema_writer or schema initialization function
rg -n "discipline" specifyweb/specify/migration_utils/schema_writer.py | head -20
# Look for when discipline schema items (splocalecontaineritems) were created
rg -n "schematype.*0\|Splocalecontainer.*discipline" specifyweb/specify/migration_utils/ --type=py | head -30
# Check if the field exists at all and what its initial isrequired value is
# by looking at helper_0001 for any schema init
grep -n "isrequired" specifyweb/specify/migrations/0001_initial.py | head -20Repository: specify/specify7
Length of output: 1992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the database schema definition (model definition in models.py)
# to see if Splocalecontaineritem.isrequired has a default value
rg -n -B 5 -A 5 "class Splocalecontaineritem" specifyweb/specify/models.py | head -50
# Look at the initial migration definition of Splocalecontaineritem
rg -n -B 10 -A 10 "name='splocalecontaineritem'" specifyweb/specify/migrations/0001_initial.pyRepository: specify/specify7
Length of output: 3037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full model definition for Splocalecontaineritem
rg -n -A 50 "class Splocalecontaineritem" specifyweb/specify/models.py | head -100
# Also look for the migration definition of isrequired field
rg -n "isrequired.*BooleanField\|isrequired.*null.*True" specifyweb/specify/migrations/0001_initial.py | grep -A 2 -B 2 "splocalecontaineritem"Repository: specify/specify7
Length of output: 4026
Fix rollback asymmetry for isrequired on discipline.type.
Forward migration sets isrequired=True (line 73), but rollback does not reset isrequired (line 85 only clears picklistname). Since the field defaults to null=True, the field was initially None; rollback must reset it to None to restore the previous state.
Update the rollback to:
.update(
picklistname=None,
isrequired=None
)This matches the pattern used in other schema migrations (e.g., helper_0007).
🤖 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/migration_helpers/helper_0042_discipline_type_picklist.py`
around lines 67 - 86, The rollback function
`revert_discipline_type_splocalecontaineritem` is incomplete because it only
resets the `picklistname` field to None but fails to reset the `isrequired`
field that was set to True in the forward migration. Update the `.update()` call
in `revert_discipline_type_splocalecontaineritem` to also include
`isrequired=None` alongside the existing `picklistname=None` to fully restore
the original state before the migration.
Comment on lines
+40
to
+94
| def get_or_create_splocalecontainer(Splocalecontainer, Splocaleitemstr, table_label: str | None = None, table_description: str | None = None, **container_attrs: Unpack[ContainerAttrs]): | ||
| if "name" not in container_attrs.keys(): | ||
| raise ValueError("Trying to create a SpLocaleContainer without a name!") | ||
|
|
||
| if "discipline_id" not in container_attrs.keys(): | ||
| raise ValueError("Trying to create a SpLocaleContianer without a Discipline") | ||
|
|
||
| resolved_container_attrs: ContainerAttrs = { | ||
| "ishidden": False, | ||
| "issystem": False, | ||
| "schematype": 0, | ||
| "version": 0, | ||
| # The order of this unpacking matters | ||
| # If the defaults were specified in container_attrs, make sure to prioritize them over the defaults | ||
| **container_attrs | ||
| } | ||
|
|
||
| resolved_container_attrs['name'] = resolved_container_attrs['name'].lower() | ||
|
|
||
| sp_local_container = ( | ||
| Splocalecontainer.objects.filter( | ||
| name=resolved_container_attrs['name'], | ||
| discipline_id=resolved_container_attrs['discipline_id'], | ||
| schematype=resolved_container_attrs['schematype'] | ||
| ) | ||
| .order_by('id') | ||
| .first() | ||
| ) | ||
|
|
||
| if sp_local_container is not None: | ||
| # BUG?: Not sure if we want to handle also checking for and (if needed) | ||
| # creating the container strings here | ||
| return sp_local_container | ||
|
|
||
| sp_local_container = Splocalecontainer.objects.create(**resolved_container_attrs) | ||
|
|
||
| common_string_attrs = { | ||
| "language": "en", | ||
| "version": 0 | ||
| } | ||
| container_string_rows = [ | ||
| { | ||
| **common_string_attrs, | ||
| "containername": sp_local_container, | ||
| "text": table_label or camel_to_spaced_title_case(uncapitilize(resolved_container_attrs["name"])) | ||
| }, | ||
| { | ||
| **common_string_attrs, | ||
| "containerdesc": sp_local_container, | ||
| "text": table_description or camel_to_spaced_title_case(uncapitilize(resolved_container_attrs["name"])) | ||
| } | ||
| ] | ||
|
|
||
| bulk_create_splocaleitemstr_idempotent(Splocaleitemstr, container_string_rows) | ||
| return sp_local_container |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect camel_to_spaced_title_case to confirm it depends on camelCase boundaries
sed -n '113,145p' specifyweb/specify/migration_utils/schema_reader.pyRepository: specify/specify7
Length of output: 1234
🏁 Script executed:
#!/bin/bash
# Get the full function to verify lines 40-94
sed -n '40,94p' specifyweb/specify/migration_utils/schema_writer.pyRepository: specify/specify7
Length of output: 2288
🏁 Script executed:
#!/bin/bash
# Check the field updater calls mentioned at lines 243-248 and 369-374
sed -n '243,248p' specifyweb/specify/migration_utils/schema_writer.py
echo "---"
sed -n '369,374p' specifyweb/specify/migration_utils/schema_writer.pyRepository: specify/specify7
Length of output: 536
🏁 Script executed:
#!/bin/bash
# Search for all calls to get_or_create_splocalecontainer to see how it's used
rg 'get_or_create_splocalecontainer' specifyweb/specify/migration_utils/schema_writer.py -A 5 -B 2Repository: specify/specify7
Length of output: 1170
🏁 Script executed:
#!/bin/bash
# Search entire file for all calls to get_or_create_splocalecontainer
rg 'get_or_create_splocalecontainer' specifyweb/specify/migration_utils/schema_writer.py --countRepository: specify/specify7
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Get complete context around all calls to ensure we've found them all
grep -n 'get_or_create_splocalecontainer' specifyweb/specify/migration_utils/schema_writer.pyRepository: specify/specify7
Length of output: 467
🏁 Script executed:
#!/bin/bash
# Check the function signature and see what table_label/table_description are used for
# and verify the logic of uncapitilize
sed -n '1,30p' specifyweb/specify/migration_utils/schema_reader.py | head -20
sed -n '135,145p' specifyweb/specify/migration_utils/schema_reader.pyRepository: specify/specify7
Length of output: 814
Auto-generated container labels lose camelCase word boundaries.
resolved_container_attrs['name'] is lowercased at line 57, and that lowercased value is then fed to camel_to_spaced_title_case(uncapitilize(...)) at lines 84 and 89. Since the regex in camel_to_spaced_title_case detects only uppercase letters (r"(?<!^)(?=[A-Z])"), an all-lowercase input cannot be split into words (e.g., collectionobjecttype → Collectionobjecttype instead of Collection Object Type).
The field-level updaters at lines 243 and 369 call this helper with name=table.name.lower() and no table_label, so newly created containers receive mis-formatted labels. Preserve the original casing for label generation by storing it before lowercasing, and let the function apply lowercasing only for database storage and lookups.
Proposed fix
- resolved_container_attrs['name'] = resolved_container_attrs['name'].lower()
+ original_name = resolved_container_attrs['name']
+ resolved_container_attrs['name'] = original_name.lower()
@@
- "text": table_label or camel_to_spaced_title_case(uncapitilize(resolved_container_attrs["name"]))
+ "text": table_label or camel_to_spaced_title_case(uncapitilize(original_name))
@@
- "text": table_description or camel_to_spaced_title_case(uncapitilize(resolved_container_attrs["name"]))
+ "text": table_description or camel_to_spaced_title_case(uncapitilize(original_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/schema_writer.py` around lines 40 - 94,
The function get_or_create_splocalecontainer lowercases
resolved_container_attrs['name'] on line 57, and then passes this lowercased
value to camel_to_spaced_title_case(uncapitilize()) at lines 84 and 89 for label
generation. Since the camelCase detection regex only recognizes uppercase
letters, lowercased input like "collectionobjecttype" cannot be properly split
into words. Preserve the original casing of the name before the lowercasing
operation by storing it in a separate variable, then use that original-cased
value when calling camel_to_spaced_title_case() for the table_label and
table_description defaults, while keeping the lowercased version only for the
database operations (filter and create).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8058
Warning
This PR affects database migrations. See migration testing instructions.
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
Release Notes
Refactor
Chores