feat: add scheduler based reverse GL creation for cancelled loan records#1236
Open
Nihantra-Patel wants to merge 6 commits into
Open
feat: add scheduler based reverse GL creation for cancelled loan records#1236Nihantra-Patel wants to merge 6 commits into
Nihantra-Patel wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request converts GL entry reversal from synchronous to deferred processing during loan document cancellation. When enabled at the Company level and outside test mode, LoanDemand and LoanInterestAccrual cancellations now set a pending flag instead of immediately reversing GL entries. A new scheduled job running every 30 minutes processes these flagged documents asynchronously, executing the GL reversals with transactional safety and error logging. Two new Company custom fields control queueing behavior per company, and a data migration patch initializes these settings for existing installations. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 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
`@lending/loan_management/doctype/loan_interest_accrual/loan_interest_accrual.py`:
- Around line 148-157: The queued cancellation can post reversal GL entries with
the worker's current date because make_gl_entries computes posting dates with
getdate() at runtime; capture the intended posting/accounting date at cancel
time and pass it to the background job so the reversal uses that fixed date.
Modify the cancel flow that enqueues self.make_gl_entries to compute and pass a
posting_date (or accounting_date) argument (e.g., posting_date=self.posting_date
or frappe.utils.getdate()) and update make_gl_entries to accept and use that
posting_date instead of calling getdate() internally (also ensure the non-queued
branch calls make_gl_entries(cancel=1, posting_date=...) for parity).
In `@lending/loan_management/doctype/loan_repayment/loan_repayment.py`:
- Around line 727-736: The queued background path for cancellation causes
make_reverse_gl_entries to call posting_date=getdate() (worker run date) rather
than the actual cancellation date; update the enqueue call for
self.make_gl_entries (and the synchronous call path) to pass the cancellation
posting date explicitly (e.g., posting_date=self.posting_date or the
cancellation_date variable) and ensure make_gl_entries and its downstream
make_reverse_gl_entries accept and use the posting_date argument instead of
defaulting to getdate(); adjust the signature/ callers of make_gl_entries and
make_reverse_gl_entries accordingly so queued jobs preserve the original
cancellation posting date.
🪄 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
Run ID: b80fb692-f2fe-4539-9b45-9342c6df7c91
📒 Files selected for processing (3)
lending/loan_management/doctype/loan_demand/loan_demand.pylending/loan_management/doctype/loan_interest_accrual/loan_interest_accrual.pylending/loan_management/doctype/loan_repayment/loan_repayment.py
Comment on lines
+105
to
+114
| # Reverse GL Entries update in the background job to avoid timeout issues during demand cancellation | ||
| if not frappe.flags.in_test: | ||
| frappe.enqueue( | ||
| self.make_gl_entries, | ||
| cancel=1, | ||
| queue="long", | ||
| enqueue_after_commit=True, | ||
| ) | ||
| else: | ||
| self.make_gl_entries(cancel=1) |
There was a problem hiding this comment.
Freeze reversal posting date before enqueueing.
Line 107-Line 114 defers GL reversal, but reversal entries use runtime getdate() in cancel flow (for example Line 205 / Line 221 in this file). If the "long" queue is delayed, reversals can land on a later accounting date than the cancellation transaction date.
Suggested direction
+ reversal_posting_date = getdate()
if not frappe.flags.in_test:
frappe.enqueue(
self.make_gl_entries,
cancel=1,
+ reversal_posting_date=reversal_posting_date,
queue="long",
enqueue_after_commit=True,
)
else:
- self.make_gl_entries(cancel=1)
+ self.make_gl_entries(cancel=1, reversal_posting_date=reversal_posting_date)Then thread reversal_posting_date through make_gl_entries/add_gl_entries and use it instead of runtime getdate() for cancel entries.
Comment on lines
+148
to
+157
| # Reverse GL Entries update in the background job to avoid timeout issues during accrual cancellation | ||
| if not frappe.flags.in_test: | ||
| frappe.enqueue( | ||
| self.make_gl_entries, | ||
| cancel=1, | ||
| queue="long", | ||
| enqueue_after_commit=True, | ||
| ) | ||
| else: | ||
| self.make_gl_entries(cancel=1) |
There was a problem hiding this comment.
Asynchronous cancel can post reversal on the wrong accounting date.
Line 149-Line 157 now queues reversal, but cancel-path GL posting dates are computed with runtime getdate() (Line 264 / Line 282 / Line 301 / Line 319). A delayed worker can post reversal entries on a later date than when the document was canceled.
🤖 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
`@lending/loan_management/doctype/loan_interest_accrual/loan_interest_accrual.py`
around lines 148 - 157, The queued cancellation can post reversal GL entries
with the worker's current date because make_gl_entries computes posting dates
with getdate() at runtime; capture the intended posting/accounting date at
cancel time and pass it to the background job so the reversal uses that fixed
date. Modify the cancel flow that enqueues self.make_gl_entries to compute and
pass a posting_date (or accounting_date) argument (e.g.,
posting_date=self.posting_date or frappe.utils.getdate()) and update
make_gl_entries to accept and use that posting_date instead of calling getdate()
internally (also ensure the non-queued branch calls make_gl_entries(cancel=1,
posting_date=...) for parity).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1236 +/- ##
===========================================
- Coverage 76.37% 76.25% -0.13%
===========================================
Files 142 142
Lines 10207 10234 +27
===========================================
+ Hits 7796 7804 +8
- Misses 2411 2430 +19
🚀 New features to boost your workflow:
|
Nihantra-Patel
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lending/loan_management/utils.py (1)
372-381: ⚡ Quick winAdd scheduler-path tests for pending-flag lifecycle and retry behavior.
Given the new async flow, please add tests for: success clears
cancel_gl_pending, failure keeps it set, and repeated runs don’t double-post reversals.Also applies to: 384-408
🤖 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 `@lending/loan_management/utils.py` around lines 372 - 381, Add scheduler-path unit tests around process_cancelled_gl_entries and the underlying process_cancelled_documents flow that validate the cancel_gl_pending lifecycle and idempotence: write three tests per doctype ("Loan Demand" and "Loan Interest Accrual") that (1) simulate a successful GL reversal (mock the GL-posting call used by process_cancelled_documents) and assert cancel_gl_pending is cleared after running process_cancelled_gl_entries, (2) simulate a failing GL reversal (mock to raise/return error) and assert cancel_gl_pending remains set, and (3) run the scheduler twice with a successful mock and assert the GL reversal posting function is only invoked once (no double-post), using the real process_cancelled_documents/process_cancelled_gl_entries calls and checking the record's cancel_gl_pending flag before/after. Ensure mocks target the GL-posting helper used by process_cancelled_documents so tests cover both "Loan Demand" and "Loan Interest Accrual" paths.
🤖 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 `@lending/loan_management/utils.py`:
- Around line 386-399: The loop currently reads rows and processes reversals
without claiming them, allowing concurrent workers to process the same document;
change the flow to atomically claim a pending reversal before doing work: for
each row (row.name) perform an atomic UPDATE that sets cancel_gl_pending from 1
to a "claimed" value (e.g. 2) only when cancel_gl_pending = 1 and check the
affected row count, and only if affected_rows == 1 load the doc
(frappe.get_doc), call document.make_gl_entries(cancel=1), then set
cancel_gl_pending to 0 (document.db_set) and commit; this ensures
document.make_gl_entries is executed by a single worker and prevents duplicate
reversal postings.
---
Nitpick comments:
In `@lending/loan_management/utils.py`:
- Around line 372-381: Add scheduler-path unit tests around
process_cancelled_gl_entries and the underlying process_cancelled_documents flow
that validate the cancel_gl_pending lifecycle and idempotence: write three tests
per doctype ("Loan Demand" and "Loan Interest Accrual") that (1) simulate a
successful GL reversal (mock the GL-posting call used by
process_cancelled_documents) and assert cancel_gl_pending is cleared after
running process_cancelled_gl_entries, (2) simulate a failing GL reversal (mock
to raise/return error) and assert cancel_gl_pending remains set, and (3) run the
scheduler twice with a successful mock and assert the GL reversal posting
function is only invoked once (no double-post), using the real
process_cancelled_documents/process_cancelled_gl_entries calls and checking the
record's cancel_gl_pending flag before/after. Ensure mocks target the GL-posting
helper used by process_cancelled_documents so tests cover both "Loan Demand" and
"Loan Interest Accrual" paths.
🪄 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
Run ID: c953ccdc-cd9a-4665-9c25-57332823d9cb
📒 Files selected for processing (9)
lending/hooks.pylending/install.pylending/loan_management/doctype/loan_demand/loan_demand.jsonlending/loan_management/doctype/loan_demand/loan_demand.pylending/loan_management/doctype/loan_interest_accrual/loan_interest_accrual.jsonlending/loan_management/doctype/loan_interest_accrual/loan_interest_accrual.pylending/loan_management/utils.pylending/patches.txtlending/patches/v16_0/add_cancel_gl_queue_settings_in_company.py
✅ Files skipped from review due to trivial changes (1)
- lending/patches.txt
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.
Implemented optional scheduler-based reverse GL Entry creation for cancelled:
What’s Added
Added new Company level settings:
These settings control whether reverse GL Entries should be created immediately or through background scheduler jobs.
Added new field in:
Field:
This field is used to track records whose reverse GL Entry creation is pending in background jobs.
Flow Changes
When cancellation happens:
If queue setting is disabled
If queue setting is enabled
Cancel GL Pendingis set to checked.Scheduler Job
Cancel GL Pendingafter successful processingPatches Added
Added patches to create custom fields
Summary by CodeRabbit
New Features
Chores