Issue 8124 8126 fix 7660 7682 2 6266 after review#8154
Open
melton-jason wants to merge 49 commits into
Open
Conversation
(cherry picked from commit ab5b3e3)
(cherry picked from commit 60a6797)
(cherry picked from commit f58c785)
(cherry picked from commit 2c3b7d5)
(cherry picked from commit c452f7d)
Fix: compare full rule definitions before deleting anything in uniqueness (cherry picked from commit c61a530)
(cherry picked from commit ef0eed0)
(cherry picked from commit b1f0346)
(cherry picked from commit 00fe897)
(cherry picked from commit c633180)
(cherry picked from commit d4c0231)
(cherry picked from commit 9953d6a)
(cherry picked from commit 8c9cc13)
(cherry picked from commit 6381382)
(cherry picked from commit bab5083)
(cherry picked from commit 28b9f5b)
(cherry picked from commit 11b6608)
(cherry picked from commit af4c963)
(cherry picked from commit 57bca64)
(cherry picked from commit 00a21ef)
(cherry picked from commit 3da5761)
(cherry picked from commit e07aaeb)
(cherry picked from commit 36b307c)
(cherry picked from commit d97acbd)
(cherry picked from commit d3c6c98)
(cherry picked from commit b183031)
(cherry picked from commit 2e9e4ad)
(cherry picked from commit 2254fc4)
(cherry picked from commit 39b001a)
(cherry picked from commit d8bd82d)
Member
|
PR review by Summary by Category🛡️ Defensive / Safety Improvements
🧩 Migration Correctness / Bug Fixes
🔗 Tectonic Unit Linking / Determinism
🔐 Permissions / User Initialization
🗄️ Database Alias / Multi-DB Support
🔍 Items That Deserve Extra Attention
✅ Overall AssessmentThis PR is a well-scoped collection of fixes addressing review feedback from #8142. The changes fall primarily into:
No new features are introduced. All changes are in migration utilities, schema config helpers, and permission initialization — code that runs during migrations and setup, not hot-path request handling. The changes are well-contained, each targeting a specific problem from the linked issues. I'd recommend testing the tectonic unit rollback ( |
Member
grantfitzsimmons
left a comment
grantfitzsimmons
left a comment
There was a problem hiding this comment.
To make my contribution more worthwhile, I am looking closely at the things deepseek suggested need extra attention. It seems like most of the changes seem obvious and should have been done in the first place!
fix_global_default_rulesrefactor — The shift from ORM subqueries to in-memory set comparison is cleaner, but verify this against a real database with multiple disciplines and overlapping global rules. A migration integration test would provide confidence.
To me it seems the new code is actually a total improvement (the old code over-deleted partial Exists matches, but the new code only deletes on exact full-signature match). Happy with how this looks.
is_sp6_user_permissions_migratedremoval — This function was completely removed in favor of a directUserPolicyfilter. Double-check that the new inline check covers all cases the old function did (especially legacy SP6 migrated users who might have collection-scoped admin policies rather than global ones).
This is still here despite not being used by any runtime code, and the new solution is more straightforward. Should be removed IMO.
specify7/specifyweb/backend/permissions/initialize.py
Lines 29 to 33 in 72414b6
__iexactvs__exact+.lower()— On MySQL withutf8mb4_general_ci(case-insensitive collation, which is standard for Specify),__iexactand.lower()behave identically. However, if any database uses a case-sensitive collation,__iexactis actually more correct than the old.lower()approach. This change is an improvement.
Happy with this!
revert_create_root_tectonic_nodeordering — The fix reverses the delete order (units before tree-def items) which is correct for FK constraints. However, verify the parent tree-def items also get cleaned up — the function deletesTectonicUnitTreeDefItem.objects.filter(treedef=tectonic_tree_def).delete()which would cascade, but explicit ordering is still preferred.
Cascade is desirable here IMO.
Should we do some integration testing to make sure all is good in a multi-db environment (maybe using the scheme defined in docker-compositions/docker/specifycloud)?
Contributor
|
@grantfitzsimmons, yes integration testing is a must I think. |
This was
linked to
issues
Jun 2, 2026
7 tasks
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 (1)
specifyweb/backend/permissions/initialize.py (1)
89-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
UserPolicyassignment.
UserPolicyis assigned on line 89 and again on line 91. The second assignment is redundant.🧹 Suggested fix
def assign_users_to_roles(apps=apps) -> None: Role = apps.get_model('permissions', 'Role') UserPolicy = apps.get_model('permissions', 'UserPolicy') Agent = apps.get_model('specify', 'Agent') - UserPolicy = apps.get_model('permissions', 'UserPolicy') UserRole = apps.get_model('permissions', 'UserRole')🤖 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/backend/permissions/initialize.py` around lines 89 - 92, Remove the redundant reassignment of UserPolicy: the code calls apps.get_model('permissions', 'UserPolicy') twice (once assigned to UserPolicy on the same block alongside Agent and UserRole); delete the duplicate line so UserPolicy is only assigned once and keep the existing Agent and UserRole assignments intact to avoid shadowing or confusion.
🤖 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/tectonic_ranks.py`:
- Around line 97-106: Tighten root-node detection by requiring both parent=None
and rankid=0 when identifying roots: update the Exists filter on TectonicUnit
(used in the TectonicUnitTreeDef.objects.annotate root_node_exists) to include
rankid=0 alongside parent=None and definition=OuterRef("pk"), and make the same
change in the revert/delete path that currently selects root candidates by name
(the logic around lines referenced for deletion) so both forward and revert
paths use parent=None AND rankid=0 when locating root TectonicUnit rows.
In `@specifyweb/specify/migrations/0002_geo.py`:
- Line 75: The migration call to create_default_collection_types currently omits
the migration DB alias and thus writes to the default DB; change the call in the
RunPython migration in 0002_geo.py to pass the migration alias (use
schema_editor.connection.alias, e.g. create_default_collection_types(apps,
schema_editor.connection.alias)), then update the
create_default_collection_types function signature to accept a db_alias
parameter and ensure all DB operations inside (queries/creates/updates)
explicitly use .using(db_alias) or the equivalent to avoid cross-database
writes.
---
Outside diff comments:
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 89-92: Remove the redundant reassignment of UserPolicy: the code
calls apps.get_model('permissions', 'UserPolicy') twice (once assigned to
UserPolicy on the same block alongside Agent and UserRole); delete the duplicate
line so UserPolicy is only assigned once and keep the existing Agent and
UserRole assignments intact to avoid shadowing or confusion.
🪄 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: 1694a881-7df6-4fad-8cb1-edb41d3c353b
📒 Files selected for processing (16)
specifyweb/backend/businessrules/migration_utils.pyspecifyweb/backend/businessrules/migrations/0004_catnum_uniquerule.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/patches/migration_utils.pyspecifyweb/backend/permissions/initialize.pyspecifyweb/backend/stored_queries/execution.pyspecifyweb/backend/stored_queries/utils.pyspecifyweb/specify/api/utils.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/0002_geo.pyspecifyweb/specify/migrations/0008_ageCitations_fix.pyspecifyweb/specify/migrations/0009_tectonic_ranks.py
Comment on lines
+97
to
+106
| trees_missing_root_node = TectonicUnitTreeDef.objects.annotate( | ||
| root_node_exists=Exists( | ||
| TectonicUnit.objects.filter( | ||
| parent=None, | ||
| definition=tectonic_tree_def, | ||
| definitionitem=tectonic_tree_def_item | ||
| definition=OuterRef("pk") | ||
| ) | ||
| ) | ||
| ).filter( | ||
| root_node_exists=False | ||
| ) |
There was a problem hiding this comment.
Tighten root-node identification to include rankid=0 in both forward and revert paths.
Current root detection/deletion only keys on parentless rows (and name in revert), which can mis-handle malformed trees that have parentless non-root entries.
Proposed fix
- trees_missing_root_node = TectonicUnitTreeDef.objects.annotate(
+ trees_missing_root_node = TectonicUnitTreeDef.objects.annotate(
root_node_exists=Exists(
TectonicUnit.objects.filter(
parent=None,
+ rankid=0,
definition=OuterRef("pk")
)
)
).filter(
root_node_exists=False
@@
TectonicUnit.objects.filter(
name="Root",
definition=tectonic_tree_def,
parent__isnull=True,
+ rankid=0,
).delete()Based on learnings: root lookups in this module should combine parent=None and rankid=0 to handle malformed-tree edge cases safely.
Also applies to: 143-147
🤖 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/tectonic_ranks.py` around lines 97 - 106,
Tighten root-node detection by requiring both parent=None and rankid=0 when
identifying roots: update the Exists filter on TectonicUnit (used in the
TectonicUnitTreeDef.objects.annotate root_node_exists) to include rankid=0
alongside parent=None and definition=OuterRef("pk"), and make the same change in
the revert/delete path that currently selects root candidates by name (the logic
around lines referenced for deletion) so both forward and revert paths use
parent=None AND rankid=0 when locating root TectonicUnit rows.
| def consolidated_python_django_migration_operations(apps, schema_editor): | ||
| db_alias = schema_editor.connection.alias or 'migrator' | ||
| create_default_collection_types(apps, using=db_alias) | ||
| create_default_collection_types(apps) |
There was a problem hiding this comment.
Pass the migration DB alias into create_default_collection_types to avoid cross-database writes.
This call now executes on the default connection, while the migration is alias-aware elsewhere; on non-default aliases, this can populate/update records in the wrong database.
Proposed fix (cross-file)
--- a/specifyweb/specify/migrations/0002_geo.py
+++ b/specifyweb/specify/migrations/0002_geo.py
@@
- create_default_collection_types(apps)
+ create_default_collection_types(apps, using=db_alias)--- a/specifyweb/specify/migration_utils/default_cots.py
+++ b/specifyweb/specify/migration_utils/default_cots.py
@@
-def create_default_collection_types(apps):
+def create_default_collection_types(apps, using='default'):
@@
- for collection in Collection.objects.filter(collectionobjecttype__isnull=True):
+ for collection in Collection.objects.using(using).filter(collectionobjecttype__isnull=True):
@@
- cot, created = Collectionobjecttype.objects.get_or_create(
+ cot, created = Collectionobjecttype.objects.using(using).get_or_create(
@@
- Collectionobject.objects.filter(
+ Collectionobject.objects.using(using).filter(
collection=collection).update(collectionobjecttype=cot)
collection.collectionobjecttype = cot
- collection.save()
+ collection.save(using=using)🤖 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/0002_geo.py` at line 75, The migration call to
create_default_collection_types currently omits the migration DB alias and thus
writes to the default DB; change the call in the RunPython migration in
0002_geo.py to pass the migration alias (use schema_editor.connection.alias,
e.g. create_default_collection_types(apps, schema_editor.connection.alias)),
then update the create_default_collection_types function signature to accept a
db_alias parameter and ensure all DB operations inside (queries/creates/updates)
explicitly use .using(db_alias) or the equivalent to avoid cross-database
writes.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
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
3 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.
Taken from #8142 (comment) by @CarolineDenis:
Fixes #8124
Fixes #8126
Fixes #8101
Fixes #8065
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
Bug Fixes
Performance Improvements
Chores