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] Bank Account - Onfido page returns an error when go online #49572

Closed
3 of 6 tasks
IuliiaHerets opened this issue Sep 22, 2024 · 37 comments
Closed
3 of 6 tasks

[$250] Bank Account - Onfido page returns an error when go online #49572

IuliiaHerets opened this issue Sep 22, 2024 · 37 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 22, 2024

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


Version Number: v9.0.39-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/cases/view/2950430
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition: workspace with enabled workflows. Started and followed the flow for adding a Verifying bank account using the credentials found below until you reach the Onfido screen:
Use Regions bank
Username: user_good | Password: pass_good
Account type: Plaid Saving (2nd option)
CompanyName: Expensify
CompanyTaxID: 123456789
First Name: First
Last Name: Last

  1. Go to ws > workflows
  2. Connect bank account > Continue setup
  3. Personal info > Confirm
  4. On Onfido screen: Disable the internet connection
  5. Enable the internet connection
  6. Repeat step 3

Expected Result:

Onfido screen open and user can proceed the flow.

Actual Result:

An error appears. User is navigated back to Personal info page

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6610008_1726844385925.bandicam_2024-09-20_17-51-50-039.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838061398162430239
  • Upwork Job ID: 1838061398162430239
  • Last Price Increase: 2024-09-23
  • Automatic offers:
    • hoangzinh | Reviewer | 104084844
Issue OwnerCurrent Issue Owner: @hoangzinh
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 22, 2024
Copy link

melvin-bot bot commented Sep 22, 2024

Triggered auto assignment to @jliexpensify (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
Copy link
Author

@jliexpensify 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

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 23, 2024
@melvin-bot melvin-bot bot changed the title Bank Account - Onfido page returns an error when go online [$250] Bank Account - Onfido page returns an error when go online Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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

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

melvin-bot bot commented Sep 23, 2024

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

@jliexpensify
Copy link
Contributor

Able to reproduce, @IuliiaHerets can you confirm if this happens on iOS as well?

@narefyev91
Copy link
Contributor

Hi, I’m Nicolay from Callstack and I would like to investigate this issue.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

📣 @hoangzinh 🎉 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

@jliexpensify
Copy link
Contributor

All yours @narefyev91

@narefyev91
Copy link
Contributor

narefyev91 commented Sep 24, 2024

@jliexpensify @hoangzinh what i was able to find during investigation:
Before user starts onfido flow - we send API UpdatePersonalInformationForBankAccount and we get such response (which has onfido token):
Screenshot 2024-09-24 at 16 38 32

After disabling/enabling internet - we have initial API call to grab all available information (which were stored before) - OpenReimbursementAccountPage - and in response we get empty onfidoToken:
Screenshot 2024-09-24 at 16 41 25

That's why user got redirected to the personal screen - instead of continue working with onfido - there is no token provided
For error which is shown in pop-up - it happened inside onfido package:
[hmmm] Onfido error - {"errorType":"exception","errorMessage":"Frame with src https://sdk.onfido.com/capture/core/14.37.1/runner.html took too long to load."}
Screenshot 2024-09-24 at 16 52 58
Technically it means that we have timeout of some of the services from onfido - which probably is not working offline. We can see that in Sources tab - onfido drops some resources when i moved to offline:

Screen.Recording.2024-09-24.at.17.14.00.mov

Based on all these information - we should try to fix 2 issues:

  1. Token should either come from API in OpenReimbursementAccountPage if user already set personal information or should not come at all in OpenReimbursementAccountPage - we have a bunch of logic around onfido tokens which will be already handled if token is expired or is not correct.
  2. We should re-initiate Onfido for web - if user goes offline/online - to not waiting source from already lost onfido thread (will check how that can be possible)

Let me know wdyt guys. Thanks!

@jliexpensify
Copy link
Contributor

@joekaufmanexpensify dropping you into this bug GH as I think you worked on Onfido for ND? Do you mind sharing your thoughts on the above comment?

@narefyev91
Copy link
Contributor

narefyev91 commented Sep 25, 2024

Some more finding here:
We never correctly ternimate onfido flow - in this file src/components/Onfido/index.tsx:
Screenshot 2024-09-25 at 16 00 58
onfidoOut.tearDown(); - is not working at all because onfidoOut is an ref for div and it's not having direct onfidoSdk props and functions.

To make is working we need to return onfidoSdk instance and work with it - like this:
Screenshot 2024-09-25 at 17 01 53

Also interesting thing that in 14.15.0 (current version) there is other method to terminate onfido sdk
https://documentation.onfido.com/sdk/web/14.15.0/#sdk-tear-down
Screenshot 2024-09-25 at 16 03 05
But when i tried to use it - it says that such method does not exists lol.
I upgrade version for 14.17.0 - which is for sure has correct tearDown function.
Screenshot 2024-09-25 at 16 04 39

But it was not fixed the issue with error pop-up. But it gives correct flow to remove dead sdk thread.
Currently diving deeper - i realised that when user moves back to online what currently happened:

  1. isOffline = false
  2. FullPageOfflineBlockingView - return children - which is Onfido component
  3. we refetching data - BankAccounts.openReimbursementAccountPage
  4. Onfido run again OnfidoSdk.init and in parallel received that onfidoToken is null
  5. we drop onfidoSdk connection
  6. But onfido already started some init process and waiting to execute all instruction - and just throws timeout error - that some instructions were not loaded in time.

Again probably that's correct behaviour from Onfido - because if some resources are lost - they could not guaranteed that their product will work correctly.
In our situation root case - is that we lost onfidoToken after coming back from offline.
I think that it will be a workaround to just save onfidoToken locally - when app is moved to offline and re-use it after BankAccounts.openReimbursementAccountPage will be executed.
But also will wait your suggestions here @hoangzinh @joekaufmanexpensify @jliexpensify

@joekaufmanexpensify
Copy link
Contributor

@joekaufmanexpensify dropping you into this bug GH as I think you worked on Onfido for ND? Do you mind sharing your thoughts on the above comment?

I did not really work on implementing Onfido in NewDot, so don't have any specialized knowledge here really. But happy to take a look!

@joekaufmanexpensify
Copy link
Contributor

Yeah, Onfido is an online-only operation. It's expected that it doesn't work offline and uses pattern D. In an ideal world, I think it'd be nice to be able to land where you where in the Onfido flow if you go offline during it. But given it is an online operation with a third party service, I don't think it's a big deal if you need to restart it too. I expect the user would be understanding of that.

If we can easily drop you where you were in the flow though after reconnecting to the internet, that sounds solid.

@joekaufmanexpensify
Copy link
Contributor

Unassigning as this doesn't need two BZs, but LMK if there any other Q's I can help with!

@joekaufmanexpensify joekaufmanexpensify removed their assignment Sep 25, 2024
@narefyev91
Copy link
Contributor

Yeah, Onfido is an online-only operation. It's expected that it doesn't work offline and uses pattern D. In an ideal world, I think it'd be nice to be able to land where you where in the Onfido flow if you go offline during it. But given it is an online operation with a third party service, I don't think it's a big deal if you need to restart it too. I expect the user would be understanding of that.

If we can easily drop you where you were in the flow though after reconnecting to the internet, that sounds solid.

Yeah - fully agree! To move back to existing flow we just need to have onfido token and we can achieve that by:

  1. do not receive in OpenReimbursementAccountPage onfido token set - {key: 'onfidoToken, onyxMethod: set, value: null}
  2. receive onfido token in OpenReimbursementAccountPage if user already filled information before.
  3. store token locally and re-use it after offline - if personal information already exists in OpenReimbursementAccountPage.

@narefyev91
Copy link
Contributor

I tried to add FE fix - was able to get to onfido flow directly after move back to online, but still onfido throws error after unmount - and this part of code executed:
Screenshot 2024-09-26 at 12 43 22
Which clears token again. To avoid that we need to not keep in count "exception" errors - which in my view is not correct.

Again - seems like this issue should be fixed on BE side.

@hoangzinh could you please take a look on the discussion and maybe also suggest/make decision where we should try to fix that flow.
In a few words:

  1. Root case - onfido token is set to null after user moves back to online.
  2. that happened because OpenReimbursementAccountPage API call returns {key: 'onfidoToken, onyxMethod: set, value: null}.
  3. fixing on FE side by just storing token locally (in ref for example) is a workaround.

@hoangzinh
Copy link
Contributor

sure @narefyev91, I will focus on this issue today. was busy on PRs

@hoangzinh
Copy link
Contributor

@narefyev91 Thanks for thorough investigation here. In my opinion, we should remove {key: 'onfidoToken, onyxMethod: set, value: null} out of the response body of OpenReimbursementAccountPage API call, and let client apps decide when it will need to clear/reset onfidoToken. What do you think?

@narefyev91
Copy link
Contributor

narefyev91 commented Sep 27, 2024

@hoangzinh yeah totally agree - and also needs to remove from OpenReimbursementAccountPage one more object - {key: onfidoApplicantID} as well

@hoangzinh
Copy link
Contributor

ah yes, agreed with that. Let me quickly links what we investigated so far

Do you agree @narefyev91 ^, or you have anything else to add before we proceed with an internal engineer? Thank you.

@narefyev91
Copy link
Contributor

ah yes, agreed with that. Let me quickly links what we investigated so far

Do you agree @narefyev91 ^, or you have anything else to add before we proceed with an internal engineer? Thank you.

Yeah thanks for the summary! Agree with everything here

@hoangzinh
Copy link
Contributor

Thanks @narefyev91. Let's find an internal engineer to review our investigation.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 27, 2024

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

@flodnv
Copy link
Contributor

flodnv commented Sep 30, 2024

It seems to me like your proposal to fix this in the backend makes sense. @nkuoch can you take a look please? I see you recently modified this here: https://github.com/Expensify/Web-Expensify/commit/e787298b1da0f3192cc3333d40414755ca606c8e

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2024
@flodnv flodnv assigned nkuoch and unassigned flodnv Sep 30, 2024
@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2024
@jliexpensify
Copy link
Contributor

@nkuoch - just a heads up I'm OOO from 3rd to 14th, but I don't think anything is needed from me during this period. Feel free to reassign to another B0 if needed, otherwise I will review this when I get back re: payments.

@nkuoch
Copy link
Contributor

nkuoch commented Oct 1, 2024

Creating back end fix to not sent OnfidoToken=null here.

@hoangzinh
Copy link
Contributor

Hi @nkuoch, just to be safe, will https://github.com/Expensify/Web-Expensify/pull/43732 cause any DB if the FE is not ready yet?

@nkuoch
Copy link
Contributor

nkuoch commented Oct 1, 2024

Can you elaborate?

@hoangzinh
Copy link
Contributor

At the moment, when the user completes Personal info and goes to the Onfido screen, the App will call APIs OpenReimbursementAccountPage to get some data (also set OnfidoToken=null). If the PR above does not send OnfidoToken=null and FE is not implemented to set OnfidoToken=null when it is needed, will it cause any Onfido bug if the user revisits the Onfido screen again? (because the OnfidoToken won't be reset/set to null)

@nkuoch
Copy link
Contributor

nkuoch commented Oct 2, 2024

I believe OpenReimbursementAccountPage is only called when reloading? When user completes personal info, OnfidoToken gets set with a string. If user refreshes the page or reconnects because they got disconnected on this step, OpenReimbursementAccountPage would be called and not touch OnfidoToken (and I rely on we have a bunch of logic around onfido tokens which will be already handled if token is expired or is not correct.) in case OnfidoToken is not correct anymore. Note that if OpenReimbursementAccountPage gets called from any other steps, OnfidoToken would get reset to null though.

@narefyev91
Copy link
Contributor

I think if user will re-visit onfido flow and token or application ID which were not cleaned will be incorrect - Onfido will show and error - and we will clean up it here

const handleOnfidoError = () => {
:

Screenshot 2024-10-02 at 13 04 59

@hoangzinh
Copy link
Contributor

@nkuoch Ah, you're right.

Copy link

melvin-bot bot commented Oct 6, 2024

@nkuoch @hoangzinh @narefyev91 @jliexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Oct 6, 2024
@hoangzinh
Copy link
Contributor

Not overdue. @nkuoch is working on the BE fix here #49572 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Oct 7, 2024
@nkuoch
Copy link
Contributor

nkuoch commented Oct 7, 2024

BE fix has been deployed FYI

@hoangzinh
Copy link
Contributor

Thanks @nkuoch. It appears that the issue has been fixed with the BE fix above.

Screen.Recording.2024-10-07.at.22.02.10.mov

cc @jliexpensify

@nkuoch
Copy link
Contributor

nkuoch commented Oct 10, 2024

Cool then we can close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests

7 participants