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

[$250] Send Money - Incorrect chat conversation displayed after send money to non-existing account #9084

Closed
kbecciv opened this issue May 18, 2022 · 21 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented May 18, 2022

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


Issue was found when executing PR: https://github.com/Expensify/Web-Expensify/pull/33293

Action Performed:

  1. Go to staging.new.expensify.com
  2. Login and click on Send money
  3. Put in an amount and tap Continue
  4. Enter the email of a non-existing account
  5. Select "I'll settle up elsewhere"

Expected Result:

Correct chat conversation displayed after send money to non-existing account

Actual Result:

Incorrect chat conversation displayed after send money to non-existing account

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.63.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: Applause

Bug5577109_Recording__642.mp4

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented May 18, 2022

Triggered auto assignment to @iwiznia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@iwiznia
Copy link
Contributor

iwiznia commented May 19, 2022

Oh wow... yes, terrible bug.

@iwiznia iwiznia added the External Added to denote the issue can be worked on by a contributor label May 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 19, 2022

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@thesahindia
Copy link
Member

This bug isn't only about non-existing accounts. We just don't navigate to the report when a user sends money.

Proposal

When a user sends money payIOUReport gets called, inside the function after the api request we get two reports in the response the first one contains the reportID of the report that the user send money to we can just access that and we need to navigate user to that report.

To do this we can use this code -

            const userReportID = lodashGet(_.first(_.values(response.reports)), 'reportID', '');
            Navigation.navigate(ROUTES.getReportRoute(userReportID));

(not sure what should be the variable name)

After this -

App/src/libs/actions/IOU.js

Lines 312 to 325 in 11a1f0d

if (response.jsonCode !== 200) {
switch (response.message) {
case 'You cannot pay via Expensify Wallet until you have either a verified deposit bank account or debit card.':
Growl.error(Localize.translateLocal('bankAccount.error.noDefaultDepositAccountOrDebitCardAvailable'), 5000);
break;
case 'This report doesn\'t have reimbursable expenses.':
Growl.error(Localize.translateLocal('iou.noReimbursableExpenses'), 5000);
break;
default:
Growl.error(response.message, 5000);
}
Onyx.merge(ONYXKEYS.IOU, {error: true});
return;
}

@iwiznia iwiznia assigned adelekennedy and unassigned adelekennedy and iwiznia May 20, 2022
@adelekennedy
Copy link

Internal
External

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels May 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 20, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 20, 2022

Triggered auto assignment to @flodnv (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Send Money - Incorrect chat conversation displayed after send money to non-existing account [$250] Send Money - Incorrect chat conversation displayed after send money to non-existing account May 20, 2022
@melvin-bot melvin-bot bot added the Overdue label May 30, 2022
@flodnv
Copy link
Contributor

flodnv commented May 30, 2022

@parasharrajat can you please review the proposal from @thesahindia?

@melvin-bot melvin-bot bot removed the Overdue label May 30, 2022
@parasharrajat
Copy link
Member

Oh. I didn't see the proposal. I will review this today.

@parasharrajat
Copy link
Member

#6992 shows that this is expected behavior. cc: @flodnv @iwiznia

@thesahindia Your proposal is half of the needed solution. We indeed navigate after sendMoney request is completed but it does not navigate to the report that we sent money. So Instead of your solution we need to correct the existing navigation logic (If this change is needed).

@flodnv
Copy link
Contributor

flodnv commented May 30, 2022

#6992 shows that this is expected behavior. cc: @flodnv @iwiznia

😕 @marcaaron any thoughts on that?

@marcaaron
Copy link
Contributor

I dunno I'm not seeing the bug seems more like a feature request. If you send someone money it just sends them money. Is the expectation that we will switch you to that chat? Why and where was it decided?

@iwiznia
Copy link
Contributor

iwiznia commented May 31, 2022

Oh damn, when I looked at it I thought the same thing, but then for some reason thought the request was showing in the chat you had selected. I now realize that request was already there. I agree to close this out as this is not a bug.

@flodnv
Copy link
Contributor

flodnv commented Jun 1, 2022

Agree

1 similar comment
@Justicea83
Copy link
Contributor

Agree

@adelekennedy
Copy link

adelekennedy commented Jun 6, 2022

@flodnv @iwiznia @marcaaron @parasharrajat are we good to close this if it's expected behavior?

@iwiznia
Copy link
Contributor

iwiznia commented Jun 6, 2022

Yep

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@madmax330
Copy link
Contributor

@iwiznia why did you put 👎🏽 on the regression comment? The issue that it mentioned (#10902) is the same as this one no?
So there was a regression somewhere right?

@madmax330 madmax330 reopened this Sep 12, 2022
@iwiznia
Copy link
Contributor

iwiznia commented Sep 12, 2022

This issue was closed with no changes, so it can't have caused a regression. We decided this is not a bug.

@iwiznia iwiznia closed this as completed Sep 12, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants