Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-06-03] [$500] IOU - Creating IOU Distance offline with incomplete addresses results in incorrect error message #38213

Closed
6 tasks done
izarutskaya opened this issue Mar 13, 2024 · 81 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Mar 13, 2024

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.4.51-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: https://expensify.testrail.io/index.php?/tests/view/4418303
Email or phone of affected tester (no customers): sustinov@applausemail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Preconditions:
Set up an OldDot admin account, invite the employee to the policy https://sites.google.com/applausemail.com/applause-expensifyproject/wiki-guides/newdot-categories?authuser=0

Steps:

  1. Open https://staging.new.expensify.com/
  2. Log in with the account of the employee added to the policy
  3. Navigate to the WS room
  4. Compose Box > Request Money
  5. Select Distance option
  6. Disable the internet connection
  7. Enter a vague start location like "Fake"
  8. Enter a vague finish location like "Blah blah"
  9. Finish the request creation flow

Expected Result:

The error "No route found through the waypoints. Please edit the waypoints and try again." Should be shown in the UI.

Actual Result:

The error "Unexpected error requesting money, please try again later" is shown in the UI.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6412058_1710321526582.Recording__1419.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01df4bc3e1ba142b9b
  • Upwork Job ID: 1767941481061400576
  • Last Price Increase: 2024-04-24
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 0
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @laurenreidexpensify
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Mar 13, 2024
@melvin-bot melvin-bot bot removed the Hourly KSv2 label Mar 13, 2024
Copy link
Contributor

👋 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.

Copy link

melvin-bot bot commented Mar 13, 2024

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

@izarutskaya
Copy link
Author

@lakchote I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

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

@izarutskaya
Copy link
Author

Production video

bandicam.2024-03-13.12-56-20-421.mp4

@trjExpensify
Copy link
Contributor

Looks like a deploy blocker to me!

@lakchote
Copy link
Contributor

lakchote commented Mar 13, 2024

I can't reproduce on dev (BE won't let me do it).

Might be related to these changes

@lakchote
Copy link
Contributor

@lakchote I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

It's a frontend issue, and as such I'm opening it to external contributions.

@lakchote lakchote added the External Added to denote the issue can be worked on by a contributor label Mar 13, 2024
@melvin-bot melvin-bot bot changed the title IOU - No error after creating IOU Distance offline with incomplete addresses [$500] IOU - No error after creating IOU Distance offline with incomplete addresses Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Mar 13, 2024
@luacmartins
Copy link
Contributor

@lakchote this is a deploy blocker. Since this is the end of your day, can we find an internal engineer to keep pushing this forward?

@tgolen tgolen self-assigned this Mar 13, 2024
@tgolen
Copy link
Contributor

tgolen commented Mar 13, 2024

I don't think this is a deploy blocker or a backend issue.

If I use the addresses "SF" and "LA", then the backend is able to come up with a route that goes from San Francisco to Lousiana (which is feasible). So, I think those are valid addresses.

When I use the addresses "fake" and "blah blah", the server returns an error No route found through the waypoints. Please edit the waypoints and try again.

Full API response
{
    "code": 666,
    "jsonCode": 666,
    "type": "Expensify\\Libs\\Error\\ExpError",
    "UUID": "5C7EDD2F-FAC9-4795-AA2D-383D7E053F9D",
    "message": "No route found through the waypoints. Please edit the waypoints and try again.",
    "title": "",
    "data": {
        "onyxData": [
            {
                "onyxMethod": "merge",
                "key": "transactions_1747048417427960048",
                "value": {
                    "errorFields": {
                        "route": {
                            "1710350682438627": "No route found through the waypoints. Please edit the waypoints and try again."
                        }
                    }
                }
            }
        ]
    },
    "htmlMessage": "",
    "onyxData": [
        {
            "onyxMethod": "merge",
            "key": "transactions_1747048417427960048",
            "value": {
                "errorFields": {
                    "route": {
                        "1710350682438627": "No route found through the waypoints. Please edit the waypoints and try again."
                    }
                }
            }
        }
    ],
    "requestID": "Zxpu95"
}

image

What I think needs to be fixed here is that the UI is not displaying the error that is returned from the server, and instead, it's displaying some generic error.

I'm going to remove the deploy blocker label, and update the description of the issue.

Copy link

melvin-bot bot commented May 1, 2024

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented May 1, 2024

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@bernhardoj
Copy link
Contributor

PR is ready

cc: @abdulrahuman5196

@tienifr
Copy link
Contributor

tienifr commented May 2, 2024

@abdulrahuman5196 Could you give some feedback on my proposal?

Without the proper error clearing (in report action, IOU report createChat) here, when clearing the route error in the transaction thread, the user will see an undismissable RBR error in the report preview.

