Signin rhp #24083

daordonez11 · 2023-08-03T00:04:35Z

Details

Implementation to send sign in to RHP

Fixed Issues

$ #20558
PROPOSAL: #20558 (comment)

Tests

Open a public room as unauthenticated user (i.e. https://staging.new.expensify.com/r/8813851443373010)
Click on sign-in
Click on the back button, should see the public room instead of closing the app
Enter sign-in in RHP
//Validate claim
Enter a new email and enters the validation code
Verify the app loads with the authenticated data
Open in an incognito mode, sign in with another account and post something in the public room
Verify the message appears in the public room for the first user that claimed the anonymous account
Repeat the process again from step 4 to 8, but in step 5 enter the email of an existing account

Offline tests

N/A anonymous users only work online

QA Steps

Same as test steps

PR Author Checklist

Screenshots/Videos

Web

Web.Recording.2023-08-04.at.8.55.39.AM.mov

Web.Refresh.Testing.-.8_9_2023.8_33_09.PM.webm

Mobile Web - Chrome

mWeb.Chrome.Recording.2023-08-04.at.8.57.39.AM.mov

mWeb.Refresh.-.8_9_2023.8_38_46.PM.webm

Mobile Web - Safari

mWeb.Safari.2023-08-04.at.9.02.36.AM.mov

Desktop

N/A Public rooms are not supported in desktop

iOS

iOS.login.2023-08-07.at.1.04.18.AM.mov

Android

Android.login.2023-08-07.at.12.59.11.AM.mov

melvin-bot · 2023-08-07T06:15:53Z

@mollfpr 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]

mollfpr

Screen.Recording.2023-08-07.at.23.16.25.mov

@daordonez11 Could you confirm if this issue already notice before and will be fixed later?

src/pages/signin/SignInPage.js

src/ROUTES.js

src/pages/signin/SignInModal.js

marcochavezf · 2023-08-07T21:20:00Z

@daordonez11 @mollfpr for the test steps, also add the case to verify that the logged in user can send a message and other users receive it without refreshing the browser (you can use the same steps from here).

daordonez11 · 2023-08-08T17:55:43Z

For sure steps updated, will record again after fixing changes required by @mollfpr

daordonez11 · 2023-08-08T17:59:34Z

Screen.Recording.2023-08-07.at.23.16.25.mov
@daordonez11 Could you confirm if this issue already notice before and will be fixed later?

@marcochavezf what do you think of that issue, I have 2 ideas.

Send to home if user is already logged in (dismissModal)
Use something like authenticated views so Hmmmmm not found is shown.

I think 1 is easier but users would still see for a fraction the view. Which one do you think we should use?

Thanks for the report @mollfpr I did see it once I think it can also be reproduced by pressing back button.

marcochavezf · 2023-08-08T22:12:22Z

Screen.Recording.2023-08-07.at.23.16.25.mov
@daordonez11 Could you confirm if this issue already notice before and will be fixed later?

@marcochavezf what do you think of that issue, I have 2 ideas.

Send to home if user is already logged in (dismissModal)

Use something like authenticated views so Hmmmmm not found is shown.

I think 1 is easier but users would still see for a fraction the view. Which one do you think we should use?

Thanks for the report @mollfpr I did see it once I think it can also be reproduced by pressing back button.

My first question is, why the validation code is set after refreshing the page? I think it's fine to show the sign-in modal when the user enters the URL, but it's weird that the form is displaying the validation code and trying to submit it

daordonez11 · 2023-08-08T23:51:26Z

So basically this occurs because the state of the component is mantained, since this was originally an unprotected view the user does not see it but its just the "final" state of the page. We could use something like clearLocalSignInData before leaving the page but tbh I don't think showing the signin should be enabled when you are already logged in. Me recommendation would be to auto dismiss the modal if the view is accessed via url and the user is already logged in.

cc @mollfpr @marcochavezf

mollfpr · 2023-08-09T12:37:29Z

Me recommendation would be to auto dismiss the modal if the view is accessed via url and the user is already logged in.

@daordonez11 @marcochavezf I agree with this.

marcochavezf · 2023-08-09T15:35:54Z

Me recommendation would be to auto dismiss the modal if the view is accessed via url and the user is already logged in.

@daordonez11 @marcochavezf I agree with this.

Sounds good, let's go with that solution!

daordonez11 · 2023-08-09T20:39:54Z

Ok, guys issue is now solved with the last commit. Will now execute the tests requested by @marcochavezf and include videos with the new scenario 🫡

cc @mollfpr

daordonez11 · 2023-08-10T01:29:18Z

Ok guys had to implement a last detail in order for notifications and new scenarios to work. Final commit includes that implementation. Currently uploading videso with new implementation. So ready again for review with new scenario @mollfpr!

cc @marcochavezf

mollfpr · 2023-08-12T12:08:25Z

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

New_Recording_-_8_18_2023._10_26_18_PM.mp4

Mobile Web - Chrome

Screen_Recording_20230818_224922_Chrome.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-18.at.22.46.52.mp4

Desktop

Screen.Recording.2023-08-18.at.22.41.27.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-18.at.22.43.46.mp4

Android

Screen_Recording_20230818_235329_New.Expensify.Dev.mp4

mollfpr

Sorry for the delay, I am just feeling better now 🙏

The code mostly looks good, just couples of NABs.

There's a strange bug that showing the SignInModal page after sign in.

Steps:

Open a public room with anonymous account
Clear the browser storage
Open the sign-in page and complete the sign-in flow
The user will be redirected to the sign-in-modal with the validate code filled.

I believe this is an edge-case, but I'm afraid we will have similar issue with no step reproduce it yet.

Screen.Recording.2023-08-13.at.14.27.48.mp4

src/pages/signin/SignInModal.js

src/pages/signin/SignInPage.js

src/pages/signin/SignInPageLayout/index.js

daordonez11 · 2023-08-14T01:26:03Z

Hey @mollfpr thanks for the update and hope you get better! I'll check the NAB's and the error tomorrow, I definitely need to make sure rhp is not shown if you are already logged in, probably with more than one check. I'll let you guys know when it's ready.

cc @marcochavezf

daordonez11 · 2023-08-15T19:13:46Z

Thanks for the report @mollfpr the issue was happening because navigation was not ready at that point so dismissModal did nothing. Just uploaded a commit with the NABs and the fix with the navigation promise. The easy way of replicating was just opening a new tab with the sign in modal url 😂.

cc @marcochavezf

daordonez11 · 2023-08-17T22:37:38Z

PS: Solved conflict that appeared today!
cc @marcochavezf @mollfpr

mollfpr · 2023-08-18T14:10:45Z

Thanks, @daordonez11. Seems looks good now. I'll be finishing the records in an hour.

mollfpr

LGTM and tests well 🚀

marcochavezf

Looks great, thank you guys, you're awesome!

OSBotify · 2023-08-19T01:36:39Z

✋ 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 · 2023-08-21T22:23:43Z

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

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

OSBotify · 2023-08-24T03:06:44Z

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

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

allroundexperts · 2023-10-17T20:08:20Z

src/pages/signin/SignInPageLayout/index.js

@@ -35,6 +35,9 @@ const propTypes = {
    /** Whether to show welcome header on a particular page */
    shouldShowWelcomeHeader: PropTypes.bool.isRequired,

+    /** Whether or not the sign in page is being rendered in the RHP modal */
+    isInModal: PropTypes.bool.isRequired,


When making this required, we forgot to update the props for this component in src/pages/signin/ThirdPartySignInPage.js (https://github.com/Expensify/App/pull/23673/files#diff-7d96f9ceabb4069bb79fe99d6e1f2610c32b4190911f32f455e3cc16449c299bR59). This later caused #28255

roryabraham · 2023-10-26T23:44:40Z

Hi 👋🏼

The SignInPageLayout was dependent upon isSmallScreenWidth for it's layout to look correct. However, this displayed the SignInPageLayout in the RHP. The intention was for it to look and behave how it would in a small screen, but the isSmallScreenWidth was still true on wide screens.

We addressed this bug in #28025

shubham1206agra · 2023-11-24T13:51:04Z

Hello

You have forgot to take out extra padding for RHP, which caused #31445.

Pujan92 · 2024-03-13T18:10:10Z

src/libs/Navigation/AppNavigator/AuthScreens.js

@@ -48,6 +48,11 @@ Onyx.connect({
        }

        currentAccountID = val.accountID;
+        if (Navigation.isActiveRoute(ROUTES.SIGN_IN_MODAL)) {
+            // This means sign in in RHP was successful, so we can dismiss the modal and subscribe to user events
+            Navigation.dismissModal();


Seems we are closing the modal early which causes the issue #36976

ZhenjaHorbach · 2024-09-17T19:38:16Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+        if (Navigation.isActiveRoute(ROUTES.SIGN_IN_MODAL)) {
+            // This means sign in in RHP was successful, so we can dismiss the modal and subscribe to user events
+            Navigation.dismissModal();
+            User.subscribeToUserEvents();


Came from this issue
To solve this error, we needed to make sure that Pusher instance initialization (Pusher.init()) is complete before any subscription calls are made.

Like here

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Lines 255 to 261 in f58439c

Pusher.init({

appKey: CONFIG.PUSHER.APP_KEY,

cluster: CONFIG.PUSHER.CLUSTER,

authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/AuthenticatePusher?`,

}).then(() => {

User.subscribeToUserEvents();

});

daordonez11 added 4 commits August 2, 2023 18:28

feature(signinmodal): Version 0.1 signin modal in right modal navigator

d98441f

feature(signinmodal): Small nav fixes

f24e7ce

feat(signinmodal): Sign in working with custom action anonuser

b4f495f

task: Changes after cherrypick, make new branch work

cd30cf9

This was referenced Aug 3, 2023

Poc signin in rhp #22005

Closed

[HOLD for payment 2023-08-30] [$1000] Navigate back to public room after clicking on Sign-in as unauthenticated user #20558

Closed

daordonez11 added 6 commits August 2, 2023 19:21

task: Fix current lint issues

53b6fa9

task: Run prettier on current status of code

10b82c2

task: Make login work with new sign in command, and color!

2c5842c

task: Remove unnecessary dependency

68bdb8f

task: Remove unnecessary isInModal

8320e5d

task: Run prettier on last state

78a5641

daordonez11 marked this pull request as ready for review August 7, 2023 06:15

daordonez11 requested a review from a team as a code owner August 7, 2023 06:15

melvin-bot bot requested review from mollfpr and removed request for a team August 7, 2023 06:15

mollfpr suggested changes Aug 7, 2023

View reviewed changes

src/pages/signin/SignInPage.js Outdated Show resolved Hide resolved

src/ROUTES.js Outdated Show resolved Hide resolved

src/pages/signin/SignInModal.js Outdated Show resolved Hide resolved

marcochavezf self-requested a review August 7, 2023 21:15

marcochavezf assigned daordonez11 Aug 7, 2023

task: Renames and improvements requested by C+

961366d

task: Dismiss modal if user is already logged in and access via url

fd26e25

marcochavezf requested a review from mollfpr August 10, 2023 17:36

mollfpr reviewed Aug 13, 2023

View reviewed changes

src/pages/signin/SignInModal.js Outdated Show resolved Hide resolved

src/pages/signin/SignInPage.js Show resolved Hide resolved

src/pages/signin/SignInPageLayout/index.js Show resolved Hide resolved

tasks: NAB improvements and fix avoid opening modal if nav is not ready

a77cdb3

daordonez11 requested a review from mollfpr August 16, 2023 19:59

Merge branch 'main' into signin-rhp

e0dd7eb

Task: Missing space after solving conflicts

f346c40

mollfpr approved these changes Aug 18, 2023

View reviewed changes

marcochavezf approved these changes Aug 19, 2023

View reviewed changes

marcochavezf merged commit 2004924 into Expensify:main Aug 19, 2023
13 checks passed

OSBotify mentioned this pull request Aug 21, 2023

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

Closed

daordonez11 mentioned this pull request Sep 1, 2023

[HOLD for payment 2024-04-15] [$750] mWeb - Click on back button from country picker page redirects to wrong page #23725

Closed

6 tasks

Pujan92 mentioned this pull request Oct 3, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$500] Footer for anonymous user sign in modal distorted #28025

Closed

6 tasks

allroundexperts reviewed Oct 17, 2023

View reviewed changes

allroundexperts mentioned this pull request Oct 17, 2023

[HOLD for payment 2023-10-12] [$500] Dev: Login - Console error when visiting sign-in-with-google #28255

Closed

6 tasks

shubham1206agra mentioned this pull request Nov 24, 2023

[HOLD for payment 2023-11-30] [$500] Public room - Extra bottom padding on the header in the sign-in modal accessed from public room #31445

Closed

6 tasks

Pujan92 reviewed Mar 13, 2024

View reviewed changes

Pujan92 mentioned this pull request Mar 13, 2024

[HOLD for payment 2024-03-13] [$500] Sign in banner stays few seconds after logging in with existing account #36976

Closed

6 tasks

ZhenjaHorbach reviewed Sep 17, 2024

View reviewed changes

ZhenjaHorbach mentioned this pull request Sep 17, 2024

[$250] [Crashlytics] TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe() most likely has been called before Pusher.init() #46119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signin rhp #24083

Signin rhp #24083

daordonez11 commented Aug 3, 2023 •

edited

Loading

melvin-bot bot commented Aug 7, 2023

mollfpr left a comment

marcochavezf commented Aug 7, 2023

daordonez11 commented Aug 8, 2023

daordonez11 commented Aug 8, 2023

marcochavezf commented Aug 8, 2023

daordonez11 commented Aug 8, 2023

mollfpr commented Aug 9, 2023

marcochavezf commented Aug 9, 2023

daordonez11 commented Aug 9, 2023

daordonez11 commented Aug 10, 2023

mollfpr commented Aug 12, 2023 •

edited

Loading

mollfpr left a comment

daordonez11 commented Aug 14, 2023

daordonez11 commented Aug 15, 2023

daordonez11 commented Aug 17, 2023

mollfpr commented Aug 18, 2023

mollfpr left a comment

marcochavezf left a comment •

edited

Loading

OSBotify commented Aug 19, 2023

OSBotify commented Aug 21, 2023

OSBotify commented Aug 24, 2023

allroundexperts Oct 17, 2023

roryabraham commented Oct 26, 2023

shubham1206agra commented Nov 24, 2023

Pujan92 Mar 13, 2024

ZhenjaHorbach Sep 17, 2024

	Pusher.init({
	appKey: CONFIG.PUSHER.APP_KEY,
	cluster: CONFIG.PUSHER.CLUSTER,
	authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/AuthenticatePusher?`,
	}).then(() => {
	User.subscribeToUserEvents();
	});

Signin rhp #24083

Signin rhp #24083

Conversation

daordonez11 commented Aug 3, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 7, 2023

mollfpr left a comment

Choose a reason for hiding this comment

marcochavezf commented Aug 7, 2023

daordonez11 commented Aug 8, 2023

daordonez11 commented Aug 8, 2023

marcochavezf commented Aug 8, 2023

daordonez11 commented Aug 8, 2023

mollfpr commented Aug 9, 2023

marcochavezf commented Aug 9, 2023

daordonez11 commented Aug 9, 2023

daordonez11 commented Aug 10, 2023

mollfpr commented Aug 12, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

mollfpr left a comment

Choose a reason for hiding this comment

daordonez11 commented Aug 14, 2023

daordonez11 commented Aug 15, 2023

daordonez11 commented Aug 17, 2023

mollfpr commented Aug 18, 2023

mollfpr left a comment

Choose a reason for hiding this comment

marcochavezf left a comment • edited Loading

Choose a reason for hiding this comment

OSBotify commented Aug 19, 2023

OSBotify commented Aug 21, 2023

OSBotify commented Aug 24, 2023

allroundexperts Oct 17, 2023

Choose a reason for hiding this comment

roryabraham commented Oct 26, 2023

shubham1206agra commented Nov 24, 2023

Pujan92 Mar 13, 2024

Choose a reason for hiding this comment

ZhenjaHorbach Sep 17, 2024

Choose a reason for hiding this comment

daordonez11 commented Aug 3, 2023 •

edited

Loading

mollfpr commented Aug 12, 2023 •

edited

Loading

marcochavezf left a comment •

edited

Loading