fix(acquisition): rollover #3997
Conversation
rerowep
commented
Jan 6, 2026
rerowep
commented
- Fixes creation of rollover orders lines for not pending orders during rollover.
- Rounds allocated amount when transferring funds between accounts.
- Closes Rollover: order line creation can fail because of validation #3996
WalkthroughMultiple modules updated with defensive programming improvements (isinstance checks), batch operation optimizations for reindexing and data deletion, rounding consistency in financial calculations, and docstring refinements. Changes affect acquisition accounts, rollover processes, API utilities, serializers, and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)rero_ils/modules/acquisition/rollover.py (3)
🪛 Ruff (0.14.10)rero_ils/modules/acquisition/rollover.py444-444: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (13)
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 |
rerowep
force-pushed
the
wep-fix-rollover
branch
from
1dff508 to
1e4d7fc
Compare
![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
🤖 Fix all issues with AI Agents
In @rero_ils/modules/acquisition/rollover.py:
- Around line 33-34: Add the missing comma after AcqOrderLineIndexer in the
import/listing where AcqOrderLine, AcqOrderLineIndexer and AcqOrderLinesSearch
are declared to fix the syntax error; locate the sequence containing the symbol
AcqOrderLineIndexer in rollover.py and insert the comma so the tokens read
"AcqOrderLine, AcqOrderLineIndexer, AcqOrderLinesSearch".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rero_ils/modules/acquisition/acq_accounts/api.pyrero_ils/modules/acquisition/rollover.pyrero_ils/modules/api.py
🧰 Additional context used
🧬 Code graph analysis (1)
rero_ils/modules/api.py (2)
rero_ils/modules/users/api.py (1)
fields(213-215)rero_ils/modules/entities/remote_entities/replace.py (1)
query(115-122)
🪛 GitHub Actions: CI
rero_ils/modules/acquisition/rollover.py
[error] 34-34: SyntaxError: invalid syntax
🔇 Additional comments (9)
rero_ils/modules/acquisition/acq_accounts/api.py (2)
171-176: LGTM: Docstring improvements.The docstring updates improve clarity and fix a typo ("availabe" → "available" on Line 285).
Also applies to: 202-203, 233-236, 267-267, 285-285, 379-379
304-304: LGTM: Monetary rounding for fund transfers.Rounding allocated amounts to 2 decimals ensures consistent precision during fund transfers between accounts. This addresses potential floating-point arithmetic issues that could accumulate errors in financial calculations.
Also applies to: 326-326, 338-338, 349-349, 352-352
rero_ils/modules/acquisition/rollover.py (2)
413-413: LGTM: Batch reindexing optimization.Deferring reindexing by creating order lines with
reindex=False, accumulating them in a list, and then reindexing in batch (Lines 445-446) is a significant performance improvement. This avoids per-item index operations during the rollover process, which can be slow when processing many order lines.Also applies to: 436-437, 445-446
128-136: LGTM: Docstring formatting improvements.The docstring updates improve consistency and readability.
Also applies to: 504-504, 548-551, 584-587, 629-632, 652-654
rero_ils/modules/api.py (5)
131-131: LGTM: Improved type checking withisinstance.Using
isinstance(pids, list)is more Pythonic and handles subclasses correctly compared totype(pids) is list.
148-151: LGTM: Explicit None check forfieldsparameter.The conditional now explicitly checks
if fields is not None, which is clearer and correctly distinguishes betweenNoneand empty list[].
175-175: LGTM: Defensive copy prevents config mutation.Creating a shallow copy of the
endpointsdictionary before updating it prevents unintended modification of the application configuration.
252-252: LGTM: Explicit None check improves clarity.Changing from
if not id_:toif id_ is None:is more explicit and correctly handles cases whereid_might be a falsy but valid value (though UUID objects are unlikely to be falsy).
340-341: LGTM: Memory-efficient pagination withyield_per.Replacing manual pagination with SQLAlchemy's
yield_per(limit)is the recommended approach for memory-efficient iteration over large result sets. This prevents loading all records into memory at once.Also applies to: 347-348
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rero_ils/modules/api.py (1)
287-287: Inconsistent type check: should useisinstancehere too.Line 131 was updated to use
isinstance(pids, list), but this line still uses the oldertype(pids) is listpattern. For consistency and to handle subclasses, this should be updated as well.🔎 Proposed fix
- assert type(pids) is list + assert isinstance(pids, list)rero_ils/modules/acquisition/acq_accounts/api.py (1)
171-175: Docstring improvements are acceptable.Minor formatting and wording adjustments improve clarity without changing functionality. Note: Line 171 has a typo "acqz" instead of "acq" in the docstring.
🔎 Proposed fix for typo
- * APPROVED or ORDERED acqz order lines related to this account. + * APPROVED or ORDERED acq order lines related to this account.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rero_ils/modules/acquisition/acq_accounts/api.pyrero_ils/modules/acquisition/rollover.pyrero_ils/modules/api.py
🧰 Additional context used
🧬 Code graph analysis (1)
rero_ils/modules/acquisition/rollover.py (2)
rero_ils/modules/acquisition/acq_order_lines/api.py (1)
create(108-114)rero_ils/modules/acquisition/exceptions.py (1)
RolloverError(28-29)
🪛 Ruff (0.14.10)
rero_ils/modules/acquisition/rollover.py
444-444: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (dev)
- GitHub Check: test (deploy)
🔇 Additional comments (11)
rero_ils/modules/acquisition/rollover.py (3)
32-35: Import statement is now correct.The previous syntax error has been resolved.
AcqOrderLineIndexeris no longer imported since the batched reindexing now callsreindex()directly on each record instance.
413-446: Good optimization: Batched reindexing improves rollover performance.Deferring reindexing until all order lines are created reduces overhead during the migration loop. The approach is sound.
One consideration: if
reindex()fails for any item in the final loop (lines 445-446), the records will exist in the database but may not be fully indexed. This is a minor edge case since reindex failures are rare after successful creation, and the records would still be recoverable.For future optimization, consider using bulk indexing:
# Alternative approach using bulk indexer from rero_ils.modules.acquisition.acq_order_lines.api import AcqOrderLinesIndexer # ... if new_order_lines: indexer = AcqOrderLinesIndexer() indexer.bulk_index([line.id for line in new_order_lines])
128-136: Docstring improvements look good.The reformatted docstrings improve readability without changing behavior.
rero_ils/modules/api.py (6)
131-131: Good: Usingisinstancefor type checking.This is more Pythonic and handles subclasses properly.
148-151: Good defensive handling offieldsparameter.When
fieldsisNone, omitting thesource()call entirely is safer than passing an empty orNonevalue toincludes, which could cause unexpected behavior in some Elasticsearch versions.
175-176: Good: Defensive copy prevents config mutation.Creating a copy before calling
update()prevents theRECORDS_REST_ENDPOINTSconfig dictionary from being mutated with circulation endpoints, which could cause subtle bugs if this method is called multiple times or if other code relies on the original config state.
252-253: Good: More precise None check forid_.Using
if id_ is Noneinstead ofif not id_is correct here. While UUIDs won't be falsy values like0or"", explicitNonechecking is clearer and more intentional for this context.
340-341: Good: Memory-efficient iteration withyield_per.Using
yield_per(limit)instead of manual pagination is cleaner and more memory-efficient for large result sets. This delegates batching to SQLAlchemy, which handles the cursor-based iteration properly.
347-348: Consistent withget_all_pidsabove.Same memory-efficient pattern applied correctly.
rero_ils/modules/acquisition/acq_accounts/api.py (2)
304-304: Good: Rounding monetary amounts to 2 decimal places.This addresses floating-point precision issues when transferring funds between accounts. The rounding is correctly applied in all branches of the
transfer_fundmethod (CASE 1, 2, and 3).
326-352: Consistent rounding applied across all fund transfer cases.All allocated_amount updates in CASE 2 (lines 326, 338) and CASE 3 (lines 349, 352) correctly apply
round(..., 2)to prevent floating-point precision issues in monetary calculations. This is consistent with CASE 1 (line 304).
rerowep
force-pushed
the
wep-fix-rollover
branch
from
1e4d7fc to
bb460a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @scripts/test:
- Around line 76-77: The test script is exempting CVE-2025-53000 without
justification; update the exemption by either upgrading the pinned nbconvert
version to a patched release (replace the nbconvert 7.16.6 reference) or, if you
must keep the exemption, replace the bare add_exceptions "CVE-2025-53000" entry
with a clear inline justification comment explaining why this uncontrolled
search-path execution risk is acceptable in our environment and list any
mitigations (e.g., OS restrictions, non-Windows deployment, container isolation,
CI constraints); ensure the change is applied next to the add_exceptions call so
reviewers can see the decision and mitigation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
rero_ils/modules/acquisition/acq_accounts/api.pyrero_ils/modules/acquisition/rollover.pyrero_ils/modules/api.pyrero_ils/modules/serializers/mixins.pyscripts/test
🧰 Additional context used
🧬 Code graph analysis (2)
rero_ils/modules/acquisition/acq_accounts/api.py (3)
rero_ils/modules/api.py (3)
update(382-411)dbcommit(444-449)reindex(451-456)rero_ils/modules/acquisition/acq_order_lines/api.py (1)
update(116-132)rero_ils/modules/acquisition/acq_receipts/api.py (1)
update(110-137)
rero_ils/modules/acquisition/rollover.py (2)
rero_ils/modules/acquisition/acq_orders/api.py (1)
create(107-112)rero_ils/modules/acquisition/exceptions.py (1)
RolloverError(28-29)
🪛 Ruff (0.14.10)
rero_ils/modules/acquisition/rollover.py
444-444: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (deploy)
- GitHub Check: test (dev)
🔇 Additional comments (11)
rero_ils/modules/serializers/mixins.py (1)
170-170: LGTM: More Pythonic type checking.The change from
type(pids) is listtoisinstance(pids, list)is a good practice that allows list subclasses while maintaining type safety. This aligns with similar improvements across the PR.rero_ils/modules/api.py (6)
131-131: LGTM: More Pythonic type checking.Consistent with the broader refactoring across the PR to use
isinstance()for type checking.
175-176: LGTM: Defensive config copying.Creating a copy before mutation prevents unintended side effects on the shared config dictionary.
252-253: LGTM: Explicit None checking.Changing from
if not id_:toif id_ is None:is more precise and distinguishes None from other falsy values like 0 or empty strings.
340-341: LGTM: Memory-efficient pagination.Using
yield_per(limit)is a SQLAlchemy optimization that streams results efficiently without loading all identifiers into memory at once.Also applies to: 347-348
287-287: LGTM: Consistent type checking.Matches the isinstance() pattern used throughout the PR.
148-151: No behavioral impact from field filtering logic change.The code now uses
if fields is not None:instead ofif fields:, which changes handling of empty lists: previously an empty list would skip field filtering (returning all fields), while now it appliessource(includes=[])(returning no fields). However, all six callers in the codebase pass either explicit non-empty field lists or use the defaultNone, so this change has no practical impact. If the new behavior (empty list → no fields) is intentional, this is semantically more correct and requires no action.rero_ils/modules/acquisition/rollover.py (2)
413-413: LGTM: Efficient batched reindexing implementation.The change to defer reindexing until after all order lines are created is a solid performance optimization:
- Individual order lines are created with
reindex=False(line 436)- Lines are accumulated in
new_order_lines(line 437)- A final reindex pass processes all lines together (lines 445-446)
This reduces index operations from N individual reindexes to a single batch, significantly improving rollover performance. The approach aligns well with the PR objectives for deferred batch reindexing.
Also applies to: 436-437, 445-446
128-136: LGTM: Docstring formatting improvements.The docstring refinements improve readability without changing functionality.
Also applies to: 504-504, 549-551, 584-587, 629-632, 652-654
rero_ils/modules/acquisition/acq_accounts/api.py (2)
304-304: LGTM: Critical monetary precision fix.The addition of rounding to 2 decimals for all
allocated_amountupdates is essential for financial accuracy:
- Prevents floating-point precision errors from accumulating
- Ensures monetary values maintain standard currency precision (cents)
- Applied consistently across all three transfer cases
This directly addresses the PR objective to "round allocated amount when transferring funds between accounts."
Also applies to: 326-326, 338-338, 349-349, 352-352
171-175: LGTM: Docstring formatting refinements.The docstring improvements enhance readability and maintain consistency across the codebase.
Also applies to: 202-203, 233-236, 267-267, 285-285, 379-379
rerowep
force-pushed
the
wep-fix-rollover
branch
from
bb460a6 to
a4f9b32
Compare
|
Changes unknown |
There was a problem hiding this comment.
Pull request overview
This PR addresses acquisition rollover issues and includes several code quality improvements. The main focus is on fixing rollover order line creation and ensuring accurate financial calculations when transferring funds between accounts.
Key changes:
- Optimizes rollover process by deferring order line reindexing until after all lines are created
- Adds rounding to 2 decimal places for allocated amounts during fund transfers to prevent floating-point precision errors
- Refactors code to use
isinstance()instead oftype()comparisons and improves query efficiency withyield_per()
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates dependency versions (filelock 3.20.0→3.20.2, marshmallow 3.26.1→3.26.2, sqlalchemy 2.0.45 with additional wheel files) |
| tests/ui/holdings/test_holdings_item.py | Refactors item deletion loop to collect PIDs first and defer commits for better performance |
| scripts/test | Adds CVE-2025-53000 exception for nbconvert 7.16.6 vulnerability |
| rero_ils/modules/serializers/mixins.py | Changes type check from type() to isinstance() for better Python practices |
| rero_ils/modules/api.py | Multiple improvements: adds .copy() to prevent config mutation, uses isinstance() for type checks, improves None checking, refactors get_records_by_terms() to conditionally apply source filtering, and simplifies get_all_pids()/get_all_ids() using yield_per() |
| rero_ils/modules/acquisition/rollover.py | Optimizes order line migration by deferring reindexing (creates with reindex=False, then bulk reindexes), and reformats multi-line docstrings for readability |
| rero_ils/modules/acquisition/acq_accounts/api.py | Adds round(..., 2) to all allocated amount arithmetic in transfer_fund() to prevent floating-point precision errors in financial calculations, and reformats docstrings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PascalRepond
left a comment
PascalRepond
left a comment
There was a problem hiding this comment.
A test for rollover permissions would be nice.
* Fixes creation of rollover orders lines for not pending orders during rollover. * Rounds allocated amount when transferring funds between accounts. * Closes rero#3996 Co-Authored-by: Peter Weber <[email protected]>
rerowep
force-pushed
the
wep-fix-rollover
branch
from
a4f9b32 to
54cf4e8
Compare
3 participants
