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
3.36k stars 2.78k forks source link

Distance - System message contains blank info when submitting expense and editing rate offline #46895

Closed lanitochka17 closed 3 weeks ago

lanitochka17 commented 1 month 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.17-0 Reproducible in staging?: Y Reproducible in production?: N 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): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Expected Result:

The system message should only appear after returning online as it contains incorrect before and after amount - "which updated the amount to $0.00 (previously )"

Actual Result:

The system message is not accurate and contains blank info. It shows "which updated the amount to $0.00 (previously )". The previous amount is blank, the new amount is $0.00 which is also not accurate. The message should only appear after returning online

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/666b57aa-cfff-4aaf-a94d-4c180e616e7e

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @miljakljajic
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @marcochavezf (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 1 month ago

We think that this bug might be related to ##wave-collect - Release 1

marcochavezf commented 1 month ago

Seems this was introduced here and seems the Edit Rate is a new feature cc @neil-marcellini @paultsimura

neil-marcellini commented 1 month ago

Not a blocker, under the P2P distance beta. I agree with the expected result that the modified expense message shouldn't appear until we come back online.

melvin-bot[bot] commented 1 month ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 1 month ago

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

paultsimura commented 1 month ago

@neil-marcellini please assign me and I'll prepare the PR today or early tomorrow

paultsimura commented 1 month ago

@neil-marcellini when updating waypoints, we do not add the optimistic MODIFIEDEXPENSE action while offline, because:

We don't create a modified report action if we're updating the waypoints, since there isn't actually any optimistic data we can create for them and the report action is created on the server with the response from the MapBox API

Currently, my thought process is to not add such action when modifying the rate if the distance is pending – this can be if the request was created offline, or the route was edited offline before the rate edit. Otherwise, it will always lack the amount & mileage data.

But maybe you'd want to consider not adding the optimistic action at all, similarly to the updating waypoints scenario.

paultsimura commented 1 month ago

@neil-marcellini bump on https://github.com/Expensify/App/issues/46895#issuecomment-2272103970

neil-marcellini commented 1 month ago

Yeah that sounds good, we shouldn't create the optimistic action if we don't have the data to compute it, but we could create it if that data is present and we know it's not stale. The same could be done for the waypoints in some cases, but I think the author didn't want to bother with those edge cases.

I'm going to let @marcochavezf continue to manage this one internally since I really appreciate the extra help on these distance bugs. Please direct questions to him, but I'll be around if there's something you really need my opinion on.

aimane-chnaif commented 1 month ago

Not overdue

paultsimura commented 1 month ago

@aimane-chnaif the PR is ready: https://github.com/Expensify/App/pull/47145

paultsimura commented 1 month ago

Deployed to production yesterday. Payment is due 27 Aug.

paultsimura commented 1 month ago

@allroundexperts could you please fill out the BZ checklist? It's due payment today, but the automation failed.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @miljakljajic (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.

marcochavezf commented 1 month ago

Hi @miljakljajic! Can you handle the payment for @paultsimura as contributor and @allroundexperts as C+ reviewer?

miljakljajic commented 1 month ago

@marcochavezf is this a 250 USD or 125 USD issue?

marcochavezf commented 1 month ago

250 please

paultsimura commented 1 month ago

Bump @miljakljajic 🙏

allroundexperts commented 4 weeks ago

Checklist

  1. https://github.com/Expensify/App/pull/40021
  2. Author was aware of the issue and fixed it.
  3. N/A
  4. A regression test might be useful.

Regression test

  1. Open the app and login to an account that has multiple distance rates available
  2. Go offline and submit a distance expense to the workspace with multiple distance rates
  3. Go to transaction thread -> click Rate -> select a different rate

Verify that the system message should only appear after returning online

Do we 👍 or 👎 ?

miljakljajic commented 3 weeks ago

Payment summary for @allroundexperts

@allroundexperts is owed 250 for their work reviewing this PR

@paultsimura paid in Upwork