Skip to content

Key migration component unit tests#8056

Open
acwhite211 wants to merge 6 commits into
mainfrom
issue-8049
Open

Key migration component unit tests#8056
acwhite211 wants to merge 6 commits into
mainfrom
issue-8049

Conversation

@acwhite211
Copy link
Copy Markdown
Member

@acwhite211 acwhite211 commented May 4, 2026

Fixes #8049

Add unit tests for key migration related functions...

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone
  • Add pr to documentation list
  • Add automated tests

Testing instructions

  • See that all back tests pass in GitHub Actions

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced data migration to properly initialize language settings during schema configuration
    • Updated test expectations to reflect correct system behavior
  • Tests

    • Added comprehensive test coverage for database migration operations, including idempotency validation, deduplication logic, and constraint verification

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

📝 Walkthrough

Walkthrough

This PR adds comprehensive test coverage for the run_key_migration_functions management command and its associated migration helpers. It includes a supporting fix to explicitly set language="en" for new locale item strings, and updates a legacy test expectation to reflect corrected schema relationships.

Changes

Migration command tests and locale fix

Layer / File(s) Summary
Schema locale string creation fix
specifyweb/specify/migration_utils/update_schema_config.py
upsert_single_str now explicitly sets language="en" when creating new Splocaleitemstr records instead of relying on default behavior.
Command dispatch and error handling tests
specifyweb/specify/tests/test_run_key_migration_functions.py (imports, KeyMigrationCommandTests)
Tests validate command-level function dispatch ordering, section filtering, stdout/verbose propagation to section callables, correct apps registry passing to apply_patches, and stderr reporting for unknown function names.
Migration section execution and sequencing tests
specifyweb/specify/tests/test_run_key_migration_functions.py (KeyMigrationSectionTests)
Tests verify each migration section invokes its internal functions in correct order, passes the apps registry, applies schema defaults/overrides, and enforces create/deduplicate sequencing for app resource directory handling.
Migration helper database behavior tests
specifyweb/specify/tests/test_run_key_migration_functions.py (KeyMigrationSelectedHelperDatabaseTests)
Database-backed tests cover idempotent picklist and default item creation, discipline assignment for treedef links, selective uniqueness-rule updates, and locale/schema utilities including bulk upsert/deduplication with string repointing and loan/gift agent locale handling.
App resource directory deduplication tests
specifyweb/specify/tests/test_run_key_migration_functions.py (KeyMigrationAppResourceDirDatabaseTests)
Tests validate deduplication behavior for app resource directories, including deletion of only empty duplicates, tie-breaking by id, and preservation of scoped (personal/usertype/collection-scoped) directories.
Legacy test expectation cleanup
specifyweb/backend/stored_queries/tests/tests_legacy.py
Removes the expected not_found: ["projects"] error from CollectionObject expectations, reflecting corrected schema relationships.

Possibly related issues

  • specify/specify7#8049: This PR implements the requested unit test coverage for run_key_migration_functions, including command dispatch validation, section ordering, and database behavior tests for key migration helpers.

Possibly related PRs

  • specify/specify7#8039: Aligns with changes to run_key_migration_functions and locale/schema update helpers to prevent duplicate Splocalecontainer/string records; the new tests validate the behavior of these helpers including the language="en" fix in this PR.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Testing Instructions ⚠️ Warning Testing instructions only say "ensure backend tests pass in GitHub Actions" without identifying which tests cover the affected components or how to run them. Specify which test module/classes to run, e.g., "python manage.py test specifyweb.specify.tests.test_run_key_migration_functions" and clarify what functionality is being tested.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change—adding unit tests for key migration components as the primary objective.
Linked Issues check ✅ Passed The PR implements comprehensive test coverage for the run_key_migration_functions command and selected migration helper functions, addressing the core objectives from #8049.
Out of Scope Changes check ✅ Passed Minor updates to test expectations and schema migration logic are tightly scoped; the substantial addition of 853 lines of tests directly supports the stated objectives.
Automatic Tests ✅ Passed PR includes 853 lines of comprehensive automatic tests covering all 8 requested migration functions, command dispatch, execution order, and database behavior with 29 test methods.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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 issue-8049

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.

❤️ Share

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

@acwhite211 acwhite211 marked this pull request as ready for review May 22, 2026 21:02
@acwhite211 acwhite211 requested a review from a team May 22, 2026 21:02
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

2040-2055: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Scope the upsert to English rows to avoid deleting non-English translations.

Line 2040 fetches all locale rows for the target FK, and Line 2055 deletes all but one. With Line 2050 now creating language="en", this can still overwrite/delete existing non-English rows instead of upserting English only.

Suggested fix
         qs = Splocaleitemstr.objects.filter(
             itemdesc_id=itemdesc_id,
             itemname_id=itemname_id,
+            language="en",
         ).order_by("id")
🤖 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 2040
- 2055, The current upsert logic on Splocaleitemstr queries all rows matching
itemdesc_id and itemname_id (qs = Splocaleitemstr.objects.filter(...)) then
creates or deduplicates without scoping by language, causing non-English
translations to be deleted; modify the query and subsequent logic to restrict to
language="en" (e.g., filter language="en" when building qs, check obj from that
scoped qs, create with language="en" only if no en row exists, and delete
duplicates only among en rows) so only English rows are upserted and non-English
translations are preserved.
🤖 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.

Outside diff comments:
In `@specifyweb/specify/migration_utils/update_schema_config.py`:
- Around line 2040-2055: The current upsert logic on Splocaleitemstr queries all
rows matching itemdesc_id and itemname_id (qs =
Splocaleitemstr.objects.filter(...)) then creates or deduplicates without
scoping by language, causing non-English translations to be deleted; modify the
query and subsequent logic to restrict to language="en" (e.g., filter
language="en" when building qs, check obj from that scoped qs, create with
language="en" only if no en row exists, and delete duplicates only among en
rows) so only English rows are upserted and non-English translations are
preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ebd59b0-2eda-495c-a0a5-dda3ad7f033a

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc0f04 and 016c40a.

📒 Files selected for processing (3)
  • specifyweb/backend/stored_queries/tests/tests_legacy.py
  • specifyweb/specify/migration_utils/update_schema_config.py
  • specifyweb/specify/tests/test_run_key_migration_functions.py
💤 Files with no reviewable changes (1)
  • specifyweb/backend/stored_queries/tests/tests_legacy.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📋Back Log

Development

Successfully merging this pull request may close these issues.

Add unit test coverage for run_key_migration_functions

1 participant