[$500] On distance update, if amount is same as before, changed merchant message is displayed

m-natarajan commented 11 months 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: 1.3.87-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697731583309229

Action Performed:

Open the app
Open any distance request report or raise one and open
Note down current amount and current start finish
Click on Distance and change start or finish address OR invert start finish addresses
It will change amount and add new changed the distance message which is correct
Open amount and change it to amount which was noted in step 3
Open distance and change start finish again to addresses which were noted in step 3
Observe that since now, there was no change in amount after change in addreses, app displays changed merchant message instead of changed distance message
Expected Result:

App should display changed distance message whenever we change distance in distance request report

Actual Result:

App displays changed merchant message instead of changed distance message when even after changing distance, amount remains same

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/38435837/396b33a6-b874-40b1-87ff-ffc5779ea381

Android: mWeb Chrome

iOS: Native

https://github.com/Expensify/App/assets/38435837/9dd83152-b559-4281-aa59-b1418e609faf

iOS: mWeb Safari

https://github.com/Expensify/App/assets/38435837/12a23174-5741-4640-8bcb-00d4743795fd

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/38435837/30267ac8-64d4-4580-a793-0ce1f004c595 https://github.com/Expensify/App/assets/38435837/1ef81ec6-b2b5-4ba2-b1f5-0428769e477d

MacOS: Desktop

https://github.com/Expensify/App/assets/38435837/f21e800e-a23d-4549-95e9-1f827200669d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013c6b74407df4721a
Upwork Job ID: 1715177403214991360
Last Price Increase: 2023-12-01

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 11 months ago

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

abzokhattab commented 11 months ago

I think this is a backend issue .. as in the case of waypoints we send the request to the backend containing the waypoint0 and waypoint1 and the backend sends back the modified fields .. so in the current issue, the backend doesnt send the modifiedAmount in the response

https://github.com/Expensify/App/blob/main/src/libs/actions/IOU.js#L779-L786

barros001 commented 11 months ago

Proposal

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

When editing a distance request and updating the address but which will result in the exact same amount, the message displayed is:

changed the merchant to "11.03 mi @ $0.655 / mi" (previously "12.8 mi @ $0.655 / mi")

instead of:

changed the distance to 11.03 mi @ $0.655 / mi (previously 12.8 mi @ $0.655 / mi)

What is the root cause of that problem?

Method getModifiedExpenseMessage is responsible for generating the message after a request is edited. The only time it would generate a changed distance message is when the amount also changes, as can be seen here:

    if (hasModifiedAmount) {
        const oldCurrency = reportActionOriginalMessage.oldCurrency;
        const oldAmount = CurrencyUtils.convertToDisplayString(reportActionOriginalMessage.oldAmount, oldCurrency);

        const currency = reportActionOriginalMessage.currency;
        const amount = CurrencyUtils.convertToDisplayString(reportActionOriginalMessage.amount, currency);

        // Only Distance edits should modify amount and merchant (which stores distance) in a single transaction.
        // We check the merchant is in distance format (includes @) as a sanity check
        if (hasModifiedMerchant && reportActionOriginalMessage.merchant.includes('@')) {
            return getProperSchemaForModifiedDistanceMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, amount, oldAmount);
        }

        return getProperSchemaForModifiedExpenseMessage(amount, oldAmount, Localize.translateLocal('iou.amount'), false);
    }

When only distance is changed, the flag hasModifiedMerchant is set to true which calls the following:

    if (hasModifiedMerchant) {
        return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, Localize.translateLocal('common.merchant'), true);
    }

The MofifiedExpenseMessage reads as follow:

        updatedTheRequest: ({valueName, newValueToDisplay, oldValueToDisplay}: UpdatedTheRequestParams) =>
            `changed the ${valueName} to ${newValueToDisplay} (previously ${oldValueToDisplay})`,

where valueName is merchant.

Distance is stored in the merchant field, which causes this problem. The system thinks the merchant changed and not the distance as it can't really tell at that point what the merchant field is storing.

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

getModifiedExpenseMessage method needs to know what the merchant attribute actually holds: merchant or distance.

merchant field will either have to be split in two (merchant and distance) or getModifiedExpenseMessage will have to be given context as to what type merchant holds. This will either have to be fixed on the backend, adding the type (Distance, or something else) to reportAction or by passing it around everywhere getModifiedExpenseMessage is called.

This is honestly a poor architecture issue and resolving the real root cause will require deeper changes to the architecture.

As a stopgap solution for the immediate problem, we can match merchant against a patter such as [0-9.]+ mi @ $[0-9]+ / m to determine whether this is a distance change or a merchant change.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

paultsimura commented 11 months ago

