feat: add pre-submit credit limit warning on save#54461
Open
Jatin3128 wants to merge 2 commits intofrappe:developfrom
Open
feat: add pre-submit credit limit warning on save#54461Jatin3128 wants to merge 2 commits intofrappe:developfrom
Jatin3128 wants to merge 2 commits intofrappe:developfrom
Conversation
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
bace8c1 to
53f25c4
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Detailed notes
🚥 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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
erpnext/accounts/test/test_pre_submit_validation.py (1)
6-6: Add coverage for the public preview-validation path.These tests call
_check_credit_limit_warn()directly, so they don’t verify thatpre_submit_validation()respectspreview_mode,docstatus, and the company/config guards used by the actual hook. Add at least one positive and one disabled-setting test throughpre_submit_validation().Example coverage for the hook entry point
-from erpnext.accounts.utils import _check_credit_limit_warn +from erpnext.accounts.utils import _check_credit_limit_warn, pre_submit_validation @@ def setUp(self): + self._preview_mode = frappe.db.get_single_value("Accounts Settings", "preview_mode") _make_customer(self.CUSTOMER) set_credit_limit(self.CUSTOMER, COMPANY, CREDIT_LIMIT) frappe.message_log.clear() def tearDown(self): + frappe.db.set_single_value("Accounts Settings", "preview_mode", self._preview_mode) frappe.db.delete("Customer Credit Limit", {"parent": self.CUSTOMER}) frappe.message_log.clear() @@ def test_warns_when_amount_exceeds_credit_limit(self): """Orange warning must appear when base_grand_total > credit_limit.""" si = self._make_si(OVER) _check_credit_limit_warn(si) self.assertTrue(_get_orange_warnings(), "Expected an orange credit-limit warning") + + def test_pre_submit_validation_respects_preview_mode(self): + si = self._make_si(OVER) + + frappe.db.set_single_value("Accounts Settings", "preview_mode", 0) + pre_submit_validation(si) + self.assertFalse(_get_orange_warnings()) + + frappe.db.set_single_value("Accounts Settings", "preview_mode", 1) + pre_submit_validation(si) + self.assertTrue(_get_orange_warnings())Also applies to: 32-39, 54-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/test/test_pre_submit_validation.py` at line 6, Tests currently call _check_credit_limit_warn() directly instead of exercising the hook entry point; add at least two tests in test_pre_submit_validation.py that call pre_submit_validation() to cover the public preview-validation path: (1) a positive case where preview_mode is enabled and a draft/appropriate docstatus triggers the warning via pre_submit_validation(), ensuring company/config guards (company set and related setting enabled) allow the check, and (2) a disabled-setting case where the company/config guard disables the check and pre_submit_validation() returns/does nothing. Reference the existing helpers/fixtures used in the file and the functions pre_submit_validation and _check_credit_limit_warn to locate where to invoke and assert behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/accounts/utils.py`:
- Around line 2778-2783: The helper _check_prev_docstatus currently catches all
Exception from doc.check_prev_docstatus(), turning programming/db errors into
orange warnings; change it to catch only the validation exception type (e.g.,
frappe.exceptions.ValidationError or frappe.ValidationError used in the
codebase) and show the frappe.msgprint only for that validation error, while
re-raising any other exception so unexpected errors are not downgraded (update
imports if needed and keep the call to doc.check_prev_docstatus() and the
msgprint logic for the validation case).
- Around line 2822-2845: The Delivery Note preview calculation must use
base-currency amounts and still run even when bypass_credit_limit_check is set:
replace uses of d.amount with d.base_amount when computing unlinked_net, compute
extra_amount = (unlinked_base / flt(doc.base_net_total)) *
flt(doc.base_grand_total) (use d.base_amount, doc.base_net_total,
doc.base_grand_total), and ensure the preview check is invoked regardless of
bypass (do not return early when bypass is truthy); call
check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount)
to show the preview warning instead of relying on doc.check_credit_limit which
returns early when bypass is set.
---
Nitpick comments:
In `@erpnext/accounts/test/test_pre_submit_validation.py`:
- Line 6: Tests currently call _check_credit_limit_warn() directly instead of
exercising the hook entry point; add at least two tests in
test_pre_submit_validation.py that call pre_submit_validation() to cover the
public preview-validation path: (1) a positive case where preview_mode is
enabled and a draft/appropriate docstatus triggers the warning via
pre_submit_validation(), ensuring company/config guards (company set and related
setting enabled) allow the check, and (2) a disabled-setting case where the
company/config guard disables the check and pre_submit_validation() returns/does
nothing. Reference the existing helpers/fixtures used in the file and the
functions pre_submit_validation and _check_credit_limit_warn to locate where to
invoke and assert behavior.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 66457cc4-45a8-4a4c-9218-cc67584d4df7
📒 Files selected for processing (5)
erpnext/accounts/doctype/accounts_settings/accounts_settings.jsonerpnext/accounts/doctype/accounts_settings/accounts_settings.pyerpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.pyerpnext/hooks.py
Comment on lines
+2778
to
+2783
| def _check_prev_docstatus(doc): | ||
| try: | ||
| if hasattr(doc, "check_prev_docstatus"): | ||
| doc.check_prev_docstatus() | ||
| except Exception as e: | ||
| frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange") |
Contributor
There was a problem hiding this comment.
Don’t downgrade unexpected exceptions into warnings.
check_prev_docstatus() is expected to raise validation errors for draft linked docs; catching every Exception also hides programming/database errors and lets the save continue with only an orange warning.
Limit the preview warning to validation failures
def _check_prev_docstatus(doc):
- try:
- if hasattr(doc, "check_prev_docstatus"):
+ if hasattr(doc, "check_prev_docstatus"):
+ try:
doc.check_prev_docstatus()
- except Exception as e:
- frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")
+ except frappe.ValidationError as e:
+ frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")🧰 Tools
🪛 Ruff (0.15.10)
[warning] 2782-2782: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/utils.py` around lines 2778 - 2783, The helper
_check_prev_docstatus currently catches all Exception from
doc.check_prev_docstatus(), turning programming/db errors into orange warnings;
change it to catch only the validation exception type (e.g.,
frappe.exceptions.ValidationError or frappe.ValidationError used in the
codebase) and show the frappe.msgprint only for that validation error, while
re-raising any other exception so unexpected errors are not downgraded (update
imports if needed and keep the call to doc.check_prev_docstatus() and the
msgprint logic for the validation case).
53f25c4 to
eda0ad8
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
erpnext/accounts/utils.py (2)
2778-2783:⚠️ Potential issue | 🟠 MajorDon’t downgrade unexpected previous-document failures into warnings.
Catching
Exceptionlets programming/database errors continue as orange warnings. Only expected validation failures should be converted to pre-submit warnings.🛡️ Limit warning conversion to validation errors
def _check_prev_docstatus(doc): + if not hasattr(doc, "check_prev_docstatus"): + return + try: - if hasattr(doc, "check_prev_docstatus"): - doc.check_prev_docstatus() - except Exception as e: + doc.check_prev_docstatus() + except frappe.ValidationError as e: frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2778 - 2783, The helper _check_prev_docstatus currently catches all Exception types and converts them into orange pre-submit warnings; change this so only expected validation failures are treated as warnings: call doc.check_prev_docstatus() inside try, catch only the specific validation exception used by Frappe (frappe.exceptions.ValidationError / frappe.ValidationError) and show the msgprint for that error, and let any other exception propagate (re-raise) so programming or DB errors are not downgraded; update the except clause in _check_prev_docstatus to target the validation error class used in the project and avoid a broad except Exception.
2822-2845:⚠️ Potential issue | 🟠 MajorUse base amounts for Delivery Note credit exposure.
unlinked_netsums transaction-currencyd.amountbut prorates againstbase_net_total/base_grand_total, which can misstate warnings for multi-currency Delivery Notes.🐛 Compute Delivery Note preview exposure in base currency
elif doc.doctype == "Delivery Note": - if doc.per_billed == 100: + if flt(doc.per_billed) == 100: return bypass = cint( @@ or 0 ) - if bypass: - doc.check_credit_limit() - else: - unlinked = [ - d for d in doc.get("items") if not (d.against_sales_order or d.against_sales_invoice) - ] - if unlinked and flt(doc.base_net_total): - unlinked_net = sum(flt(d.amount) for d in unlinked) - extra_amount = (unlinked_net / flt(doc.base_net_total)) * flt(doc.base_grand_total) - if extra_amount: - check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount) + credit_limit_items = [ + d + for d in doc.get("items") + if not d.against_sales_invoice and (bypass or not d.against_sales_order) + ] + if credit_limit_items and flt(doc.base_net_total): + credit_limit_base_net = sum( + flt(d.get("base_net_amount") or d.get("base_amount")) for d in credit_limit_items + ) + extra_amount = (credit_limit_base_net / flt(doc.base_net_total)) * flt( + doc.base_grand_total + ) + if extra_amount: + check_credit_limit( + doc.customer, + doc.company, + bool(bypass), + extra_amount=extra_amount, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2822 - 2845, The code prorates unlinked item exposure using transaction-currency d.amount against base_net_total/base_grand_total which miscomputes multi-currency Delivery Notes; change the unlinked_net calculation to sum base-currency item amounts (use d.base_amount or the item base-amount field present on Delivery Note items) so extra_amount is computed entirely in base currency before calling check_credit_limit; update the computation that sets unlinked_net and leave the subsequent prorate and check_credit_limit call unchanged (refer to variables/functions: unlinked, unlinked_net, doc.base_net_total, doc.base_grand_total, extra_amount, check_credit_limit, doc.check_credit_limit).
🧹 Nitpick comments (1)
erpnext/accounts/utils.py (1)
2750-2772: Remove or implement the no-op stock preview branch.
_ALWAYS_STOCK_DOCTYPESandneeds_stockcurrently have no behavior, so this reads like unfinished validation logic. Either wire the intended stock preview check here or drop the dead branch for now.♻️ Proposed cleanup if stock preview is not part of this PR
-_ALWAYS_STOCK_DOCTYPES = ("Delivery Note", "Purchase Receipt") - - def pre_submit_validation(doc, method=None): if doc.docstatus != 0: return @@ def _run_pre_submit_checks(doc, cfg): if cfg.get("check_prev_docstatus"): _check_prev_docstatus(doc) - needs_stock = getattr(doc, "update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES - - if needs_stock: - pass - if cfg.get("check_credit_limit"): _check_credit_limit_warn(doc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2750 - 2772, The code contains a no-op branch for stock preview: _ALWAYS_STOCK_DOCTYPES, the needs_stock calculation in _run_pre_submit_checks, and the empty 'if needs_stock: pass' block; either implement the intended stock preview validation or remove the dead code. To fix, decide which approach you want; if you are not adding stock preview now, delete _ALWAYS_STOCK_DOCTYPES and the needs_stock check plus the empty branch from _run_pre_submit_checks (and any unused imports), otherwise implement the actual stock preview check logic inside _run_pre_submit_checks when needs_stock is true (use doc.update_stock and pre_submit_DOCTYPE_CONFIG to drive behavior) and call the appropriate validator functions from pre_submit_validation/_run_pre_submit_checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@erpnext/accounts/utils.py`:
- Around line 2778-2783: The helper _check_prev_docstatus currently catches all
Exception types and converts them into orange pre-submit warnings; change this
so only expected validation failures are treated as warnings: call
doc.check_prev_docstatus() inside try, catch only the specific validation
exception used by Frappe (frappe.exceptions.ValidationError /
frappe.ValidationError) and show the msgprint for that error, and let any other
exception propagate (re-raise) so programming or DB errors are not downgraded;
update the except clause in _check_prev_docstatus to target the validation error
class used in the project and avoid a broad except Exception.
- Around line 2822-2845: The code prorates unlinked item exposure using
transaction-currency d.amount against base_net_total/base_grand_total which
miscomputes multi-currency Delivery Notes; change the unlinked_net calculation
to sum base-currency item amounts (use d.base_amount or the item base-amount
field present on Delivery Note items) so extra_amount is computed entirely in
base currency before calling check_credit_limit; update the computation that
sets unlinked_net and leave the subsequent prorate and check_credit_limit call
unchanged (refer to variables/functions: unlinked, unlinked_net,
doc.base_net_total, doc.base_grand_total, extra_amount, check_credit_limit,
doc.check_credit_limit).
---
Nitpick comments:
In `@erpnext/accounts/utils.py`:
- Around line 2750-2772: The code contains a no-op branch for stock preview:
_ALWAYS_STOCK_DOCTYPES, the needs_stock calculation in _run_pre_submit_checks,
and the empty 'if needs_stock: pass' block; either implement the intended stock
preview validation or remove the dead code. To fix, decide which approach you
want; if you are not adding stock preview now, delete _ALWAYS_STOCK_DOCTYPES and
the needs_stock check plus the empty branch from _run_pre_submit_checks (and any
unused imports), otherwise implement the actual stock preview check logic inside
_run_pre_submit_checks when needs_stock is true (use doc.update_stock and
pre_submit_DOCTYPE_CONFIG to drive behavior) and call the appropriate validator
functions from pre_submit_validation/_run_pre_submit_checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8e4956fa-233d-4966-86b8-772e70a44c2e
📒 Files selected for processing (5)
erpnext/accounts/doctype/accounts_settings/accounts_settings.jsonerpnext/accounts/doctype/accounts_settings/accounts_settings.pyerpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.pyerpnext/hooks.py
✅ Files skipped from review due to trivial changes (1)
- erpnext/accounts/doctype/accounts_settings/accounts_settings.py
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
eda0ad8 to
8e786a0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
erpnext/accounts/utils.py (2)
2822-2845:⚠️ Potential issue | 🟠 MajorFix Delivery Note bypass logic and per_billed comparison.
Two issues remain:
Line 2823: The
per_billedcomparison should useflt()to handle potential null/empty values consistently with other percentage fields.Lines 2835-2845: When
bypass_credit_limit_checkis set, callingdoc.check_credit_limit()(line 2836) returns early in the Delivery Note controller without raising an exception, so the preview warning is skipped entirely. The logic should compute the appropriate item subset based onbypass, then always call the standalonecheck_credit_limitfunction with thebypassparameter andextra_amount.🔧 Proposed fix
elif doc.doctype == "Delivery Note": - if doc.per_billed == 100: + if flt(doc.per_billed) == 100: return bypass = cint( frappe.db.get_value( "Customer Credit Limit", filters={"parent": doc.customer, "parenttype": "Customer", "company": doc.company}, fieldname="bypass_credit_limit_check", ) or 0 ) if bypass: - doc.check_credit_limit() + credit_limit_items = [d for d in doc.get("items") if not d.against_sales_invoice] else: - unlinked = [ + credit_limit_items = [ d for d in doc.get("items") if not (d.against_sales_order or d.against_sales_invoice) ] - if unlinked and flt(doc.base_net_total): - unlinked_net = sum(flt(d.base_amount) for d in unlinked) - extra_amount = (unlinked_net / flt(doc.base_net_total)) * flt(doc.base_grand_total) - if extra_amount: - check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount) + + if credit_limit_items and flt(doc.base_net_total): + credit_limit_base_net = sum(flt(d.base_amount) for d in credit_limit_items) + extra_amount = (credit_limit_base_net / flt(doc.base_net_total)) * flt(doc.base_grand_total) + if extra_amount: + check_credit_limit(doc.customer, doc.company, bypass, extra_amount=extra_amount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2822 - 2845, The per_billed check should use flt(doc.per_billed) instead of direct comparison to handle null/empty values, and the delivery note bypass logic must not call doc.check_credit_limit() (which returns early) when bypass is set; instead compute the unlinked items and extra_amount as currently done, then always call the standalone check_credit_limit(customer, company, bypass, extra_amount=extra_amount) passing the bypass flag and computed extra_amount so the preview warning runs; update the block around the bypass variable, doc.check_credit_limit(), unlinked/unlinked_net/extra_amount calculations to use flt() for numeric casts and call check_credit_limit(...) with bypass and extra_amount.
2778-2783:⚠️ Potential issue | 🟠 MajorDon't downgrade unexpected exceptions into warnings.
Catching all
Exceptiontypes hides programming/database errors and lets the save continue with only an orange warning. Limit the catch tofrappe.ValidationErrorso that only expected validation failures are downgraded to warnings.🔧 Proposed fix
def _check_prev_docstatus(doc): - try: - if hasattr(doc, "check_prev_docstatus"): + if hasattr(doc, "check_prev_docstatus"): + try: doc.check_prev_docstatus() - except Exception as e: - frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange") + except frappe.ValidationError as e: + frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2778 - 2783, The current _check_prev_docstatus function catches all Exception and downgrades every error to a warning; change the exception handling to only catch frappe.ValidationError (or the framework's specific validation exception) when calling doc.check_prev_docstatus(), showing the message as an orange warning, and let any other exceptions propagate (re-raise) so programming/database errors are not suppressed; update the except clause that references doc.check_prev_docstatus() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@erpnext/accounts/utils.py`:
- Around line 2822-2845: The per_billed check should use flt(doc.per_billed)
instead of direct comparison to handle null/empty values, and the delivery note
bypass logic must not call doc.check_credit_limit() (which returns early) when
bypass is set; instead compute the unlinked items and extra_amount as currently
done, then always call the standalone check_credit_limit(customer, company,
bypass, extra_amount=extra_amount) passing the bypass flag and computed
extra_amount so the preview warning runs; update the block around the bypass
variable, doc.check_credit_limit(), unlinked/unlinked_net/extra_amount
calculations to use flt() for numeric casts and call check_credit_limit(...)
with bypass and extra_amount.
- Around line 2778-2783: The current _check_prev_docstatus function catches all
Exception and downgrades every error to a warning; change the exception handling
to only catch frappe.ValidationError (or the framework's specific validation
exception) when calling doc.check_prev_docstatus(), showing the message as an
orange warning, and let any other exceptions propagate (re-raise) so
programming/database errors are not suppressed; update the except clause that
references doc.check_prev_docstatus() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ea72d21a-7a00-43fc-b1f2-a676e157844f
📒 Files selected for processing (5)
erpnext/accounts/doctype/accounts_settings/accounts_settings.jsonerpnext/accounts/doctype/accounts_settings/accounts_settings.pyerpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.pyerpnext/hooks.py
✅ Files skipped from review due to trivial changes (2)
- erpnext/accounts/doctype/accounts_settings/accounts_settings.py
- erpnext/accounts/test/test_pre_submit_validation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- erpnext/hooks.py
- erpnext/accounts/doctype/accounts_settings/accounts_settings.json
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
8e786a0 to
16a63a3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
erpnext/accounts/utils.py (3)
2729-2748: Renamepre_submit_DOCTYPE_CONFIGto follow PEP 8 constant casing.The mixed-case identifier is unusual and inconsistent with surrounding constants (e.g.
GL_REPOSTING_CHUNK,_ALWAYS_STOCK_DOCTYPES,OUTSTANDING_DOCTYPES). PreferPRE_SUBMIT_DOCTYPE_CONFIG(module-private_PRE_SUBMIT_DOCTYPE_CONFIGif not intended for external use).♻️ Proposed rename
-pre_submit_DOCTYPE_CONFIG = { +PRE_SUBMIT_DOCTYPE_CONFIG = { "Sales Invoice": { @@ } @@ - cfg = pre_submit_DOCTYPE_CONFIG.get(doc.doctype) + cfg = PRE_SUBMIT_DOCTYPE_CONFIG.get(doc.doctype)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2729 - 2748, The identifier pre_submit_DOCTYPE_CONFIG should be renamed to follow PEP 8 constant naming (e.g., PRE_SUBMIT_DOCTYPE_CONFIG or _PRE_SUBMIT_DOCTYPE_CONFIG if meant to be module-private); change the variable name declaration and update every reference to pre_submit_DOCTYPE_CONFIG throughout the module (functions, imports, and uses) to the new name to ensure consistency and avoid breaking references, keeping the existing dictionary contents unchanged.
2769-2772: Deadneeds_stockbranch.
needs_stockis computed and then guards an emptypass. Either drop it until there is a real check to run, or add aTODOdescribing the intended stock validation — as written it only adds noise and a stray attribute lookup on every validate.♻️ Suggested cleanup
- needs_stock = getattr(doc, "update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES - - if needs_stock: - pass - if cfg.get("check_credit_limit"): _check_credit_limit_warn(doc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2769 - 2772, The computed variable needs_stock (from getattr(doc, "update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES) guards an empty branch — remove the dead branch or make the intent explicit: either delete the needs_stock calculation and the if-block entirely to avoid the unnecessary getattr call, or replace the pass with a TODO comment describing the intended stock validation and why/when it should run (e.g., validate stock levels when needs_stock is true), keeping the same condition using needs_stock so callers and future work can find the spot; reference needs_stock, doc.update_stock and _ALWAYS_STOCK_DOCTYPES when making the change.
2757-2758: Usefrappe.db.get_single_valuefor Single DocType reads.
frappe.get_cached_value("Accounts Settings", None, "preview_mode")reads a Single via a generic method. The idiomatic and explicit API for Singles isfrappe.db.get_single_value("Accounts Settings", "preview_mode"), which is consistently used throughout the codebase (startup/boot.py, controllers, utilities modules, and patches). Using the canonical API keeps caching behavior predictable.♻️ Proposed change
- if not frappe.get_cached_value("Accounts Settings", None, "preview_mode"): + if not frappe.db.get_single_value("Accounts Settings", "preview_mode"): return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2757 - 2758, Replace the generic single-doc read using frappe.get_cached_value with the canonical Single API: change the condition that checks preview_mode (currently using frappe.get_cached_value("Accounts Settings", None, "preview_mode")) to use frappe.db.get_single_value("Accounts Settings", "preview_mode") so the code reads the single doctype via the explicit Single API (keep the same boolean check logic around "preview_mode" in erpnext.accounts.utils).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@erpnext/accounts/utils.py`:
- Around line 2729-2748: The identifier pre_submit_DOCTYPE_CONFIG should be
renamed to follow PEP 8 constant naming (e.g., PRE_SUBMIT_DOCTYPE_CONFIG or
_PRE_SUBMIT_DOCTYPE_CONFIG if meant to be module-private); change the variable
name declaration and update every reference to pre_submit_DOCTYPE_CONFIG
throughout the module (functions, imports, and uses) to the new name to ensure
consistency and avoid breaking references, keeping the existing dictionary
contents unchanged.
- Around line 2769-2772: The computed variable needs_stock (from getattr(doc,
"update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES) guards an empty
branch — remove the dead branch or make the intent explicit: either delete the
needs_stock calculation and the if-block entirely to avoid the unnecessary
getattr call, or replace the pass with a TODO comment describing the intended
stock validation and why/when it should run (e.g., validate stock levels when
needs_stock is true), keeping the same condition using needs_stock so callers
and future work can find the spot; reference needs_stock, doc.update_stock and
_ALWAYS_STOCK_DOCTYPES when making the change.
- Around line 2757-2758: Replace the generic single-doc read using
frappe.get_cached_value with the canonical Single API: change the condition that
checks preview_mode (currently using frappe.get_cached_value("Accounts
Settings", None, "preview_mode")) to use frappe.db.get_single_value("Accounts
Settings", "preview_mode") so the code reads the single doctype via the explicit
Single API (keep the same boolean check logic around "preview_mode" in
erpnext.accounts.utils).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5eba6ba0-ec2a-4ae6-99d7-de84da94d75b
📒 Files selected for processing (5)
erpnext/accounts/doctype/accounts_settings/accounts_settings.jsonerpnext/accounts/doctype/accounts_settings/accounts_settings.pyerpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.pyerpnext/hooks.py
✅ Files skipped from review due to trivial changes (2)
- erpnext/accounts/doctype/accounts_settings/accounts_settings.py
- erpnext/accounts/doctype/accounts_settings/accounts_settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/test/test_pre_submit_validation.py
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54461 +/- ##
===========================================
+ Coverage 79.44% 79.47% +0.02%
===========================================
Files 1160 1161 +1
Lines 126064 126348 +284
===========================================
+ Hits 100156 100411 +255
- Misses 25908 25937 +29
🚀 New features to boost your workflow:
|
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
16a63a3 to
a8690d5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
erpnext/accounts/utils.py (2)
2815-2829:⚠️ Potential issue | 🟠 MajorDelivery Note bypass branch still silently skips the warning.
When
bypass_credit_limit_checkis set,doc.check_credit_limit()in the Delivery Note controller returns early, so the preview produces no warning — which is exactly the scenario where users need to see the exposure. Computeextra_amountin base currency and route throughcheck_credit_limit(..., ignore_outstanding_sales_order=bypass, extra_amount=...)for both branches instead of short-circuiting todoc.check_credit_limit().♻️ Proposed fix
elif doc.doctype == "Delivery Note": - if doc.per_billed == 100: + if flt(doc.per_billed) == 100: return - if bypass: - doc.check_credit_limit() - else: - unlinked = [ - d for d in doc.get("items") if not (d.against_sales_order or d.against_sales_invoice) - ] - if unlinked and flt(doc.base_net_total): - unlinked_net = sum(flt(d.base_amount) for d in unlinked) - extra_amount = (unlinked_net / flt(doc.base_net_total)) * flt(doc.base_grand_total) - if extra_amount: - check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount) + if bypass: + items_for_check = [d for d in doc.get("items") if not d.against_sales_invoice] + else: + items_for_check = [ + d for d in doc.get("items") if not (d.against_sales_order or d.against_sales_invoice) + ] + + if items_for_check and flt(doc.base_net_total): + unlinked_net = sum(flt(d.base_amount) for d in items_for_check) + extra_amount = (unlinked_net / flt(doc.base_net_total)) * flt(doc.base_grand_total) + if extra_amount: + check_credit_limit( + doc.customer, doc.company, bool(bypass), extra_amount=extra_amount + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2815 - 2829, The Delivery Note branch currently short-circuits to doc.check_credit_limit() when bypass is true, hiding preview warnings; instead compute extra_amount in base currency (use base_amount/base_net_total/base_grand_total) from the unlinked items and call check_credit_limit(customer, company, False, ignore_outstanding_sales_order=bypass, extra_amount=extra_amount) in both bypass and non-bypass paths so the exposure is evaluated and the warning shown; locate the Delivery Note handling (doc.doctype == "Delivery Note"), the unlinked calculation using d.base_amount/base_net_total/base_grand_total, and replace the doc.check_credit_limit() short-circuit with the unified check_credit_limit(...) call including ignore_outstanding_sales_order=bypass.
2778-2783:⚠️ Potential issue | 🟠 MajorNarrow the blind
except Exceptiontofrappe.ValidationError.Catching bare
Exceptionconverts programming/DB errors into an orange preview warning and lets the save silently succeed. Restrict to the validation type this helper is designed to downgrade. Also flagged by Ruff (BLE001) at line 2782.🛡️ Proposed fix
def _check_prev_docstatus(doc): - try: - if hasattr(doc, "check_prev_docstatus"): + if hasattr(doc, "check_prev_docstatus"): + try: doc.check_prev_docstatus() - except Exception as e: - frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange") + except frappe.ValidationError as e: + frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2778 - 2783, The helper _check_prev_docstatus currently catches all exceptions and shows them as a pre-submit warning; change the broad "except Exception as e" to only catch frappe.ValidationError so programming/DB errors still propagate while validation errors are downgraded to an orange msgprint. Update the except clause in _check_prev_docstatus to "except frappe.ValidationError as e:" and keep the existing frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange") behavior unchanged.
🧹 Nitpick comments (2)
erpnext/accounts/utils.py (2)
2729-2750: Nit: rename constants to upper-snake-case.
pre_submit_DOCTYPE_CONFIGmixes lower and upper casing. Module-level constants are conventionallyUPPER_SNAKE_CASE(like the adjacent_ALWAYS_STOCK_DOCTYPES,GL_REPOSTING_CHUNK,OUTSTANDING_DOCTYPES). Consider renaming toPRE_SUBMIT_DOCTYPE_CONFIGfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2729 - 2750, Rename the module-level constant pre_submit_DOCTYPE_CONFIG to PRE_SUBMIT_DOCTYPE_CONFIG to follow upper-snake-case convention; update its definition and replace all references/usages in this module (and any imports from other modules) to the new name so nothing breaks, and ensure any adjacent constants like _ALWAYS_STOCK_DOCTYPES remain unchanged; run tests/lint to verify no remaining references to pre_submit_DOCTYPE_CONFIG.
2753-2762: Verifypreview_modelookup and considerfrappe.get_cached_valuealternative.Using
frappe.get_cached_value("Accounts Settings", None, "preview_mode")withNonefor a Single DocType relies on Frappe's single-doctype fallback;frappe.db.get_single_value("Accounts Settings", "preview_mode")is the more idiomatic read here and avoids any ambiguity. Also note thatvalidatefires on both draft save and on submit (whiledocstatusis still0), so the same preview warnings will emit again during the submit click — acceptable, but worth confirming the UX is desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2753 - 2762, Replace the ambiguous single-DocType lookup in pre_submit_validation by using the single-value API: in pre_submit_validation (and any related callers) replace frappe.get_cached_value("Accounts Settings", None, "preview_mode") with frappe.db.get_single_value("Accounts Settings", "preview_mode") to read the preview_mode setting unambiguously; also verify that the preview warnings emitted by _run_pre_submit_checks when doc.docstatus == 0 are acceptable during both draft saves and the submit action (adjust call-site logic if you want to suppress warnings only on the submit click).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/accounts/utils.py`:
- Around line 2765-2775: The computed needs_stock in _run_pre_submit_checks is a
no-op (the branch contains only pass); remove the dead variable and branch to
keep the dispatcher clean (also remove or defer _ALWAYS_STOCK_DOCTYPES if it's
now unused), or if a stock validation was intended, replace the pass with the
actual check call (e.g., call the intended stock check function such as
_check_stock_preview/_check_stock_availability with doc) and add a TODO with an
issue reference so the work isn't lost; update or remove any related unused
symbols accordingly.
---
Duplicate comments:
In `@erpnext/accounts/utils.py`:
- Around line 2815-2829: The Delivery Note branch currently short-circuits to
doc.check_credit_limit() when bypass is true, hiding preview warnings; instead
compute extra_amount in base currency (use
base_amount/base_net_total/base_grand_total) from the unlinked items and call
check_credit_limit(customer, company, False,
ignore_outstanding_sales_order=bypass, extra_amount=extra_amount) in both bypass
and non-bypass paths so the exposure is evaluated and the warning shown; locate
the Delivery Note handling (doc.doctype == "Delivery Note"), the unlinked
calculation using d.base_amount/base_net_total/base_grand_total, and replace the
doc.check_credit_limit() short-circuit with the unified check_credit_limit(...)
call including ignore_outstanding_sales_order=bypass.
- Around line 2778-2783: The helper _check_prev_docstatus currently catches all
exceptions and shows them as a pre-submit warning; change the broad "except
Exception as e" to only catch frappe.ValidationError so programming/DB errors
still propagate while validation errors are downgraded to an orange msgprint.
Update the except clause in _check_prev_docstatus to "except
frappe.ValidationError as e:" and keep the existing frappe.msgprint(str(e),
title=_("Pre-Submit Warning"), indicator="orange") behavior unchanged.
---
Nitpick comments:
In `@erpnext/accounts/utils.py`:
- Around line 2729-2750: Rename the module-level constant
pre_submit_DOCTYPE_CONFIG to PRE_SUBMIT_DOCTYPE_CONFIG to follow
upper-snake-case convention; update its definition and replace all
references/usages in this module (and any imports from other modules) to the new
name so nothing breaks, and ensure any adjacent constants like
_ALWAYS_STOCK_DOCTYPES remain unchanged; run tests/lint to verify no remaining
references to pre_submit_DOCTYPE_CONFIG.
- Around line 2753-2762: Replace the ambiguous single-DocType lookup in
pre_submit_validation by using the single-value API: in pre_submit_validation
(and any related callers) replace frappe.get_cached_value("Accounts Settings",
None, "preview_mode") with frappe.db.get_single_value("Accounts Settings",
"preview_mode") to read the preview_mode setting unambiguously; also verify that
the preview warnings emitted by _run_pre_submit_checks when doc.docstatus == 0
are acceptable during both draft saves and the submit action (adjust call-site
logic if you want to suppress warnings only on the submit click).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0194718c-c43b-4ad2-a00f-b7cd5ae4fb15
📒 Files selected for processing (5)
erpnext/accounts/doctype/accounts_settings/accounts_settings.jsonerpnext/accounts/doctype/accounts_settings/accounts_settings.pyerpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.pyerpnext/hooks.py
✅ Files skipped from review due to trivial changes (1)
- erpnext/accounts/doctype/accounts_settings/accounts_settings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- erpnext/accounts/doctype/accounts_settings/accounts_settings.json
- erpnext/accounts/test/test_pre_submit_validation.py
Comment on lines
+2765
to
+2775
| def _run_pre_submit_checks(doc, cfg): | ||
| if cfg.get("check_prev_docstatus"): | ||
| _check_prev_docstatus(doc) | ||
|
|
||
| needs_stock = getattr(doc, "update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES | ||
|
|
||
| if needs_stock: | ||
| pass | ||
|
|
||
| if cfg.get("check_credit_limit"): | ||
| _check_credit_limit_warn(doc) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dead needs_stock branch or implement the intended check.
needs_stock is computed but the body is just pass, so it has no effect. Either drop the variable/branch now to keep the dispatcher clean, or wire in the actual stock-preview check this was meant for.
♻️ Suggested cleanup
def _run_pre_submit_checks(doc, cfg):
if cfg.get("check_prev_docstatus"):
_check_prev_docstatus(doc)
- needs_stock = getattr(doc, "update_stock", False) or doc.doctype in _ALWAYS_STOCK_DOCTYPES
-
- if needs_stock:
- pass
-
if cfg.get("check_credit_limit"):
_check_credit_limit_warn(doc)If the stock check is intended to land later, consider gating it behind a TODO + issue reference instead of leaving a no-op (and _ALWAYS_STOCK_DOCTYPES can then also be removed until it's used).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/utils.py` around lines 2765 - 2775, The computed needs_stock
in _run_pre_submit_checks is a no-op (the branch contains only pass); remove the
dead variable and branch to keep the dispatcher clean (also remove or defer
_ALWAYS_STOCK_DOCTYPES if it's now unused), or if a stock validation was
intended, replace the pass with the actual check call (e.g., call the intended
stock check function such as _check_stock_preview/_check_stock_availability with
doc) and add a TODO with an issue reference so the work isn't lost; update or
remove any related unused symbols accordingly.
|
|
||
| def _check_prev_docstatus(doc): | ||
| try: | ||
| if hasattr(doc, "check_prev_docstatus"): |
Member
There was a problem hiding this comment.
Reduce extensive use of hasattr, getattr. if doc is mostly a Document type just use . notation for fields.
Contributor
Author
There was a problem hiding this comment.
i have used doc.get() for the conditions is that fine?
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
a8690d5 to
4e01eb9
Compare
Jatin3128
force-pushed
the
CL_pre_submit
branch
from
4e01eb9 to
26d3a25
Compare
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.
Runs credit limit validation on every save for Sales Invoice,
Sales Order and Delivery Note so users see a warning before they
hit submit.
no-docs