Fix CollectionObject business rule crash on missing CollectionObjectType#7905
Fix CollectionObject business rule crash on missing CollectionObjectType#7905acwhite211 wants to merge 7 commits into
Conversation
acwhite211
changed the title
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
There was a problem hiding this comment.
- Verify the save completes normally instead of hanging on the
"Loading..."dialog. - Verify the page remains usable after save and no infinite loading state appears.
- Refresh or reopen the record and confirm the new determination was actually saved.
- Run the same CO QB query to make sure that the COT is not null anymore.
Everything looks good, except when i refresh the page and rerun the query, the COT is still null.
Db:https://kuentoissue7870-issue-7870.test.specifysystems.org/specify/query/new/collectionobject/
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
📝 WalkthroughWalkthroughThis PR adds automatic defaulting of CollectionObject collection object type, resolving a hang that occurs when saving records without an assigned type. It introduces utility functions to derive and persist defaults from collection metadata, integrates them into pre-save hooks, and adds frontend null-guard logic to prevent errors. ChangesCollectionObject Type Defaulting
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
specifyweb/backend/businessrules/tests/test_collectionobject.py (1)
62-91: 💤 Low valueSmall note on the discipline rename.
Renaming the discipline to
"Fallback Discipline"before triggering the helper looks like a workaround forget_or_create(name=discipline.name, ...)colliding with aCollectionobjecttypethat the test fixture already created under the original discipline name. The test passes, but it makes the intent a bit opaque — a one-line comment ("rename so the fallbackget_or_createactually creates a new COT") would help the next person who touches this.Also consider asserting on
Collectionobjecttype.objects.filter(name="Fallback Discipline", collection=self.collection).count() == 1to lock in "creates exactly one" behavior and guard against a future regression that double-creates under the race noted inutils.py.🤖 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/businessrules/tests/test_collectionobject.py` around lines 62 - 91, In test_existing_collectionobject_without_type_creates_collection_default_on_save: add a one-line comment explaining why the discipline name is renamed to "Fallback Discipline" (so get_or_create(name=discipline.name, ...) will create a new Collectionobjecttype instead of colliding with the fixture), and after saving and refreshing assert that exactly one Collectionobjecttype exists for that name and collection (e.g. assert Collectionobjecttype.objects.filter(name="Fallback Discipline", collection=self.collection).count() == 1) to guard against duplicate creation; reference the test method name, the Collectionobjecttype model and the get_or_create logic in utils.py when making the change.specifyweb/specify/api/utils.py (1)
65-91: 💤 Low valueLogic for
ensure_collection_object_typereads cleanly.The branching (already-set → collection default → derive-from-discipline) matches the intended fallback chain, and gating the
persistORM update onpk is not None+collectionobjecttype__isnull=Trueis the right defensive shape so we never clobber a value that another writer has set in the meantime. Nice.One small polish: when
collection_object.pk is Noneand the collection has no default, the function returnsNoneand the caller relies on Django's normal save path to write the (still-null) field. That's correct given the frontend now null-guards, but a one-line docstring spelling out the three branches and the "may return None" contract would save the next reader a trip through the call sites.🤖 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/api/utils.py` around lines 65 - 91, Add a one-line docstring to the ensure_collection_object_type function explaining its three-branch behavior and return contract: if collection_object.collectionobjecttype is already set return it; otherwise use collection.collectionobjecttype as a default; otherwise derive via get_or_create_default_collection_object_type; the function may return None when no type can be determined and callers should handle that (persist and pk gating behavior remains unchanged). Reference ensure_collection_object_type, collection_object, persist, and get_or_create_default_collection_object_type in the docstring for clarity.
🤖 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/api/utils.py`:
- Around line 38-42: The helper that reads collection.discipline (where
discipline_name = collection.discipline.name and taxon_tree_def_id =
collection.discipline.taxontreedef_id) currently returns None silently when
taxon_tree_def_id is null; update that helper in specify/api/utils.py to emit a
visible log warning (e.g., logger.warning) mentioning the collection identifier
and that discipline.taxontreedef_id is missing before returning None so the
missing COT/backfill state is observable; reference the discipline_name,
taxon_tree_def_id and collectionobjecttype variables in the message to aid
debugging.
---
Nitpick comments:
In `@specifyweb/backend/businessrules/tests/test_collectionobject.py`:
- Around line 62-91: In
test_existing_collectionobject_without_type_creates_collection_default_on_save:
add a one-line comment explaining why the discipline name is renamed to
"Fallback Discipline" (so get_or_create(name=discipline.name, ...) will create a
new Collectionobjecttype instead of colliding with the fixture), and after
saving and refreshing assert that exactly one Collectionobjecttype exists for
that name and collection (e.g. assert
Collectionobjecttype.objects.filter(name="Fallback Discipline",
collection=self.collection).count() == 1) to guard against duplicate creation;
reference the test method name, the Collectionobjecttype model and the
get_or_create logic in utils.py when making the change.
In `@specifyweb/specify/api/utils.py`:
- Around line 65-91: Add a one-line docstring to the
ensure_collection_object_type function explaining its three-branch behavior and
return contract: if collection_object.collectionobjecttype is already set return
it; otherwise use collection.collectionobjecttype as a default; otherwise derive
via get_or_create_default_collection_object_type; the function may return None
when no type can be determined and callers should handle that (persist and pk
gating behavior remains unchanged). Reference ensure_collection_object_type,
collection_object, persist, and get_or_create_default_collection_object_type in
the docstring for clarity.
🪄 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: 953818a5-0b81-4763-b70f-9154865b16cc
📒 Files selected for processing (6)
specifyweb/backend/businessrules/rules/collectionobject_rules.pyspecifyweb/backend/businessrules/rules/determination_rules.pyspecifyweb/backend/businessrules/tests/test_collectionobject.pyspecifyweb/frontend/js_src/lib/components/DataModel/__tests__/businessRules.test.tsspecifyweb/frontend/js_src/lib/components/DataModel/businessRuleDefs.tsspecifyweb/specify/api/utils.py
| discipline_name = collection.discipline.name | ||
| taxon_tree_def_id = collection.discipline.taxontreedef_id | ||
|
|
||
| if discipline_name is None or taxon_tree_def_id is None: | ||
| return None |
There was a problem hiding this comment.
Silent no-op when taxontreedef_id is missing.
discipline.name is a required field and unlikely to be None, but discipline.taxontreedef_id can legitimately be null for some legacy disciplines. When that happens this helper returns None without any signal, the caller silently leaves collectionobjecttype unset, and the pre-save hook completes "successfully" — which puts us right back in the original "save proceeds with null COT" territory that the frontend null-guard now has to catch.
Consider at least a logger.warning(...) so this state is observable in server logs when the backend backfill can't run.
📝 Suggested logging
if discipline_name is None or taxon_tree_def_id is None:
+ logger.warning(
+ "Cannot create default Collectionobjecttype for collection %s: "
+ "discipline name or taxontreedef is missing.",
+ collection.pk,
+ )
return None🤖 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/api/utils.py` around lines 38 - 42, The helper that reads
collection.discipline (where discipline_name = collection.discipline.name and
taxon_tree_def_id = collection.discipline.taxontreedef_id) currently returns
None silently when taxon_tree_def_id is null; update that helper in
specify/api/utils.py to emit a visible log warning (e.g., logger.warning)
mentioning the collection identifier and that discipline.taxontreedef_id is
missing before returning None so the missing COT/backfill state is observable;
reference the discipline_name, taxon_tree_def_id and collectionobjecttype
variables in the message to aid debugging.
3 participants
Fixes #7870
Saving a
CollectionObjectwith a missingcollectionObjectTypecould trigger a front-end business rule crash and leave the record stuck on the"Loading..."dialog instead of completing the save. This affected older records, including records created in Specify 6, whereCollectionObjectTypeIDmay still benull.Added a null guard in the
CollectionObjectcollectionObjectTypebusiness rule and skip determination taxon tree validation when no collection object type is set. Clear any existing determination taxon save blockers whencollectionObjectTypeis missing. Also added unit tests for saving whencollectionObjectTypeisnulland clearing invalid determination blockers whencollectionObjectTypebecomesnullOn the back-end, I decided to go ahead and opportunistically normalize legacy null
collectionObjectTypevalues when an existingCollectionObjectis saved in Specify 7. If the collection already has a default Collection Object Type, that value is applied on save. If both the record and the collection default are null, Specify creates or reuses the collection’s discipline-based default type, assigns it to the collection, and uses it for the saved record. I'm ok with reverting this section if anyone thinks we should leave them as null.Checklist
self-explanatory (or properly documented)
Testing instructions
CollectionObjectwith no Collection Object Type set (CollectionObjectTypeID = null). Older Specify 6-created records are the main target.Data Entry, open one of those records and confirm the Collection Object Type field is blank.Determinationto the record.Save."Loading..."dialog.Summary by CodeRabbit
Bug Fixes
Tests