[HOLD for payment 2024-10-14] [$250] Web - Pay - Not here page opens when Gmail user Pay with Expensify

[IuliiaHerets]

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.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): gocemate+a2207@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Open 1:1 chat with other user
2. Go to Pay
3. Select USD -$ currency>Enter amount> Next> Pay with Expensify

Expected Result:

Add bank account page should open

Actual Result:

Not here page opens when Gmail user Pay with Expensify

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6609354_1726782468479.Recording__3967.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021837168425885571920
• Upwork Job ID: 1837168425885571920
• Last Price Increase: 2024-09-20
• Automatic offers:
• ikevin127 | Reviewer | 104140946
• cretadn22 | Contributor | 104140948

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

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

[IuliiaHerets]

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

[IuliiaHerets]

@isabelastisser FYI 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

[etCoderDysto]

Edited by proposal-police: This proposal was edited at 2024-09-20 11:10:39 UTC.

Proposal

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

Pay - Not here page opens when gmail user Pay with Expensify

What is the root cause of that problem?

When user is not validated and navigates to bankAccountRoute (settings/wallet/add-bank-account) hmm it not page is displayed

App/src/components/MoneyRequestConfirmationList.tsx

Line 849 in d009d8e

addBankAccountRoute={bankAccountRoute}

App/src/pages/AddPersonalBankAccountPage.tsx

Line 70 in d009d8e

<FullPageNotFoundView shouldShow={!isUserValidated}>

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

We should check if the user is validated first before we display pay with expensify button here. And hide the button when the user is not validated

• get currentUserPersoanlDetails using

const currentUserPersonalDetails = useCurrentUserPersonalDetails();

• Display paywith Expensify button when the user is validated

const canUseWallet = !isExpenseReport && !isInvoiceReport && currency === CONST.CURRENCY.USD && currentUserPersonalDetails.validated;

App/src/components/SettlementButton/index.tsx

Line 98 in d009d8e

const canUseWallet = !isExpenseReport && !isInvoiceReport && currency === CONST.CURRENCY.USD;

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[isabelastisser]

@davidcardoza, I can't add this issue to the bill pay project. Can you please help? Thanks!

[1subodhpathak]

Not able to reproduce!

[eucool]

Edited by proposal-police: This proposal was edited at 2024-09-20 18:01:01 UTC.

Proposal

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

Not here page appears for New Unvalidated Accounts when they try to Pay someone using expensify.

What is the root cause of that problem?



We do not allow unvalidated accounts to add bank account, hence when a unvalidated user is navigated to the page, he sees a not found page:






App/src/pages/AddPersonalBankAccountPage.tsx

Line 70 in d009d8e

<FullPageNotFoundView shouldShow={!isUserValidated}>

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

Like we have for Security Page, when an unvalidated user clicks on Two factor Authentication, we show them the RHP to first validated their account:

We can do the same here, when the user clicks Pay with Expensify, we can show them the same screen

A sample mock is as follows:

I used the existing ValidateAccountMessage component from here, the changes will be implemented in AddPersonalBankAccountPage page

What alternative solutions did you explore? (Optional)

[eucool]

Updated Proposal

Updated the proposal to add mock of the changes needed for the PR

[cretadn22]

Proposal

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

When New Unvalidated Accounts attempt to pay someone via Expensify, the Not Here page shows.

What is the root cause of that problem?

TriggerKYCF is triggered when an unverified user clicks "Pay with Expensify". This function will navigate to the '/settings/wallet/add-bank-account' URL (AddPersonalBankAccountPage)

App/src/components/SettlementButton/index.tsx

Lines 184 to 188 in 513e6b3

	if (iouPaymentType === CONST.IOU.PAYMENT_TYPE.EXPENSIFY || iouPaymentType === CONST.IOU.PAYMENT_TYPE.VBBA) {
	triggerKYCFlow(event, iouPaymentType);
	BankAccounts.setPersonalBankAccountContinueKYCOnSuccess(ROUTES.ENABLE_PAYMENTS);
	return;
	}

After navigating to AddPersonalBankAccountPage, If the user isn't validated, we show a not found page

App/src/pages/AddPersonalBankAccountPage.tsx

Line 70 in d009d8e

<FullPageNotFoundView shouldShow={!isUserValidated}>

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

In this PR, we have created a new page, which we will access whenever an unauthenticated user performs an action that necessitates authenticating their account.

I believe we can apply the same strategy to this bug as well.

App/src/components/SettlementButton/index.tsx

Line 184 in 513e6b3