Proposal

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

After the distance request had amount changed manually, the waypoints modification do not change the amount.

What is the root cause of that problem?

There are actually 2 faulty scenarios here. Let's call them Issue 1 and Issue 2.

Issue 1

If we update the waypoints by simply switching the Start and Finish points, the UpdateDistanceRequest API call does not return a merge operation for the transaction. It must return this operation with the modified waypoints.

This is the response for the operation with switched start and finish:

This is the response for the operation with changed waypoints (added/removed):

Issue 2

This issue is a bit deeper than just "not updating the amount" – it's reverting the waypoints to the previous state. This happens when we have a 3-waypoints route, then remove one waypoint, and then do any kind of modification of the waypoints.

When executing the UpdateDistanceRequest API call, we build the Onyx data with waypoints of the updated transaction here:

https://github.com/Expensify/App/blob/b1ae0c20bd6926cf03ebd7d011beb2ee61c1814a/src/libs/TransactionUtils.ts#L148-L151

Let's take a look at what happens when we remove one waypoint. The simple merge operation for the following object:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
            "waypoint2": { wp-2 }
        }
    }
}

To the following object:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
        }
    }
}

...will result in the same as pre-merge:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
            "waypoint2": { wp-2 }
        }
    }
}

This is because the waypoint2 merge key was not passed to Onyx, so it's ignored, as Onyx replaces values key-by-key to the latest nested object. So this merge operation modifies only waypoint0 and waypoint1, leaving the waypoint2 untouched.

These waypoints are later used to build the next optimistic transaction when modifying the request, which causes the buggy behavior.

Recording that shows the broken flow with the corresponding Onyx data:

https://github.com/Expensify/App/assets/12595293/f3ec7df6-86c4-49ae-a4d0-0930f11c56ae

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

Issue 1

This issue requires BE changes. The API response must return a merge transaction operation with the modified waypoints, even if the distance & price didn't change (because the start and finish were just flipped).

Issue 2

We should explicitly mark the deleted waypoints as null when passing them to Onyx.

First, create a new function in TransactionUtils:

function getReplacementWaypoints(transaction: Transaction, updatedWaypoints: WaypointCollection): WaypointCollection {
    const existingWaypoints = getWaypoints(transaction) ?? {};

    if (Object.keys(updatedWaypoints).length >= Object.keys(existingWaypoints).length) {
        return updatedWaypoints;
    }

    return lodashReduce(existingWaypoints, (result, value, key) => ({
        ...result,
        // Explicitly set the non-existing waypoints to null so that Onyx can remove them
        [key]: updatedWaypoints[key] ?? null,
    }), {});
}

Second, use it here when building the updated transaction:

    if (Object.hasOwn(transactionChanges, 'waypoints')) {
-       updatedTransaction.modifiedWaypoints = transactionChanges.waypoints;
+       updatedTransaction.modifiedWaypoints = getReplacementWaypoints(transaction, transactionChanges.waypoints!!);
        shouldStopSmartscan = true;
    }

Third, we are also passing these waypoints to MapBox on the API call. We pass them as a stringified JSON, and there we should remove the null-ish waypoints:

https://github.com/Expensify/App/blob/3b86da6dc1ec2ca15033bbdc09702892966c2580/src/libs/actions/IOU.js#L697

We should use the following:

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transactionDetails.waypoints, true)),

Similar to what we do on the createDistanceRequest:

https://github.com/Expensify/App/blob/3b86da6dc1ec2ca15033bbdc09702892966c2580/src/libs/actions/IOU.js#L649

This solves the issue of invalid updates on the waypoint removal.

Result:

https://github.com/Expensify/App/assets/12595293/cbf9d9a0-f337-4032-b1c8-ea12e109e4e2

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 11 months ago

@muttmuure, @robertKozik Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

robertKozik commented 11 months ago

@paultsimura Can we be sure that after the BE would be fixed to return the missing merge operation, distance message will be properly set?

paultsimura commented 11 months ago

@paultsimura Can we be sure that after the BE would be fixed to return the missing merge operation, distance message will be properly set?

Yes, it should fix the message, but with one remark: actually, there are 3 Onyx operations missing on waypoints swap:

Operation with added waypoint:
Operation with swapped waypoints:

I believe, there is some check on the BE side that, if the amount doesn't change, doesn't fire these 3 events. We should consider, for distance requests, checking not just if the amount has changed, but whether the route has changed.

These operations will provide the necessary updates that will be used here to calculate the new message:

https://github.com/Expensify/App/blob/7d6e94be0f124c07c6563aa09b1baac272dfd662/src/libs/ReportUtils.js#L1741-L1745

