fix: handle missing repayment schedule in advance payment validation by Hemil-Sangani · Pull Request #1243 · frappe/lending

Hemil-Sangani · 2026-05-26T06:52:04Z

Description

Handles cases where no matching Loan Repayment Schedule exists during Advance Payment validation for term loans.

frappe.db.get_value() returns None when no matching repayment schedule is found, which causes invalid numeric comparisons during validation.

This change safely normalizes values using flt() before comparison.

Changes

Wrap monthly_repayment_amount with flt()
Normalize amount_paid using flt()
Prevent NoneType comparison issues during Advance Payment validation

Before

monthly_repayment_amount = frappe.db.get_value(
    "Loan Repayment Schedule",
    filters,
    "monthly_repayment_amount",
)

After

monthly_repayment_amount = flt(
    frappe.db.get_value(
        "Loan Repayment Schedule",
        filters,
        "monthly_repayment_amount",
    ),
    precision,
)

amount_paid = flt(amount_paid, precision)

Closes #1240

Summary by CodeRabbit

Bug Fixes
- Tightened advance loan payment validation: advance payments are now rejected if there’s no active monthly EMI to compare against.
- Validation now consistently rounds the monthly EMI and the entered payment to currency precision before checking that the amount falls between 1× and 2× the EMI.

coderabbitai · 2026-05-26T06:54:30Z

Walkthrough

The PR updates validate_advance_payment to fail if no active monthly_repayment_amount is found, round monthly_repayment_amount and amount_paid to currency precision once, and perform the existing check that the rounded advance payment lies between one and two times the rounded EMI.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: handling missing repayment schedule in advance payment validation, which directly addresses the core issue.
Linked Issues check	✅ Passed	The PR implements the proposed fix from issue `#1240` by wrapping monthly_repayment_amount with flt() and normalizing amount_paid to handle NoneType errors during comparisons.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the advance payment validation issue; no unrelated modifications were introduced.

_{✏️ 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 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_repayment/loan_repayment.py`:
- Around line 1727-1741: The code currently converts the DB result directly with
flt which turns None into 0 and leads to misleading validation; change the logic
in loan_repayment.py around the retrieval of "monthly_repayment_amount" so you
first read the raw value (e.g., raw_monthly = frappe.db.get_value("Loan
Repayment Schedule", filters, "monthly_repayment_amount")), check if raw_monthly
is None and if so raise a clear error with frappe.throw (e.g., "Loan Repayment
Schedule not found for selected criteria" or similar), and only then set
monthly_repayment_amount = flt(raw_monthly, precision) before performing the
amount_paid bounds check; reference the variables monthly_repayment_amount,
raw_monthly, filters, and amount_paid when making the change.

🪄 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: dfe332c0-e012-4ad2-b4dc-7b72247009e8

📥 Commits

Reviewing files that changed from the base of the PR and between 305337f and ab2244a.

📒 Files selected for processing (1)

lending/loan_management/doctype/loan_repayment/loan_repayment.py

codecov-commenter · 2026-05-27T09:07:40Z

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.45%. Comparing base (4cef4b2) to head (8d0446d).

Files with missing lines	Patch %	Lines
...anagement/doctype/loan_repayment/loan_repayment.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1243      +/-   ##
===========================================
- Coverage    76.45%   76.45%   -0.01%     
===========================================
  Files          142      142              
  Lines        10241    10245       +4     
===========================================
+ Hits          7830     7833       +3     
- Misses        2411     2412       +1

Files with missing lines	Coverage Δ
...anagement/doctype/loan_repayment/loan_repayment.py	`81.48% <80.00%> (-0.02%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…tion

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (2)

lending/loan_management/doctype/loan_repayment/loan_repayment.py (2)

2368-2368: ⚠️ Potential issue | 🟡 Minor | 💤 Low value

Principal Capitalization payment account mapping added.

Consistent with other capitalization types (Interest, Penalty, Charges) that also map to "loan_account", but not documented in the PR objectives.

🤖 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_repayment/loan_repayment.py` at line
2368, The new mapping entry "Principal Capitalization": "loan_account" was added
to the payment account map in loan_repayment.py but was not documented in the PR
objectives; confirm this is intentional and either add/update the PR
description/changelog to include this mapping or remove the entry if unintended.
Locate the payment account mapping dictionary in loan_repayment.py (the mapping
that currently contains "Principal Capitalization": "loan_account") and either
(a) keep the mapping and add a brief inline comment explaining why "Principal
Capitalization" maps to "loan_account" and update the PR description/changelog
to mention the change, or (b) remove the mapping if it was added by mistake and
revert to the prior set of capitalization types.

2039-2041: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Additional Principal Capitalization logic not documented in PR.

This guard prevents GL entries for Principal Capitalization during loan restructure when no sales invoices are present in repayment details. Like the change at lines 127-134, this Principal Capitalization functionality is not mentioned in the PR objectives.

🤖 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_repayment/loan_repayment.py` around
lines 2039 - 2041, The new conditional in loan_repayment.py (checking
self.repayment_type == "Principal Capitalization" and self.loan_restructure and
not any(d.sales_invoice for d in self.get("repayment_details"))) introduces
undocumented behavior that suppresses GL entries for Principal Capitalization
during restructures; update the code and PR by either removing or adjusting this
guard to match intended business logic in the loan repayment flow, or if the
guard is required, add an inline comment next to this conditional referencing
the business rule and add a note to the PR description explaining why Principal
Capitalization GL entries are suppressed when loan_restructure is true and no
repayment_details have sales_invoice (refer to repayment_details and the use of
any(d.sales_invoice for d in self.get("repayment_details")) for locating the
check).

🧹 Nitpick comments (1)

lending/loan_management/doctype/loan_repayment/loan_repayment.py (1)
127-134: ⚡ Quick win

Clarify scope of the “Principal Capitalization” handling in this PR

“Principal Capitalization” is already used across the lending flow (e.g., in lending/loan_management/doctype/loan_adjustment_detail/loan_adjustment_detail.py and lending/loan_management/doctype/loan_restructure/loan_restructure.py when creating loan_repayment).

This PR further extends loan_repayment.py to (1) allow payable_charges for Principal Capitalization during loan_restructure and (2) add a GL-entry guard + map Principal Capitalization to loan_account.

Please ensure this PR (or #1240) explicitly calls out these Principal Capitalization behavior changes, or confirm they should be tracked as a separate item.
🤖 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_repayment/loan_repayment.py` around
lines 127 - 134, The change now allows payable_charges for repayment_type
"Principal Capitalization" when loan_restructure is true and adds GL-entry guard
+ maps "Principal Capitalization" to loan_account in loan_repayment.py; update
the PR description and/or changelog and add an inline comment in
loan_repayment.py near the repayment_type check (the block that references
self.repayment_type, self.get("payable_charges"), and self.loan_restructure) to
explicitly call out that Principal Capitalization behavior has been extended
here (and mention the related behavior in loan_adjustment_detail and
loan_restructure modules), or split this behavioral change into a separate
PR/issue if it should be tracked independently.

🤖 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_repayment/loan_repayment.py`:
- Around line 1744-1752: Add a unit test in
lending/loan_management/doctype/loan_repayment/test_loan_repayment.py that
triggers the advance-payment code path where monthly_repayment_amount is missing
and asserts that the call raises the exact message "Cannot process Advance
Payment: No active Loan Repayment Schedule found for this loan" (use
frappe.raises or pytest.raises as used elsewhere); name it something like
test_advance_payment_no_active_schedule and set up a loan with no active
repayment schedule before calling the advance-payment routine. Also, consider
tightening the guard in loan_repayment.py by changing the check from "if not
monthly_repayment_amount:" to "if monthly_repayment_amount is None:" to avoid
false negatives for zero values.

---

Duplicate comments:
In `@lending/loan_management/doctype/loan_repayment/loan_repayment.py`:
- Line 2368: The new mapping entry "Principal Capitalization": "loan_account"
was added to the payment account map in loan_repayment.py but was not documented
in the PR objectives; confirm this is intentional and either add/update the PR
description/changelog to include this mapping or remove the entry if unintended.
Locate the payment account mapping dictionary in loan_repayment.py (the mapping
that currently contains "Principal Capitalization": "loan_account") and either
(a) keep the mapping and add a brief inline comment explaining why "Principal
Capitalization" maps to "loan_account" and update the PR description/changelog
to mention the change, or (b) remove the mapping if it was added by mistake and
revert to the prior set of capitalization types.
- Around line 2039-2041: The new conditional in loan_repayment.py (checking
self.repayment_type == "Principal Capitalization" and self.loan_restructure and
not any(d.sales_invoice for d in self.get("repayment_details"))) introduces
undocumented behavior that suppresses GL entries for Principal Capitalization
during restructures; update the code and PR by either removing or adjusting this
guard to match intended business logic in the loan repayment flow, or if the
guard is required, add an inline comment next to this conditional referencing
the business rule and add a note to the PR description explaining why Principal
Capitalization GL entries are suppressed when loan_restructure is true and no
repayment_details have sales_invoice (refer to repayment_details and the use of
any(d.sales_invoice for d in self.get("repayment_details")) for locating the
check).

---

Nitpick comments:
In `@lending/loan_management/doctype/loan_repayment/loan_repayment.py`:
- Around line 127-134: The change now allows payable_charges for repayment_type
"Principal Capitalization" when loan_restructure is true and adds GL-entry guard
+ maps "Principal Capitalization" to loan_account in loan_repayment.py; update
the PR description and/or changelog and add an inline comment in
loan_repayment.py near the repayment_type check (the block that references
self.repayment_type, self.get("payable_charges"), and self.loan_restructure) to
explicitly call out that Principal Capitalization behavior has been extended
here (and mention the related behavior in loan_adjustment_detail and
loan_restructure modules), or split this behavioral change into a separate
PR/issue if it should be tracked independently.

🪄 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: fa3712f0-8565-4d46-bf67-4a242381bc87

📥 Commits

Reviewing files that changed from the base of the PR and between 6efcde9 and 7656808.

📒 Files selected for processing (1)

lending/loan_management/doctype/loan_repayment/loan_repayment.py

…ayment schedule exists

Hemil-Sangani · 2026-06-02T14:29:21Z

@Nihantra-Patel Sir, could you please review this when you get a chance?

Hemil-Sangani requested review from Nihantra-Patel and deepeshgarg007 as code owners May 26, 2026 06:52

github-actions Bot added the needs-tests label May 26, 2026

Hemil-Sangani force-pushed the fix/advance-payment-none-validation branch from f5ef3ae to ab2244a Compare May 26, 2026 11:13

coderabbitai Bot reviewed May 26, 2026

View reviewed changes

Comment thread lending/loan_management/doctype/loan_repayment/loan_repayment.py Outdated

Hemil-Sangani added 2 commits June 2, 2026 10:15

fix: handle missing repayment schedule in advance payment validation

25b43d5

fix: handle missing Loan Repayment Schedule in advance payment valida…

7656808

…tion

Hemil-Sangani force-pushed the fix/advance-payment-none-validation branch from 8d0446d to 7656808 Compare June 2, 2026 04:45

coderabbitai Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread lending/loan_management/doctype/loan_repayment/loan_repayment.py

Hemil-Sangani added 2 commits June 2, 2026 16:04

Merge branch 'develop' into fix/advance-payment-none-validation

d498e77

test(loan_repayment): add test for advance payment when no active rep…

ac867bb

…ayment schedule exists

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle missing repayment schedule in advance payment validation#1243

fix: handle missing repayment schedule in advance payment validation#1243
Hemil-Sangani wants to merge 4 commits into
frappe:developfrom
Hemil-Sangani:fix/advance-payment-none-validation

Hemil-Sangani commented May 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 26, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Hemil-Sangani commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Hemil-Sangani commented May 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Before

After

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2026

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hemil-Sangani commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Hemil-Sangani commented May 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 26, 2026 •

edited

Loading