Merge V7.12.0.5 into main#8043
Draft
acwhite211 wants to merge 39 commits intomainfrom
Draft
Conversation
|
Important Review skippedDraft detected. 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 refactors migration utilities to improve determinism and maintainability, updates FormParse column definition selection to include OS-specific fallback behavior, normalizes delete-blocker serialization for many-to-many relationships, and makes permissions policy JSON serialization deterministic. Changes
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Also, define a helper on Table to fetch/filter fields
…alecontaineritem Duplicate items and strings would be created everytime the function is run. The cause of this is the same as #7988.
There's no easy way to differentiate schema config items for which these should be applied. I suppose the BEST method might be to be exact: only updating Containers/Items/Strings that exactly match the previous values expected with the migrations they aim to fix/resolve
Fix form column definition precedence
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
specifyweb/specify/tests/test_delete_blockers.py (1)
165-329:⚠️ Potential issue | 🟠 MajorPlease remove the duplicated module block appended at the end.
The entire test module is repeated from Line 165 onward, including imports and class redefinitions. This shadows earlier class bindings and makes the test file brittle/confusing. Keep a single copy of the module content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/specify/tests/test_delete_blockers.py` around lines 165 - 329, The file contains a duplicated module block (duplicate imports and repeated definitions of TestDeleteBlockers, TestDeleteBlockersCascade, helper _url and tests) appended after the original content; remove the entire second copy so only one definition of helpers (_url, _get_blockers, _assertSame, _assertContains) and the test classes TestDeleteBlockers and TestDeleteBlockersCascade remain, ensuring references to _collect_delete_blockers, GeographyTree, and model usages (e.g. Spexportschema, Spexportschemamapping) are preserved from the first occurrence.specifyweb/specify/migration_utils/default_cots.py (1)
71-86:⚠️ Potential issue | 🟠 MajorBackfill the default COG items on reruns.
Picklistitem.get_or_create(...)was already idempotent, so moving the item loop underpicklist_createdmeansfix_cotscan no longer heal an existingSystemCOGTypespicklist that is missing one or more default rows. This turns a safe rerun path into a partial no-op.Suggested fix
- if picklist_created: - for cog_type in DEFAULT_COG_TYPES: - Picklistitem.objects.using(using).get_or_create( - title=cog_type, - value=cog_type, - picklist=cog_type_picklist - ) + for cog_type in DEFAULT_COG_TYPES: + Picklistitem.objects.using(using).get_or_create( + title=cog_type, + value=cog_type, + picklist=cog_type_picklist + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/specify/migration_utils/default_cots.py` around lines 71 - 86, The code currently only seeds DEFAULT_COG_TYPES into the Picklist when picklist_created is True, which prevents healing missing Picklistitem rows on reruns; update the logic in the function (e.g., fix_cots in default_cots.py) so that after obtaining cog_type_picklist via Picklist.objects.using(using).get_or_create(...) you always iterate DEFAULT_COG_TYPES and call Picklistitem.objects.using(using).get_or_create(...) for each cog_type (using cog_type_picklist as the picklist argument) rather than restricting that loop to the picklist_created branch, ensuring idempotent backfill of missing items.
🧹 Nitpick comments (2)
specifyweb/backend/delete_blockers/views.py (1)
45-49: Optional cleanup: remove the temporary nested list +flatten()roundtrip.You can return the serialized list directly for simpler and clearer code.
Proposed simplification
- return flatten([ - [ - _serialize_delete_blocker(field, sub_objs) - ] for field, sub_objs in collector.delete_blockers - ]) + return [ + _serialize_delete_blocker(field, sub_objs) + for field, sub_objs in collector.delete_blockers + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/backend/delete_blockers/views.py` around lines 45 - 49, The current return builds a temporary nested list and then flattens it; simplify by returning a flat list comprehension directly instead of wrapping each item in its own list and calling flatten — replace the expression that returns flatten([... for field, sub_objs in collector.delete_blockers]) with a direct comprehension that calls _serialize_delete_blocker(field, sub_objs) for each (referencing collector.delete_blockers and _serialize_delete_blocker) so the function returns the serialized list without the unnecessary nested-list + flatten roundtrip.specifyweb/specify/models_utils/load_datamodel.py (1)
152-174: Consider making this iterator public.
update_schema_config.pynow depends ontable._all_fields(...)directly, so this is effectively part of the externalTableAPI now. Renaming it to a public method likeiter_all_fields(...)would make that contract explicit and reduce future refactor breakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/specify/models_utils/load_datamodel.py` around lines 152 - 174, The iterator method _all_fields is being used externally so make it an explicit public API by adding a new method iter_all_fields(self, exclude_fields=False, exclude_relationships=False, exclude_id_field=False, exclude_virtual_fields=True) that implements the same yield logic and update the all_fields property to call iter_all_fields(); then keep _all_fields as a thin compatibility wrapper that calls iter_all_fields (or vice versa) and update any internal call sites to use iter_all_fields to avoid future breakage while preserving backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specifyweb/specify/migration_utils/tectonic_ranks.py`:
- Around line 115-126: The current lookup for tectonic_tree_def_item only
filters by treedef and parent=None and can pick a wrong parentless row; update
the initial query on TectonicUnitTreeDefItem (the tectonic_tree_def_item
assignment) to include rankid=0 (i.e., filter by treedef=tectonic_tree_def,
parent=None, rankid=0) so it matches the same root invariant used in the
get_or_create path; ensure the get_or_create call for TectonicUnitTreeDefItem
also uses rankid=0 as a lookup key (not just a value) so both retrieval and
creation use the same unique identifying fields.
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 1963-1969: The loop currently calls
update_table_field_schema_config_with_defaults which only applies ishidden when
creating new Splocalecontaineritem rows; change the call (or the function) to
ensure existing Splocalecontaineritem rows are updated to set ishidden=True for
the targeted fields. Concretely, modify
update_table_field_schema_config_with_defaults (or add a small wrapper) so that
for each (table, field_name) it queries Splocalecontaineritem for the matching
table/field/discipline and if a row exists performs a direct update (e.g., set
ishidden=True and save or use queryset.update), otherwise create with the
default; keep references to MIGRATION_0038_FIELDS, Discipline.objects.all(),
update_table_field_schema_config_with_defaults and Splocalecontaineritem when
implementing the change.
---
Outside diff comments:
In `@specifyweb/specify/migration_utils/default_cots.py`:
- Around line 71-86: The code currently only seeds DEFAULT_COG_TYPES into the
Picklist when picklist_created is True, which prevents healing missing
Picklistitem rows on reruns; update the logic in the function (e.g., fix_cots in
default_cots.py) so that after obtaining cog_type_picklist via
Picklist.objects.using(using).get_or_create(...) you always iterate
DEFAULT_COG_TYPES and call Picklistitem.objects.using(using).get_or_create(...)
for each cog_type (using cog_type_picklist as the picklist argument) rather than
restricting that loop to the picklist_created branch, ensuring idempotent
backfill of missing items.
In `@specifyweb/specify/tests/test_delete_blockers.py`:
- Around line 165-329: The file contains a duplicated module block (duplicate
imports and repeated definitions of TestDeleteBlockers,
TestDeleteBlockersCascade, helper _url and tests) appended after the original
content; remove the entire second copy so only one definition of helpers (_url,
_get_blockers, _assertSame, _assertContains) and the test classes
TestDeleteBlockers and TestDeleteBlockersCascade remain, ensuring references to
_collect_delete_blockers, GeographyTree, and model usages (e.g. Spexportschema,
Spexportschemamapping) are preserved from the first occurrence.
---
Nitpick comments:
In `@specifyweb/backend/delete_blockers/views.py`:
- Around line 45-49: The current return builds a temporary nested list and then
flattens it; simplify by returning a flat list comprehension directly instead of
wrapping each item in its own list and calling flatten — replace the expression
that returns flatten([... for field, sub_objs in collector.delete_blockers])
with a direct comprehension that calls _serialize_delete_blocker(field,
sub_objs) for each (referencing collector.delete_blockers and
_serialize_delete_blocker) so the function returns the serialized list without
the unnecessary nested-list + flatten roundtrip.
In `@specifyweb/specify/models_utils/load_datamodel.py`:
- Around line 152-174: The iterator method _all_fields is being used externally
so make it an explicit public API by adding a new method iter_all_fields(self,
exclude_fields=False, exclude_relationships=False, exclude_id_field=False,
exclude_virtual_fields=True) that implements the same yield logic and update the
all_fields property to call iter_all_fields(); then keep _all_fields as a thin
compatibility wrapper that calls iter_all_fields (or vice versa) and update any
internal call sites to use iter_all_fields to avoid future breakage while
preserving backward compatibility.
🪄 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: 0711e959-159f-42a0-9232-05dbdd5b23fa
📒 Files selected for processing (13)
specifyweb/backend/delete_blockers/views.pyspecifyweb/frontend/js_src/lib/components/FormParse/__tests__/index.test.tsspecifyweb/frontend/js_src/lib/components/FormParse/index.tsspecifyweb/frontend/js_src/lib/components/Permissions/index.tsspecifyweb/specify/api/crud.pyspecifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/migration_utils/default_cots.pyspecifyweb/specify/migration_utils/sp7_schemaconfig.pyspecifyweb/specify/migration_utils/tectonic_ranks.pyspecifyweb/specify/migration_utils/update_schema_config.pyspecifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/models_utils/load_datamodel.pyspecifyweb/specify/tests/test_delete_blockers.py
💤 Files with no reviewable changes (2)
- specifyweb/specify/migrations/0039_agent_fields_for_loan_and_gift.py
- specifyweb/specify/migration_utils/sp7_schemaconfig.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Triggered by c10446e on branch refs/heads/v7_12_0_5
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.
Merge the PRs added in the
v7_12_0_5branch into main.Checklist
self-explanatory (or properly documented)
Testing instructions