Apple & Google Sign In by cdanwards · Pull Request #23673 · Expensify/App

cdanwards · 2023-07-26T17:59:49Z

Details

Fixed Issues

NOTE: Reading through contributingGuides/APPLE_GOOGLE_SIGNIN.md provides more details about testing in development as well as rationale for design decisions.

Tests

Apple Sign In

iOS

Ensure app builds correctly
Select the Apple Icon Button
Enter valid Apple ID and Password
If prompted, validate with 2 factor authentication
Select to share email with app
Select continue and sign in with Expensify
Ensure logged in correctly

Android

Ensure app builds correctly
Select the Apple Icon Button
Enter valid Apple ID and Password
If prompted, validate with 2 factor authentication
Select to share email with app
Select continue and sign in with Expensify
Ensure logged in correctly

Web

Ensure no errors in console
Select the Apple Icon Button
Ensure Pop Up window appears
Enter valid Apple ID and Password
If prompted, validate with 2 factor authentication
Select to share email with app, if prompted.
Select continue and sign in with Expensify
Ensure correct redirect to Expensify tab
Ensure login occurs

Desktop

Google Sign In

iOS

Ensure app builds correctly
Select the Google Icon Button
Enter valid email and Password
If prompted, validate with 2 factor authentication
Ensure logged in correctly

Android

Ensure app builds correctly
Select the Google Icon Button
Enter valid email and Password
If prompted, validate with 2 factor authentication
Select to share email with app
Select continue and sign in with Expensify
Ensure logged in correctly

Web

Ensure no errors in console
Select the Google Icon Button
Ensure Pop Up window appears
Enter valid email and Password
If prompted, validate with 2 factor authentication
Select to share email with app, if prompted.
Ensure correct redirect to Expensify tab
Ensure login occurs

Desktop

Offline tests

Feature is not available in offline mode

QA Steps

Same as Tests

PR Author Checklist

Screenshots/Videos

Web

Apple:

Apple.Web.Flow_LowRes.mp4

Google:

Google.Web.Flow_LowRes.mp4

Mobile Web - Chrome

Chrome.Mobile.Web_LowRes.mp4

Mobile Web - Safari

Safari.Mobile.Web_LowRes.mp4

Desktop

Apple:

Apple.Desktop.w.Redirect_LowRes.mp4

Google:

Google.Desktop.w.Redirect_LowRes.mp4

iOS

IOS_LowRes.mp4

Android

Android_LowRes.mp4

…3-apple-sign-in" This reverts commit 948f7a1, reversing changes made to 99519f2.

cdanwards · 2023-08-16T19:16:27Z

Yeah! @situchan it's pretty easy to get your authToken and agent up and running. Here's the explanation on their site: https://dashboard.ngrok.com/get-started/setup - Once you login they list through the steps of setting it up and running the agent.

situchan · 2023-08-17T14:50:11Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

cdanwards · 2023-08-17T17:54:49Z

@marcochavezf are there any other checks we need to do to get this through?

Also, just to clarify. I'm going to create an issue preemptively for the Bug4 (COOP error likely from Google's GSI), were there any other errors that should have issues created to track? I know we solved the white background issue on dev by hiding the buttons on dev.

marcochavezf · 2023-08-17T18:22:07Z

We're just tracking the issue here, we can link the bug4 there

situchan · 2023-08-17T18:24:07Z

@marcochavezf We should test web based platforms after deploy to staging, right?

marcochavezf · 2023-08-17T18:27:27Z

Yes, given the limitations to testing localhost in both platforms, I'm going to merge it to include it in today's deployment and test properly the remaining parts on staging

cdanwards · 2023-08-17T18:32:36Z

@marcochavezf just to make sure I'm on the same page. Link the conversation about bug4 in the tracking issue, or create a new issue about bug4 and link that in the tracking issue?

OSBotify · 2023-08-17T18:35:43Z

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

