Add unlink secondary login flow #15811

NikkiWines · 2023-03-10T02:03:14Z

Details

Adds the ability to unlink claimed but unvalidated secondary logins, inline with what we do on oldDot

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/268314

Tests

Log into an account (loginA) on oldDot or newDot and add (but do not validate) a secondary login (loginB)
Log out of the account and navigate to the newDot sign in page
Try to sign in loginB and confirm you're directed to a view that says you'd need to validate that login from the account settings on loginA. Confirm there is an Unlink button that you can press to unlink the two logins.
Press Unlink and confirm that you see the text Link sent! appear on the page
Confirm you receive a notification (to loginB) with a link to unlink your logins
Copy the validation link but modify the validationCode so that it's no longer a valid code
Go to that link and confirm that you see an error message notifying you that your link is invalid or expired.
- Note: On Web, if you open the link in the same session, you'll see the unlink page because we'll have stored the credentials and know which view to display. If you open the link in a new session (i.e. in an incognito window) the error message will display on the main sign in view because we won't have the login stored.
Click Unlink again to get a new link
Click the link in the notification and confirm you're directed to the sign in page with a success message saying that your secondary login has been successfully unlinked.

Verify that no errors appear in the JS console

Offline tests

While online

Log into an account (loginA) on oldDot or newDot and add (but do not validate) a secondary login (loginB)
Log out of the account and navigate to the newDot sign in page
Try to sign in loginB and confirm you're directed to a view that says you'd need to validate that login from the account settings on loginA.
Go offline
Confirm there is an Unlink button, but that it is disabled and you cannot press it
Confirm that you see the offline indicator at the bottom of the page.

QA Steps

Log into an account (loginA) on oldDot or newDot and add (but do not validate) a secondary login (loginB)
Log out of the account and navigate to the newDot sign in page
Try to sign in loginB and confirm you're directed to a view that says you'd need to validate that login from the account settings on loginA. Confirm there is an Unlink button that you can press to unlink the two logins.
Press Unlink and confirm that you see the text Link sent! appear on the page
Confirm you receive a notification (to loginB) with a link to unlink your logins
Copy the validation link but modify the validationCode so that it's no longer a valid code
Go to that link and confirm that you see an error message notifying you that your link is invalid or expired.
- Note: On Web, if you open the link in the same session, you'll see the unlink page because we'll have stored the credentials and know which view to display. If you open the link in a new session (i.e. in an incognito window) the error message will display on the main sign in view because we won't have the login stored.
Click Unlink again to get a new link
Click the link in the notification and confirm you're directed to the sign in page with a success message saying that your secondary login has been successfully unlinked.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

Screen.Recording.2023-04-21.at.14.52.44.mov

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mov

iOS

(unable to test link redirecting but can show the updated UI)

ios.mp4

Android

(unable to test link redirecting but can show the updated UI)

add more copy

disable button after link is sent

…indicators for success and failure messages

NikkiWines · 2023-04-10T23:41:07Z

Been fighting with my android emulator for the past 48 hours but this is otherwise ready for review. It's on hold for the corresponding (internal) Web-E PR though because it adds the necessary API commands.

cc: @Gonals as you're assigned to the Web-E PR

MelvinBot · 2023-04-10T23:41:47Z

@mananjadhav @MonilBhavsar One of you needs to 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]

…kki-unlink-login

mananjadhav · 2023-04-11T11:23:18Z

@NikkiWines Is this ready for review because the title says [Hold]?

NikkiWines · 2023-04-11T16:15:08Z

Ah, yeah @mananjadhav you won't be able to review this yet because it's dependent on an internal PR that's in review. I can unassign you for now and re-assign once this isn't on hold any more if you'd like!

mananjadhav · 2023-04-11T16:22:16Z

I think you can keep me assigned and I can get to it once the internal PR is through. I just clarified because the title said HOLD.

NikkiWines · 2023-04-14T16:37:54Z

Internal PR is on prod! Taking this off hold 🎊

NikkiWines · 2023-05-03T17:24:44Z

Sorry @mananjadhav I got pulled into some other work today and didn't have a chance to take a further look into this. Will prioritize this tomorrow

…link-login

NikkiWines · 2023-05-04T14:18:52Z

Ok, still unable to reproduce but @mananjadhav I pushed a small change that I suspect might resolve the issue. Could you retry your steps once more to see if you still encounter the error? Thank you 🙌

mananjadhav · 2023-05-04T20:25:57Z

Thanks @NikkiWines I think the last change worked. I was able to successfully test this on Web. I am now going to test this on other platforms.

mananjadhav · 2023-05-04T21:16:34Z

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

web-unlink-secondary-link.mov

Mobile Web - Chrome

mweb-chrome-unlink-secondary-link.mov

Mobile Web - Safari

mweb-safari-unlink-secondary-link.mov

Desktop

desktop-unlink-secondary-link.mov

iOS

ios-unlink-secondary-link.mov

