feat: add multi-currency support to Blanket Order #49346
Conversation
kavin-114
requested review from
rohitwaghchaure,
mihir-kandoi and
ruthra-kumar
as code owners
📝 WalkthroughWalkthroughAdds multi-currency support to Blanket Order: new DocType fields Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (4)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
erpnext/manufacturing/doctype/blanket_order/blanket_order.json (2)
169-174: Use numeric precision, not string
"precision": "9"should be a number to match DocField schema.- "precision": "9", + "precision": 9,
157-163: Consider printing currency on documentHiding
currencyon print may confuse recipients for multi-currency documents. Recommend not hiding it, or ensure templates render currency elsewhere.- "print_hide": 1, + "print_hide": 0,erpnext/manufacturing/doctype/blanket_order/blanket_order.py (1)
169-172: Populate defaults on mapped docs beyond Purchase OrderSales Orders and Quotations may miss currency/exchange-rate defaults without
set_missing_values().-if target_doc.doctype == "Purchase Order": - target_doc.set_missing_values() +if target_doc.doctype in ("Purchase Order", "Sales Order", "Quotation"): + target_doc.set_missing_values()erpnext/manufacturing/doctype/blanket_order/blanket_order.js (2)
108-140: Fix placeholder in exchange-rate description and clear when 1:1UI shows
"[?]"instead of the actual rate; also keep the description clean for same-currency cases.- callback: function (r) { + callback: function (r) { if (r.message) { frm.set_value("conversion_rate", r.message); - frm.set_df_property( - "conversion_rate", - "description", - "1 " + frm.doc.currency + " = [?] " + company_currency - ); + frm.set_df_property( + "conversion_rate", + "description", + "1 " + frm.doc.currency + " = " + r.message + " " + company_currency + ); frm.trigger("calculate_base_rate"); } }, }); } else { frm.set_value("conversion_rate", 1.0); + frm.set_df_property("conversion_rate", "description", ""); frm.trigger("calculate_base_rate"); }
185-212: Don’t override user-selected currency after manual change
set_party_currencyforces party default every time party/company changes, even if the user manually picked a different currency. Consider applying only on new docs or whenfrm.doc.currencyis empty.- if (party_name) { + if (party_name && (!frm.doc.__unsaved || !frm.doc.currency)) {Alternatively, set a flag when currency was manually changed and skip auto-set if set.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
erpnext/buying/doctype/purchase_order_item/purchase_order_item.json(2 hunks)erpnext/manufacturing/doctype/blanket_order/blanket_order.js(2 hunks)erpnext/manufacturing/doctype/blanket_order/blanket_order.json(3 hunks)erpnext/manufacturing/doctype/blanket_order/blanket_order.py(3 hunks)erpnext/manufacturing/doctype/blanket_order/test_blanket_order.py(1 hunks)erpnext/manufacturing/doctype/blanket_order_item/blanket_order_item.json(3 hunks)erpnext/manufacturing/doctype/blanket_order_item/blanket_order_item.py(1 hunks)erpnext/selling/doctype/quotation_item/quotation_item.json(2 hunks)erpnext/selling/doctype/sales_order_item/sales_order_item.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/manufacturing/doctype/blanket_order/test_blanket_order.py (2)
erpnext/buying/doctype/supplier/test_supplier.py (1)
create_supplier(154-177)erpnext/manufacturing/doctype/blanket_order/blanket_order.py (1)
make_order(135-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (10)
erpnext/buying/doctype/purchase_order_item/purchase_order_item.json (1)
555-561: Confirm currency semantics for Blanket Order Rate on PO Item.Setting options: "currency" will display PO currency. If the intention is to preserve/display the Blanket Order’s original currency rate, this will mask that. Please confirm the intended semantics across cross-currency flows and adjust/help-text if needed.
erpnext/manufacturing/doctype/blanket_order_item/blanket_order_item.py (1)
17-18: Type hint addition matches JSON; good.base_rate: DF.Currency aligns with the new field on the DocType.
erpnext/selling/doctype/quotation_item/quotation_item.json (2)
603-611: Currency binding on blanket_order_rate is consistent.Using options: "currency" here is correct and aligns with PO/SO items.
708-709: Verifyrow_format: "Dynamic"support in Frappe v15
Grep across ERPNext shows thatrow_format: "Dynamic"is used in dozens of doctype JSON definitions (including erpnext/selling/doctype/quotation_item/quotation_item.json lines 708–709). Before backporting, please confirm that Frappe v15’s JSON schema parser and list view respect this property and behave identically to newer versions.Files/locations to double-check:
- erpnext/selling/doctype/quotation_item/quotation_item.json (lines 708–709)
- Any other doctypes where
row_format: "Dynamic"is set (see yourrgoutput)If Frappe v15 doesn’t support or behaves differently with this setting, adjust or remove it for compatibility.
erpnext/selling/doctype/sales_order_item/sales_order_item.json (2)
617-625: Currency binding on blanket_order_rate is appropriate.Matches Quotation/PO items for consistency.
981-982: Ensure framework support forrow_format: "Dynamic"I see that
"row_format": "Dynamic"has been added here as part of the v15 backport—this change is mirrored across all DocType JSON definitions (e.g. Quotation Item, Stock Ledger Entry, etc.). Before merging, please confirm that the target Frappe framework version includes the client-side and backend support for dynamic row formatting introduced in v15; otherwise, consider reverting to the default static format to prevent list‐view rendering issues.• erpnext/selling/doctype/sales_order_item/sales_order_item.json (lines 981–982)
- added
"row_format": "Dynamic"- retained
"sort_field": "creation"erpnext/manufacturing/doctype/blanket_order_item/blanket_order_item.json (1)
46-52: Rate bound to document currency — good.options: "currency" leverages parent Blanket Order’s currency.
erpnext/manufacturing/doctype/blanket_order/blanket_order.json (1)
203-204: No action needed:row_format: "Dynamic"is supportedVerified that the
row_format: "Dynamic"property appears ubiquitously across core ERPNext DocType JSON definitions—over 200 occurrences in manufacturing, stock, sales, CRM, accounting, assets, and more—indicating first-class framework support. The Blanket Order JSON aligns with these conventions, so grid label adjustments at the DocType level are valid as is.erpnext/manufacturing/doctype/blanket_order/test_blanket_order.py (1)
144-146: Good: sensible defaults for new required fieldsSetting
currencyandconversion_ratein the helper ensures new required fields don’t break fixture creation.erpnext/manufacturing/doctype/blanket_order/blanket_order.js (1)
28-28: LGTM: explicit doctype args for mapped docsPassing
{ doctype: ... }makes intent clear and avoids downstream ambiguity.Also applies to: 39-39, 53-53
| "base_rate", | ||
| "ordered_qty", |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
New base_rate is read_only + reqd — likely to break saves/migrations.
A newly required, read-only field can block saves on existing records unless a server patch backfills and server-side validate always sets it. Recommend making it non-required or ship a patch ensuring population for all existing rows and on validate.
Apply if opting non-required:
{
"fieldname": "base_rate",
"fieldtype": "Currency",
"label": "Base Rate (Company Currency)",
"options": "Company:company:default_currency",
"print_hide": 1,
"read_only": 1,
- "reqd": 1
+ "reqd": 0
}If keeping required, please add a data patch to compute base_rate = rate * parent.conversion_rate for all existing Blanket Orders and ensure server-side validate sets it when missing.
Also applies to: 77-83
Ensure new base_rate field won’t block existing records
-
The
Currencyfield base_rate is currently defined asread_only: 1andreqd: 1. Without a backfill or server-side population, saves and migrations for existing Blanket Order Item records will be blocked. -
To address this, choose one of the following:
-
Make base_rate optional
Inerpnext/manufacturing/doctype/blanket_order_item/blanket_order_item.json, remove thereqdflag:{ "fieldname": "base_rate", "fieldtype": "Currency", "label": "Base Rate (Company Currency)", "options": "Company:company:default_currency", "print_hide": 1, "read_only": 1, - "reqd": 1 + "reqd": 0 } -
Keep it required but backfill and enforce on save
- Add a data migration patch under
erpnext/patches/to compute and populatebase_rate = rate * parent.conversion_ratefor all existing Blanket Order Item records. - In the DocType Python class, ensure that on
validate(orbefore_save) any missingbase_rateis automatically calculated.
- Add a data migration patch under
-
-
Apply the same review to the
ordered_qtyfield definitions (lines 77–83).
Committable suggestion skipped: line range outside the PR's diff.
| "collapsible": 1, | ||
| "fieldname": "currency_section", | ||
| "fieldtype": "Section Break", | ||
| "label": "Currency" | ||
| }, | ||
| { | ||
| "fieldname": "currency", | ||
| "fieldtype": "Link", | ||
| "label": "Currency", | ||
| "options": "Currency", | ||
| "print_hide": 1, | ||
| "reqd": 1 | ||
| }, | ||
| { | ||
| "fieldname": "column_break_yilv", | ||
| "fieldtype": "Column Break" | ||
| }, | ||
| { | ||
| "fieldname": "conversion_rate", | ||
| "fieldtype": "Float", | ||
| "label": "Exchange Rate", | ||
| "precision": "9", | ||
| "print_hide": 1, | ||
| "reqd": 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Required new fields need migration/backfill and a server-side default path
- Adding required fields
currencyandconversion_ratewill block edits/amendments of existing Blanket Orders unless you backfill. Provide a patch to setcurrency = company.default_currencyandconversion_rate = 1.0for existing rows. - Also reduce reliance on client scripts by auto-fetching the company’s default currency into
currency.
Apply this diff to auto-fetch the company currency:
{
"fieldname": "currency",
"fieldtype": "Link",
"label": "Currency",
"options": "Currency",
+ "fetch_from": "company.default_currency",
"print_hide": 1,
"reqd": 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "collapsible": 1, | |
| "fieldname": "currency_section", | |
| "fieldtype": "Section Break", | |
| "label": "Currency" | |
| }, | |
| { | |
| "fieldname": "currency", | |
| "fieldtype": "Link", | |
| "label": "Currency", | |
| "options": "Currency", | |
| "print_hide": 1, | |
| "reqd": 1 | |
| }, | |
| { | |
| "fieldname": "column_break_yilv", | |
| "fieldtype": "Column Break" | |
| }, | |
| { | |
| "fieldname": "conversion_rate", | |
| "fieldtype": "Float", | |
| "label": "Exchange Rate", | |
| "precision": "9", | |
| "print_hide": 1, | |
| "reqd": 1 | |
| } | |
| { | |
| "fieldname": "currency_section", | |
| "fieldtype": "Section Break", | |
| "label": "Currency" | |
| }, | |
| { | |
| "fieldname": "currency", | |
| "fieldtype": "Link", | |
| "label": "Currency", | |
| "options": "Currency", | |
| "fetch_from": "company.default_currency", | |
| "print_hide": 1, | |
| "reqd": 1 | |
| }, | |
| { | |
| "fieldname": "column_break_yilv", | |
| "fieldtype": "Column Break" | |
| }, | |
| { | |
| "fieldname": "conversion_rate", | |
| "fieldtype": "Float", | |
| "label": "Exchange Rate", | |
| "precision": "9", | |
| "print_hide": 1, | |
| "reqd": 1 | |
| } |
| def calculate_base_rate(self): | ||
| for d in self.items: | ||
| d.base_rate = flt(d.rate * self.conversion_rate, d.precision("base_rate")) | ||
|
|
There was a problem hiding this comment.
Guard against None/empty values in base rate calculation
If rate is None/empty, d.rate * self.conversion_rate raises TypeError before flt() runs.
- for d in self.items:
- d.base_rate = flt(d.rate * self.conversion_rate, d.precision("base_rate"))
+ for d in self.items:
+ d.base_rate = flt(flt(d.rate) * flt(self.conversion_rate), d.precision("base_rate"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def calculate_base_rate(self): | |
| for d in self.items: | |
| d.base_rate = flt(d.rate * self.conversion_rate, d.precision("base_rate")) | |
| def calculate_base_rate(self): | |
| for d in self.items: | |
| d.base_rate = flt(flt(d.rate) * flt(self.conversion_rate), d.precision("base_rate")) |
🤖 Prompt for AI Agents
In erpnext/manufacturing/doctype/blanket_order/blanket_order.py around lines
129–132, the base_rate calculation assumes d.rate is set and will raise a
TypeError if d.rate is None/empty; guard this by checking d.rate (and
conversion_rate if needed) before multiplying: if d.rate is falsy set
d.base_rate to 0 (using flt(0, d.precision("base_rate"))) otherwise compute
d.base_rate = flt(d.rate * self.conversion_rate, d.precision("base_rate")).
Ensure you treat empty strings and None as falsy and keep the flt() call for
precision.
| def test_multicurrency_blanket_order(self): | ||
| from erpnext.buying.doctype.supplier.test_supplier import create_supplier | ||
|
|
||
| supplier = create_supplier(supplier_name="_Test BO USD Supplier", default_currency="USD") | ||
| bo = make_blanket_order( | ||
| blanket_order_type="Purchasing", | ||
| supplier=supplier.name, | ||
| currency="USD", | ||
| conversion_rate=86, | ||
| quantity=10, | ||
| rate=5, | ||
| ) | ||
| self.assertEqual(bo.items[0].base_rate, 430) | ||
|
|
||
| frappe.flags.args.doctype = "Purchase Order" | ||
| po = make_order(bo.name) | ||
|
|
||
| self.assertEqual(po.currency, "USD") | ||
| self.assertEqual(po.conversion_rate, 86) | ||
| self.assertEqual(po.items[0].rate, 5) | ||
| self.assertEqual(po.items[0].base_rate, 430) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize FX expectations in test to avoid flakiness
The assertions assume USD→company FX is 86 on the test date. Seed a Currency Exchange row to make the test deterministic.
def test_multicurrency_blanket_order(self):
from erpnext.buying.doctype.supplier.test_supplier import create_supplier
+ from frappe.utils import today
+ company_currency = get_company_currency("_Test Company")
+ # Ensure deterministic FX for today's date
+ if not frappe.db.exists(
+ "Currency Exchange",
+ {"from_currency": "USD", "to_currency": company_currency, "date": today()},
+ ):
+ frappe.get_doc({
+ "doctype": "Currency Exchange",
+ "from_currency": "USD",
+ "to_currency": company_currency,
+ "exchange_rate": 86,
+ "date": today(),
+ }).insert()
supplier = create_supplier(supplier_name="_Test BO USD Supplier", default_currency="USD")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In erpnext/manufacturing/doctype/blanket_order/test_blanket_order.py around
lines 116 to 137, the test assumes a USD→company conversion rate of 86 and can
be flaky; before creating the blanket order, create (seed) a Currency Exchange
entry for USD→company with the expected rate (86) and a known date used by the
test so conversion_rate calculations are deterministic, then proceed to create
the supplier/blanket order and optionally tear down/remove the seeded Currency
Exchange after the test.
Issue: Unable to create a Blanket Order with the party’s default currency and exchange rate.
Description:
Ref: 46964
Before:
feat.multicurrency.in.blanket.order.before.webm
After:
feat.multi-currency.in.blanket.order.after.webm
Backport Needed: v15