Merge pull request #6062 from specify/issue-5466#8157
Closed
CarolineDenis wants to merge 6 commits into
Closed
Conversation
Allow for re-running of creating default cots with django command
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsolidates default CollectionObjectType backfill into utils.create_default_collection_types, updates migration 0002 to delegate to that utility, adds an atomic migration 0021 to run it, adjusts migration 0022 dependency, and provides a management command to invoke the utility manually. ChangesDefault COTS Creation Refactoring
Sequence Diagram(s)sequenceDiagram
participant ManagementCommand
participant Migration0021
participant Utils
participant Database
ManagementCommand->>Utils: create_default_collection_types(apps)
Migration0021->>Utils: handle_default_collection_types(apps, schema_editor)
Utils->>Database: read/update Collection, Collectionobjecttype, Collectionobject
Database-->>Utils: results
Utils-->>ManagementCommand: complete
Utils-->>Migration0021: complete
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)
⚔️ Resolve merge conflicts
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 (3)
specifyweb/specify/utils.py (2)
56-56: 💤 Low valuePrefix unused variable with underscore.
The
createdvariable fromget_or_createis never used.♻️ Apply linter suggestion
- cot, created = Collectionobjecttype.objects.get_or_create( + cot, _created = Collectionobjecttype.objects.get_or_create(🤖 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/utils.py` at line 56, The tuple unpacking from Collectionobjecttype.objects.get_or_create assigns an unused variable named created; update the unpacking in the call to get_or_create (the line with cot, created = Collectionobjecttype.objects.get_or_create) to prefix the unused variable with an underscore (cot, _created = Collectionobjecttype.objects.get_or_create) so the linter recognizes it as intentionally unused.
43-44: 💤 Low valueRemove the stale implementation reference comment.
The comment references a specific file and line number that will become outdated as the codebase evolves.
♻️ Suggested cleanup
logger.debug(raw_sql) - # Run in the storred_queries.execute file, in the execute function, right before the return statement, line 546 - # from specifyweb.specify.utils import log_sqlalchemy_query; log_sqlalchemy_query(query)🤖 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/utils.py` around lines 43 - 44, Remove the stale implementation-reference comment lines in specifyweb.specify.utils that mention "Run in the storred_queries.execute file, in the execute function, right before the return statement, line 546" and the example call; locate the comment by searching for the string "log_sqlalchemy_query(query)" or the exact phrase "Run in the storred_queries.execute file" and delete those lines (or replace them with a short, stable note such as a module-level docstring that explains how to use log_sqlalchemy_query without file/line references).specifyweb/specify/management/commands/create_default_cots.py (1)
12-13: ⚡ Quick winWrap the manual repair path in a transaction.
0021_ensure_default_cots.pyruns the same helper atomically, but this command does not. If one collection fails midway, operators can be left with a partially repaired database. Usingtransaction.atomic()here keeps the manual path consistent with the migration behavior.Suggested change
import logging from django.core.management.base import BaseCommand from django.apps import apps +from django.db import transaction from specifyweb.businessrules.exceptions import BusinessRuleException from specifyweb.specify.utils import create_default_collection_types @@ help = "Create default COTS for the Specify database if they haven't been previously set." def handle(self, *args, **options): - create_default_collection_types(apps) + with transaction.atomic(): + create_default_collection_types(apps)🤖 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/management/commands/create_default_cots.py` around lines 12 - 13, The handle method currently calls create_default_collection_types(apps) without transactional protection; wrap that call in a Django atomic transaction (use transaction.atomic()) so the manual repair path is all-or-nothing, and ensure transaction is imported from django.db if not already; specifically modify the handle function to execute create_default_collection_types(apps) inside a transaction.atomic() context to match the migration's atomic behavior.
🤖 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/0002_geo.py`:
- Around line 15-17: The migration 0002_geo.py currently imports and calls
create_default_collection_types (via handle_default_collection_types) from
runtime code, which can change and break this shipped migration; remove that
import and instead inline the migration-safe logic (or move it into a new frozen
migration-only module) so the migration does not depend on
specifyweb.specify.utils at runtime. Specifically, replace the import of
create_default_collection_types and the call to handle_default_collection_types
with a copy of the minimal deterministic code required to create the default
collection types inside 0002_geo.py (or create a new module alongside migrations
that contains a frozen implementation) and ensure you do not reference runtime
symbols like BusinessRuleException or logger from utils—use migration-local
error handling and a migration-local logger if needed. Ensure the migration
remains initial=True and that the new code operates only on the provided
apps/models from the migration framework.
In `@specifyweb/specify/migrations/0021_ensure_default_cots.py`:
- Around line 1-2: The migration currently imports
create_default_collection_types from application code which risks future
divergence; instead inline a frozen copy of that helper directly into the
migration file (e.g., define a local function create_default_collection_types
within the migration module) and update the RunPython call to invoke this local
function (or reference it from the Migration class) so the migration is
self-contained and deterministic; ensure you copy any constants or behaviors the
helper depends on into the migration and remove the external import.
In `@specifyweb/specify/utils.py`:
- Around line 68-76: The loop mutates collection.code each iteration causing
cumulative suffixes; save the original value (e.g., original_code =
collection.code) before entering the while loop and then set collection.code =
f'{original_code}-{i}' on each attempt (incrementing i) and check against
code_set; once unique, add to code_set and break. Ensure you reference
collection.code, code_set and the loop counter i when applying this fix.
- Around line 67-81: The except BusinessRuleException handler currently only
handles the 'Collection must have unique code in discipline' case and silently
ignores any other BusinessRuleException; update the handler (the except
BusinessRuleException as e block that manipulates collection.code, code_set and
calls collection.save()) to log unexpected BusinessRuleException instances
before continuing — use logger.error or logger.warning to record the exception
message and context (e.g., include repr(collection) and str(e) or traceback) for
the branch where the error message does not match the uniqueness check so
unexpected errors are visible.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/create_default_cots.py`:
- Around line 12-13: The handle method currently calls
create_default_collection_types(apps) without transactional protection; wrap
that call in a Django atomic transaction (use transaction.atomic()) so the
manual repair path is all-or-nothing, and ensure transaction is imported from
django.db if not already; specifically modify the handle function to execute
create_default_collection_types(apps) inside a transaction.atomic() context to
match the migration's atomic behavior.
In `@specifyweb/specify/utils.py`:
- Line 56: The tuple unpacking from Collectionobjecttype.objects.get_or_create
assigns an unused variable named created; update the unpacking in the call to
get_or_create (the line with cot, created =
Collectionobjecttype.objects.get_or_create) to prefix the unused variable with
an underscore (cot, _created = Collectionobjecttype.objects.get_or_create) so
the linter recognizes it as intentionally unused.
- Around line 43-44: Remove the stale implementation-reference comment lines in
specifyweb.specify.utils that mention "Run in the storred_queries.execute file,
in the execute function, right before the return statement, line 546" and the
example call; locate the comment by searching for the string
"log_sqlalchemy_query(query)" or the exact phrase "Run in the
storred_queries.execute file" and delete those lines (or replace them with a
short, stable note such as a module-level docstring that explains how to use
log_sqlalchemy_query without file/line references).
🪄 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: d50373e5-ded0-48bc-869d-5481b91a3985
📒 Files selected for processing (4)
specifyweb/specify/management/commands/create_default_cots.pyspecifyweb/specify/migrations/0002_geo.pyspecifyweb/specify/migrations/0021_ensure_default_cots.pyspecifyweb/specify/utils.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
specifyweb/specify/utils.py (2)
67-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnexpected
BusinessRuleExceptionis still silently swallowed.When the message doesn't match the uniqueness check, the exception is dropped without logging. Add an
elsebranch to log it, as previously suggested on this hunk.🤖 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/utils.py` around lines 67 - 81, The except block for BusinessRuleException (around the uniqueness handling for collection.code) currently only handles the 'Collection must have unique code in discipline' case and silently ignores all other BusinessRuleException instances; modify the except BusinessRuleException as e: block to add an else branch that logs unexpected exceptions (include the exception details and the collection identifier) using logger.warning or logger.exception so they aren't swallowed, keeping the existing uniqueness-renaming logic that calls collection.save() and manipulates code_set intact.
68-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCumulative suffix bug in the unique-code retry.
collection.code = f'{collection.code}-{i}'mutatescodein place each iteration, producingABC-1-2-3rather thanABC-1,ABC-2, … Save the original code before the loop and rebuild from it. This matches a prior review comment on this hunk.🤖 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/utils.py` around lines 68 - 76, The retry loop that handles duplicate collection codes mutates collection.code each iteration causing cumulative suffixes; before the while loop in the block that checks "Collection must have unique code in discipline" (around where collection.code is reassigned) capture the original code into a variable (e.g., orig_code) and inside the loop set collection.code = f'{orig_code}-{i}' (incrementing i) until a unique value is found, then add to code_set and break; update the logic in the surrounding function/method in specify.utils to use the preserved orig_code instead of repeatedly appending to collection.code.specifyweb/specify/migrations/0021_ensure_default_cots.py (1)
1-2: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMigration imports mutable application code.
Same concern as 0002_geo:
0021importscreate_default_collection_typesfrom runtimeutils, coupling a historical migration to code that can change. Inline/freeze it. This restates a prior review comment on these lines.🤖 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/0021_ensure_default_cots.py` around lines 1 - 2, The migration currently imports create_default_collection_types from mutable application code; instead copy (inline) the exact implementation of create_default_collection_types into this migration and use that local function in the RunPython operation (or implement the creation logic directly inside the migration's forward function), then remove the import from specifyweb.specify.utils so the migration is self-contained and not dependent on runtime code changes; reference the existing symbol create_default_collection_types and ensure any helper names you introduce are declared inside the migration file.specifyweb/specify/migrations/0002_geo.py (1)
15-17: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftInitial migration now depends on mutable runtime code.
0002_geo(initial = True) importscreate_default_collection_typesfromspecifyweb.specify.utils; future edits to that helper can alter this already-shipped migration's behavior. Freeze the logic in a migration-owned module. This restates a prior review comment on these lines.
🧹 Nitpick comments (2)
specifyweb/specify/management/commands/create_default_cots.py (1)
12-13: ⚡ Quick winWrap the run in a transaction and surface the result.
Unlike migration 0021 (
atomic=True), the command runs without a surrounding transaction, so a mid-run failure leaves partial commits. Wrapping intransaction.atomic()plus a short success message improves the manual-run UX.♻️ Proposed fix
+from django.db import transaction ... def handle(self, *args, **options): - create_default_collection_types(apps) + with transaction.atomic(): + create_default_collection_types(apps) + self.stdout.write(self.style.SUCCESS("Default COTs ensured."))🤖 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/management/commands/create_default_cots.py` around lines 12 - 13, The handle method currently calls create_default_collection_types(apps) without transactional protection; wrap that call in a django.db.transaction.atomic() block inside the Command.handle to ensure all-or-nothing commits, and after successful completion write a concise success message (e.g., using self.stdout.write or self.stdout.success) so the user sees the result; update imports to include transaction and keep the reference to create_default_collection_types(apps) and Command.handle as the change points.specifyweb/specify/migrations/0021_ensure_default_cots.py (1)
4-15: ⚡ Quick winMove the migration callable to module scope and add a reverse.
handle_default_collection_typesis defined as a method inside theMigrationclass — it works only because the name resolves in the class body, which is non-idiomatic and fragile. Also, theRunPythonoperation has noreverse_code, so the migration is irreversible (the PR checklist flags adding a reverse). Define it at module level and passmigrations.RunPython.noopfor the reverse.♻️ Proposed fix
from django.db import migrations from specifyweb.specify.utils import create_default_collection_types -class Migration(migrations.Migration): - - def handle_default_collection_types(apps, schema_editor): - create_default_collection_types(apps) +def handle_default_collection_types(apps, schema_editor): + create_default_collection_types(apps) +class Migration(migrations.Migration): + dependencies = [ ('specify', '0020_add_tectonicunit_to_pc_in_schema_config'), ] operations = [ - migrations.RunPython(handle_default_collection_types, atomic=True), + migrations.RunPython(handle_default_collection_types, migrations.RunPython.noop, atomic=True), ]🤖 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/0021_ensure_default_cots.py` around lines 4 - 15, The migration defines handle_default_collection_types as a nested function inside the Migration class and omits a reversible operation; move handle_default_collection_types out to module scope (so it's a top-level function that calls create_default_collection_types(apps)), update the Migration.operations entry to use migrations.RunPython(handle_default_collection_types, reverse_code=migrations.RunPython.noop) (or pass migrations.RunPython.noop as the second arg) and keep the Migration class and dependencies as-is so the migration becomes non-fragile and reversible.
🤖 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/utils.py`:
- Around line 62-66: The current update unconditionally repoints every
Collectionobject and overwrites the collection's default COT; change it to only
affect unset values: update Collectionobject rows using
Collectionobject.objects.filter(collection=collection,
collectionobjecttype__isnull=True).update(collectionobjecttype=cot) and only set
collection.collectionobjecttype and call collection.save() when
collection.collectionobjecttype is None (or not already set) so existing
user-assigned COTs and collection defaults are preserved.
- Around line 56-60: The get_or_create call in create_default_collection_types
can raise MultipleObjectsReturned on dirty DBs; replace it with a safe
fetch-first-then-create pattern: use
Collectionobjecttype.objects.filter(name=discipline_name, collection=collection,
taxontreedef_id=discipline.taxontreedef_id).order_by("id").first() and if that
returns None call Collectionobjecttype.objects.create(...) to insert the record;
optionally wrap the create in a transaction.atomic block to avoid race
conditions. Ensure you update any variable names (cot, created) accordingly so
downstream code continues to work with the retrieved or newly created
Collectionobjecttype instance.
---
Duplicate comments:
In `@specifyweb/specify/migrations/0021_ensure_default_cots.py`:
- Around line 1-2: The migration currently imports
create_default_collection_types from mutable application code; instead copy
(inline) the exact implementation of create_default_collection_types into this
migration and use that local function in the RunPython operation (or implement
the creation logic directly inside the migration's forward function), then
remove the import from specifyweb.specify.utils so the migration is
self-contained and not dependent on runtime code changes; reference the existing
symbol create_default_collection_types and ensure any helper names you introduce
are declared inside the migration file.
In `@specifyweb/specify/utils.py`:
- Around line 67-81: The except block for BusinessRuleException (around the
uniqueness handling for collection.code) currently only handles the 'Collection
must have unique code in discipline' case and silently ignores all other
BusinessRuleException instances; modify the except BusinessRuleException as e:
block to add an else branch that logs unexpected exceptions (include the
exception details and the collection identifier) using logger.warning or
logger.exception so they aren't swallowed, keeping the existing
uniqueness-renaming logic that calls collection.save() and manipulates code_set
intact.
- Around line 68-76: The retry loop that handles duplicate collection codes
mutates collection.code each iteration causing cumulative suffixes; before the
while loop in the block that checks "Collection must have unique code in
discipline" (around where collection.code is reassigned) capture the original
code into a variable (e.g., orig_code) and inside the loop set collection.code =
f'{orig_code}-{i}' (incrementing i) until a unique value is found, then add to
code_set and break; update the logic in the surrounding function/method in
specify.utils to use the preserved orig_code instead of repeatedly appending to
collection.code.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/create_default_cots.py`:
- Around line 12-13: The handle method currently calls
create_default_collection_types(apps) without transactional protection; wrap
that call in a django.db.transaction.atomic() block inside the Command.handle to
ensure all-or-nothing commits, and after successful completion write a concise
success message (e.g., using self.stdout.write or self.stdout.success) so the
user sees the result; update imports to include transaction and keep the
reference to create_default_collection_types(apps) and Command.handle as the
change points.
In `@specifyweb/specify/migrations/0021_ensure_default_cots.py`:
- Around line 4-15: The migration defines handle_default_collection_types as a
nested function inside the Migration class and omits a reversible operation;
move handle_default_collection_types out to module scope (so it's a top-level
function that calls create_default_collection_types(apps)), update the
Migration.operations entry to use
migrations.RunPython(handle_default_collection_types,
reverse_code=migrations.RunPython.noop) (or pass migrations.RunPython.noop as
the second arg) and keep the Migration class and dependencies as-is so the
migration becomes non-fragile and reversible.
🪄 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: aaf3e2c7-b381-451b-a62c-e3f9b5dcc1f9
📒 Files selected for processing (4)
specifyweb/specify/management/commands/create_default_cots.pyspecifyweb/specify/migrations/0002_geo.pyspecifyweb/specify/migrations/0021_ensure_default_cots.pyspecifyweb/specify/utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/specify/utils.py (1)
84-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog unexpected
BusinessRuleExceptionerrors.When a
BusinessRuleExceptionis raised that doesn't match the uniqueness message, the code falls through tocontinuewithout logging. This silently swallows unexpected exceptions, making debugging difficult. (This appears to have been addressed previously but the fix is not present in the current code.)🛡️ Proposed fix
except BusinessRuleException as e: if 'Collection must have unique code in discipline' in str(e): # May want to do something besides numbering, but users can edit if after the migrqation if they want. original_code = collection.code i = 1 while True: collection.code = f'{original_code}-{i}' i += 1 if collection.code not in code_set: code_set.add(collection.code) break try: collection.save() except BusinessRuleException as e: logger.warning(f'Problem saving collection {collection}: {e}') + else: + logger.warning(f'Unexpected BusinessRuleException saving collection {collection}: {e}') continue🤖 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/utils.py` around lines 84 - 99, The except BusinessRuleException block currently only handles the specific uniqueness message and otherwise silently continues; update the handler for BusinessRuleException in the function where collection is processed so that when the exception message does NOT contain 'Collection must have unique code in discipline' you log the unexpected error (use logger.error or logger.exception) including the collection identifier (e.g., collection.code or repr(collection)) and the exception details before the continue; keep the existing uniqueness-handling branch that renumbers and calls collection.save(), and ensure you still catch and warn on BusinessRuleException from collection.save().
🤖 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/utils.py`:
- Around line 84-99: The except BusinessRuleException block currently only
handles the specific uniqueness message and otherwise silently continues; update
the handler for BusinessRuleException in the function where collection is
processed so that when the exception message does NOT contain 'Collection must
have unique code in discipline' you log the unexpected error (use logger.error
or logger.exception) including the collection identifier (e.g., collection.code
or repr(collection)) and the exception details before the continue; keep the
existing uniqueness-handling branch that renumbers and calls collection.save(),
and ensure you still catch and warn on BusinessRuleException from
collection.save().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b2de207-69d3-48b5-aa81-34d3df906a87
📒 Files selected for processing (1)
specifyweb/specify/utils.py
This was referenced Jun 3, 2026
Contributor
Author
|
Changes are not relevant anymore with code changes present in newer versions |
github-project-automation
Bot
moved this from Dev Attention Needed
to ✅Done
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
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.
Allow for re-running of creating default cots with django command
Fixes #5466
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
New Features
Chores
Bug Fixes