fix(pwa): multicurrency support in expense & advance forms#4083
Open
iamkhanraheel wants to merge 13 commits intofrappe:developfrom
Open
fix(pwa): multicurrency support in expense & advance forms#4083iamkhanraheel wants to merge 13 commits intofrappe:developfrom
iamkhanraheel wants to merge 13 commits intofrappe:developfrom
Conversation
iamkhanraheel
changed the title
Collaborator
Author
|
@Mergifyio rebase |
…n claim, fix currency value in list & form view
Contributor
✅ Branch has been successfully rebased |
iamkhanraheel
force-pushed
the
fix/v16_changes_pwa
branch
from
6ef3a41 to
cde96b3
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:
WalkthroughThis PR introduces two composables, 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/views/expense_claim/Form.vue (1)
193-200:⚠️ Potential issue | 🟡 MinorCondition may not trigger for new documents where
docstatusis undefined.For newly created documents,
expenseClaim.value.docstatusmay beundefinedrather than0. The strict equality check=== 0would evaluate tofalse, preventing the advances reload.Proposed fix
- if (!props.id && expenseClaim.value.docstatus === 0) { + if (!props.id && !expenseClaim.value.docstatus) { advances.reload() }Or use
(expenseClaim.value.docstatus ?? 0) === 0for explicit intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 193 - 200, The watch callback's condition uses strict equality expenseClaim.value.docstatus === 0 which fails when docstatus is undefined for new documents; update the condition inside the watcher (the function referencing expenseClaim, props.id and calling advances.reload) to treat undefined as 0 (e.g., use (expenseClaim.value.docstatus ?? 0) === 0) so advances.reload() runs for new drafts when props.id is falsy.
🧹 Nitpick comments (8)
roster/src/components/Link.vue (2)
32-35: Default object prop should use a factory function.Using a static
{}as the default value means all component instances share the same object reference. This can cause unexpected mutation issues.Suggested fix
filters: { type: Object, - default: {}, + default: () => ({}), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roster/src/components/Link.vue` around lines 32 - 35, The props definition for filters in Link.vue uses a shared object literal as the default which can cause shared-state mutations; change the filters prop to use a factory function that returns a new object (i.e., replace default: {} with default: () => ({})) so each component instance gets its own object; update the props block where filters is declared to use this factory-style default.
107-111: Consider addingdeep: truefor nested filter changes.The watcher on
props.filtersonly detects reference changes by default. If the parent component mutates filter properties in place rather than replacing the entire object, this watcher won't trigger. If filters are always replaced with new objects, this is fine—otherwise, add{ deep: true }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roster/src/components/Link.vue` around lines 107 - 111, The watcher on props.filters only observes top-level reference changes; update the watch call that currently reads watch(() => props.filters, () => reloadOptions(""), ) to pass a third options argument with { deep: true } so nested mutations trigger reloadOptions (i.e. watch(() => props.filters, () => reloadOptions(""), { deep: true })). Keep the existing callback and getter (props.filters) and only add the options object; if you prefer not to use deep watching consider switching to toRef(props, 'filters') and watching that if filters are always replaced.frontend/src/composables/useCurrencyConversion.js (1)
10-51: Composable creates a watcher without cleanup.The
updateCurrencyLabelsfunction creates awatchinternally. In Vue 3, watchers created insidesetup()are automatically stopped when the component unmounts, but this composable could be called outside that context. If the composable is called dynamically or multiple times, consider returning a cleanup function or usingwatchEffectwith explicit stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCurrencyConversion.js` around lines 10 - 51, The composable updateCurrencyLabels creates a Vue watch that isn't cleaned up; capture the watcher stop handle returned by watch(...) and return a cleanup function (e.g., stop or stopWatcher) from updateCurrencyLabels so callers can stop the watcher when needed, or alternatively call onScopeDispose inside updateCurrencyLabels if you want automatic cleanup when used inside setup; ensure you reference the existing watch call, the returned stop handle, and the helper functions/refs (fetchCompanyCurrency, companyCurrency, updateLabels) when wiring the stop logic.frontend/src/components/ExpensesTable.vue (2)
277-290: Mutating prop data directly.The watch mutates
props.expenseClaim.expensesrows directly viaupdateBaseFieldsAmount. While this works due to Vue's reactivity, mutating props is generally discouraged as it can make data flow harder to trace. Consider emitting an event to update the parent instead, or document this as intentional two-way binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ExpensesTable.vue` around lines 277 - 290, The watch on props.expenseClaim.exchange_rate is mutating prop objects directly by calling updateBaseFieldsAmount on items from props.expenseClaim.expenses; instead change the behavior to avoid direct prop mutation—create a shallow/deep copy of expenseClaim.expenses, apply updateBaseFieldsAmount to the copied rows, then emit an event (e.g., 'update-expenses' or 'update:expenseClaim') with the updated array so the parent updates the prop. Alternatively, if two-way binding is intended, explicitly use v-model/update:expenseClaim and document it; reference the watch callback, updateBaseFieldsAmount, and props.expenseClaim.expenses when implementing the change.
264-275: Guard condition may not prevent execution on empty object.
expenseItem.valueis initialized to{}(line 166), which is truthy. The checkif (expenseItem.value)on line 267 will pass even when the item is empty, potentially callingupdateBaseFieldsAmountwith an empty doc before user interaction.Suggested fix
watch( () => [expenseItem.value.amount, expenseItem.value.sanctioned_amount], () => { - if (expenseItem.value) { + if (expenseItem.value.amount !== undefined || expenseItem.value.sanctioned_amount !== undefined) { updateBaseFieldsAmount({ doc: expenseItem.value, fields: ['amount', 'sanctioned_amount'], exchangeRate: props.expenseClaim.exchange_rate, }); } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ExpensesTable.vue` around lines 264 - 275, The watch currently calls updateBaseFieldsAmount even when expenseItem.value is an empty object because expenseItem is initialized to {} (truthy); change the guard in the watcher to verify the item is populated (e.g., check a stable identifier or required fields like expenseItem.value.amount !== undefined || expenseItem.value.sanctioned_amount !== undefined or Object.keys(expenseItem.value).length > 0) before calling updateBaseFieldsAmount, keeping the same parameters (doc: expenseItem.value, fields: ['amount','sanctioned_amount'], exchangeRate: props.expenseClaim.exchange_rate).frontend/src/components/Link.vue (1)
28-31: Default object prop should use a factory function.Same issue as in
roster/src/components/Link.vue—using a static{}as the default value means all component instances share the same object reference.Suggested fix
filters: { type: Object, - default: {}, + default: () => ({}), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Link.vue` around lines 28 - 31, The filters prop in the Link.vue component is using a shared static object as its default which can lead to shared mutable state; change the prop definition for filters (the filters prop inside the component options) to use a factory function that returns a new object (e.g., default: () => ({})) so each component instance gets its own object; locate the filters prop in the Link.vue component props/options and replace the static {} default with a function returning an empty object.frontend/src/views/expense_claim/Form.vue (2)
266-290:{ deep: true }is unnecessary for this watcher.The watched value is an array of primitives returned from the getter function. Vue already detects changes because the getter produces a new array reference on each evaluation. The
deepoption adds overhead without benefit here.Suggested simplification
- { deep: true } + { immediate: true }Or remove the options object entirely if immediate evaluation isn't needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 266 - 290, The watcher on expenseClaim that observes an array of primitive fields (defined in the inline getter) is using the unnecessary { deep: true } option; remove the deep option (or the entire options object) from the watch call that wraps the getter so Vue won't perform needless deep observation—update the watch invocation around the getter that references expenseClaim.value.* and calls updateBaseFieldsAmount (and keeps fieldsToConvert and exchangeRate as-is) to pass no deep option.
158-163: Consider adding error handling for exchange rate fetch.If the exchange rate API call fails,
expenseClaim.value.exchange_rateremains undefined, which could cause calculation issues inupdateBaseFieldsAmountdownstream. Consider adding anonErrorhandler or a fallback:Suggested improvement
const exchangeRate = createResource({ url: "erpnext.setup.utils.get_exchange_rate", onSuccess(data) { expenseClaim.value.exchange_rate = data }, + onError(error) { + console.error("Failed to fetch exchange rate:", error) + // Fallback to 1 or show user notification + expenseClaim.value.exchange_rate = 1 + }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 158 - 163, The exchange rate fetch via createResource (exchangeRate) lacks error handling: add an onError handler on the same createResource call (alongside onSuccess) to set a safe fallback on expenseClaim.value.exchange_rate (e.g., 1 or a previously cached value), log or surface the error for debugging, and ensure you trigger/update updateBaseFieldsAmount after setting the fallback so downstream calculations are not left with undefined values; reference the createResource export named exchangeRate, its onSuccess/onError handlers, the expenseClaim reactive object, and the updateBaseFieldsAmount function when implementing this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ExpenseClaimItem.vue`:
- Line 17: The API is not returning the currency field so props.doc.currency is
undefined; update the get_expense_claims function to include the expense claim
currency by adding "tabExpense Claim.currency" (or its equivalent column
reference) to the fields list returned by get_expense_claims in
hrms/api/__init__.py so formatCurrency receives a valid currency value; ensure
the field name matches the table alias used in the query and update any
field-selection arrays or SQL SELECT clause inside get_expense_claims
accordingly.
In `@frontend/src/composables/useCurrencyConversion.js`:
- Line 56: The variable name excahnge_rate is misspelled; rename it to
exchange_rate in useCurrencyConversion (update the declaration const
exchange_rate = flt(exchangeRate || doc.exchange_rate || 1, 9)) and update any
subsequent references within that module or function (e.g., places using
excahnge_rate) to the corrected symbol to avoid undefined variable bugs.
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 497-513: The setExchangeRate function wrongly treats undefined
companyCurrency as equal to currency, so add an explicit guard for
companyCurrency.value (or expenseClaim.value.company) before comparing: if
companyCurrency.value is falsy, return early (do not set exchange_rate or hide
the field) so currencies can initialize; keep the existing logic that sets
exchange_rate = 1 and toggles exchange_rate_field.hidden only when
companyCurrency.value is defined and currency.value === companyCurrency.value,
otherwise call exchangeRate.fetch(...) and unhide the field (references:
setExchangeRate, expenseClaim, companyCurrency, currency, exchangeRate.fetch,
exchange_rate_field).
- Around line 119-138: The advances resource captures params at creation so
advances.reload() uses stale expense_claim; replace the reload call with a fetch
that supplies the current params (e.g., call advances.fetch({ params: {
expense_claim: expenseClaim.value } })) so the latest expenseClaim is sent, or
alternatively make the params dynamic if createResource supports a params
function or remove auto: true and explicitly call advances.fetch(...) after
expenseClaim is populated; update references to advances.reload() to use
advances.fetch with current expenseClaim.value.
---
Outside diff comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 193-200: The watch callback's condition uses strict equality
expenseClaim.value.docstatus === 0 which fails when docstatus is undefined for
new documents; update the condition inside the watcher (the function referencing
expenseClaim, props.id and calling advances.reload) to treat undefined as 0
(e.g., use (expenseClaim.value.docstatus ?? 0) === 0) so advances.reload() runs
for new drafts when props.id is falsy.
---
Nitpick comments:
In `@frontend/src/components/ExpensesTable.vue`:
- Around line 277-290: The watch on props.expenseClaim.exchange_rate is mutating
prop objects directly by calling updateBaseFieldsAmount on items from
props.expenseClaim.expenses; instead change the behavior to avoid direct prop
mutation—create a shallow/deep copy of expenseClaim.expenses, apply
updateBaseFieldsAmount to the copied rows, then emit an event (e.g.,
'update-expenses' or 'update:expenseClaim') with the updated array so the parent
updates the prop. Alternatively, if two-way binding is intended, explicitly use
v-model/update:expenseClaim and document it; reference the watch callback,
updateBaseFieldsAmount, and props.expenseClaim.expenses when implementing the
change.
- Around line 264-275: The watch currently calls updateBaseFieldsAmount even
when expenseItem.value is an empty object because expenseItem is initialized to
{} (truthy); change the guard in the watcher to verify the item is populated
(e.g., check a stable identifier or required fields like
expenseItem.value.amount !== undefined || expenseItem.value.sanctioned_amount
!== undefined or Object.keys(expenseItem.value).length > 0) before calling
updateBaseFieldsAmount, keeping the same parameters (doc: expenseItem.value,
fields: ['amount','sanctioned_amount'], exchangeRate:
props.expenseClaim.exchange_rate).
In `@frontend/src/components/Link.vue`:
- Around line 28-31: The filters prop in the Link.vue component is using a
shared static object as its default which can lead to shared mutable state;
change the prop definition for filters (the filters prop inside the component
options) to use a factory function that returns a new object (e.g., default: ()
=> ({})) so each component instance gets its own object; locate the filters prop
in the Link.vue component props/options and replace the static {} default with a
function returning an empty object.
In `@frontend/src/composables/useCurrencyConversion.js`:
- Around line 10-51: The composable updateCurrencyLabels creates a Vue watch
that isn't cleaned up; capture the watcher stop handle returned by watch(...)
and return a cleanup function (e.g., stop or stopWatcher) from
updateCurrencyLabels so callers can stop the watcher when needed, or
alternatively call onScopeDispose inside updateCurrencyLabels if you want
automatic cleanup when used inside setup; ensure you reference the existing
watch call, the returned stop handle, and the helper functions/refs
(fetchCompanyCurrency, companyCurrency, updateLabels) when wiring the stop
logic.
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 266-290: The watcher on expenseClaim that observes an array of
primitive fields (defined in the inline getter) is using the unnecessary { deep:
true } option; remove the deep option (or the entire options object) from the
watch call that wraps the getter so Vue won't perform needless deep
observation—update the watch invocation around the getter that references
expenseClaim.value.* and calls updateBaseFieldsAmount (and keeps fieldsToConvert
and exchangeRate as-is) to pass no deep option.
- Around line 158-163: The exchange rate fetch via createResource (exchangeRate)
lacks error handling: add an onError handler on the same createResource call
(alongside onSuccess) to set a safe fallback on expenseClaim.value.exchange_rate
(e.g., 1 or a previously cached value), log or surface the error for debugging,
and ensure you trigger/update updateBaseFieldsAmount after setting the fallback
so downstream calculations are not left with undefined values; reference the
createResource export named exchangeRate, its onSuccess/onError handlers, the
expenseClaim reactive object, and the updateBaseFieldsAmount function when
implementing this.
In `@roster/src/components/Link.vue`:
- Around line 32-35: The props definition for filters in Link.vue uses a shared
object literal as the default which can cause shared-state mutations; change the
filters prop to use a factory function that returns a new object (i.e., replace
default: {} with default: () => ({})) so each component instance gets its own
object; update the props block where filters is declared to use this
factory-style default.
- Around line 107-111: The watcher on props.filters only observes top-level
reference changes; update the watch call that currently reads watch(() =>
props.filters, () => reloadOptions(""), ) to pass a third options argument with
{ deep: true } so nested mutations trigger reloadOptions (i.e. watch(() =>
props.filters, () => reloadOptions(""), { deep: true })). Keep the existing
callback and getter (props.filters) and only add the options object; if you
prefer not to use deep watching consider switching to toRef(props, 'filters')
and watching that if filters are always replaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 645f7d54-7c23-4e09-9116-5c0b7ab04397
📒 Files selected for processing (10)
frontend/src/components/ExpenseClaimItem.vuefrontend/src/components/ExpensesTable.vuefrontend/src/components/Link.vuefrontend/src/composables/useCurrencyConversion.jsfrontend/src/data/currencies.jsfrontend/src/views/employee_advance/Form.vuefrontend/src/views/expense_claim/Form.vuefrontend/src/views/expense_claim/List.vuehrms/api/__init__.pyroster/src/components/Link.vue
💤 Files with no reviewable changes (1)
- hrms/api/init.py
Collaborator
Author
|
@Mergifyio rebase |
Contributor
☑️ Nothing to do, the required conditions are not metDetails
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/views/expense_claim/Form.vue (1)
119-138:⚠️ Potential issue | 🟠 MajorStale params when reloading advances resource.
The
paramsobject capturesexpenseClaim.valueat resource creation time. Whenadvances.reload()is called (line 200), it reuses these stale initial params rather than the currentexpenseClaim.value. At creation time, fields likeemployeemay not yet be populated.Use
.fetch()with current params instead of.reload():- advances.reload() + advances.fetch({ expense_claim: expenseClaim.value })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 119 - 138, The advances resource captured a stale params object at creation (params: { expense_claim: expenseClaim.value }) so calling advances.reload() reuses the old snapshot; instead, when you need fresh data (where reload() is currently invoked) call advances.fetch and pass the current params object built from expenseClaim.value (i.e., request the resource with params:{ expense_claim: expenseClaim.value }) so the request uses the up-to-date expenseClaim fields; update the call site that uses advances.reload() to use advances.fetch with the current params and keep existing onSuccess handling (e.g., the logic that sets expenseClaim.value.advances and calls calculateTotalAdvance()).
🧹 Nitpick comments (4)
frontend/src/composables/useCurrencyConversion.js (2)
14-17: Unnecessaryasync/awaiton synchronous function.
getCompanyCurrencyreturns a plain value synchronously (percurrencies.js), not a Promise. Theasync/awaithere is misleading and unnecessary overhead.Proposed fix
- const fetchCompanyCurrency = async () => { + const fetchCompanyCurrency = () => { if (!doc.company) return - companyCurrency.value = await getCompanyCurrency(doc.company) + companyCurrency.value = getCompanyCurrency(doc.company) }Also update the watcher callback:
watch( () => [doc.company, doc.currency], - async () => { - await fetchCompanyCurrency() + () => { + fetchCompanyCurrency() updateLabels() }, { immediate: true } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCurrencyConversion.js` around lines 14 - 17, The fetchCompanyCurrency function is marked async and uses await even though getCompanyCurrency returns a synchronous value; remove the async keyword and the await and assign companyCurrency.value directly from getCompanyCurrency(doc.company). Also update any watcher callback that calls fetchCompanyCurrency (the reactive watcher in this composable) to call the now-synchronous fetchCompanyCurrency directly (no need to handle a Promise).
41-50: Watcher created inside function call may leak.Each invocation of
updateCurrencyLabelsregisters a newwatch()without cleanup. If the caller's outer watcher (e.g., inForm.vue) fires multiple times, watchers accumulate. Consider capturing and returning the stop handle, or restructuring so the caller manages the watch lifecycle.Possible restructure
+ const stopWatch = watch( () => [doc.company, doc.currency], - async () => { - await fetchCompanyCurrency() + () => { + fetchCompanyCurrency() updateLabels() }, { immediate: true } ) - return { updateLabels, companyCurrency } + return { updateLabels, companyCurrency, stopWatch }The caller can then call
stopWatch()before re-invoking or on unmount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCurrencyConversion.js` around lines 41 - 50, The watcher created by watch(...) inside this composable is registered every time the caller invokes the function and never cleaned up; capture the stop handle (const stop = watch(...)) and expose it (e.g., return stopWatch or stop from the composable) or move the watch out so the caller manages lifecycle; specifically, change the code that defines the watch around fetchCompanyCurrency and updateLabels to store the stop handle and return a stop function alongside updateLabels and companyCurrency so the caller can call stop() before re-invoking or on unmount.frontend/src/views/expense_claim/Form.vue (2)
269-293:{ deep: true }is unnecessary for primitive values.The watch dependencies are all primitive number values.
deep: trueadds overhead for deep comparison but provides no benefit here since primitives are compared by value anyway.Proposed fix
- { deep: true } + { immediate: true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 269 - 293, The watcher is using { deep: true } unnecessarily for primitive numeric dependencies; in the watch call that observes expenseClaim.value.total_sanctioned_amount, total_advance_amount, grand_total, total_claimed_amount, total_taxes_and_charges and exchange_rate, remove the { deep: true } option so Vue will use the default shallow comparison for these primitive values; keep the rest of the handler intact (the fieldsToConvert array and the call to updateBaseFieldsAmount with doc: expenseClaim.value and exchangeRate: expenseClaim.value.exchange_rate).
242-267: Redundant watcher setup may cause accumulation.This watch calls
updateCurrencyLabelseach timecurrencychanges, butupdateCurrencyLabelsinternally sets up its ownwatchondoc.currency. Each invocation creates a new internal watcher without cleanup, leading to multiple watchers tracking the same properties.Consider calling
updateCurrencyLabelsonce (on mount or initial data load) and relying on its internal watcher, or removing the internal watcher from the composable and keeping the external control here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 242 - 267, The external watch in Form.vue is creating duplicate watchers because updateCurrencyLabels itself registers a watch on doc.currency; remove the surrounding watch and instead call updateCurrencyLabels once when formFields.data and expenseClaim.value.currency are available (e.g., in mounted or a small computed/nextTick helper) so the composable's internal watcher is the sole watcher; target the existing updateCurrencyLabels invocation and replace the watch(...) block with a one-time guarded call that checks for fields and currency before calling updateCurrencyLabels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 500-517: In setExchangeRate, guard against companyCurrency.value
being undefined and fix the hide/unhide logic: first return early if
currency.value or companyCurrency.value or formFields.data are missing; then
locate the exchange_rate_field (from formFields.data.find(...)) and only set
exchange_rate_field.hidden = 1 when currency.value === companyCurrency.value and
set expenseClaim.value.exchange_rate = 1; otherwise, if
expenseClaim.value.exchange_rate is falsy, call exchangeRate.fetch({
from_currency: currency.value, to_currency: companyCurrency.value }) and only
then set exchange_rate_field.hidden = 0. Ensure all checks reference
expenseClaim, currency, companyCurrency, exchange_rate_field and
exchangeRate.fetch in that function.
---
Duplicate comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 119-138: The advances resource captured a stale params object at
creation (params: { expense_claim: expenseClaim.value }) so calling
advances.reload() reuses the old snapshot; instead, when you need fresh data
(where reload() is currently invoked) call advances.fetch and pass the current
params object built from expenseClaim.value (i.e., request the resource with
params:{ expense_claim: expenseClaim.value }) so the request uses the up-to-date
expenseClaim fields; update the call site that uses advances.reload() to use
advances.fetch with the current params and keep existing onSuccess handling
(e.g., the logic that sets expenseClaim.value.advances and calls
calculateTotalAdvance()).
---
Nitpick comments:
In `@frontend/src/composables/useCurrencyConversion.js`:
- Around line 14-17: The fetchCompanyCurrency function is marked async and uses
await even though getCompanyCurrency returns a synchronous value; remove the
async keyword and the await and assign companyCurrency.value directly from
getCompanyCurrency(doc.company). Also update any watcher callback that calls
fetchCompanyCurrency (the reactive watcher in this composable) to call the
now-synchronous fetchCompanyCurrency directly (no need to handle a Promise).
- Around line 41-50: The watcher created by watch(...) inside this composable is
registered every time the caller invokes the function and never cleaned up;
capture the stop handle (const stop = watch(...)) and expose it (e.g., return
stopWatch or stop from the composable) or move the watch out so the caller
manages lifecycle; specifically, change the code that defines the watch around
fetchCompanyCurrency and updateLabels to store the stop handle and return a stop
function alongside updateLabels and companyCurrency so the caller can call
stop() before re-invoking or on unmount.
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 269-293: The watcher is using { deep: true } unnecessarily for
primitive numeric dependencies; in the watch call that observes
expenseClaim.value.total_sanctioned_amount, total_advance_amount, grand_total,
total_claimed_amount, total_taxes_and_charges and exchange_rate, remove the {
deep: true } option so Vue will use the default shallow comparison for these
primitive values; keep the rest of the handler intact (the fieldsToConvert array
and the call to updateBaseFieldsAmount with doc: expenseClaim.value and
exchangeRate: expenseClaim.value.exchange_rate).
- Around line 242-267: The external watch in Form.vue is creating duplicate
watchers because updateCurrencyLabels itself registers a watch on doc.currency;
remove the surrounding watch and instead call updateCurrencyLabels once when
formFields.data and expenseClaim.value.currency are available (e.g., in mounted
or a small computed/nextTick helper) so the composable's internal watcher is the
sole watcher; target the existing updateCurrencyLabels invocation and replace
the watch(...) block with a one-time guarded call that checks for fields and
currency before calling updateCurrencyLabels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4850c360-6a66-409d-ad36-dbbdbb8b3b8b
📒 Files selected for processing (2)
frontend/src/composables/useCurrencyConversion.jsfrontend/src/views/expense_claim/Form.vue
|
This pull request is being marked as inactive because of no recent activity. It will be closed in 3 days if no further activity occurs. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/views/expense_claim/Form.vue (3)
111-114:⚠️ Potential issue | 🟠 MajorAvoid reloading the whole schema on currency changes.
formFields.reload()retriggersformFields.onSuccess(), which reloads company details and writescost_center/payable_accountback to defaults. A currency toggle can therefore wipe a user-selected payable account, and it also replaces the field objects thatsetExchangeRate()just mutated.Also applies to: 151-155, 185-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 111 - 114, The onSuccess handler is causing full schema reloads (via formFields.reload()) which triggers formFields.onSuccess() and resets company defaults (cost_center/payable_account) and overwrites mutations made by setExchangeRate(); instead, stop reloading the entire formFields on currency toggle — remove or avoid calling formFields.reload() in the currency-change path and instead update only the specific fields changed (call a targeted update/patch or mutate the particular field objects and then trigger a minimal reactive refresh). Ensure expenseApproverDetails.reload() and companyDetails.reload() are only invoked where truly necessary and that setExchangeRate() mutations are preserved by updating the same field objects (cost_center, payable_account) rather than replacing them.
177-195:⚠️ Potential issue | 🟠 MajorRefresh the exchange rate whenever the currency pair changes.
This only reacts to
expenseClaim.value.currency, andsetExchangeRate()only fetches whenexpenseClaim.value.exchange_rateis falsy. After the first lookup, changing USD → EUR or changingcompanykeeps the old rate, so the base-field recalculation below uses the previous pair.Also applies to: 269-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 177 - 195, The currency watcher (and the company watcher flow) doesn't force a new lookup because setExchangeRate() only fetches when expenseClaim.value.exchange_rate is falsy; change the flow so that changing the currency pair always refreshes the rate: add a force parameter to setExchangeRate(force = false) and make setExchangeRate bypass the "only when falsy" guard when force is true, then update the watcher for expenseClaim.value.currency (and the company-related watcher that can affect the pair) to call setExchangeRate(true) and then call formFields.reload() after the forced fetch resolves; this ensures functions named setExchangeRate, the currency watcher, and the company watcher refresh the exchange rate whenever the pair changes.
444-450:⚠️ Potential issue | 🟠 MajorZero totals must clear any existing advance allocation.
If the user removes all expenses/taxes after an advance was allocated, Line 450 returns before resetting
allocated_amount,selected, andtotal_advance_amount. The form can keep stale advances and a wronggrand_total.Suggested fix
function allocateAdvanceAmount() { // allocate reqd advance amount let amount_to_be_allocated = parseFloat(expenseClaim.value.total_sanctioned_amount) + parseFloat(expenseClaim.value.total_taxes_and_charges) - if (!amount_to_be_allocated) return + if (!amount_to_be_allocated) { + expenseClaim?.value?.advances?.forEach((advance) => { + advance.allocated_amount = 0 + advance.selected = false + }) + expenseClaim.value.total_advance_amount = 0 + calculateGrandTotal() + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 444 - 450, The allocateAdvanceAmount function exits early when amount_to_be_allocated is falsy, leaving stale advance state; update it so when amount_to_be_allocated is 0 or invalid you explicitly clear/reset advance-related state (set allocated_amount to 0, clear selected allocation(s) and set total_advance_amount to 0) before returning. Locate allocateAdvanceAmount and add a guard that normalizes/parses totals (avoid NaN), then if amount_to_be_allocated === 0 or isNaN(amount_to_be_allocated) perform the resets for allocated_amount, selected, and total_advance_amount and then return.
♻️ Duplicate comments (1)
frontend/src/views/expense_claim/Form.vue (1)
500-517:⚠️ Potential issue | 🟠 Major
exchange_rateis still forced visible in every path.When transaction and company currencies match, Lines 506-509 hide the field, but Line 516 immediately unhides it again. Please also guard against a missing base currency before comparing or fetching.
Suggested fix
function setExchangeRate() { - if (!expenseClaim.value.currency || !formFields.data) return + if (!expenseClaim.value.currency || !formFields.data || !companyCurrency.value) return const exchange_rate_field = formFields.data?.find( (field) => field.fieldname === "exchange_rate" ) if (currency.value === companyCurrency.value) { expenseClaim.value.exchange_rate = 1 if (exchange_rate_field) exchange_rate_field.hidden = 1 + return } + if (exchange_rate_field) exchange_rate_field.hidden = 0 if (!expenseClaim.value.exchange_rate) { exchangeRate.fetch({ from_currency: currency.value, to_currency: companyCurrency.value, }) } - if (exchange_rate_field) exchange_rate_field.hidden = 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/expense_claim/Form.vue` around lines 500 - 517, In setExchangeRate(), avoid unconditionally setting exchange_rate_field.hidden = 0 at the end which re-shows the field in every path; instead make visibility changes conditional: if currency.value and companyCurrency.value are present, set expenseClaim.value.exchange_rate = 1 and hide exchange_rate_field when currency.value === companyCurrency.value, otherwise if expenseClaim.value.exchange_rate is falsy call exchangeRate.fetch({ from_currency: currency.value, to_currency: companyCurrency.value }) and show the field; also add guards at the top to return early if either currency.value or companyCurrency.value is missing to prevent invalid comparisons/fetches. Use the existing symbols: setExchangeRate, expenseClaim.value, formFields.data, exchange_rate_field, currency.value, companyCurrency.value, and exchangeRate.fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 242-267: You are registering updateCurrencyLabels repeatedly via a
watcher on formFields.data and expenseClaim.value.currency; remove this watcher
and instead call updateCurrencyLabels only once to initialize labels when
formFields.data becomes available (or invoke it from onMounted/after
formFields.reload), letting the composable's own watch(() => [doc.company,
doc.currency], ...) handle subsequent updates; locate the current watch block
using updateCurrencyLabels, formFields.data and expenseClaim.value.currency and
replace it with a single initialization call (guarded by if (!fields ||
!currency) return) or trigger the composable's exposed refresh method rather
than re-watching.
- Around line 213-231: The watcher on expenseClaim.value should be changed to
deeply watch the advances array so in-place mutations (like
advances[i].allocated_amount or advances[i].selected from ExpenseAdvancesTable)
trigger re-checking; update the watcher to observe expenseClaim.value.advances
with { deep: true, immediate: true } and keep the same body that flips
advance.selected and calls calculateTotalAdvance() when needed; also audit
similar watchers/functions that rely on selected (e.g., the other watchers
referenced around validateForm and the blocks at lines ~469-479 and ~488-494)
and convert them to deep watchers or watch the advances array directly so
validateForm’s filtering logic stays consistent with calculateTotalAdvance().
---
Outside diff comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 111-114: The onSuccess handler is causing full schema reloads (via
formFields.reload()) which triggers formFields.onSuccess() and resets company
defaults (cost_center/payable_account) and overwrites mutations made by
setExchangeRate(); instead, stop reloading the entire formFields on currency
toggle — remove or avoid calling formFields.reload() in the currency-change path
and instead update only the specific fields changed (call a targeted
update/patch or mutate the particular field objects and then trigger a minimal
reactive refresh). Ensure expenseApproverDetails.reload() and
companyDetails.reload() are only invoked where truly necessary and that
setExchangeRate() mutations are preserved by updating the same field objects
(cost_center, payable_account) rather than replacing them.
- Around line 177-195: The currency watcher (and the company watcher flow)
doesn't force a new lookup because setExchangeRate() only fetches when
expenseClaim.value.exchange_rate is falsy; change the flow so that changing the
currency pair always refreshes the rate: add a force parameter to
setExchangeRate(force = false) and make setExchangeRate bypass the "only when
falsy" guard when force is true, then update the watcher for
expenseClaim.value.currency (and the company-related watcher that can affect the
pair) to call setExchangeRate(true) and then call formFields.reload() after the
forced fetch resolves; this ensures functions named setExchangeRate, the
currency watcher, and the company watcher refresh the exchange rate whenever the
pair changes.
- Around line 444-450: The allocateAdvanceAmount function exits early when
amount_to_be_allocated is falsy, leaving stale advance state; update it so when
amount_to_be_allocated is 0 or invalid you explicitly clear/reset
advance-related state (set allocated_amount to 0, clear selected allocation(s)
and set total_advance_amount to 0) before returning. Locate
allocateAdvanceAmount and add a guard that normalizes/parses totals (avoid NaN),
then if amount_to_be_allocated === 0 or isNaN(amount_to_be_allocated) perform
the resets for allocated_amount, selected, and total_advance_amount and then
return.
---
Duplicate comments:
In `@frontend/src/views/expense_claim/Form.vue`:
- Around line 500-517: In setExchangeRate(), avoid unconditionally setting
exchange_rate_field.hidden = 0 at the end which re-shows the field in every
path; instead make visibility changes conditional: if currency.value and
companyCurrency.value are present, set expenseClaim.value.exchange_rate = 1 and
hide exchange_rate_field when currency.value === companyCurrency.value,
otherwise if expenseClaim.value.exchange_rate is falsy call exchangeRate.fetch({
from_currency: currency.value, to_currency: companyCurrency.value }) and show
the field; also add guards at the top to return early if either currency.value
or companyCurrency.value is missing to prevent invalid comparisons/fetches. Use
the existing symbols: setExchangeRate, expenseClaim.value, formFields.data,
exchange_rate_field, currency.value, companyCurrency.value, and
exchangeRate.fetch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb212c6d-cd27-4ec8-af8f-4095c001cf8b
📒 Files selected for processing (2)
frontend/src/components/ExpensesTable.vuefrontend/src/views/expense_claim/Form.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/ExpensesTable.vue
Comment on lines
+213
to
+231
| watch( | ||
| () => expenseClaim.value, | ||
| (newDoc) => { | ||
| if (newDoc?.advances?.length > 0) { | ||
| let needsRecalc = false | ||
| newDoc.advances.forEach(advance => { | ||
| // Reapply the "selected" flag if money is allocated | ||
| if (parseFloat(advance.allocated_amount || 0) > 0 && !advance.selected) { | ||
| advance.selected = true | ||
| needsRecalc = true | ||
| } | ||
| }) | ||
| if (needsRecalc) { | ||
| calculateTotalAdvance() | ||
| } | ||
| } | ||
| }, | ||
| { immediate: true } | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'ExpenseAdvancesTable\.vue$' -x sh -c '
echo "== {} =="
rg -n -C3 "allocated_amount|selected|v-model:expenseClaim|emit\\(|expenseClaim" "{}"
'Repository: frappe/hrms
Length of output: 1246
🏁 Script executed:
#!/bin/bash
# Check Form.vue at the mentioned line ranges
fd 'Form\.vue$' -path '*/expense_claim/*' -x sh -c '
echo "=== Lines 469-479 ==="
sed -n "469,479p" "{}"
echo ""
echo "=== Lines 488-494 ==="
sed -n "488,494p" "{}"
'Repository: frappe/hrms
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Find Form.vue in expense_claim directory
fd 'Form\.vue' | grep expense_claim
# Read the specific lines from Form.vue
file=$(fd 'Form\.vue' | grep expense_claim)
if [ -n "$file" ]; then
echo "=== Lines 469-479 ==="
sed -n '469,479p' "$file"
echo ""
echo "=== Lines 488-494 ==="
sed -n '488,494p' "$file"
fiRepository: frappe/hrms
Length of output: 768
🏁 Script executed:
#!/bin/bash
# Find all watch declarations in Form.vue
rg -n "watch\(" frontend/src/views/expense_claim/Form.vue | head -20Repository: frappe/hrms
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Get more context around the watch statements
rg -n "watch\(" -A 5 frontend/src/views/expense_claim/Form.vue | head -100Repository: frappe/hrms
Length of output: 1679
🏁 Script executed:
#!/bin/bash
# Get the full watcher at line 205-210 with more context
sed -n '205,232p' frontend/src/views/expense_claim/Form.vueRepository: frappe/hrms
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Get context around calculateTotalAdvance and validateForm to understand the flow
echo "=== calculateTotalAdvance (around 469) ==="
sed -n '469,480p' frontend/src/views/expense_claim/Form.vue
echo ""
echo "=== validateForm (around 488) ==="
sed -n '488,500p' frontend/src/views/expense_claim/Form.vue
echo ""
echo "=== Check if there are other watchers watching expenseClaim value (shallow) ==="
rg -n "watch\(\s*\(\)\s*=>\s*expenseClaim\.value," frontend/src/views/expense_claim/Form.vueRepository: frappe/hrms
Length of output: 988
🏁 Script executed:
#!/bin/bash
# Check the exact structure of the watchers - is there any other shallow watch on expenseClaim.value?
echo "=== All watch calls with their complete structure ==="
sed -n '205,280p' frontend/src/views/expense_claim/Form.vue | grep -A 6 "watch("Repository: frappe/hrms
Length of output: 1013
Change this shallow watcher to deeply watch the advances array so it re-applies the selected flag when allocated_amount is mutated.
watch(() => expenseClaim.value, ...) only triggers when the claim object reference changes. When ExpenseAdvancesTable mutates advances[i].allocated_amount or selected in place, this callback never re-applies the selected flag. Meanwhile, calculateTotalAdvance() counts allocations where either selected is true OR allocated_amount > 0, so unsaved amounts get included in the total. When validateForm() later filters advances to only those with selected: true, the count becomes inaccurate.
Change to: watch(() => expenseClaim.value.advances, ..., { deep: true, immediate: true })
This applies to other watchers or functions that depend on the selected flag state at lines 469-479 and 488-494.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/expense_claim/Form.vue` around lines 213 - 231, The
watcher on expenseClaim.value should be changed to deeply watch the advances
array so in-place mutations (like advances[i].allocated_amount or
advances[i].selected from ExpenseAdvancesTable) trigger re-checking; update the
watcher to observe expenseClaim.value.advances with { deep: true, immediate:
true } and keep the same body that flips advance.selected and calls
calculateTotalAdvance() when needed; also audit similar watchers/functions that
rely on selected (e.g., the other watchers referenced around validateForm and
the blocks at lines ~469-479 and ~488-494) and convert them to deep watchers or
watch the advances array directly so validateForm’s filtering logic stays
consistent with calculateTotalAdvance().
Comment on lines
+242
to
+267
| watch( | ||
| () => [formFields.data, expenseClaim.value.currency], | ||
| ([fields, currency]) => { | ||
| if (!fields || !currency) return | ||
|
|
||
| updateCurrencyLabels({ | ||
| formFields: fields, | ||
| doc: expenseClaim.value, | ||
| baseFields: [ | ||
| "base_total_sanctioned_amount", | ||
| "base_total_taxes_and_charges", | ||
| "base_total_advance_amount", | ||
| "base_grand_total", | ||
| "base_total_claimed_amount" | ||
| ], | ||
| transactionFields: [ | ||
| "total_sanctioned_amount", | ||
| "total_taxes_and_charges", | ||
| "total_advance_amount", | ||
| "grand_total", | ||
| "total_claimed_amount" | ||
| ], | ||
| }) | ||
| }, | ||
| { immediate: true } | ||
| ) |
Contributor
There was a problem hiding this comment.
updateCurrencyLabels() is being subscribed over and over.
The composable already installs its own watch(() => [doc.company, doc.currency], ...). Calling it from another watcher here registers a new watcher every time formFields.data or currency changes, so currency toggles / formFields.reload() will stack extra company-currency fetches and label mutations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/expense_claim/Form.vue` around lines 242 - 267, You are
registering updateCurrencyLabels repeatedly via a watcher on formFields.data and
expenseClaim.value.currency; remove this watcher and instead call
updateCurrencyLabels only once to initialize labels when formFields.data becomes
available (or invoke it from onMounted/after formFields.reload), letting the
composable's own watch(() => [doc.company, doc.currency], ...) handle subsequent
updates; locate the current watch block using updateCurrencyLabels,
formFields.data and expenseClaim.value.currency and replace it with a single
initialization call (guarded by if (!fields || !currency) return) or trigger the
composable's exposed refresh method rather than re-watching.
| "`tabExpense Claim`.name", | ||
| "`tabExpense Claim`.employee", | ||
| "`tabExpense Claim`.employee_name", | ||
| "`tabExpense Claim`.currency", |
Member
There was a problem hiding this comment.
Currency shows up in list view but not in recent expenses
Before:
After:
…company currency - also simplify value emit in Link field
ruchamahabal
force-pushed
the
fix/v16_changes_pwa
branch
from
f0bccf5 to
3908367
Compare
| () => setExchangeRate() | ||
| (currency) => { | ||
| if (!currency) return | ||
| formFields.reload() |
Member
There was a problem hiding this comment.
Why do we need to reload formfields when currency changes?
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.
Changes done
expenseClaim.PWA.mov
useCurrencyConversioncomposable in whichupdateCurrencyLabelsfunc helps to dynamically update currency field labels to reflect transaction and company base currencyupdateBaseFieldsAmountfunc helps to convert base field amount to base currencyRemoved employee currency fetch from salary strucutre since the currency field is added in the Employee Advance & also removed the exchange rate fetch as the field is removed from the form
Removed employee advance fetch & it's api as currency filter is added in the field to show account as per the currency selected
Improved advances resources as it's api is also updated & updated the logic to get data & make it selected as per the sanctioned amount
Updated Link component to reload options when filters change -- Contribution by @ruchamahabal
Closes: #3986
Summary by CodeRabbit
New Features
Improvements
Chores