[HOLD for payment 2022-11-11] [$250] Andriod/IOS - Connect Bank Account - Company page displayed instead Personal Information Page after tap back button on Onfido flow, error message displayed

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Launch the app
2. Log in with the expensifail account
3. Go to Setting - Workspace
4. Connect Bank account
5. Select Connect manually
6. Put routing number and account number
7. Follow the flow until you hit Onfido flow
8. Tap back button

Expected Result:

Personal page displayed instead company Information Page after tap back button on Onfido flow, no error message

Actual Result:

Company page displayed instead Personal Information Page after tap back button on Onfido flow, error message displayed

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.2.10.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20220929-190017_New.Expensify.1.mp4

Upwork URL: https://www.upwork.com/jobs/~010e6ad9c23c041b36

Issue reported by: Applause - Internal Team (originally @thesahindia)
Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

⚠️ 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.

[stitesExpensify]

I'm pretty confident this can be fixed externally by passing a different prop to the onFido component

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Current assignee @stitesExpensify is eligible for the External assigner, not assigning anyone new.

[trjExpensify]

👋 The contributor will need an account number and routing number to test with, no?

[mananjadhav]

@stitesExpensify @trjExpensify I tested this flow, but are we sure this is what we want?

I can see comments in RequestorStep.js which says we're explicitly sending them back to the CompanyStep,

// We're taking the user back to the company step. They will need to come back to the requestor step to make the Onfido flow appear again.

[akshayasalvi]

Proposal
This is happening because in the following lines of RequestorStep,

App/src/pages/ReimbursementAccount/RequestorStep.js

Lines 189 to 197 in 6748e8a

	onUserExit={() => {
	// We're taking the user back to the company step. They will need to come back to the requestor step to make the Onfido flow appear again.
	BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.COMPANY);
	}}
	onError={() => {
	// In case of any unexpected error we log it to the server, show a growl, and return the user back to the company step so they can try again.
	Growl.error(this.props.translate('onfidoStep.genericError'), 10000);
	BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.COMPANY);
	}}

We're setting the step as CONST.BANK_ACCOUNT.STEP.COMPANY, which we need to set it to CONST.BANK_ACCOUNT.STEP.REQUESTOR

But just updating the step doesn't help as the onFido component rendered in the RequestorStep itself, so on back press it shows a blank page. To fix this we need to clear the onfidoToken, so that shouldShowOnfido is false.

<Onfido
      sdkToken={this.props.onfidoToken}
      onUserExit={() => {
+         BankAccounts.clearOnfidoToken();
-          BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.COMPANY);
+        BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.REQUESTOR);
      }}
      onError={() => {
          Growl.error(this.props.translate('onfidoStep.genericError'), 10000);
+        BankAccounts.clearOnfidoToken();
-         BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.COMPANY);
+        BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.REQUESTOR);

      }}

[trjExpensify]

@stitesExpensify @trjExpensify I tested this flow, but are we sure this is what we want?

I can see comments in RequestorStep.js which says we're explicitly sending them back to the CompanyStep

Oh, interesting! 🤔 I wonder why that is? The step before Onfido is personal details, so why do we skip over that page and go back to CompanyStep instead? @kevinksullivan & @marcaaron might know as they worked on this originally, I believe.

[kevinksullivan]

Hmm trying to scroll back in my DM with @marcaaron because I believe this was part of the original design since it was easier at the time. I think we kick the user back to the company info step because we don't create the VBA until after a certain point, so this may have to do with that timing and the need to re-submit the info. @marcaaron can you fill in the large gaps in my explanation?

[mananjadhav]

so this may have to do with that timing and the need to re-submit the info.

Can you elaborate on resubmitting the info?

I did a basic test on @akshayasalvi's proposal and it seems to be working. But I haven't tested thoroughly so if there's something specific to be considered when doing this, it'll be helpful if can be linked to the issue.

[mananjadhav]

@kevinksullivan @marcaaron @stitesExpensify Quick bump.

[Uros787]

Proposal

The problem is with the error messages we check on catching errors. We don't want to display the error when messages are one of these:
USER_CANCELLED: 'User canceled flow'
USER_TAPPED_BACK: 'User exited by clicking the back button.',
USER_EXITED: 'User exited by manual action.',

Currently, we are missing USER_EXITED message, I'm guessing since it was implemented in some of the later versions on react native onfido SDK.

Solution:

• Add a new onfido error message in CONST.js:

USER_EXITED: 'User exited by manual action.',

App/src/CONST.js

Lines 587 to 606 in 11c2d49

	ONFIDO: {
	CONTAINER_ID: 'onfido-mount',
	TYPE: {
	DOCUMENT: 'document',
	FACE: 'face',
	},
	VARIANT: {
	VIDEO: 'video',
	},
	SMS_NUMBER_COUNTRY_CODE: 'US',
	ERROR: {
	USER_CANCELLED: 'User canceled flow',
	USER_TAPPED_BACK: 'User exited by clicking the back button.',
	USER_CAMERA_DENINED: 'Onfido.OnfidoFlowError',
	USER_CAMERA_PERMISSION: 'Encountered an error: cameraPermission',
	// eslint-disable-next-line max-len
	USER_CAMERA_CONSENT_DENIED: 'Unexpected result Intent. It might be a result of incorrect integration, make sure you only pass Onfido intent to handleActivityResult. It might be due to unpredictable crash or error. Please report the problem to [email protected]. Intent: null \n resultCode: 0',
	},
	},

