fix(pwa): multicurrency support in expense & advance forms#4083
fix(pwa): multicurrency support in expense & advance forms#4083iamkhanraheel wants to merge 6 commits intofrappe:developfrom
Conversation
iamkhanraheel
changed the title
|
@Mergifyio rebase |
…n claim, fix currency value in list & form view
✅ Branch has been successfully rebased |
iamkhanraheel
force-pushed
the
fix/v16_changes_pwa
branch
from
6ef3a41 to
cde96b3
Compare
WalkthroughThis pull request refactors currency handling across the frontend and removes a backend API endpoint. New composable functions 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 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
| <span class="whitespace-pre"> · </span> | ||
| <span class="whitespace-nowrap"> | ||
| {{ formatCurrency(props.doc.total_claimed_amount, currency) }} | ||
| {{ formatCurrency(props.doc.total_claimed_amount, props.doc.currency) }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for get_expense_claims API definition
rg -n "def get_expense_claims" --type py -A 20Repository: frappe/hrms
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Also search for any API response or field definitions for expense claims
rg -n "get_expense_claims" --type py -B 2 -A 10Repository: frappe/hrms
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Search for expense claim field definitions or serializers
rg -n "currency" --type py -i | grep -i "expense" -A 2 -B 2 | head -50Repository: frappe/hrms
Length of output: 5000
🏁 Script executed:
#!/bin/bash
# Get the complete get_expense_claims function to see all fields being fetched
sed -n '486,530p' hrms/api/__init__.pyRepository: frappe/hrms
Length of output: 1357
Add currency to the fields fetched by get_expense_claims API.
The get_expense_claims function in hrms/api/__init__.py does not fetch the currency field, so props.doc.currency will be undefined. This will cause formatCurrency to fail silently. Add \tabExpense Claim`.currency` to the fields list in the API function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ExpenseClaimItem.vue` at 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.
| // function to update base currency fields data | ||
| export function updateBaseFieldsAmount({doc, fields, exchangeRate}) { | ||
| if (!doc) return; | ||
| const excahnge_rate = flt(exchangeRate || doc.exchange_rate || 1, 9); |
There was a problem hiding this comment.
Typo in variable name: excahnge_rate → exchange_rate.
Proposed fix
- const excahnge_rate = flt(exchangeRate || doc.exchange_rate || 1, 9);
+ const exchange_rate = flt(exchangeRate || doc.exchange_rate || 1, 9);
fields.forEach(f => {
- const val = flt(flt(doc[f]) * excahnge_rate);
+ const val = flt(flt(doc[f]) * exchange_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.
| const excahnge_rate = flt(exchangeRate || doc.exchange_rate || 1, 9); | |
| const exchange_rate = flt(exchangeRate || doc.exchange_rate || 1, 9); | |
| fields.forEach(f => { | |
| const val = flt(flt(doc[f]) * exchange_rate); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/composables/useCurrencyConversion.js` at 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.
| const advances = createResource({ | ||
| url: "hrms.hr.doctype.expense_claim.expense_claim.get_advances", | ||
| params: { employee: currEmployee.value }, | ||
| params: { expense_claim: expenseClaim.value }, | ||
| auto: true, | ||
| transform(data) { | ||
| if (!data) return [] | ||
| return data.map((item) => ({ | ||
| ...item, | ||
| selected: parseFloat(item.allocated_amount || 0) > 0, | ||
| allocated_amount: item.allocated_amount || 0 | ||
| })) | ||
| }, | ||
| onSuccess(data) { | ||
| // set advances | ||
| if (props.id) { | ||
| expenseClaim.value.advances?.map((advance) => (advance.selected = true)) | ||
| } else { | ||
| expenseClaim.value.advances = [] | ||
| // Only replace if the resource found data | ||
| if (data && data.length > 0) { | ||
| expenseClaim.value.advances = data | ||
| calculateTotalAdvance() | ||
| } | ||
|
|
||
| return data.forEach((advance) => { | ||
| if ( | ||
| props.id && | ||
| expenseClaim.value.advances?.some( | ||
| (entry) => entry.employee_advance === advance.name | ||
| ) | ||
| ) | ||
| return | ||
|
|
||
| expenseClaim.value.advances?.push({ | ||
| employee_advance: advance.name, | ||
| purpose: advance.purpose, | ||
| posting_date: advance.posting_date, | ||
| advance_account: advance.advance_account, | ||
| advance_paid: advance.paid_amount, | ||
| unclaimed_amount: advance.paid_amount - advance.claimed_amount, | ||
| allocated_amount: 0, | ||
| }) | ||
| }) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Stale params when reloading advances resource.
The params object is captured once at resource creation time. When advances.reload() is called (line 197), it reuses these stale initial params rather than the current expenseClaim.value. At creation time, expenseClaim.value may not yet have populated fields like employee.
Use .fetch() with current params instead of .reload() to ensure fresh data is sent:
Proposed fix
Update line 197 to pass current expense claim data:
- advances.reload()
+ advances.fetch({ expense_claim: expenseClaim.value })Alternatively, make params a function if frappe-ui supports it, or remove auto: true and trigger the initial fetch explicitly after data is populated.
🤖 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 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.
| function setExchangeRate() { | ||
| if (!expenseClaim.value.currency || !formFields.data) 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 | ||
| } else { | ||
| exchangeRate.fetch({ | ||
| from_currency: currency.value, | ||
| to_currency: companyCurrency.value, | ||
| }) | ||
| if (exchange_rate_field) exchange_rate_field.hidden = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against undefined companyCurrency in exchange rate logic.
If expenseClaim.value.company isn't set, companyCurrency.value could be undefined. The comparison currency.value === companyCurrency.value would then evaluate to true (undefined === undefined), incorrectly setting exchange_rate to 1 and hiding the field before currencies are properly initialized.
Proposed 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"
)📝 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.
| function setExchangeRate() { | |
| if (!expenseClaim.value.currency || !formFields.data) 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 | |
| } else { | |
| exchangeRate.fetch({ | |
| from_currency: currency.value, | |
| to_currency: companyCurrency.value, | |
| }) | |
| if (exchange_rate_field) exchange_rate_field.hidden = 0 | |
| } | |
| } | |
| function setExchangeRate() { | |
| 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 | |
| } else { | |
| 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 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).
|
@Mergifyio rebase |
☑️ Nothing to do, the required conditions are not metDetails
|
2 participants
Closes: #3986
Summary by CodeRabbit
Release Notes
New Features
Improvements