@bernhardoj's solution will have the above problem (mentioned and solved in my proposal)

@laurenreidexpensify
Copy link
Contributor

@abdulrahuman5196 do you still have capacity for C+ on this one or can I reassign?

@abdulrahuman5196
Copy link
Contributor

@abdulrahuman5196 Could you give some feedback on my proposal?

Without the proper error clearing (in report action, IOU report createChat) here, when clearing the route error in the transaction thread, the user will see an undismissable RBR error in the report preview.

bernhardoj's solution will have the above problem (mentioned and solved in my proposal)

Given the expectation change, your proposal seemed to be an extended fix of Bernhardoj's solution. That why went with the solution which came first.

Regarding the error clearing issue, it is an extended fix of the core issue(showing proper error in this case) which would be mostly handled in PR state.

@tienifr
Copy link
Contributor

tienifr commented May 8, 2024

Regarding the error clearing issue, it is an extended fix of the core issue(showing proper error in this case) which would be mostly handled in PR state.

@abdulrahuman5196 It's a bug in the same flow so I'm not sure why it's only considered an "extended fix", it's part of the final solution. So it could be said that the selected proposal only fixes a part of the issue (only shows the error correctly, but not able to clear the errors) and my proposal fixes it fully (shows the error correctly and able to clear the errors).

Another way to think about this is, would we be fine for the error to show correctly but the user is unable to clear it? If yes, my solution is an "extended fix", if no, it's a "required fix" and should be factored in the proposal evaluation.

@tgolen What do you think?

@tgolen
Copy link
Contributor

tgolen commented May 8, 2024

I can understand and agree with both sides. Ultimately, I'd like to see the most complete fix (the right error message AND being able to clear it) and I'm less interested about who does it or how it gets done :D

@tienifr If you'd like to open a new bug report for not being able to clear the error, I'd support just assigning it to you and have you fix it. Would that work?

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented May 8, 2024

Hi @tienifr , to mention we are clearing the error in the PR raised. In the PR, the error is shown properly and cleared properly AFAIK.

But basically the fix to clear the error is considered as an extended solution or improvement to the core fix which is showing the proper error(as per OP). Afaik we select the first original proposal which suggested the core fix if the improvements could/would have handled in the PR state. Because multiple improvements can be made on any selected proposal on any issue. In this case, clearing of error would have been mostly handled in the PR state.

Another way to think about this is, would we be fine for the error to show correctly but the user is unable to clear it? If yes, my solution is an "extended fix", if no, it's a "required fix" and should be factored in the proposal evaluation

We would want to implement any improvements along with the original fix. Contributors are encouraged to add improvements whenever possible. But proposals with minor/PR kind of improvements to the core issue might not be selected if the proposal is not the first posted compared to the proposal which has the core fix.

Cc @tgolen

@tgolen
Copy link
Contributor

tgolen commented May 8, 2024

Yeah, I agree with that too. It's not always a given that stuff like that gets caught in the PR which is why I can see both sides to it. If it is caught and addressed in this PR, then let's just move forward with the single PR like it is.

@laurenreidexpensify
Copy link
Contributor

@abdulrahuman5196 can you give an update of status of this issue? thank you

@bernhardoj
Copy link
Contributor

The PR is merged 4 days ago, waiting for the deploy to staging and prod.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 27, 2024
@melvin-bot melvin-bot bot changed the title [$500] IOU - Creating IOU Distance offline with incomplete addresses results in incorrect error message [HOLD for payment 2024-06-03] [$500] IOU - Creating IOU Distance offline with incomplete addresses results in incorrect error message May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.75-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-03. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented May 27, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@abdulrahuman5196] The PR that introduced the bug has been identified. Link to the PR:
  • [@abdulrahuman5196] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@abdulrahuman5196] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@abdulrahuman5196] Determine if we should create a regression test for this bug.
  • [@abdulrahuman5196] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@laurenreidexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 2, 2024
@laurenreidexpensify
Copy link
Contributor

Payment Summary:

C+ @abdulrahuman5196 $500 paid in upwork
Contributor @bernhardoj $500 paid in upwork

@laurenreidexpensify
Copy link
Contributor

@abdulrahuman5196 please confirm checklist above so we can close this out thanks

@abdulrahuman5196
Copy link
Contributor

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not a regression.

Determine if we should create a regression test for this bug.

Yes.

If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

  1. Open a workspace chat
  2. Have at least 1 request
  3. Go offline
  4. Create a new distance request with invalid waypoints
  5. Go online
  6. Open the expense report
  7. Verify there is an invalid waypoint error message below the money request preview
  8. Open the transaction thread
  9. Verify there is an invalid waypoint error message below the distance map image

@laurenreidexpensify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

10 participants