• Add check for this message in index.native.js where we are conditional asking on should we display the error(did user exit the flow or something else happened)

App/src/components/Onfido/index.native.js

Lines 44 to 54 in 2f5603d

	if (_.contains([CONST.ONFIDO.ERROR.USER_CANCELLED, CONST.ONFIDO.ERROR.USER_TAPPED_BACK], errorMessage)) {
	this.props.onUserExit();
	return;
	}
	// Handle user camera permission on iOS and Android
	if (_.contains(
	[
	CONST.ONFIDO.ERROR.USER_CAMERA_PERMISSION,
	CONST.ONFIDO.ERROR.USER_CAMERA_DENINED,
	CONST.ONFIDO.ERROR.USER_CAMERA_CONSENT_DENIED,

if (
  _.contains(
    [
      CONST.ONFIDO.ERROR.USER_CANCELLED,
      CONST.ONFIDO.ERROR.USER_TAPPED_BACK,
      CONST.ONFIDO.ERROR.USER_EXITED,
    ],
    errorMessage
  )
) {
  this.props.onUserExit();
  return;
}

And now there is no error anymore

• Also clear onfido token on RequestorOnfidoStep.js

<Onfido
  sdkToken={this.props.onfidoToken}
  onUserExit={() => {
    BankAccounts.clearOnfidoToken();
    BankAccounts.goToWithdrawalAccountSetupStep(
      CONST.BANK_ACCOUNT.STEP.REQUESTOR
    );
  }}
  onError={() => {
    // In case of any unexpected error we log it to the server, show a growl and return the user back to the company step so they can try again.
    Growl.error(this.props.translate("onfidoStep.genericError"), 10000);
    BankAccounts.clearOnfidoToken();
    BankAccounts.goToWithdrawalAccountSetupStep(
      CONST.BANK_ACCOUNT.STEP.REQUESTOR
    );
  }}
  onSuccess={(onfidoData) => {
    this.submit(onfidoData);
  }}
/>;

VIDEO:

Screen.Recording.2022-10-05.at.12.01.37.mov

[mananjadhav]

@trjExpensify Based on @kevinksullivan's comment it seems we did that only because it was easier that time. Should I evaluate the proposal?

[trjExpensify]

Yeah, at a minimum we want to fix the error appearing when navigating back. I do also think it makes more sense to navigate back to the personal information step instead of the company info step, but I don't have enough insight as to whether there's a show stopper in this:

I think we kick the user back to the company info step because we don't create the VBA until after a certain point, so this may have to do with that timing and the need to re-submit the info

If the proposal encompasses these things, then I don't see a reason why that isn't fine:

1. No error message when clicking back from Onfido
2. navigates back to the personal information step
3. navigating forward again to Onfido to complete it, the request to create the VBA works correctly (i.e you don't run into errors and need to go and re-enter any info in the company step etc)

[mananjadhav]

@trjExpensify In that case @Uros787's proposal seems to be fixing the error part. Should we go ahead and fix atleast that part?

[trjExpensify]

#11461 (comment) - the video seems to navigate back to the personal info page as well though?

[melvin-bot]

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

[mananjadhav]

PR changes are fine, I'll finish the testing by tomorrow.

[trjExpensify]

The PR has gone through C+ review, it's now over to @MariaHCD & @ctkochan22 for review. Actually, @stitesExpensify why aren't you also assigned the PR to review as the CME? 🤔

[melvin-bot]

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

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here: https://github.com/Expensify/Expensify/issues/242845
• The PR that introduced the bug has been identified. Discussed here, none to point to.
• 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. Discussed here, none to point to.
• A discussion in #contributor-plus 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: https://expensify.slack.com/archives/C01SKUP7QR0/p1668435315565399
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[mananjadhav]

PR was merged, but I am not sure about BugZero Checklist. Can you take a look at it @trjExpensify ?

[trjExpensify]

Sure, that's the new checklist for BugZero to ensure we square all these items away. Obviously, the last three won't happen until we hit +7 days from the production deploy, but I can get going on the suggested regression tests to add to TestRail once we hit staging and it passes QA regression testing.

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:

For this one. IIRC, this wasn't actually a regression, we just implemented it to return to the Company step instead of the Personal step. Is that right? What about the Onfido error though, do we know what PR caused that one?

[melvin-bot]

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

• Fix: Onfido error and step back #11842

If no regressions arise, payment will be issued on 2022-11-11. 🎊

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[trjExpensify]

What about the Onfido error though, do we know what PR caused that one?

Bump on this. If I understand it, we think the USER_EXITED error was added to the Onfido SDK later than when we initially implemented it, and therefore we didn't suppress showing it like we did USER_CANCELLED and USER_TAPPED_BACK. Is that correct?

[MariaHCD]

we think the USER_EXITED error was added to the Onfido SDK later than when we initially implemented it, and therefore we didn't suppress showing it like we did USER_CANCELLED and USER_TAPPED_BACK. Is that correct?

That sounds correct to me.

[mananjadhav]

@trjExpensify This is due for payment

[trjExpensify]

I've settled up with @Uros787. Sent you an offer to accept for C+, and @thesahindia for reporting.

[thesahindia]

Accepted, thanks!

[trjExpensify]

^^ Settled!

[trjExpensify]

All payments have been made, checklist complete. Closing it out. Thanks everyone! 🥂 to the next one..