Android

android-unlink-secondary-link.mov

Thanks for the patience here @NikkiWines @MonilBhavsar. As I mentioned, I was finally able to test without issues. @MonilBhavsar All yours.

melvin-bot · 2023-05-04T21:17:00Z

🎯 @mananjadhav, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #18445.

MonilBhavsar

Some NAB's, else looks good 👍

MonilBhavsar · 2023-05-05T04:22:02Z

src/libs/actions/Session/index.js

@@ -187,6 +187,8 @@ function beginSignIn(login) {
            value: {
                ...CONST.DEFAULT_ACCOUNT_DATA,
                isLoading: true,
+                errors: null,


Hmm, errors is already null in the default data, may be we just need message field

MonilBhavsar · 2023-05-05T04:23:26Z

src/pages/UnlinkLoginPage.js

+const propTypes = {
+    /** The accountID and validateCode are passed via the URL */
+    route: validateLinkPropTypes,
+


NAB, empty line

MonilBhavsar · 2023-05-05T04:24:59Z

src/pages/signin/SignInPage.js

@@ -28,6 +29,9 @@ const propTypes = {

        /** Whether the account is validated */
        validated: PropTypes.bool,
+
+        /** The primaryLogin associated with the account */
+        primaryLogin: PropTypes.string,


Yeah, I think there was a lint rule that required default props or the prop to be required. Looks like it is not working lately.

NikkiWines · 2023-05-05T08:43:09Z

woo thank you @MonilBhavsar and @mananjadhav 🙌 updated once more

MonilBhavsar

Thanks! Looks good

OSBotify · 2023-05-05T11:28:08Z

✋ 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-05-05T20:02:14Z

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.11-0 🚀

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

Beamanator · 2023-05-08T09:39:35Z

src/libs/actions/Session/index.js

+ */
+function requestUnlinkValidationLink() {
+    const optimisticData = [{
+        onyxMethod: CONST.ONYX.METHOD.MERGE,


FYI this just got removed! And it was replaced with Onyx.METHOD.MERGE, where Onyx is imported from react-native-onyx.

I can spin up a fix for this real quick

Oh I guess this was found in this bug report: #18526

OSBotify · 2023-05-09T04:04:49Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀

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

0xmiros · 2023-06-02T15:11:07Z

src/pages/signin/SignInPage.js

@@ -143,6 +164,7 @@ class SignInPage extends Component {
                        <PasswordForm isVisible={showPasswordForm} />
                    )}
                    {showResendValidationForm && <ResendValidationForm />}
+                    {showUnlinkLoginForm && <UnlinkLoginForm />}


This line caused console error as showUnlinkLoginForm can be string value. More details - #19202 (comment)

New checklist item added to prevent such bugs:

I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.

parasharrajat · 2023-06-26T18:35:12Z

src/libs/actions/Session/index.js

+        key: ONYXKEYS.ACCOUNT,
+        value: {
+            isLoading: false,
+            message: Localize.translateLocal('unlinkLoginForm.linkSent'),


Because this will return a static value, it will not translate on runtime when the use changes the language after the message is shown to the user which caused #18827

mollfpr · 2023-08-07T17:36:09Z

src/libs/actions/Session/index.js

+        key: ONYXKEYS.ACCOUNT,
+        value: {
+            isLoading: false,
+            message: Localize.translateLocal('unlinkLoginForm.succesfullyUnlinkedLogin'),


Hi ✋ Coming from #22934

We should return the message as the phrase key instead to get the dynamic message based on the language.

thanks for highlighting this.

NikkiWines added 3 commits March 9, 2023 17:05

add unlinkLoginForm

c0ddf22

update SignInPage to conditionally show unlink form

5213996

Merge branch 'main' of github.com:Expensify/App into nikki-unlink-login

58281b8

This comment was marked as outdated.

Sign in to view

NikkiWines self-assigned this Mar 13, 2023

NikkiWines added 9 commits March 13, 2023 22:40

Add Session.requestUnlinkValidationLink command

7ba9f6e

add more copy

Add unlinkLogin command

810b3b1

jsx style

0d105e6

disable button after link is sent

Merge branch 'main' of github.com:Expensify/App into nikki-unlink-login

a7dcb51

Add UnlinkLogin route and page

e686df5

clear login from credentials, add some spanish translations, add dot …

a7dc7c7

…indicators for success and failure messages

move styles to prevent padding jump when moving between sign in views

5cc40ba

update copy

0b2ff29

f35a043

NikkiWines changed the title ~~Add unlink secondary login flow~~ [HOLD] Add unlink secondary login flow Apr 10, 2023

NikkiWines marked this pull request as ready for review April 10, 2023 23:41

NikkiWines requested a review from a team as a code owner April 10, 2023 23:41

melvin-bot bot requested review from mananjadhav and MonilBhavsar and removed request for a team April 10, 2023 23:41

NikkiWines added 2 commits April 10, 2023 17:58

minor style + copy update

b175e73

Merge branch 'nikki-unlink-login' of github.com:Expensify/App into ni…

cda9544

…kki-unlink-login

NikkiWines mentioned this pull request Apr 11, 2023

[$1000] Extra horizontal spacing (extra 13px on left and right side) on the resend link password page #17177

Closed

6 tasks

NikkiWines changed the title ~~[HOLD] Add unlink secondary login flow~~ Add unlink secondary login flow Apr 14, 2023

NikkiWines added 2 commits May 4, 2023 15:47

Merge branch 'main' of https://github.com/Expensify/App into nikki-un…

c12f968

…link-login

optimistically remove message from account key when beginning sign in

f2333c6

mananjadhav previously approved these changes May 4, 2023

View reviewed changes

melvin-bot bot mentioned this pull request May 4, 2023

[Hold for payment 2023-05-15] Payment issue for E/App PR#15811 #18445

Closed

MonilBhavsar previously approved these changes May 5, 2023

View reviewed changes

minor style updates

93cf44c

NikkiWines dismissed stale reviews from MonilBhavsar and mananjadhav via 93cf44c May 5, 2023 08:42

MonilBhavsar approved these changes May 5, 2023

View reviewed changes

MonilBhavsar merged commit 7e957af into main May 5, 2023

MonilBhavsar deleted the nikki-unlink-login branch May 5, 2023 11:20

OSBotify mentioned this pull request May 5, 2023

Deploy Checklist: New Expensify 2023-05-05 #18498

Closed

31 tasks

kbecciv mentioned this pull request May 6, 2023

[HOLD for payment 2023-05-16] Login - Text "Link sent" doesn't appear and link is not sent after Unlink button clicked, console error appears #18526

Closed

6 tasks

Beamanator reviewed May 8, 2023

View reviewed changes

Beamanator mentioned this pull request May 8, 2023

Add ability to set a contact method your default contact method #16750

Merged

55 tasks

situchan mentioned this pull request May 22, 2023

[HOLD for payment 2023-06-07] [$1000] DEV: Console error on click Continue button in login page #19202

Closed

6 tasks

0xmiros reviewed Jun 2, 2023

View reviewed changes

parasharrajat reviewed Jun 26, 2023

View reviewed changes

parasharrajat mentioned this pull request Jun 26, 2023

[HOLD for payment 2023-06-21] [$1000] 'Link sent!' message is not dynamically updated when changing language preference #18827

Closed

6 tasks

mollfpr reviewed Aug 7, 2023

View reviewed changes

mollfpr mentioned this pull request Aug 7, 2023

[HOLD for payment 2023-08-08] [$1000] Dev: Web - App crashes when trying to unlink secondary contact method #22934

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unlink secondary login flow #15811

Add unlink secondary login flow #15811

NikkiWines commented Mar 10, 2023 •

edited

Loading

This comment was marked as outdated.

NikkiWines commented Apr 10, 2023 •

edited

Loading

MelvinBot commented Apr 10, 2023

mananjadhav commented Apr 11, 2023

NikkiWines commented Apr 11, 2023

mananjadhav commented Apr 11, 2023

NikkiWines commented Apr 14, 2023

NikkiWines commented May 3, 2023

NikkiWines commented May 4, 2023

mananjadhav commented May 4, 2023

mananjadhav commented May 4, 2023

melvin-bot bot commented May 4, 2023

MonilBhavsar left a comment

MonilBhavsar May 5, 2023

MonilBhavsar May 5, 2023

MonilBhavsar May 5, 2023

NikkiWines commented May 5, 2023

MonilBhavsar left a comment

OSBotify commented May 5, 2023

OSBotify commented May 5, 2023

Beamanator May 8, 2023

Beamanator May 8, 2023

OSBotify commented May 9, 2023

0xmiros Jun 2, 2023

parasharrajat Jun 26, 2023

mollfpr Aug 7, 2023

mananjadhav Aug 7, 2023

Add unlink secondary login flow #15811

Add unlink secondary login flow #15811

Conversation

NikkiWines commented Mar 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

This comment was marked as outdated.

NikkiWines commented Apr 10, 2023 • edited Loading

MelvinBot commented Apr 10, 2023

mananjadhav commented Apr 11, 2023

NikkiWines commented Apr 11, 2023

mananjadhav commented Apr 11, 2023

NikkiWines commented Apr 14, 2023

NikkiWines commented May 3, 2023

NikkiWines commented May 4, 2023

mananjadhav commented May 4, 2023

mananjadhav commented May 4, 2023

Reviewer Checklist

Screenshots/Videos

melvin-bot bot commented May 4, 2023

MonilBhavsar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines commented May 5, 2023

MonilBhavsar left a comment

Choose a reason for hiding this comment

OSBotify commented May 5, 2023

OSBotify commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines commented Mar 10, 2023 •

edited

Loading

NikkiWines commented Apr 10, 2023 •

edited

Loading