marcochavezf · 2023-08-17T18:36:04Z

Ah sorry for the confusion @cdanwards, let's create a new issue for bug4 and link it to the tracking issue please

cdanwards · 2023-08-17T18:50:23Z

@marcochavezf no worries! Just wanted to make sure I understood. I'll get that issue created!

OSBotify · 2023-08-21T22:25:46Z

🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.3.56-0 🚀

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

dangrous · 2023-08-23T14:41:28Z

Hi all! See the above deploy blocker - #25768 - Can we look into why that Google sign in button jumps on desktop Safari? Thanks!

cdanwards · 2023-08-23T14:50:21Z

@dangrous Hey! I'm taking a look at this! We mainly tested on Chrome and Brave browsers during development and I haven't been able to reproduce there (We were informed that Firefox wasn't supported). Is Safari one of the supported browsers? It definitely seems like an issue of the iframe rendering and possible re-rendering the button and the styles being updated on a millisecond delay.

dangrous · 2023-08-23T15:04:36Z

Yeah that's what I was thinking too. I wonder if we will need to apply the styles/width/height to an empty element and then replace it with the Google button when it's loaded, or something?

As for browsers, I'm confirming in Slack but it looks like Safari was on our QA test list so it seems like yes.

EDIT: confirmed, we support Safari

OSBotify · 2023-08-24T03:08:47Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀

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

napster125 · 2023-08-26T01:54:05Z

+            setHasShownPrompt(true);
+        } else {
+            // Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not
+            if (currentScreen === undefined || isAuthenticated === false) {


In this context, why would we use isAuthenticated === false?

cc: @cdanwards

#25790

@napster125 We don't want to show the deeplink prompt to unauthenticated users.

There's discussion of it within this feature's design document. Trying to find if we included that conversation in the contributing guide.

Following up, here are the slack threads where this was discussed:
https://expensify.slack.com/archives/C01GTK53T8Q/p1683070404415199
https://expensify.slack.com/archives/C01GTK53T8Q/p1689886614735019

parasharrajat · 2023-09-05T11:17:43Z

Google sign-in uses a system theme and adjusts coloring accordingly. We didn't handle the color scheme for this which caused a white background on the G-sign button. #25638

alitoshmatov · 2023-12-19T10:34:21Z

+        if (!isMacOSWeb() || isUnsupportedDeeplinkRoute || CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV || hasShownPrompt) {
            return;
        }
+        // We want to show the prompt immediately if the user is already authenticated.
+        // Otherwise, we want to wait until the navigation state is set up
+        // and we know the user is on a screen that supports deeplinks.
+        if (isAuthenticated) {
+            promptToOpenInDesktopApp();
+            setHasShownPrompt(true);
+        } else {
+            // Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not
+            if (currentScreen === undefined || isAuthenticated === false) {


These conditions did not consider opening magic link on logged in user, and caused this issue #32795

lindboe and others added 30 commits July 21, 2023 16:44

Revert "Merge pull request Expensify#23295 from Expensify/revert-1672…

e30d9d2

…3-apple-sign-in" This reverts commit 948f7a1, reversing changes made to 99519f2.

Added react-native-google-signin

243c7eb

Added contributing guide text

cee1a2d

Ran pod install

409be2f

Added url scheme to info plist

bf6fa12

Add google ids to const

75a205c

Adds Desktop Pages

7cf33eb

Adds jest config fix

1e893fd

Adds linkingConfig and Icon

fa47fa7

Adds Google Sign In Buttons

2dd569d

Adds begin google sign in function

7bdf16f

Updates LoginForm ThirdPartySignInPage and styles

cbd132b

Create DesktopRedirectPage

b052e1e

Added Desktop Login method to onyx

5a8e513

Set up everything to display redirect screen

4eb4ecb

Switch to using DeeplinkRedirectLoadingIndicator

75d01cf

Redirection to desktop flow is working

4153b86

Revert back Deeplinkredirectloadingindicator

11fd24c

Updated logic on DesktopRedirectPage

257e3c4

Fix navigation logic when navigating back to Desktop

cb2b07e

Add navigation to HOME for continue to web app

22d0db4

Cleanup after self review

c0dc536

Fix issue with google sign in button

4e3a024

Switch from onyx key to url pathname redirect approach

60afcb0

Remove console log

518b912

Added Spanish Translations

26676a6

Removed unused line

194e809

Some updates after self-review

34fc06a

Reverting a change

04d5d6e

Adds loadingindicator when making expensify api call

78ad536

marcochavezf approved these changes Aug 17, 2023

View reviewed changes

marcochavezf merged commit 2b52597 into Expensify:main Aug 17, 2023

cdanwards mentioned this pull request Aug 17, 2023

FEATURE REQUEST: Add Google/Apple sign in on New Dot, like we have on Old Dot #7079

Closed

OSBotify mentioned this pull request Aug 21, 2023

Deploy Checklist: New Expensify 2023-08-21 #25625

Closed

marcaaron mentioned this pull request Aug 22, 2023

Log in - Apple & Google Sign in buttons are too close and are touching outlines #25637

Closed

6 tasks

lanitochka17 mentioned this pull request Aug 22, 2023

[HOLD for payment 2023-09-04] Login - Can't sign in with Apple #25697

Closed

6 tasks

This was referenced Aug 22, 2023

Fix for frozen UI state on desktop after redirect from 3rd party sign in #25706

Merged

Console Error appears when signing in with Google Sign In (Cross-Origin-Opener-Policy) #25427

Closed

dangrous mentioned this pull request Aug 23, 2023

[HOLD for payment 2023-09-04] Login - Sign in with Google logo jumps after the login page is refreshed #25768

Closed

6 tasks

pradeepmdk mentioned this pull request Aug 25, 2023

[$1000] Deeplink - No prompt to open in app when navigating to deep link as logged-out web user #25790

Closed

6 tasks

napster125 reviewed Aug 26, 2023

View reviewed changes

parasharrajat mentioned this pull request Sep 5, 2023

[HOLD for payment 2023-09-04] [$1000] BUG: Rectangular white container surrounds the Google sign-in button #25638

Closed

6 tasks

alitoshmatov reviewed Dec 19, 2023

View reviewed changes

alitoshmatov mentioned this pull request Dec 19, 2023

[HOLD for payment 2023-12-20] [$500] Desktop - Expired magic code page for a valid code with no prior use + inability to exit the screen #32795

Closed

6 tasks

Uh oh!

Conversation

cdanwards commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Apple Sign In

iOS

Android

Web

Desktop

Google Sign In

iOS

Android

Web

Desktop

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

cdanwards commented Aug 16, 2023

Uh oh!

situchan commented Aug 17, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

cdanwards commented Aug 17, 2023

Uh oh!

marcochavezf commented Aug 17, 2023

Uh oh!

situchan commented Aug 17, 2023

Uh oh!

marcochavezf commented Aug 17, 2023

Uh oh!

cdanwards commented Aug 17, 2023

Uh oh!

OSBotify commented Aug 17, 2023

Uh oh!

marcochavezf commented Aug 17, 2023

Uh oh!

cdanwards commented Aug 17, 2023

Uh oh!

OSBotify commented Aug 21, 2023

Uh oh!

dangrous commented Aug 23, 2023

Uh oh!

cdanwards commented Aug 23, 2023

Uh oh!

dangrous commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Aug 24, 2023

Uh oh!

napster125 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

ArekChr Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

cdanwards Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

cdanwards Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

cdanwards Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Sep 5, 2023

Uh oh!

alitoshmatov Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

cdanwards commented Jul 26, 2023 •

edited

Loading

dangrous commented Aug 23, 2023 •

edited

Loading