Merge v7.12.0.4 into main

[grantfitzsimmons]

• Revert Back-end changes from Handle Disambiguation Case for Host Taxo… #7981
• Fix QB date_part field formatting #7970
• Fix crash for negated tree rank field query filters #7986
• Fix blank partial date values in QB results #7987
• Deduplicate splocalecontainer records for migrations #7989
• Correct creation of and deduplicate SpAppResourceDirs #7993
• Show all app resources to users lacking preferences permission #7990
• Prevent regranting legacy user roles and access #7995

Summary by CodeRabbit

• New Features

  • QueryComboBox accepts an optional on-save callback to pre-populate newly created related records (used to set Agent division).
  • Stored queries can extract date parts (year, month, day) from related datetime fields.

• Bug Fixes / Improvements

  • Safer handling of tree-rank fields, nulls and aliasing in stored queries.
  • Better deduplication and reconciliation for schema and app-resource directories.
  • More robust background task handling and safer permission initialization to avoid unintended user-role assignment.

• Tests

  • Added tests for date-part extraction and tree-rank query scenarios.

[acwhite211]

Do we want to create another PR merging the issue in v7.12.0.5, or add it to this one?
#7999

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a migrate_sp6_users flag to permission initialization; changes app-resource discipline mapping to use discipline.name and relaxes selection filters; reads Celery task id defensively in background tasks; tightens stored-query temporal and tree-rank join logic; refactors schema-config creation and adds deduplication utilities; updates migration/command flow to use task-style invocation and skip SP6 user migration; extends frontend QueryComboBox with an onSavingNewRecord hook to set Agent.division; and adds related tests.

Changes

Cohort / File(s)	Summary
Permissions init specifyweb/backend/permissions/initialize.py, specifyweb/specify/management/commands/run_key_migration_functions.py	initialize() gains *, migrate_sp6_users: bool = True. User-to-role assignment is skipped when false; migration command now calls initialize(..., migrate_sp6_users=False).
App-resource & setup-tool specifyweb/backend/setup_tool/app_resource_defaults.py, specifyweb/backend/setup_tool/schema_defaults.py	Discipline resource lookup no longer requires specifyuser__isnull=True; SpAppResourceDir.disciplinetype now uses discipline.name (not type). Celery task id is read with getattr and cleanup is conditional on presence.
Stored-queries: formatting & join logic specifyweb/backend/stored_queries/format.py, specifyweb/backend/stored_queries/query_construct.py, specifyweb/backend/stored_queries/queryfieldspec.py	Temporal formatting now requires field be an InstrumentedAttribute before calling _dateformat. Tree-rank/join logic simplified: handle_tree_field uses the provided node, join-cache keys updated, ancestor aliasing uses repeated orm.aliased(node), treedef items with None are filtered out, null checks use is_(None), and null-safe negation chooses mod_orm_field when present.
Stored-queries tests specifyweb/backend/stored_queries/tests/test_execution/test_execute.py, specifyweb/backend/stored_queries/tests/test_execution/test_run_ephemeral_query.py	Adds tests: date-part extraction from a related datetime field; ephemeral query test covering taxonomy tree-rank fields and isnot/null behavior.
Frontend — QueryComboBox & Agent wiring specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx, specifyweb/frontend/js_src/lib/components/Security/MissingAgentsDialog.tsx, specifyweb/frontend/js_src/lib/components/Security/UserCollections.tsx	QueryComboBox gains optional onSavingNewRecord prop and threads it into ResourceView. Callers use it to map saved Agent resources via toTable and set division to the current division URL on create.
Frontend — AppResources & filters/tests specifyweb/frontend/js_src/lib/components/AppResources/__tests__/AppResourcesAside.test.tsx, specifyweb/frontend/js_src/lib/components/AppResources/filtersHelpers.ts	Tests mock hasPermission (default true) and add a denied-permission snapshot. filterAppResources now only includes CollectionPreferences when hasEditPermission is true.
Schema-config migration & dedupe specifyweb/specify/migration_utils/update_schema_config.py	Replaced get-or-create patterns with explicit fetch-or-create for Splocalecontainer and Splocalecontaineritem, skip table.idField when populating defaults, added deduplication functions deduplicate_splocalecontainers and deduplicate_containeritems_and_strings, and wrapped them in deduplicate_schema_config_orm; removed redundant no-op loops.
Migration command & resource-dir dedupe specifyweb/specify/management/commands/run_key_migration_functions.py	Schema-default invocation switched to task-style apply(...). App-resource-dir fix split: ensure missing discipline SpAppResourceDir rows then deduplicate duplicates using an ORM Exists/OuterRef delete preserving earliest row by timestampcreated/id.

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Automatic Tests	⚠️ Warning	PR includes tests for stored_queries and AppResourcesAside modules, but critical changes lack coverage: permissions initialization, setup_tool modules, Security callbacks, management command (+45 lines), and migration utility refactoring (+97 lines) have no corresponding tests.	Add comprehensive tests for untested critical areas: permissions initialization parameter, Security component callbacks, management command changes, and migration utility refactoring functions before merging.
Testing Instructions	⚠️ Warning	PR lacks explicit testing instructions covering affected components across backend permissions, schema defaults, stored queries, and frontend features.	Add testing instructions documenting which components require testing, steps to run tests, manual verification procedures, and verification for migration logic.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'Merge v7.12.0.4 into main' accurately describes the primary purpose of the pull request, which is a version merge from the v7.12.0.4-paris branch into main.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v7.12.0.4-paris

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[grantfitzsimmons]

