fix: hide redundant company currency fields on transactions#54691
fix: hide redundant company currency fields on transactions#54691barredterra wants to merge 3 commits intofrappe:developfrom
Conversation
📝 WalkthroughWalkthroughUpdates transaction.js to add base-currency–aware visibility and label handling. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Changed reset_currency_labels from stripping every parenthesized fragment to only removing a parenthesized suffix.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
erpnext/public/js/controllers/transaction.js (3)
2107-2123:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
base_net_*item columns are still force-shown fromshowalone.
toggle_item_grid_columns()now callsset_column_disp(fname, show)forbase_net_rateandbase_net_amount, which ignores_base_currency_field_visibility. In a foreign-currency document with discounts or inclusive taxes, that re-surfaces item columns that were intentionally hidden. Please gate these throughshould_show_base_currency_field(...) && showinstead of showing them unconditionally fromshow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/public/js/controllers/transaction.js` around lines 2107 - 2123, In toggle_item_grid_columns(), the base currency item columns (base_net_rate, base_net_amount) are being shown solely based on show; update the second $.each block so that when calling item_grid.set_column_disp for base_net_rate and base_net_amount you combine the existing show flag with should_show_base_currency_field(...) (e.g. should_show_base_currency_field(item_grid.doctype, fname) && show) while keeping the frappe.meta.get_docfield(...) guard and using the same item_grid and fname symbols.
1969-1992:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe foreign-currency path still re-shows cached-hidden
base_*fields.These hard-coded
toggle_display/set_column_dispcalls still bypass_base_currency_field_visibilityand force overlappingbase_*fields back on whenever currencies differ. That means customized hides likebase_total,base_operating_cost,base_rate, or payment-schedulebase_*columns can still leak back in multi-currency documents. Let the cached helper remain the single source of truth for overlappingbase_*visibility here too.Also applies to: 2047-2073
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/public/js/controllers/transaction.js` around lines 1969 - 1992, The hard-coded frm.toggle_display and set_column_disp calls are forcing base_* fields visible and bypassing the canonical helper; replace these explicit toggles with a call to the existing _base_currency_field_visibility helper so cached/ customized visibility remains the single source of truth. Specifically, remove the explicit frm.toggle_display([...]) / set_column_disp blocks (the one shown and the similar block at the 2047-2073 range) and invoke this._base_currency_field_visibility(this.frm) or this._base_currency_field_visibility() as implemented in this file so base_* field visibility is determined by that helper (preserving any cached hides) rather than re-showing fields when this.frm.doc.currency != company_currency.
1861-1866:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly remove a suffix that was actually generated by currency labeling.
reset_currency_labels()still strips any trailing parenthesized text. On the first same-currency render, a custom label likeAmount (Tax Incl.)becomesAmounteven if no currency suffix was ever appended. This should only remove the expected generated currency suffix, not arbitrary trailing parentheses.Also applies to: 1873-1883
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/public/js/controllers/transaction.js` around lines 1861 - 1866, The current reset_currency_labels implementation unconditionally strips any trailing parenthesized text (e.g., " (Tax Incl.)") which removes custom labels; change reset_currency_labels so it only removes a suffix that exactly matches the currency-label pattern previously appended (e.g., " (USD)" or the string produced by add_currency_label for the current company currency). Implement this by matching and removing only a well-defined regex or exact string built from the currency code/symbol used when labeling (not any parentheses), update the logic used in the block around the fields.filter(...) call and the analogous block at the later lines (1873-1883) to use the same guarded removal, and keep all other custom trailing parentheses intact.
🤖 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/public/js/controllers/transaction.js`:
- Around line 2107-2123: In toggle_item_grid_columns(), the base currency item
columns (base_net_rate, base_net_amount) are being shown solely based on show;
update the second $.each block so that when calling item_grid.set_column_disp
for base_net_rate and base_net_amount you combine the existing show flag with
should_show_base_currency_field(...) (e.g.
should_show_base_currency_field(item_grid.doctype, fname) && show) while keeping
the frappe.meta.get_docfield(...) guard and using the same item_grid and fname
symbols.
- Around line 1969-1992: The hard-coded frm.toggle_display and set_column_disp
calls are forcing base_* fields visible and bypassing the canonical helper;
replace these explicit toggles with a call to the existing
_base_currency_field_visibility helper so cached/ customized visibility remains
the single source of truth. Specifically, remove the explicit
frm.toggle_display([...]) / set_column_disp blocks (the one shown and the
similar block at the 2047-2073 range) and invoke
this._base_currency_field_visibility(this.frm) or
this._base_currency_field_visibility() as implemented in this file so base_*
field visibility is determined by that helper (preserving any cached hides)
rather than re-showing fields when this.frm.doc.currency != company_currency.
- Around line 1861-1866: The current reset_currency_labels implementation
unconditionally strips any trailing parenthesized text (e.g., " (Tax Incl.)")
which removes custom labels; change reset_currency_labels so it only removes a
suffix that exactly matches the currency-label pattern previously appended
(e.g., " (USD)" or the string produced by add_currency_label for the current
company currency). Implement this by matching and removing only a well-defined
regex or exact string built from the currency code/symbol used when labeling
(not any parentheses), update the logic used in the block around the
fields.filter(...) call and the analogous block at the later lines (1873-1883)
to use the same guarded removal, and keep all other custom trailing parentheses
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2315cf6d-8bbc-4431-a871-7850c5807157
📒 Files selected for processing (1)
erpnext/public/js/controllers/transaction.js
Hide redundant company-currency fields on transactions when the document currency is already the company currency.
base_fields on parent and child tables when document currency equals company currency.Multi-Currency (unchanged)
Single-Currency (cleaned up)