Issue 8101 fix 6266 after review#8102
Open
CarolineDenis wants to merge 20 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors migration and uniqueness-rule behavior, makes patch migrations DB-alias aware, changes per-user permission migration and schema-config matching to be case-insensitive, inlines CO-children migration logic, and gates SQLAlchemy debug logging on settings.DEBUG. ChangesMigration Logic & Uniqueness Rule Improvements
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
specifyweb/specify/migration_utils/update_schema_config.py (1)
1937-1950: 💤 Low valueAddress the TODO comment and verify intended behavior.
The TODO at line 1940 questions whether unconditionally setting
ishidden = Trueis correct. This forces all fields processed by this migration to be hidden, regardless of their current state or user preferences. If this is intentional, consider removing the TODO; otherwise, consider adding a condition or making the behavior configurable.The change to allow partial updates (saving
desc_strorname_strindependently) is a good improvement over the previous all-or-nothing approach.🤖 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 1937 - 1950, The code currently sets item.ishidden = True for every matched item unconditionally (in update_schema_config.py within the loop processing items and Splocaleitemstr updates), which may be unintended; either remove the TODO and leave this behavior if hiding every processed field is intentional, or change it to a conditional/configurable action (e.g., respect an item's existing ishidden value, check a migration flag/parameter, or only set ishidden when a specific condition is met) and document the behavior; ensure Splocaleitemstr handling (desc_str/text and name_str/text updates) remains unchanged so partial saves still occur independently.
🤖 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/businessrules/uniqueness_rules.py`:
- Around line 459-471: The if-check and deletion are incorrectly placed outside
the loop so only the last UniquenessRule is evaluated; inside the for loop that
iterates
UniquenessRule.objects.filter(discipline_id=discipline_id).prefetch_related("uniquenessrulefield_set")
compute the signature (using rule.modelName, rule.isDatabaseConstraint and
frozenset(rule.uniquenessrulefield_set.values_list("fieldPath", "isScope"))) and
then immediately check if signature in global_rule_signatures, and if so call
rule.uniquenessrulefield_set.all().delete() and rule.delete() so each rule is
evaluated and removed as needed.
In `@specifyweb/specify/migration_utils/default_cots.py`:
- Around line 139-148: The code pairs two unordered querysets
(empty_tectonic_unit_treedefs and empty_disciplines) using zip which yields
nondeterministic matches; make the pairing deterministic by applying a stable
ordering (e.g., .order_by('id') or another immutable unique column) to both
QuerySets before converting to lists and zipping them (update the assignments
for empty_tectonic_unit_treedefs and empty_disciplines and any other similar
pairings around 154-160 so both sides use the same deterministic order).
In `@specifyweb/specify/migrations/0027_CO_children.py`:
- Around line 30-31: The migration defines camel_to_spaced_title_case which
calls re.sub but the regex module isn't imported; add an import for the re
module at the top of the file (e.g., import re) so camel_to_spaced_title_case
can call re.sub without raising NameError.
- Around line 69-77: The get_or_create call for Splocalecontaineritem is using
too many lookup fields causing duplicates; change
Splocalecontaineritem.objects.get_or_create(...) to use only the unique lookup
keys (name=field_name, container=sp_local_container) and move all other
attributes (type=java_type, ishidden=field_name.lower() in HIDDEN_FIELDS,
isrequired=field.required, issystem=table.system, version=0) into the defaults=
{...} argument so existing records are matched by name+container and new records
receive the other values via defaults.
---
Nitpick comments:
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 1937-1950: The code currently sets item.ishidden = True for every
matched item unconditionally (in update_schema_config.py within the loop
processing items and Splocaleitemstr updates), which may be unintended; either
remove the TODO and leave this behavior if hiding every processed field is
intentional, or change it to a conditional/configurable action (e.g., respect an
item's existing ishidden value, check a migration flag/parameter, or only set
ishidden when a specific condition is met) and document the behavior; ensure
Splocaleitemstr handling (desc_str/text and name_str/text updates) remains
unchanged so partial saves still occur independently.
🪄 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: 1c19e341-37fd-48e9-ab86-d9dd68ad79e7
📒 Files selected for processing (12)
specifyweb/backend/businessrules/migration_utils.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/patches/migration_utils.pyspecifyweb/backend/permissions/initialize.pyspecifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/migration_utils/default_cots.pyspecifyweb/specify/migration_utils/tectonic_ranks.pyspecifyweb/specify/migration_utils/update_schema_config.pyspecifyweb/specify/migrations/0002_geo.pyspecifyweb/specify/migrations/0008_ageCitations_fix.pyspecifyweb/specify/migrations/0009_tectonic_ranks.pyspecifyweb/specify/migrations/0027_CO_children.py
💤 Files with no reviewable changes (1)
- specifyweb/specify/migrations/0002_geo.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
6 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
specifyweb/specify/migration_utils/default_cots.py (1)
146-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the
ziplength mismatch explicit withstrict=False.Ruff (B905) is flagging this
zip()for missing an explicitstrict=argument. Since the two lists are intentionally different lengths (the leftover disciplines are handled in the loop at line 154), passingstrict=Falsedocuments that intent and silences the lint.Proposed tweak
- for discipline, tectonic_unit_treedef in zip( - empty_disciplines, empty_tectonic_unit_treedefs - ): + for discipline, tectonic_unit_treedef in zip( + empty_disciplines, empty_tectonic_unit_treedefs, strict=False + ):🤖 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/default_cots.py` around lines 146 - 148, The zip call over empty_disciplines and empty_tectonic_unit_treedefs should be made explicit about allowing length mismatch to satisfy Ruff B905; update the loop that iterates "for discipline, tectonic_unit_treedef in zip(empty_disciplines, empty_tectonic_unit_treedefs)" to pass strict=False so the intentional unequal lengths are documented and the lint is silenced.specifyweb/specify/migrations/0027_CO_children.py (1)
50-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse case-insensitive container matching throughout this migration.
These lookups are all exact lowercase matches. If a
Splocalecontaineralready exists with different casing, the forward path can create a duplicate container while the update/revert passes skip the existing rows. Please switch the lookup side toname__iexactand only normalize the name when creating a new row.Also applies to: 121-123, 149-150
🤖 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/0027_CO_children.py` around lines 50 - 60, Change the exact-lowercase lookups for Splocalecontainer to case-insensitive matching: use name__iexact when calling get_or_create or filter (e.g., where Splocalecontainer.objects.get_or_create(...) and Splocalecontainer.objects.filter(...).first() are used) so existing containers with different casing are matched; only normalize (lowercase) the name value when actually creating a new Splocalecontainer record (i.e., pass the normalized name into the create path), and apply the same change to the other similar lookup sites in this migration (the other Splocalecontainer get_or_create/filter usages mentioned).
🤖 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/migrations/0027_CO_children.py`:
- Around line 70-80: The reverse migration currently unconditionally deletes
Splocalecontaineritem rows created/reused by
update_table_field_schema_config_with_defaults; change the reverse logic so it
does not blindly delete matching items — instead, detect which rows were
actually created by this forward pass (preferably by checking the
get_or_create() returned-created case or by comparing the row's current
attributes to the exact defaults we set) and only delete those; for rows that
were reused (attributes differ or created flag absent), skip deletion and
restore their localized flags/values (ishidden/isrequired/issystem/version) in
place rather than removing the record; apply this change to the deletion block
around the Splocalecontaineritem removal and the later "unhide" loop (the code
that iterates over Splocalecontaineritem rows in
update_table_field_schema_config_with_defaults and the reverse path lines
~144-158).
---
Outside diff comments:
In `@specifyweb/specify/migration_utils/default_cots.py`:
- Around line 146-148: The zip call over empty_disciplines and
empty_tectonic_unit_treedefs should be made explicit about allowing length
mismatch to satisfy Ruff B905; update the loop that iterates "for discipline,
tectonic_unit_treedef in zip(empty_disciplines, empty_tectonic_unit_treedefs)"
to pass strict=False so the intentional unequal lengths are documented and the
lint is silenced.
In `@specifyweb/specify/migrations/0027_CO_children.py`:
- Around line 50-60: Change the exact-lowercase lookups for Splocalecontainer to
case-insensitive matching: use name__iexact when calling get_or_create or filter
(e.g., where Splocalecontainer.objects.get_or_create(...) and
Splocalecontainer.objects.filter(...).first() are used) so existing containers
with different casing are matched; only normalize (lowercase) the name value
when actually creating a new Splocalecontainer record (i.e., pass the normalized
name into the create path), and apply the same change to the other similar
lookup sites in this migration (the other Splocalecontainer get_or_create/filter
usages mentioned).
🪄 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: 76d67dfc-0cad-40cc-9ebb-25c5f9ae0735
📒 Files selected for processing (4)
specifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/stored_queries/execution.pyspecifyweb/specify/migration_utils/default_cots.pyspecifyweb/specify/migrations/0027_CO_children.py
✅ Files skipped from review due to trivial changes (1)
- specifyweb/backend/stored_queries/execution.py
Comment on lines
+70
to
+80
| sp_local_container_item, _ = Splocalecontaineritem.objects.get_or_create( | ||
| name=field_name, | ||
| container=sp_local_container, | ||
| defaults={ | ||
| 'type': java_type, | ||
| 'ishidden': field_name.lower() in HIDDEN_FIELDS, | ||
| 'isrequired': field.required, | ||
| 'issystem': table.system, | ||
| 'version': 0, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Don't delete schema-config rows that the forward path may have reused.
update_table_field_schema_config_with_defaults() can reuse an existing Splocalecontaineritem, but the reverse path unconditionally deletes every matching item before the later "unhide" loop runs. That makes the second pass ineffective for these fields, and on drifted databases a reverse migration can remove preexisting schema-config rows instead of restoring their prior state. Please make the reverse path distinguish created rows from reused rows, or restore the original localized values/flags without deleting reused records.
Also applies to: 144-158
🤖 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/0027_CO_children.py` around lines 70 - 80, The
reverse migration currently unconditionally deletes Splocalecontaineritem rows
created/reused by update_table_field_schema_config_with_defaults; change the
reverse logic so it does not blindly delete matching items — instead, detect
which rows were actually created by this forward pass (preferably by checking
the get_or_create() returned-created case or by comparing the row's current
attributes to the exact defaults we set) and only delete those; for rows that
were reused (attributes differ or created flag absent), skip deletion and
restore their localized flags/values (ishidden/isrequired/issystem/version) in
place rather than removing the record; apply this change to the deletion block
around the Splocalecontaineritem removal and the later "unhide" loop (the code
that iterates over Splocalecontaineritem rows in
update_table_field_schema_config_with_defaults and the reverse path lines
~144-158).
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
1 participant
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 #8101
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
Test again #6308 (comment)
Summary by CodeRabbit
Chores
Bug Fixes