-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix multiple Onfido screens #24079
Fix multiple Onfido screens #24079
Conversation
@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-03.at.11.49.32.AM.movMobile Web - ChromeScreen.Recording.2023-08-03.at.11.58.17.AM.movMobile Web - SafariScreen.Recording.2023-08-03.at.11.57.11.AM.movDesktopScreen.Recording.2023-08-03.at.11.55.19.AM.moviOSScreen.Recording.2023-08-03.at.12.02.06.PM.movAndroidScreen.Recording.2023-08-10.at.3.03.13.PM.mov |
@ginsuma Your changes on android are behaving weirdly for me. Can you please check? Screen.Recording.2023-08-03.at.12.00.57.PM.mov |
@allroundexperts I have just tested and see this bug also happens in the current main. |
This is the error when we open the app from the background Below is the condition to request camera permission App/src/components/Onfido/index.native.js Line 46 in 2f750ad
Lines 1006 to 1007 in 2f750ad
|
@ginsuma On main, this error does occur but it ultimately redirects you back to the ONFIDO screen. With the changes that you've done, its redirecting me to the personal information step instead. This is the discrepancy that I was talking about earlier. |
@allroundexperts Ah, I see it.
|
@ginsuma the expected behaviour is same as how it is right now on iOS. |
There are three approaches to it:
What do you think @allroundexperts ? |
@ginsuma In the original video that you uploaded, why was android working fine? I mean, what exact change is causing this issue? |
My mistake, I should reopen the app by clicking to app icon rather than using this button. The app wasn't reopened from the background.
When we reopen the app from the background:
|
|
Upgrade to When we reopen the app from the background:
https://github.com/Expensify/App/assets/13113013/9ae38f7c-f1c5-42e0-8069-96a40aee223f |
@ginsuma This sounds good. But still, why is it showing the previous screen when you re-open the app unlike iOS? |
@allroundexperts I just tested the new SDK version, not apply any fix in the video. App/src/components/Onfido/index.native.js Lines 40 to 43 in 549bdb3
App/src/pages/ReimbursementAccount/RequestorOnfidoStep.js Lines 59 to 62 in e26d900
This is a meaning error message than the previous Onfido SDK version. The old one will lead to open the dialog. We should hold this PR and create a new one to upgrade Onfido SDK first. If the upgrade doesn't cause any issues, we will continue again here. The idea is resetting |
I think we can do both in this PR! |
@ginsuma Any luck with above? |
@allroundexperts I have some updates.
My point until now:
|
Update:
Screen.Recording.2023-08-08.at.5.40.32.PM.movcc: @allroundexperts |
@ginsuma I think it makes sense to add our fix to iOS only and leave android as it was previously. What do you think? |
@allroundexperts I agree with it. We can recheck Android behavior again when we upgrade the SDK version. |
@allroundexperts I updated the Android video and pushed a new commit. Please review it. Thank you. |
@ginsuma Can you please merge main if you haven't already? |
@allroundexperts I did it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@NikkiWines We decided to dis-regard our changes on native Android because of an issue in the Onfido Android sdk. More context in the discussion proceeding this comment. |
@allroundexperts sounds good, can you post the same update in the linked issue as well so that if someone references that issue in the future it's easy to track down why Android will have different behavior? @ginsuma could you update the QA steps to reflect the different behavior on Android so that this doesn't end up failing QA after being deployed? |
I updated the steps. |
Added a comment in the issue @NikkiWines! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ah sorry! I thought I merged this. @ginsuma you've got merge conflicts but if you update this PR I'll merge it ASAP |
@NikkiWines It's ready to merge. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/NikkiWines in version: 1.3.56-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀
|
Details
iOS: Onfido screen present multiple time when app pushed to Background
Fixed Issues
$ #23642
PROPOSAL: #23642 (comment)
Tests
8.1. Click on the back button on the Onfido screen.
8.2. Verify that the previous screen is not an Onfido screen.
9.1. Verify that the Onfido screen reopens.
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-08-03.at.3.11.22.AM.mp4
Mobile Web - Safari
Screen.Recording.2023-08-03.at.3.14.41.AM.mov
Mobile Web - Chrome
Screen.Recording.2023-08-03.at.3.21.36.AM.mov
Desktop
Screen.Recording.2023-08-03.at.3.38.03.AM.mov
iOS
Screen.Recording.2023-08-03.at.3.15.44.AM.mp4
Android
Screen.Recording.2023-08-10.at.3.34.03.PM.mov