Do we want to create another PR merging the issue in v7.12.0.5, or add it to this one? #7999

@acwhite211 Please go ahead and create another PR merging that into main.

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

specifyweb/specify/migration_utils/update_schema_config.py (1)

762-807: ⚠️ Potential issue | 🟠 Major

Repoint SpLocaleItemStr rows to the keeper instead of deleting them.

Here the cleanup deletes all strings attached to duplicate items before deleting the items themselves. If the surviving item is the one without strings, the schema field keeps existing but loses its label/description. The safer pattern already exists lower in this file in update_hidden_prop(): move itemname_id / itemdesc_id to the keeper first, then delete the duplicate items.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@specifyweb/specify/migration_utils/update_schema_config.py` around lines 762
- 807, The current deduplicate_containeritems_and_strings deletes all
SpLocaleItemStr rows for duplicate ContainerItems which can drop
labels/descriptions; instead, locate the keeper ContainerItem for each duplicate
group (the row with rn == 1), build a mapping from each duplicate id -> keeper
id, then update ItemStr rows to set itemname_id and itemdesc_id to the keeper id
where they reference any duplicate id, and only after that delete the duplicate
ContainerItem rows; use the existing qs annotated with rn on ContainerItem and
the ItemStr model fields itemname_id/itemdesc_id to perform the updates within
the same transaction.atomic block before calling
ContainerItem.objects.filter(id__in=ids_to_delete).delete().

🤖 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/backend/stored_queries/query_construct.py`:
- Around line 32-35: The method currently has two lint issues: move the inline
raise so the raise AssertionError is on its own line (replace the one-line `if
...: raise ...` pattern used with `query.collection` in query_construct.py so
the conditional and the raise are separate statements), and replace the
SQLAlchemy NULL comparison that uses `== None` (around the expression on/near
line 99) with the SQLAlchemy-aware `.is_(None)` call to correctly produce IS
NULL in SQLAlchemy queries; update the code references where `query.collection`
is checked and where the `== None` SQLAlchemy comparison occurs (use the same
function or method containing those checks) accordingly.
- Around line 41-61: The join-cache is keyed only by (table, 'TreeRanks') but
the cached ancestors are built from the incoming alias parameter node, so
different alias chains collide; change the cache key to include the starting
alias (e.g. use (table, node, 'TreeRanks') or another stable identifier derived
from node) both when checking query.join_cache and when storing it, so lookups
use the same composite key; update references around ancestors, node,
query.join_cache and the cache-setting block (where query =
query._replace(join_cache=...); query.join_cache[...] = ...) to use the new key
so cached ancestor lists are bound to the correct starting alias.

In
`@specifyweb/frontend/js_src/lib/components/AppResources/__tests__/AppResourcesAside.test.tsx`:
- Around line 43-56: Move the jest.mock call for '../../Permissions/helpers' to
the top-level of the test file (before any imports) and create the mock shape
with hasPermission and hasToolPermission as jest.fn(); then in the
beforeAll/afterAll hooks mutate the mocked functions using mockReturnValue()
(e.g., set hasPermission.mockReturnValue(false) in beforeAll and
hasPermission.mockReturnValue(true) in afterAll) so the module is rewired
reliably; refer to the functions hasPermission and hasToolPermission and the
hooks beforeAll/afterAll to locate where to change behavior.

In `@specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx`:
- Around line 658-666: The onSaving callback should accept the
unsetUnloadProtect parameter and call it when we return false in the
dependent-create branch to clear unload protection; update the onSaving
signature to (unsetUnloadProtect?: () => void): false | void and, inside the if
(field.isDependent()) block after setState({ type: 'MainState' }), call
unsetUnloadProtect?.() before returning false — keeping existing calls to
handleSavingNewRecord, resource?.set(field.name, state.resource as never) and
setState intact.

In `@specifyweb/specify/management/commands/run_key_migration_functions.py`:
- Around line 118-125: The Exists predicate (earlier_exists) only checks
timestampcreated__lt so rows with equal timestamps are both kept; update the
filter on SpAppResourceDir to treat ties deterministically by selecting rows
with either timestampcreated < OuterRef('timestampcreated') OR (timestampcreated
== OuterRef('timestampcreated') AND id < OuterRef('id')) so one row is kept as
the tie-breaker; use Q(...) to combine these conditions and keep the rest of the
common_filters intact (reference SpAppResourceDir, earlier_exists,
OuterRef('timestampcreated'), OuterRef('id'), and common_filters).

