Skip to content

Enforce loan preparation availability on insert (fixes #8188)#8189

Open
foozleface wants to merge 2 commits into
mainfrom
issue-cas-8188
Open

Enforce loan preparation availability on insert (fixes #8188)#8189
foozleface wants to merge 2 commits into
mainfrom
issue-cas-8188

Conversation

@foozleface

@foozleface foozleface commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #8188

The fix for #7665 (79c3a4f) added an early return to loanprep_quantity_must_be_lte_availability that skips the rule whenever ipreparation.id is None — i.e. on every INSERT. Since then the server has not enforced preparation availability when a loan preparation is created, so a specimen can be placed on two open loans at once through any path that bypasses the frontend InteractionDialog pre-query ("Add Unassociated Item", direct REST API POSTs, WorkBench loan uploads). The gift and exchange-out rules in the same file validate on insert; loans were the only interaction that did not.

This PR removes the early return so the availability check runs on insert again, matching the sibling rules. The null-safe quantity handling from the #7665 fix (int(x or 0)) is kept — that alone fixes #7665, which was a TypeError from calling int(None) when Quantity Resolved was unmapped in a WorkBench upload. On insert, get_availability is called with iprepid=None and correctly skips the self-exclusion clause, so no other change is needed.

A second commit fixes the matching null-safety gap in get_availability's SQL: sum(lp.quantity - lp.quantityresolved) goes NULL for a row whose quantityresolved is NULL (e.g. created by a WorkBench upload with Quantity Resolved unmapped), so that row contributed nothing to the on-loan amount and availability was overstated. Both fields are now coalesced to 0, matching the Python side.

Added specifyweb/backend/businessrules/tests/test_loanpreparation.py covering:

  • inserting a loan preparation that exceeds availability raises BusinessRuleException (the regression)
  • inserting within availability succeeds
  • a resolved loan frees availability for a new loan
  • a null quantityresolved does not crash the rule ("Quantity Resolved" incorrectly enforced as required in WorkBench #7665 regression guard) and still counts as fully unresolved
  • the existing update-path enforcement still works

All existing loan-preparation-creating tests in businessrules, interactions, specify, and workbench stay within availability, so no other test changes were needed.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced availability calculation to properly handle missing or zero values in quantity fields.
    • Strengthened loan-preparation validation to apply consistently across all data conditions.

The #7665 fix added an early return that skipped the
loanprep_quantity_must_be_lte_availability rule whenever the row was
new, disabling server-side over-loan protection on every creation path
(Add Unassociated Item, REST API, WorkBench uploads). Remove the early
return so the rule validates inserts again, matching the gift and
exchangeout sibling rules. The null-safe int(x or 0) handling that
actually fixed #7665 is unchanged.
A loanpreparation row with a NULL quantityresolved (e.g. created by a
WorkBench upload with Quantity Resolved unmapped) made the whole
quantity-quantityresolved term NULL, so the row contributed nothing to
the amount on loan and availability was overstated. Coalesce both
fields to 0, matching the int(x or 0) handling on the Python side.
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 73ce8490-d479-4d24-8640-04ea4c6b9faf

📥 Commits

Reviewing files that changed from the base of the PR and between 9f0ef8c and b5811e8.

📒 Files selected for processing (2)
  • specifyweb/backend/businessrules/rules/interaction_rules.py
  • specifyweb/backend/businessrules/tests/test_loanpreparation.py

📝 Walkthrough

Walkthrough

This PR fixes a regression in loan preparation availability validation. The rule was inadvertently disabled for new records by an earlier NULL-safety fix. The change restores enforcement at insert time by making the quantity calculation fully NULL-safe and removing a conditional that skipped validation for unsaved objects. Comprehensive tests verify the behavior across insert, update, and resolved scenarios.

Changes

Loan Preparation Availability Validation

Layer / File(s) Summary
NULL-safe availability check and insert-time validation
specifyweb/backend/businessrules/rules/interaction_rules.py
get_availability SQL wraps lp.quantity and lp.quantityresolved in coalesce(..., 0) to safely handle NULL values. The early return guarding ipreparation.id is removed so the availability rule runs on both INSERT and UPDATE, not just UPDATE.
Test fixtures and insert-time boundary cases
specifyweb/backend/businessrules/tests/test_loanpreparation.py
New test suite with shared fixtures (Preptype, Preparation, two Loan records) validates that inserting a loan preparation exceeding availability raises BusinessRuleException, and insertion within available quantity succeeds without error.
Resolved status and update-time validation
specifyweb/backend/businessrules/tests/test_loanpreparation.py
Tests confirm that marking a loan preparation resolved frees availability for new records, NULL quantityresolved is treated as zero in calculations, and updating an existing record to exceed availability raises BusinessRuleException.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Testing Instructions ⚠️ Warning Test cases accurately cover PR objectives but lack explicit instructions: no docstrings, no comments explaining purpose, no documented commands to run tests locally. Add class/method docstrings and a comment block explaining how to run: python manage.py test specifyweb.backend.businessrules.tests.test_loanpreparation.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the primary change: restoring loan preparation availability enforcement on insert, which directly addresses the regression described in issue #8188.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from #8188 (remove early return, restore insert enforcement) and #7665 (maintain null-safe handling with coalesce operations) with comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes directly support the linked issues: business rule fixes align with #8188 requirements and null-safety with #7665, while tests verify both INSERT enforcement and null quantity handling without extraneous modifications.
Automatic Tests ✅ Passed PR includes comprehensive automatic tests in test_loanpreparation.py with 5 test methods covering INSERT/UPDATE enforcement, NULL handling, and availability calculations.

✏️ 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-cas-8188

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.

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

1 participant