We just need to decide what is the expected behavior here: do we want to get any kind of message if the waypoints are swapped? If this is the case, we might end up getting messages like this (because technically the distance and amount did not change):

changed the distance to 10 mi @ zł0.655 / mi (previously 10 mi @ zł0.655 / mi), which updated the amount to PLN 6.5 (previously PLN 6.5)

paultsimura commented 11 months ago

Actually @robertKozik the deeper I dig into this flow, the more inconsistent it looks. I need some time to narrow down the buggy waypoints swap flow

barros001 commented 11 months ago

I'll chime in here. I'm brand new to the codebase and could be missing something here, so please keep that in mind.

I believe @paultsimura is onto something here, but it does look like it's a different problem, and solving it would not really solve the message problem the current issue is trying to address.

My understanding is that this issue is about the message that's displayed when the distance changes but the amount remains the same. When this happens, the message displayed says the Merchant have been updated instead of saying the Distance.

There reason for this is because getModifiedExpenseMessage (which is the method responsible for generating the message) cannot tell whether the modified value was a distance or a merchant, because the same attribute (merchant) is used to store both values:

    const hasModifiedAmount =
        _.has(reportActionOriginalMessage, 'oldAmount') &&
        _.has(reportActionOriginalMessage, 'oldCurrency') &&
        _.has(reportActionOriginalMessage, 'amount') &&
        _.has(reportActionOriginalMessage, 'currency');

    console.debug({reportActionOriginalMessage, reportAction});

    const hasModifiedMerchant = _.has(reportActionOriginalMessage, 'oldMerchant') && _.has(reportActionOriginalMessage, 'merchant');

When merchant only has changed, hasModifiedAmount will be false and hasModifiedMerchant will be true.

Later one we have this:

    if (hasModifiedMerchant) {
        return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, Localize.translateLocal('common.merchant'), true);
    }

Note the hardcoded Localize.translateLocal('common.merchant'). getProperSchemaForModifiedExpenseMessage will eventually render the following message:

        updatedTheRequest: ({valueName, newValueToDisplay, oldValueToDisplay}: UpdatedTheRequestParams) =>
            `changed the ${valueName} to ${newValueToDisplay} (previously ${oldValueToDisplay})`,

valueName will be always set to merchant in this scenario because there's simply not enough information to determine whether the changed value represents a merchant or a distance. As I mentioned on my proposal, I don't think there's a clean way (other than doing a pattern match against the merchant attribute, which on itself is not really very clean) without making BE changes.

Would it make sense to raise a new issue to handle the issues with the waypoints that @paultsimura uncovered, which appears to be a new issue?

paultsimura commented 11 months ago

Would it make sense to raise a new issue to handle the issues with the waypoints that @paultsimura uncovered, which appears to be a new issue?

I'd agree with @barros001 here – I misinterpreted this issue from the beginning. Reported that one separately.

melvin-bot[bot] commented 11 months ago

@muttmuure, @robertKozik Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

paultsimura commented 11 months ago

Proposal

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

After the distance request had amount changed manually, the waypoints modification show incorrect system message (if the amount was the same).

What is the root cause of that problem?

The issue is that we use kind of a hack to determine whether the distance was changed: we check the merchant change and use RegExp:

https://github.com/Expensify/App/blob/a59d56f00a67607a1dbae1b422807f76f548f1f0/src/libs/ReportUtils.js#L1861-L1865

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

First, I suggest re-thinking the "Update Route" approach, starting from BE.