if (iouPaymentType === CONST.IOU.PAYMENT_TYPE.EXPENSIFY || iouPaymentType === CONST.IOU.PAYMENT_TYPE.VBBA) {

        if (iouPaymentType === CONST.IOU.PAYMENT_TYPE.EXPENSIFY || iouPaymentType === CONST.IOU.PAYMENT_TYPE.VBBA) {
            if (!isUserValidated) {
                Navigation.navigate(ROUTES.SETTINGS_WALLET_VERIFY_ACCOUNT.getRoute(currentRoute));   // This route will be added in https://github.com/Expensify/App/pull/49230
                return;
            }
            triggerKYCFlow(event, iouPaymentType);
            BankAccounts.setPersonalBankAccountContinueKYCOnSuccess(ROUTES.ENABLE_PAYMENTS);
            return;
        }

What alternative solutions did you explore? (Optional)

[ikevin127]

🎉 Thanks everybody for the proposals and great variety in solutions!

From reviewing the existing proposals, three variants are surfacing (see below) and I'm not sure whether this is a design or business decision but I'll tag @Expensify/design anyways to get their take on this before assignment since the team was active recently in PR #49230.

ℹ️ As reviewer, I lean towards proposal 3 because, given PR #49230, we can agree that some work was put into this flow already specifically for the case when user account is not validated yet. My second option would be proposal 2 since it's the closest to what I think will be preferred here, and we wouldn't need to HOLD if we were to go with this version.

Proposal 1: ♻️ Hide Pay with Expensify button via canUseWallet when using unvalidated account, ⏳ Can be implemented right away	Proposal 2: ♻️ Display ValidateAccountMessage component when pressing Pay with Expensify with unvalidated account, ⏳ Can be implemented right away	Proposal 3 [HOLD #49230]: ♻️ Display new VERIFY_ACCOUNT screen when pressing Pay with Expensify with unvalidated account, ⚠️ Can be implemented as soon as #49230 is merged

[dubielzyk-expensify]

Yeah I kinda wanna hear what @JmillsExpensify and @trjExpensify thinks too. I can see an argument for both 1 and 3. 2 feels weird to me

[dannymcclain]

Agree that I'd like to hear from Jason and Tom, but my understanding was that Proposal 3 was how we wanted to handle all these unvalidated account situations (basically just add the validate screen to the beginning of whatever flow the user was trying to go through) so I have a strong preference for that option.

[trjExpensify]

Yep, same. Proposal 3 for the win.

I'd also say let's make sure that copy gives them a hint to go check their mail to grab the magic code we just fired off. I.e "This feature requires you to validate your account. Enter the magic code sent to your email."

[cretadn22]

@ikevin127, are we ready to proceed?

[ikevin127]

Based on the latest discussions looks like @cretadn22's proposal is the way to go here. The RCA is correct and the proposed solution was verified by the design team.

Note: Even though we assign the contributor now, we should HOLD #49230 until the PR is merged since the proposed solution is dependent on that PRs additions.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @cretadn22 🎉 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 📖

[cretadn22]

Still on hold

[melvin-bot]

@nkuoch, @isabelastisser, @ikevin127, @cretadn22 Eep! 4 days overdue now. Issues have feelings too...

[ikevin127]

Even though we assign the contributor now, we should [HOLD #49230] until the PR is merged since the assigned solution is dependent on that PRs additions.

[ikevin127]

ℹ️ Status update:

• PR fix: requires validate when opening bank account #49230 was merged which means we can move forward with our selected proposal solution and open the PR with the fix for this issue

• ♻️ PR Require verifying account when paying $ by Expensify #50169 is in review

[twilight294]

The related PR might have caused a regression, please check: #50285 (comment)

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker 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 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.

[melvin-bot]

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

[melvin-bot]

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

• Require verifying account when paying $ by Expensify #50169

If no regressions arise, payment will be issued on 2024-10-14. 🎊

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

• @ikevin127 requires payment automatic offer (Reviewer)
• @cretadn22 requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127] 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:
• [@ikevin127] 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:
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[isabelastisser]

@ikevin127 @cretadn22, can you please provide an update? Did the PR cause a regression? What's next here?

[ikevin127]

@isabelastisser Yes, we had the following regression:

• [$250] Pay someone - User is signed out after verifying account during pay someone flow #50284

which was caused by lack of testing on both PR reviewer and author sides. The issue mentioned above is open for proposals, unless the author wants to fix the issue given that we're still within the regression period.

Note: The issue was that we relied on the other PRs implementation of the new route without actually testing if the implementation works, ignoring this issue's Expected result which states Add bank account page should open as we stopped the testing at "seeing the magic code input showing up in RHP".

[cretadn22]

So unless the author wants to fix this, we're open for proposals. Peace out ✌️

@ikevin127 Are you suggesting that I need to create a PR to address this new issue? I believe it's outside the scope of this problem.

[ikevin127]

@cretadn22 No. Truth is that because neither of us actually tested what happens once the validation code is inputted - we're both going to get the regression penalty here.

It's also true that the issue of being logged out was not caused directly by logic you introduced in the PR, but rather an edge case / side effect of using the logic of the other PR in our specific case.

Given this, as I mentioned in the regression issue -> it's open for proposals.

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Consolidate options on settlement "Pay" button #37174.
• [@ikevin127] 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: https://github.com/Expensify/App/pull/37174/files#r1797481938.
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

1. Login to new account (unverified).
2. Start a chat with an existing user > [+] > Pay [userName].
3. Select USD > Pay with Expensify.
4. Verify that the 'verify account page' shows up in RHP prompting user to enter the magic code.

Do we agree 👍 or 👎.

@isabelastisser As for payment, there was one regression:

• [$250] Pay someone - User is signed out after verifying account during pay someone flow #50284

which wasn't caused directly by the code implemented by this issue's fix but as a side effect of our previous logic not being adjusted to handle updating the authToken while logged in - more details in issue #50284 (comment).

Given this, both me (reviewer) and contributor should get a -50% penalty on the issue's bounty.