Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.02k stars 3.01k forks source link

[$250] Track - Error submitting distance expense to workspace created from "Share with accountant" #56374

Open vincdargento opened 5 days ago

vincdargento commented 5 days ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.94-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): sjdnojsdoijsdo@gmail.com Issue reported by: Applause Internal Team Device used: Mac 15.0 / Chrome App Component: Money Requests

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. Go to self DM.
  3. Submit a manual expense to self DM.
  4. Click Share it with my accountant.
  5. Enter merchant and submit the expense to the new workspace.
  6. Go back to self DM.
  7. Submit a distance expense to self DM.
  8. Click Share it with my accountant.
  9. Select the workspace created in Step 5.
  10. Click Rate and select a rate.
  11. Click Create on the confirmation page.
  12. Go to transaction thread.

Expected Result:

The distance expense is submitted to the workspace chat without issue.

Actual Result:

The distance expense cannot be submitted and it throws error - Unexpected error submitting this expense. Please try again later. The selected rate has been deleted.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/b75482f7-a537-41e6-9906-8fc049ef8935

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021887103848568086243
  • Upwork Job ID: 1887103848568086243
  • Last Price Increase: 2025-02-05
Issue OwnerCurrent Issue Owner: @shubham1206agra
melvin-bot[bot] commented 5 days ago

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

bernhardoj commented 5 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error sharing a distance request to a workspace after sharing a manual expense without previously having a workspace.

What is the root cause of that problem?

When we don't have any workspace and share the manual expense, it will create a draft workspace/policy, which includes a draft/optimistic custom unit rate. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/ReportUtils.ts#L8812

Let's say the optimistic custom unit is A. When we confirm the share, we will be navigated to the workspace chat. The OpenReport API will then return the full workspace/policy onyx data, which includes another custom unit from the BE, let's say it's B.

Now, when we want to share the distance request to the new workspace just created, an error appears on the confirmation page telling that the P2P rate is invalid and we need to choose a valid rate available from the workspace. When we open the distance rate page, there is only 1 rate on the list (0.67). If we use it, then we will get the error. Why?

Because the selected rate is from the optimistic custom unit A rate, but we need to use the custom unit B rate to be able to share the expense successfully.

The custom unit A is being used because it's the first distance custom unit found on the policy.customUnits.

https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx#L58 https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/DistanceRequestUtils.ts#L51 https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/PolicyUtils.ts#L151-L152

What changes do you think we should make in order to solve the problem?

I guess that there could only be 1 distance custom unit at a time. To fix this, we can remove the optimistic custom unit A after successfully sharing the tracked expense. In getTrackExpenseInformation, there is already a code to build the policy onyx data from the existing policy draft.

https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/IOU.ts#L3044-L3049

We can clear the optimistic custom unit inside buildPolicyData. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/Policy/Policy.ts#L1867-L1884

customUnits: {
    [customUnitID]: null,
},

We can conditionally apply the clearing since buildPolicyData is being used in other places too.

Btw, I noticed another issue where the workspace rate is in miles, but the transaction rate is still in km. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx#L96-L97

We can fix it by passing the selected rate unit and update the transaction draft using the new unit.

const unit = rates[customUnitRateID].unit;
IOU.setMoneyRequestDistanceRate(transactionID, customUnitRateID, policy?.id ?? '-1', !isEditing, unit);

https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/IOU.ts#L936-L938

Onyx.merge(..., {comment: {customUnit: {customUnitRateID: rateID, distanceUnit: unit}}});

Let me know if we should fix this too!

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

melvin-bot[bot] commented 5 days ago

Job added to Upwork: https://www.upwork.com/jobs/~021887103848568086243

melvin-bot[bot] commented 5 days ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External)

Ariqkip commented 3 days ago

Does the issue still need assistance or its already assigned

Ariqkip commented 3 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error submitting distance expense to workspace created from "Share with accountant"

What is the root cause of that problem?

When a manual expense is submitted without an existing workspace, the system creates a draft workspace with an optimistic distance rate.

Problematic Code:

1️⃣ Draft workspace creates an optimistic distance rate

📌 File: App/src/libs/ReportUtils.ts
📍 Code (Line 8812):

return createDraftWorkspaceAndNavigateToConfirmationScreen(transactionID, actionName);

Issue: This function creates a workspace draft with an optimistic rate that persists after the actual workspace data is received.

2️⃣ App incorrectly selects the first available (outdated) rate

📌 File: App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx
📍 Code (Line 58):

const rates = DistanceRequestUtils.getMileageRates(policy, false, currentRateID);

Issue: The getMileageRates() function selects the first available distance rate, which could be the outdated optimistic rate instead of the correct backend rate.

3️⃣ Function fetching the distance unit doesn't filter outdated rates

📌 File: App/src/libs/DistanceRequestUtils.ts
📍 Code (Line 51):

const distanceUnit = getDistanceRateCustomUnit(policy);

Issue: This function retrieves the first custom unit without checking if it has been synced with the backend.


What changes do you think we should make in order to solve the problem?

1️⃣ Remove outdated optimistic distance rates after a successful share

Modify buildPolicyData() in Policy.ts to clear outdated distance rates after receiving the final workspace data from the backend.

📌 File: App/src/libs/actions/Policy/Policy.ts

Fix: Ensures that outdated distance rates are removed after workspace creation.


2️⃣ Ensure the latest backend distance rate is always selected

Modify getDistanceRateCustomUnit() in PolicyUtils.ts to fetch only backend-synced rates.

📌 File: App/src/libs/PolicyUtils.ts

Fix: Ensures that the selected rate comes from the backend and not an outdated optimistic unit.


3️⃣ Fix distance unit mismatches (Miles vs. Kilometers)

Modify setMoneyRequestDistanceRate() in IOU.ts to pass the correct unit along with the rate ID.

📌 File: App/src/libs/actions/IOU.ts
📍 Code Update (Lines 936-938):

function setMoneyRequestDistanceRate(transactionID: string, rateID: string, policyID: string, isDraft: boolean, unit?: string) { 
    Onyx.merge(ONYXKEYS.NVP_LAST_SELECTED_DISTANCE_RATES, {[policyID]: rateID}); 
    Onyx.merge(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {comment: {customUnit: {customUnitRateID: rateID, distanceUnit: unit}}});
}

Fix: Prevents mismatched distance units (miles vs. kilometers) when submitting distance requests.


What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A



**Email:** ariqngoneshi@gmail.com

**Upwork Profile:https://www.upwork.com/freelancers/~01e8fca580ad3567b3
shubham1206agra commented 1 day ago

@bernhardoj Can we not adapt the changes from https://github.com/Expensify/App/pull/54669/files?

Let me know if we need BE changes and I will ping the internal team about this.

shubham1206agra commented 1 day ago

@paultsimura If you want to take a look into this issue and give a proposal here, since you worked on this flow.

bernhardoj commented 1 day ago

Sorry, I don't get what you mean by "not adapt the changes".

Let me know if we need BE changes

No need BE changes from my testing when working on the proposal