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

Fix blank screen when exiting the Onfido page in IOU flow #6461

Merged
merged 7 commits into from
Dec 3, 2021

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Nov 24, 2021

Details

The onfido.tearDown() function causes the app to crash on Desktop.

Fixed Issues

$ #6082

Tests

  1. Launch the Desktop app and login
  2. Tap on [+] and select Send money > enter amount > next
  3. Enter the email of a user you will send money to in the field 'Name, email or phone"
  4. Tap on Continue for "Verify identity" page
  5. Close 'Verify identity" modal

QA Steps

Same as above.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Mobile Web

Desktop

desktop.mov

iOS

Android

@luacmartins luacmartins self-assigned this Nov 24, 2021
@luacmartins luacmartins marked this pull request as ready for review November 24, 2021 18:15
@luacmartins luacmartins requested a review from a team as a code owner November 24, 2021 18:15
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team November 24, 2021 18:16
@luacmartins luacmartins changed the title Fix blank screen appears when exiting the onfido page in IOU flow Fix blank screen when exiting the Onfido page in IOU flow Nov 24, 2021
@luacmartins luacmartins mentioned this pull request Nov 24, 2021
5 tasks
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit wary of this change (as always with platform checking). Did you ever get to the bottom of what was causing the crash? I read the issue and this Slack thread, and didn't see a resolution per se. Would you be willing to explain this change a bit more?

@luacmartins
Copy link
Contributor Author

luacmartins commented Nov 24, 2021

I share your concerns and wish that I had an explanation for why onfido.tearDown() crashes the desktop app, but I don't. Checking the function definition, I see that it just renders null. I ran some local tests and we seem to completely remove the component when we close that navigator even without tearDown(), so this change should be harmless.

Sorry that's not a great explanation of the solution!

@Luke9389
Copy link
Contributor

Luke9389 commented Dec 1, 2021

Hmm, well I don't think this is too egregious, but I'm gonna request another pair of eyes just in case.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the fix for the crash, I feel like it's safe enough. If it works, it works 🤷

That said, I don't really want to condone the use of getPlatform() in this context because, even though it's less code than the alternative, it goes against our style guideline for platform-specific code. Instead, the way we would normally implement this change is using platform-specific file extensions:

  1. Create a Base component, in this case, BaseOnfidoWeb or something. It would be the same as this, except without the componentWillUnmount
  2. Create lightweight wrappers in index.website.js and index.desktop.js, and pass a ref to the BaseOnfidoWeb component.
  3. Implement componentWillUnmount in index.website.js only.

So the resultant files would look something like this:

index.desktop.js

import BaseOnfidoWeb from './BaseOnfidoWeb';
import onfidoPropTypes from './onfidoPropTypes';

// On desktop, we do not want to teardown onfido, because it causes a crash.
// See https://github.com/Expensify/App/issues/6082
const Onfido = BaseOnfidoWeb;

Onfido.propTypes = onfidoPropTypes;
Onfido.displayName = 'Onfido';

export default Onfido;

index.website.js

import React, {Component} from 'react';
import lodashGet from 'lodash/get';
import BaseOnfidoWeb from './BaseOnfidoWeb';
import onfidoPropTypes from './onfidoPropTypes';

class Onfido extends Component {
    constructor(props) {
        super(props);
        this.baseOnfido = null;
    }
    
    componentWillUnmount() {
        const onfidoOut = lodashGet(this, 'baseOnfido.onfidoOut');
        if (!onfidoOut) {
            return;
        }
        
        onfidoOut.tearDown();
    }
    
    render() {
        return (
            <BaseOnfidoWeb
                ref={e => this.baseOnfido = e}
                
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...props}
            />
        );
    }
}

Onfido.propTypes = onfidoPropTypes;

export default Onfido;

@luacmartins
Copy link
Contributor Author

Thanks for the detailed explanation @roryabraham! Updated!

roryabraham
roryabraham previously approved these changes Dec 3, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB: I'm hesitant about calling it BaseOnfido because it's not used by index.native.js, like other BaseXYZ components usually would be. That's why I suggested BaseOnfidoWeb (because desktop is essentially a subset of web, thanks to Electron.js)

@luacmartins
Copy link
Contributor Author

Updated!

@Luke9389 Luke9389 merged commit 0c9187d into main Dec 3, 2021
@Luke9389 Luke9389 deleted the cmartins-fixOnfidoDesktop branch December 3, 2021 19:56
@Luke9389
Copy link
Contributor

Luke9389 commented Dec 3, 2021

Thanks @roryabraham

@OSBotify
Copy link
Contributor

OSBotify commented Dec 3, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

🚀 Deployed to staging by @Luke9389 in version: 1.1.17-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants