feat: pre-submit validation error for packed quantity mismatch#54541
feat: pre-submit validation error for packed quantity mismatch#54541Jatin3128 wants to merge 2 commits intofrappe:developfrom
Conversation
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
erpnext/accounts/utils.py (2)
2729-2748: Optional: collapsePRE_SUBMIT_DOCTYPE_CONFIGandpre_submit_validation_doctypesinto a single source of truth.The hook list lives in
hooks.py(pre_submit_validation_doctypes) and the per-doctype check matrix lives here. The two must stay in sync manually — adding a new doctype requires editing both files, and there's no guard against drift. Consider deriving the hook list from the keys ofPRE_SUBMIT_DOCTYPE_CONFIG(or vice versa) to prevent silent skew.🤖 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, PRE_SUBMIT_DOCTYPE_CONFIG and the hook list pre_submit_validation_doctypes are duplicated sources of truth; change the code so the hook list is derived from PRE_SUBMIT_DOCTYPE_CONFIG keys (or move PRE_SUBMIT_DOCTYPE_CONFIG into a shared module) to avoid drift. Update hooks.py to import PRE_SUBMIT_DOCTYPE_CONFIG (or its exported keys) and set pre_submit_validation_doctypes = list(PRE_SUBMIT_DOCTYPE_CONFIG.keys()), remove the hard-coded duplicate list, and add a short comment documenting the single source of truth; ensure imports don’t create circular dependencies by relocating the config to a neutral module if necessary.
2782-2832: Semantic mismatch:bypass_credit_limit_checkfield is passed asignore_outstanding_sales_orderparameter.The variable
bypassis read fromCustomer Credit Limit.bypass_credit_limit_checkand passed as the third argument tocheck_credit_limit(customer, company, ignore_outstanding_sales_order, extra_amount). These represent different concepts:
bypass_credit_limit_check→ "skip the credit limit check entirely"ignore_outstanding_sales_order→ "still check, but exclude outstanding SOs from the balance calculation"The condition
validate_against_credit_limit = bypass or any(...)compounds the confusion: it validates whenbypassis true, opposite to what the field name suggests. This is also inconsistent withSalesOrder.check_credit_limit(), which skips the check whenbypass_credit_limit_checkis true.While benign today (credit_limit returns 0 when bypass is set, causing early return), the intent is misleading for maintainers. Decouple the "should we check" logic from
ignore_outstanding_sales_order, and rename the local variable to clarify: reservebypass_credit_limit_checkfor the field value and use a separate flag for whether outstanding SOs should be excluded from the outstanding balance calculation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/utils.py` around lines 2782 - 2832, The code in _check_credit_limit_warn wrongly uses the Customer Credit Limit field bypass_credit_limit_check as the ignore_outstanding_sales_order argument to check_credit_limit and inverts its meaning when deciding whether to validate; fix by reading the field into a clearly named variable (e.g., bypass_credit_limit_check), compute a separate flag for whether to validate (e.g., should_validate = not bypass_credit_limit_check and other conditions) and another flag for ignoring outstanding SOs (e.g., ignore_outstanding_sales_order = any(not (d.sales_order or d.delivery_note) for d in doc.get("items"))), then call check_credit_limit(doc.customer, doc.company, ignore_outstanding_sales_order, extra_amount=...) only when should_validate is true; also ensure for Sales Order and Delivery Note branches you respect bypass_credit_limit_check to skip the check entirely (matching SalesOrder.check_credit_limit behavior).erpnext/accounts/test/test_pre_submit_validation.py (1)
1-274: Coverage gaps: no test for_check_prev_docstatus, no test for thepreview_modegate, and no end-to-end save flow.Given the critical issue flagged in
utils.py(doc.get("check_prev_docstatus")never resolving to the method) and the hook-order concern around packed-qty mismatches, the test suite as written would not catch either regression:
_check_prev_docstatushas zero tests — please add one that asserts the orange warning is raised when an SI references an unsubmitted SO.pre_submit_validation(the public entrypoint) is never tested directly; in particular thepreview_mode == 0short-circuit is untested.- Packed-qty test patches
validate_packed_qtyand calls_check_packed_qty_warndirectly, bypassing the actualdoc.save()path. An integration test that submits a Packing Slip and then saves a partial DN would verify the full hook chain actually reaches the warning code (and would have surfaced the ordering issue mentioned in the utils.py review).Want me to draft these missing test cases?
🤖 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` around lines 1 - 274, Add tests covering _check_prev_docstatus, the preview_mode gate in pre_submit_validation, and an end-to-end save flow for packed-qty: write a unit test that creates a draft Sales Invoice referencing an unsubmitted Sales Order and calls pre_submit_validation (or directly _check_prev_docstatus) to assert an orange warning is produced; add a test that calls pre_submit_validation with a doc having preview_mode=0 and preview_mode=1 to ensure the short-circuit behaves correctly; and add an integration-style test that creates/submits a Packing Slip, then attempts to save a Delivery Note with mismatched packed quantities (so validate_packed_qty runs via doc.save()) to ensure _check_packed_qty_warn is hit in the actual save hook rather than only when patched. Ensure tests exercise the code paths around doc.get("check_prev_docstatus") and the hook ordering so the method resolution and packed-qty warning flow are validated.
🤖 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/test/test_pre_submit_validation.py`:
- Around line 100-109: Two tests, test_warns_on_first_save_when_limit_exceeded
and test_warns_when_amount_exceeds_credit_limit, have identical bodies (both
create an SO with OVER and assert the same orange warning) making one redundant;
remove or merge them. Keep one test (pick either name) and if you want to
preserve the intent of asserting newness retain the is_new() assertion inside
that single test; update or delete the duplicate (references:
test_warns_on_first_save_when_limit_exceeded,
test_warns_when_amount_exceeds_credit_limit, helper _make_so, and assertions
_check_credit_limit_warn/_get_orange_warnings).
- Around line 37-40: The setUp methods in _CreditLimitBase and TestPackedQtyWarn
override the base initializer but do not call super().setUp(), so any test-suite
initialization in ERPNextTestSuite is skipped; update the setUp implementations
(the setUp method shown and any other overrides in
_CreditLimitBase/TestPackedQtyWarn) to begin by calling super().setUp() before
performing _make_customer, set_credit_limit, frappe.message_log.clear() (or
other setup steps) to ensure ERPNextTestSuite initialization runs.
In `@erpnext/accounts/utils.py`:
- Around line 2774-2780: _in _check_prev_docstatus replace the incorrect
doc.get("check_prev_docstatus") gate (which checks a field) with a method
presence/callable check such as hasattr(doc, "check_prev_docstatus") or
callable(getattr(doc, "check_prev_docstatus", None)) so the
doc.check_prev_docstatus() method on relevant doctypes is actually invoked; also
narrow the except clause to catch frappe.ValidationError (instead of Exception)
and keep the existing frappe.msgprint(str(e), title=_("Pre-Submit Warning"),
indicator="orange") handling.
- Around line 2811-2825: The Delivery Note branch is asymmetric and uses a
fragile equality check: replace the direct equality per_billed check with a
float-safe comparison (use flt(doc.per_billed) >= 100) and make the bypass path
behave the same as the non-bypass path by computing and passing the same
proportional extra_amount for unlinked items when calling
doc.check_credit_limit() / check_credit_limit(); ensure you compute unlinked_net
= sum(flt(d.base_amount) for d in doc.get("items") if not (d.against_sales_order
or d.against_sales_invoice)), derive extra_amount = (unlinked_net /
flt(doc.base_net_total)) * flt(doc.base_grand_total) only when
doc.base_net_total is truthy, and call the existing
check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount)
(or doc.check_credit_limit with the same extra_amount) so both bypass and
non-bypass paths are consistent.
- Around line 2835-2844: The pre-submit packed-qty warning never runs by default
because pre_submit_validation() returns early when Accounts
Settings.preview_mode is false, so update the code and tests so the warning
actually fires: either remove the preview_mode gate in pre_submit_validation()
so that _check_packed_qty_warn(doc) is always invoked for doc.docstatus == 0, or
explicitly document/require enabling preview_mode and update the test
test_warns_when_packed_qty_mismatches to enable/patch Accounts
Settings.preview_mode (or call the save flow instead of mocking
validate_packed_qty()) so the integration is exercised; locate and change the
pre_submit_validation function and ensure _check_packed_qty_warn,
validate_packed_qty (delivery_note.py), and the failing test are updated
accordingly.
---
Nitpick comments:
In `@erpnext/accounts/test/test_pre_submit_validation.py`:
- Around line 1-274: Add tests covering _check_prev_docstatus, the preview_mode
gate in pre_submit_validation, and an end-to-end save flow for packed-qty: write
a unit test that creates a draft Sales Invoice referencing an unsubmitted Sales
Order and calls pre_submit_validation (or directly _check_prev_docstatus) to
assert an orange warning is produced; add a test that calls
pre_submit_validation with a doc having preview_mode=0 and preview_mode=1 to
ensure the short-circuit behaves correctly; and add an integration-style test
that creates/submits a Packing Slip, then attempts to save a Delivery Note with
mismatched packed quantities (so validate_packed_qty runs via doc.save()) to
ensure _check_packed_qty_warn is hit in the actual save hook rather than only
when patched. Ensure tests exercise the code paths around
doc.get("check_prev_docstatus") and the hook ordering so the method resolution
and packed-qty warning flow are validated.
In `@erpnext/accounts/utils.py`:
- Around line 2729-2748: PRE_SUBMIT_DOCTYPE_CONFIG and the hook list
pre_submit_validation_doctypes are duplicated sources of truth; change the code
so the hook list is derived from PRE_SUBMIT_DOCTYPE_CONFIG keys (or move
PRE_SUBMIT_DOCTYPE_CONFIG into a shared module) to avoid drift. Update hooks.py
to import PRE_SUBMIT_DOCTYPE_CONFIG (or its exported keys) and set
pre_submit_validation_doctypes = list(PRE_SUBMIT_DOCTYPE_CONFIG.keys()), remove
the hard-coded duplicate list, and add a short comment documenting the single
source of truth; ensure imports don’t create circular dependencies by relocating
the config to a neutral module if necessary.
- Around line 2782-2832: The code in _check_credit_limit_warn wrongly uses the
Customer Credit Limit field bypass_credit_limit_check as the
ignore_outstanding_sales_order argument to check_credit_limit and inverts its
meaning when deciding whether to validate; fix by reading the field into a
clearly named variable (e.g., bypass_credit_limit_check), compute a separate
flag for whether to validate (e.g., should_validate = not
bypass_credit_limit_check and other conditions) and another flag for ignoring
outstanding SOs (e.g., ignore_outstanding_sales_order = any(not (d.sales_order
or d.delivery_note) for d in doc.get("items"))), then call
check_credit_limit(doc.customer, doc.company, ignore_outstanding_sales_order,
extra_amount=...) only when should_validate is true; also ensure for Sales Order
and Delivery Note branches you respect bypass_credit_limit_check to skip the
check entirely (matching SalesOrder.check_credit_limit 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: 2e25be99-32c7-43e0-85fe-91ae52ad27e2
📒 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
| def setUp(self): | ||
| _make_customer(self.CUSTOMER) | ||
| set_credit_limit(self.CUSTOMER, COMPANY, CREDIT_LIMIT) | ||
| frappe.message_log.clear() |
There was a problem hiding this comment.
setUp does not call super().setUp().
_CreditLimitBase.setUp (and TestPackedQtyWarn.setUp) override setUp without invoking ERPNextTestSuite.setUp(). If the base class performs initialization (test data, transaction setup, role context), it will be silently skipped, which can lead to flaky behavior depending on test runner order.
♻️ Proposed fix
class _CreditLimitBase(ERPNextTestSuite):
CUSTOMER = "_Pre Submit Test Customer"
def setUp(self):
+ super().setUp()
_make_customer(self.CUSTOMER)
set_credit_limit(self.CUSTOMER, COMPANY, CREDIT_LIMIT)
frappe.message_log.clear()🤖 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` around lines 37 - 40,
The setUp methods in _CreditLimitBase and TestPackedQtyWarn override the base
initializer but do not call super().setUp(), so any test-suite initialization in
ERPNextTestSuite is skipped; update the setUp implementations (the setUp method
shown and any other overrides in _CreditLimitBase/TestPackedQtyWarn) to begin by
calling super().setUp() before performing _make_customer, set_credit_limit,
frappe.message_log.clear() (or other setup steps) to ensure ERPNextTestSuite
initialization runs.
| def test_warns_on_first_save_when_limit_exceeded(self): | ||
| so = self._make_so(OVER) | ||
| self.assertTrue(so.is_new(), "Doc should be new (not yet in DB)") | ||
| _check_credit_limit_warn(so) | ||
| self.assertTrue(_get_orange_warnings()) | ||
|
|
||
| def test_warns_when_amount_exceeds_credit_limit(self): | ||
| so = self._make_so(OVER) | ||
| _check_credit_limit_warn(so) | ||
| self.assertTrue(_get_orange_warnings()) |
There was a problem hiding this comment.
Duplicate test bodies.
test_warns_on_first_save_when_limit_exceeded (lines 100–104) and test_warns_when_amount_exceeds_credit_limit (lines 106–109) construct the same SO with OVER and assert the same warning. The only difference is the additional is_new() assertion, which is redundant since _make_so always returns an unsaved doc. Drop one or merge.
🤖 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` around lines 100 - 109,
Two tests, test_warns_on_first_save_when_limit_exceeded and
test_warns_when_amount_exceeds_credit_limit, have identical bodies (both create
an SO with OVER and assert the same orange warning) making one redundant; remove
or merge them. Keep one test (pick either name) and if you want to preserve the
intent of asserting newness retain the is_new() assertion inside that single
test; update or delete the duplicate (references:
test_warns_on_first_save_when_limit_exceeded,
test_warns_when_amount_exceeds_credit_limit, helper _make_so, and assertions
_check_credit_limit_warn/_get_orange_warnings).
| def _check_prev_docstatus(doc): | ||
| try: | ||
| if doc.get("check_prev_docstatus"): | ||
| doc.check_prev_docstatus() | ||
| except Exception as e: | ||
| frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange") | ||
|
|
There was a problem hiding this comment.
Critical: check_prev_docstatus() is never invoked — gate uses .get() on a method name.
doc.get("check_prev_docstatus") looks up a field value by that name on the document. None of Sales Invoice, Purchase Invoice, Delivery Note, or Purchase Receipt has a field called check_prev_docstatus — only a method. As a result doc.get(...) is always falsy and the check is silently skipped, defeating the entire check_prev_docstatus config branch in PRE_SUBMIT_DOCTYPE_CONFIG.
Use hasattr (or callable(getattr(doc, ..., None))) to gate the call. Also tighten the exception type since the wrapped methods raise frappe.ValidationError (Ruff BLE001).
🐛 Proposed fix
def _check_prev_docstatus(doc):
- try:
- if doc.get("check_prev_docstatus"):
- doc.check_prev_docstatus()
- except Exception as e:
- frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")
+ if not hasattr(doc, "check_prev_docstatus"):
+ return
+ try:
+ doc.check_prev_docstatus()
+ except frappe.ValidationError as e:
+ frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")🧰 Tools
🪛 Ruff (0.15.11)
[warning] 2778-2778: 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 2774 - 2780, _in
_check_prev_docstatus replace the incorrect doc.get("check_prev_docstatus") gate
(which checks a field) with a method presence/callable check such as
hasattr(doc, "check_prev_docstatus") or callable(getattr(doc,
"check_prev_docstatus", None)) so the doc.check_prev_docstatus() method on
relevant doctypes is actually invoked; also narrow the except clause to catch
frappe.ValidationError (instead of Exception) and keep the existing
frappe.msgprint(str(e), title=_("Pre-Submit Warning"), indicator="orange")
handling.
| elif doc.doctype == "Delivery Note": | ||
| if 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) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Delivery Note.check_credit_limit to see if it tolerates is_new() docs.
ast-grep --pattern $'class DeliveryNote($_) {
$$$
def check_credit_limit($_):
$$$
$$$
}'
rg -nP -B2 -A20 '\bdef check_credit_limit\s*\(\s*self\s*\)' --type=py -g '**/delivery_note*'Repository: frappe/erpnext
Length of output: 2093
🏁 Script executed:
# Get the full DeliveryNote.check_credit_limit method
head -c 10000 erpnext/stock/doctype/delivery_note/delivery_note.py | tail -c 5000 > /tmp/dn_preview.txt
cat /tmp/dn_preview.txt
# Now read the exact method
sed -n '571,650p' erpnext/stock/doctype/delivery_note/delivery_note.pyRepository: frappe/erpnext
Length of output: 7662
🏁 Script executed:
# Get the context in utils.py around lines 2811-2825
sed -n '2800,2850p' erpnext/accounts/utils.pyRepository: frappe/erpnext
Length of output: 1513
🏁 Script executed:
# Search for self.name usage in the check_credit_limit method and related code
rg -nA 30 'def check_credit_limit\(self\)' erpnext/stock/doctype/delivery_note/delivery_note.pyRepository: frappe/erpnext
Length of output: 1147
🏁 Script executed:
# Check what type per_billed is defined as
rg -B5 -A5 'per_billed' erpnext/stock/doctype/delivery_note/delivery_note.json | head -50Repository: frappe/erpnext
Length of output: 475
🏁 Script executed:
# Check what the bypass parameter in utils.py represents
sed -n '2780,2830p' erpnext/accounts/utils.pyRepository: frappe/erpnext
Length of output: 1645
🏁 Script executed:
# Search for other uses of per_billed == 100 to see if there's a pattern
rg -n 'per_billed.*==.*100' erpnext/Repository: frappe/erpnext
Length of output: 1431
🏁 Script executed:
# Check if Percent fieldtype in Frappe causes float precision issues
rg -n 'Percent' erpnext/stock/doctype/delivery_note/delivery_note.json | grep -i fieldtypeRepository: frappe/erpnext
Length of output: 153
🏁 Script executed:
# Look for any documentation or issues about unsaved docs and self.get("items")
rg -B3 -A3 'is_new()' erpnext/stock/doctype/delivery_note/delivery_note.pyRepository: frappe/erpnext
Length of output: 40
Remove incorrect self.name concern; focus on actual asymmetric credit limit logic and float comparison inconsistency.
The bypass branch (doc.check_credit_limit()) uses the full self.base_grand_total, while the non-bypass branch calculates a proportional extra_amount based on unlinked items only. This creates asymmetric behavior for the same credit limit configuration, though both paths operate within the warning wrapper.
Additionally, per_billed == 100 uses direct equality on a Percent (float) field, inconsistent with other comparisons in the codebase (e.g., delivery_note_list.js uses flt(doc.per_billed, 2) === 100). Use flt(doc.per_billed) >= 100 for robustness against floating-point precision issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/accounts/utils.py` around lines 2811 - 2825, The Delivery Note branch
is asymmetric and uses a fragile equality check: replace the direct equality
per_billed check with a float-safe comparison (use flt(doc.per_billed) >= 100)
and make the bypass path behave the same as the non-bypass path by computing and
passing the same proportional extra_amount for unlinked items when calling
doc.check_credit_limit() / check_credit_limit(); ensure you compute unlinked_net
= sum(flt(d.base_amount) for d in doc.get("items") if not (d.against_sales_order
or d.against_sales_invoice)), derive extra_amount = (unlinked_net /
flt(doc.base_net_total)) * flt(doc.base_grand_total) only when
doc.base_net_total is truthy, and call the existing
check_credit_limit(doc.customer, doc.company, False, extra_amount=extra_amount)
(or doc.check_credit_limit with the same extra_amount) so both bypass and
non-bypass paths are consistent.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54541 +/- ##
===========================================
+ 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
packedqty_pre_submit
branch
from
89259f1 to
96696eb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
erpnext/accounts/test/test_pre_submit_validation.py (1)
116-129: Bypass mutation has no teardown — risk of state leak into subsequent tests/classes.
test_no_warning_when_bypass_is_setflipsCustomer Credit Limit.bypass_credit_limit_checkto1for_Pre Submit Test Customerdirectly viafrappe.db.set_value. There is notearDownresetting it, and the same customer is reused byTestCreditLimitWarnDeliveryNote(which also relies on bypass-true vs bypass-false semantics). Depending on test execution order,TestCreditLimitWarnDeliveryNote.test_bypass_false_*cases may unexpectedly run with bypass=1 left over.Consider resetting
bypass_credit_limit_checkin_CreditLimitBase.setUp(or adding atearDown) so each test starts from a known state.🤖 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` around lines 116 - 129, The test mutates Customer Credit Limit.bypass_credit_limit_check via frappe.db.set_value in test_no_warning_when_bypass_is_set and doesn't reset it, causing state leakage; fix by ensuring each test resets this flag (either clear it in a tearDown or explicitly set the intended default in _CreditLimitBase.setUp) so tests like TestCreditLimitWarnDeliveryNote rely on a known state; locate the mutation in test_no_warning_when_bypass_is_set and add code in _CreditLimitBase.setUp (or add a tearDown) to set Customer Credit Limit.bypass_credit_limit_check back to 0 for the test customer (refer to methods _make_so and the Customer Credit Limit parent key used in frappe.db.set_value).
🤖 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/test/test_pre_submit_validation.py`:
- Around line 116-129: The test mutates Customer Credit
Limit.bypass_credit_limit_check via frappe.db.set_value in
test_no_warning_when_bypass_is_set and doesn't reset it, causing state leakage;
fix by ensuring each test resets this flag (either clear it in a tearDown or
explicitly set the intended default in _CreditLimitBase.setUp) so tests like
TestCreditLimitWarnDeliveryNote rely on a known state; locate the mutation in
test_no_warning_when_bypass_is_set and add code in _CreditLimitBase.setUp (or
add a tearDown) to set Customer Credit Limit.bypass_credit_limit_check back to 0
for the test customer (refer to methods _make_so and the Customer Credit Limit
parent key used in frappe.db.set_value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 76a07f50-0e92-4dd9-b62f-b23bfdddbadf
📒 Files selected for processing (2)
erpnext/accounts/test/test_pre_submit_validation.pyerpnext/accounts/utils.py
1 participant
If the delivery note has a submitted packing slip in which a partial item is being shipped, this will throw an error at the time of save.
no-docs