In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 734-759: Currently duplicate_containers is computed using
timestampcreated__lt and later containers are deleted which loses unique
ContainerItem/ItemStr rows; instead, for each group (discipline_id, name,
schematype=0) determine the keeper as the row with the lowest id (use id as
deterministic tie-breaker), find all other container ids in that group, reparent
unique child rows by updating ContainerItem.container_id and ItemStr.*_container
references to the keeper for rows that don't already exist on the keeper (avoid
creating duplicates), then run deduplicate_containeritems_and_strings() and
finally delete the non-keeper container rows; update the logic around
duplicate_containers, ContainerItem, ItemStr, and use id-based selection rather
than timestampcreated__lt so same-timestamp records are handled
deterministically.
- Around line 314-324: Replace the non-atomic read-then-create for
Splocalecontainer with an atomic DB-backed operation: either add a DB uniqueness
constraint on the identifying columns used in container_attrs and use
Splocalecontainer.objects.get_or_create(defaults={...}, **container_attrs) to
create-or-fetch in one call, or wrap the logic in transaction.atomic() and
acquire a row lock on the owner (via select_for_update() on the
discipline/container model) before re-checking and creating; update the block
that references Splocalecontainer.objects.filter(...).order_by("id").first() and
the create(...) call to use get_or_create or the locked transaction path, and
apply the same change to the analogous code at the later block (lines noted
467-481) so both codepaths consistently avoid race conditions.

---

Outside diff comments:
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 762-807: The current deduplicate_containeritems_and_strings
deletes all SpLocaleItemStr rows for duplicate ContainerItems which can drop
labels/descriptions; instead, locate the keeper ContainerItem for each duplicate
group (the row with rn == 1), build a mapping from each duplicate id -> keeper
id, then update ItemStr rows to set itemname_id and itemdesc_id to the keeper id
where they reference any duplicate id, and only after that delete the duplicate
ContainerItem rows; use the existing qs annotated with rn on ContainerItem and
the ItemStr model fields itemname_id/itemdesc_id to perform the updates within
the same transaction.atomic block before calling
ContainerItem.objects.filter(id__in=ids_to_delete).delete().

🪄 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: 5964b48f-2634-466a-80a3-86d339ab6f69

📥 Commits

Reviewing files that changed from the base of the PR and between b50a592 and 20bb77b.

⛔ Files ignored due to path filters (1)

• specifyweb/frontend/js_src/lib/components/AppResources/__tests__/__snapshots__/AppResourcesAside.test.tsx.snap is excluded by !**/*.snap

📒 Files selected for processing (15)

• specifyweb/backend/permissions/initialize.py
• specifyweb/backend/setup_tool/app_resource_defaults.py
• specifyweb/backend/setup_tool/schema_defaults.py
• specifyweb/backend/stored_queries/format.py
• specifyweb/backend/stored_queries/query_construct.py
• specifyweb/backend/stored_queries/queryfieldspec.py
• specifyweb/backend/stored_queries/tests/test_execution/test_execute.py
• specifyweb/backend/stored_queries/tests/test_execution/test_run_ephemeral_query.py
• specifyweb/frontend/js_src/lib/components/AppResources/__tests__/AppResourcesAside.test.tsx
• specifyweb/frontend/js_src/lib/components/AppResources/filtersHelpers.ts
• specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx
• specifyweb/frontend/js_src/lib/components/Security/MissingAgentsDialog.tsx
• specifyweb/frontend/js_src/lib/components/Security/UserCollections.tsx
• specifyweb/specify/management/commands/run_key_migration_functions.py
• specifyweb/specify/migration_utils/update_schema_config.py

[grantfitzsimmons]

Introduced in this branch: #8040

[coderabbitai]

🧹 Nitpick comments (1)

specifyweb/backend/stored_queries/query_construct.py (1)

37-37: Consider lowering this to debug-level logging.

This path can execute frequently during query construction; keeping it at info may add avoidable log volume.

Suggested tweak

-        logger.info('handling treefield %s rank: %s field: %s', table, tree_rank.name, next_join_path)
+        logger.debug('handling treefield %s rank: %s field: %s', table, tree_rank.name, next_join_path)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@specifyweb/backend/stored_queries/query_construct.py` at line 37, Lower the
verbosity of the log emitted in query_construct: replace the logger.info call
that logs "handling treefield %s rank: %s field: %s" (the logger.info(...) that
uses variables table, tree_rank.name, next_join_path) with logger.debug(...) so
the same formatted message and arguments are preserved but recorded at debug
level instead of info.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@specifyweb/backend/stored_queries/query_construct.py`:
- Line 37: Lower the verbosity of the log emitted in query_construct: replace
the logger.info call that logs "handling treefield %s rank: %s field: %s" (the
logger.info(...) that uses variables table, tree_rank.name, next_join_path) with
logger.debug(...) so the same formatted message and arguments are preserved but
recorded at debug level instead of info.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 90c7fb04-5cf0-444a-b136-6660dcd3ae92

📥 Commits

Reviewing files that changed from the base of the PR and between 20bb77b and 03bf3df.

📒 Files selected for processing (1)

• specifyweb/backend/stored_queries/query_construct.py