The way all the other money requests updates are handled now is the following (let's take the example of a manual money request):

The transaction.amount field remains unchanged – it reflects the initially requested amount. Instead, the transaction.modifiedAmount is updated on every modification.

With the distance requests, we currently do it differently – we update the original transaction.comment.waypoints.

We should implement the same approach for the distance requests modifications. Update the transaction.modifiedWaypoints, and keep the original transaction.comment.waypoints untouched.

We already build the updatedTransaction.modifiedWaypoints here: https://github.com/Expensify/App/blob/86094548d6d97939da8e589ad89a169c0bf683e6/src/libs/TransactionUtils.ts#L148-L151

And we read this value when retrieving the waypoints:

https://github.com/Expensify/App/blob/86094548d6d97939da8e589ad89a169c0bf683e6/src/libs/TransactionUtils.ts#L273-L278

But it's set only optimistically, it's not updated in the BE on the Distance Requests modification, and is not returned from the API call (all the other modified* fields are returned):

{
  "onyxMethod": "merge",
  "key": "transactions_8849286097733348684",
  "value": {
    "amount": -11234,
    "billable": false,
    "cardID": 18550310,
    "category": "Car",
    "comment": {
      "comment": "",
      "customUnit": {
        "attributes": [],
        "customUnitID": "C4FC57BEA2908",
        "customUnitRateID": "2C52D2B1A4916",
        "name": "Distance",
        "quantity": 387.54,
        "subRates": []
      },
      "type": "customUnit",
      "waypoints": {
        "waypoint0": {
          "address": "Kraków, Poland",
          "lat": 50.06465009999999,
          "lng": 19.9449799,
          "name": "Kraków"
        },
        "waypoint1": {
          "address": "Wrocław, Poland",
          "lat": 51.1078852,
          "lng": 17.0385376,
          "name": "Wrocław"
        },
        "waypoint2": {
          "address": "Warsaw, Poland",
          "lat": 52.2296756,
          "lng": 21.0122287,
          "name": "Warsaw"
        }
      }
    },
    "created": "2023-10-29",
    "currency": "PLN",
    "filename": "w_9c6814e87c3ca9e256322b24606a3975e0fa57a6.png",
    "merchant": "171.51 mi @ zł0.655 / mi",
    "modifiedAmount": -25384,
    "modifiedCreated": "2023-10-29",
    "modifiedCurrency": "PLN",
    "modifiedMerchant": "387.54 mi @ zł0.655 / mi",
    "originalAmount": 0,
    "originalCurrency": "",
    "parentTransactionID": "",
    "receipt": {
      "receiptID": 669153517,
      "state": "OPEN",
      "source": "https://www.expensify.com/receipts/w_9c6814e87c3ca9e256322b24606a3975e0fa57a6.png"
    },
    "reimbursable": true,
    "reportID": "8023553457850147",
    "status": "Posted",
    "tag": "",
    "transactionID": "8849286097733348684",
    "hasEReceipt": false
  }
}

Second, after the BE changes are done, we should change the logic for building the distance messages in this function:

https://github.com/Expensify/App/blob/a59d56f00a67607a1dbae1b422807f76f548f1f0/src/libs/ReportUtils.js#L1832-L1901

We should check for hasModifiedWaypoints similar to the other checks, and build the distance message if the waypoints have changed.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 11 months ago

@muttmuure, @robertKozik 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

muttmuure commented 11 months ago

@robertKozik some proposals above for you to take a look at

melvin-bot[bot] commented 11 months ago

@muttmuure @robertKozik this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@muttmuure, @robertKozik Huh... This is 4 days overdue. Who can take care of this?

muttmuure commented 10 months ago

Bump @robertKozik

melvin-bot[bot] commented 10 months ago

@muttmuure @robertKozik this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 10 months ago

@muttmuure @robertKozik this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

muttmuure commented 10 months ago

Unable to test this again, I'm not sure it's a big priority tbh

DylanDylann commented 10 months ago

Proposal

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

On distance update, if amount is same as before, changed merchant message is displayed

What is the root cause of that problem?

The BE is only return oldMerchant and merchant in the reportAction.originalMessage. So that the App only trigger this condition https://github.com/Expensify/App/blob/f08baad07d44bdbccaf2ebc31e72e82f6dfb40b1/src/libs/ReportUtils.js#L1990-L1992

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

I think we should remove this condition https://github.com/Expensify/App/blob/f08baad07d44bdbccaf2ebc31e72e82f6dfb40b1/src/libs/ReportUtils.js#L1970-L1972

and create another one in the before of https://github.com/Expensify/App/blob/f08baad07d44bdbccaf2ebc31e72e82f6dfb40b1/src/libs/ReportUtils.js#L1961

like this

if (TransactionUtils.isDistanceRequest(transaction) && hasModifiedMerchant && reportActionOriginalMessage.merchant.includes('@')) {
            if (hasModifiedAmount) {

                return getProperSchemaForModifiedDistanceMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, displayAmountChange: true ,amount, oldAmount);
            } else {

                return getProperSchemaForModifiedDistanceMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, displayAmountChange: false);
            }

        }

we also need to create new param displayAmountChange in the getProperSchemaForModifiedDistanceMessage function

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@muttmuure, @robertKozik Still overdue 6 days?! Let's take care of this!

muttmuure commented 10 months ago

Still seems like low priority but @robertKozik if you could take a look at some of the proposals that would be helpful

muttmuure commented 10 months ago

Still lower priority

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 9 months ago

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

muttmuure commented 9 months ago

I can't actually reproduce this now.

paultsimura commented 9 months ago

@muttmuure I've just reproduced on latest app version:

cubuspl42 commented 9 months ago

@muttmuure Would you confront your findings with what @paultsimura found? Do we need to share each other's more precise reproduction steps?

Expensify / App