New Contact methods page linked from Profile page #15039

Beamanator · 2023-02-10T15:55:06Z

cc @cristipaval since you're helping with the Account Settings implementation a lot

Details

We're moving LoginField to the Contact methods page so we can do bite-sized changes instead of making & implementing ALL of the new contact method pages at the same time

Note: The "Add secondary login" flow currently only works with passwords, but we're going to update that very soon in a future PR 👍

Fixed Issues

$ #15038

Tests

Log in with an account that ONLY has an email login
Navigate to Settings -> Profile
Verify "Contact method" shows your login nicely
Navigate to Contact method page
Verify title looks good
Verify "Email address" field shows your email login and isn't clickable
Verify "Phone number" field is clickable
Click "Phone number" field and verify you're taken to the "Add phone number" page
Enter a phone number you have access to
Verify it was added successfully (unverified of course)

Log in with an account that ONLY has a phone number login
Navigate to Settings -> Profile
Verify "Contact method" shows your login nicely
Navigate to Contact method page
Verify the Phone number field is showing your phone number login nicely
Verify the "Email address" field is empty. Click it, verify you're navigated to the Add email address page
Add an email address secondary login
Verify it was added successfully (unverified of course)
Now verify the back & close buttons work and look good when navigating between the profile, contact methods, and "add email / phone number" pages

Verify that no errors appear in the JS console

Offline tests

QA Steps

Same as above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-02-14.at.2.32.48.PM.mov

Mobile Web - Chrome

Screen.Recording.2023-02-14.at.2.49.05.PM.mov

Mobile Web - Safari

Screen.Recording.2023-02-14.at.2.55.43.PM.mov

Desktop

Screen.Recording.2023-02-14.at.2.36.48.PM.mov

iOS

Screen.Recording.2023-02-14.at.2.54.10.PM.mov

Android

Screen.Recording.2023-02-14.at.2.44.18.PM.mov

MelvinBot · 2023-02-10T15:55:51Z

@Santhosh-Sellavel @aldo-expensify 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]

Beamanator · 2023-02-10T16:12:05Z

I was optimistic this would be ready to review by end of my day, but looks like there's some errors I'll have to debug :D So don't worry about reviewing today @aldo-expensify @Santhosh-Sellavel 🙏

Beamanator · 2023-02-10T16:54:30Z

Actually I figured out the error so this can be tested :D I just haven't taken any screenshots yet, so I'll get to that Monday 👍

src/languages/es.js

…tMethodsPage

Beamanator · 2023-02-14T12:57:53Z

PR Author checklist filled 👍 Ready for review! (Please slightly prioritize if y'all can, since this is the first part of multiple upcoming "Contact method" updates coming very soon 🙏 )

aldo-expensify

NAB: Considering that email in "Contact Methods" is not clickeable, I don't think we are using the right cursor when hovering it:

Screen.Recording.2023-02-14.at.1.45.06.PM.mov

NAB: The next is probably unrelated to your PR, but 10. Verify it was added successfully (unverified of course) doesn't pass:

Adding my phone throws the error: An error occurred using the phone number provided, please use your email address instead.
if I try again, it get a different error: The new phone number you are trying to add is already a secondary login of your account.
If I reload the page, the phone has been added, so it did work the first time 🤷

Screen.Recording.2023-02-14.at.1.59.05.PM.mov

NAB: If I try to create an account with a phone number, I get the same error:

NAB: When you add a new contact method, if you click "Resend", you can click again the button when it is a check mark and it will still perform the action:

UPDATE: leaving all this as NAB because I think it is not related to your PR moving stuff to a new page. I think we should create new GH issues to improve these, but leaving them here meanwhile.

aldo-expensify

Commented some stuff in the backend that was giving me error for my phone number and I was able to test.

The original functionality seems to be working fine. I left some comments mainly related to refactoring the component ContactMethodsPage which uses a state that seems unnecessary. I know you didn't write the code in the PR from scratch, but since you are moving it, it may be a good time to improve it?

Offline behaviour: doesn't seem to be implemented? clicking "Send validation" when adding a new email doesn't do anything, no feedback. Is this in the scope of this PR?

Another NAB: the form for adding a new phone/emails seems to be inconsistently clearing something and sometimes not when I navigate back. Captured in the video below when it cleaned, but I saw it not cleaning a few times too. It seems to depend on whether the component AddSecondaryLoginPage gets unmounted or not which I guess is up to the react navigation stuff 🤷

Screen.Recording.2023-02-14.at.4.47.25.PM.mov

src/pages/settings/Profile/Contacts/ContactMethodsPage.js

aldo-expensify · 2023-02-15T00:20:31Z

src/pages/settings/Profile/Contacts/ContactMethodsPage.js

+            return;
+        }
+
+        // eslint-disable-next-line react/no-did-update-set-state


I wonder if we should be act on this warning instead of just disable it. I see that we have disable it in other places.

According to the rule documentation, we should do these setStates wrapped in a function:

https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-did-update-set-state.md

this.onUpdate(function callback(newName) {

🤢 I feel ya... If we end up getting rid of logins stored in state, we don't have to think about this so I'm going to do nothing for now as we discuss further :D

aldo-expensify · 2023-02-15T00:22:20Z

src/pages/settings/Profile/Contacts/ContactMethodsPage.js

+        this.state = {
+            logins: this.getLogins(),
+        };


Do you know why we copy this to a state instead of just directly calling this.getLogins() on render and use what we get there? synchronizing the prop with the state adds more complexity and in this case doesn't seem to serve any purpose.

Def agreed - i have no idea why we did this in the first place, and if we want to take the time to refactor I def agree it makes sense to just directly use what we get from props and not deal with syncing it with state in componentDidUpdate 👍

aldo-expensify · 2023-02-15T00:28:21Z

src/pages/settings/Profile/Contacts/ContactMethodsPage.js

+            logins: this.getLogins(),
+        };
+
+        this.getLogins = this.getLogins.bind(this);


Unnecessary bindings

I see a this.props.loginList in getLogins - we don't need to bind for that?

We don't need to bind here, we need to bind only if we pass a method as the prop to other components.

@Beamanator bump!

Since this is just a copy paste and the component will get redone eventually (taken from here), maybe we don't care about this type of polishing. What do you think @Santhosh-Sellavel ?

All right then!

aldo-expensify · 2023-02-15T00:52:11Z

If you just remove the synchronized state, all my comments related to that can be ignored :P

Beamanator · 2023-02-15T12:30:31Z

Commented some stuff in the backend that was giving me error for my phone number and I was able to test.

Oh shoot I don't think I mentioned i had to change if (in_array($carrierType, ['voip', 'landline'])) { to if (in_array($carrierType, ['landline'])) { in User_Utils::throwIfSuspectedSMSFraud since my phone number if voip :D

The original functionality seems to be working fine. I left some comments mainly related to refactoring the component ContactMethodsPage which uses a state that seems unnecessary. I know you didn't write the code in the PR from scratch, but since you are moving it, it may be a good time to improve it?

I love your ideas for refactoring... Maybe it's worth it to do this... BUT I 1000% plan to cut a lot of the existing weird getLogins stuff as well as the LoginField components once we get this merged and start showing EVERY login the user has for Expensify. There's more details about how that willlll work in the design doc, this was supposed to be just a very small step in that direction so that we're not doing large changes that take lots of testing.

I'd love your professional opinion if it's worth it to do the refactoring at this point. In my opinion no it's not, since in a few very soon follow-up PRs we're going to gut all the old code (including the AddSecondaryLoginPage) since we're building all new pages in this area, related to contact methods. While we do that we're also going to completely focus on offline first & error messages (RBR) & such. So.... I'm preferring we save a lot of the "cleanup" till later so we don't do the refactoring & then gut what we spent time refactoring. Thoughts?

Offline behaviour: doesn't seem to be implemented? clicking "Send validation" when adding a new email doesn't do anything, no feedback. Is this in the scope of this PR?

Mentioned in my previous comment - we're going to be creating a new API command for "Send validation" soon, but not in this PR - so I think we should save that for when we implement it in the upcoming brand new "contact details" page

Another NAB: the form for adding a new phone/emails seems to be inconsistently clearing something and sometimes not when I navigate back. Captured in the video below when it cleaned, but I saw it not cleaning a few times too. It seems to depend on whether the component AddSecondaryLoginPage gets unmounted or not which I guess is up to the react navigation stuff 🤷

This is quite interesting and will be good for us to keep in mind as we move forward with the new "Add contact method" page - but for now on the same line of thinking as my previous comments, I thinkkkkk we should just :donothing: on that page since it's going to be gutted very soon.

Again, I'm quite interested in hearing your opinions on my plan, let me know what you think 👍

…tMethodsPage

aldo-expensify · 2023-02-15T17:54:55Z

In my opinion no it's not, since in a few very soon follow-up PRs we're going to gut all the old code (including the AddSecondaryLoginPage) since we're building all new pages in this area, related to contact methods. While we do that we're also going to completely focus on offline first & error messages (RBR) & such. So.... I'm preferring we save a lot of the "cleanup" till later so we don't do the refactoring & then gut what we spent time refactoring. Thoughts?

Offline behaviour: doesn't seem to be implemented? clicking "Send validation" when adding a new email doesn't do anything, no feedback. Is this in the scope of this PR?

Mentioned in my previous comment - we're going to be creating a new API command for "Send validation" soon, but not in this PR - so I think we should save that for when we implement it in the upcoming brand new "contact details" page

Ahh, that context is good to know :)

The proposed refactor is pretty trivial in my opinion, but considering that we will throw away and redo a lot of the code anyway, I agree with that it is very fine to leave things as there are and not waste time on it!

Beamanator · 2023-02-15T18:37:33Z

@Santhosh-Sellavel can you please review today or tomorrow? And if not please let us know :)

Santhosh-Sellavel · 2023-02-15T21:55:40Z

On it now!

Santhosh-Sellavel · 2023-02-15T22:15:48Z

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 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 correct English and 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web & Desktop

Screen.Recording.2023-02-16.at.4.13.31.AM.mov

Mobile Web - Chrome

mweb_android.mp4

Mobile Web - Safari & iOS

Uploading Simulator Screen Recording - iPhone 14 - 2023-02-16 at 04.34.47.mp4…

Android

Screen_Recording_20230216_044744_New.Expensify.mp4

Santhosh-Sellavel · 2023-02-16T00:32:56Z

I am unable to test any success case for mobile. I don't have many numbers to test, I tried online free numbers does not work. I tried to log in with one of my mobile accounts which I created earlier but was unsuccessful because I forgot the password and I'm not receiving a magic link to reset my password.

Beamanator · 2023-02-16T07:54:05Z

@Santhosh-Sellavel one tip for testing phone numbers - if you already linked a phone number to another account, you can remove it via OldDot (our old website) - in the near future we'll allow users to also remove secondary logins via NewDot, but we gotta get this out first 😅

@aldo-expensify Since Santhosh is having some trouble testing the "Phone number only" flow, would you mind helping with that? 🙏

aldo-expensify · 2023-02-16T18:18:05Z

@Beamanator Got conflicts :(

aldo-expensify · 2023-02-16T18:18:34Z

@aldo-expensify Since Santhosh is having some trouble testing the "Phone number only" flow, would you mind helping with that? 🙏

ok!

Santhosh-Sellavel

Just waiting for the conflicts to be reviewed!

…tMethodsPage

Beamanator · 2023-02-20T10:51:58Z

Huh I pulled main and there were no merge conflicts?? Weird?? I'll check that AddSecondaryLoginPage merged correctly

Beamanator · 2023-02-20T10:53:34Z

Ya seems like it auto-merged fine 🤷 Requesting another, hopefully final review @aldo-expensify ! 🙏

Julesssss

LGTM

Beamanator · 2023-02-21T10:45:19Z

Merging since I got enough approvals and I'm trying to move quickly 👍 👍

OSBotify · 2023-02-21T21:15:16Z

🚀 Deployed to staging by https://github.com/Beamanator in version: 1.2.75-0 🚀

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

OSBotify · 2023-02-22T22:57:45Z

🚀 Deployed to production by https://github.com/melvin-bot[bot] in version: 1.2.75-0 🚀

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

Beamanator · 2023-03-10T11:25:58Z

Don't mind meeeeeee!!!! I got my PRs mixed up... No regression here!!

Beamanator added 5 commits February 10, 2023 17:49

Set up new contact methods route

3211801

Add contact method copy

9bb6e0f

Move LoginField to contacts dir

f44a00e

New ContactMethods page

b33ae5a

Link to new contact methods page & simplify

e3d13cd

Beamanator requested a review from a team as a code owner February 10, 2023 15:55

Beamanator self-assigned this Feb 10, 2023

melvin-bot bot requested review from aldo-expensify and Santhosh-Sellavel and removed request for a team February 10, 2023 15:55

Beamanator added 4 commits February 10, 2023 18:04

Reorder for correct animation direction

7be6099

add plural contact methods

667377f

Move page to contacts dir

4515a31

Fix title and previous page

836e42b

Beamanator marked this pull request as draft February 10, 2023 16:11

Beamanator added 2 commits February 10, 2023 18:44

Navigate to contact methods page after adding login

69cb7f6

Remove sms domain & some cleanup

944bf6e

Fix lint errors

aefdc88

aldo-expensify reviewed Feb 10, 2023

View reviewed changes

src/languages/es.js Outdated Show resolved Hide resolved

Beamanator added 2 commits February 13, 2023 18:46

Add spanish translations for contact methods

0fcd2e1

Merge branch 'main' of github.com:Expensify/App into beaman-newContac…

5b4ca01

…tMethodsPage

Beamanator requested a review from aldo-expensify February 13, 2023 16:48

Merge branch 'main' of github.com:Expensify/App into beaman-newContac…

b3db92d

…tMethodsPage

Beamanator marked this pull request as ready for review February 14, 2023 12:56

aldo-expensify reviewed Feb 14, 2023

View reviewed changes

aldo-expensify requested changes Feb 15, 2023

View reviewed changes

Beamanator mentioned this pull request Feb 15, 2023

[$1000] [HOLD for payment 2023-03-01] Create "Contact method" menu in Profile page & "Contact methods" page #15038

Closed

Beamanator added 2 commits February 15, 2023 14:30

Little cleanup

5bb4975

Merge branch 'main' of github.com:Expensify/App into beaman-newContac…

a345257

…tMethodsPage

aldo-expensify approved these changes Feb 15, 2023

View reviewed changes

Beamanator mentioned this pull request Feb 16, 2023

Show all Expensify logins in Contact Methods page & view each in new Contact Method Details page #15204

Merged

54 tasks

Santhosh-Sellavel reviewed Feb 16, 2023

View reviewed changes

Merge branch 'main' of github.com:Expensify/App into beaman-newContac…

0427126

…tMethodsPage

Julesssss approved these changes Feb 21, 2023

View reviewed changes

Beamanator merged commit 6f27058 into main Feb 21, 2023

Beamanator deleted the beaman-newContactMethodsPage branch February 21, 2023 10:44

OSBotify mentioned this pull request Feb 21, 2023

Deploy Checklist: New Expensify 2023-02-21 #15329

Closed

19 tasks

kbecciv mentioned this pull request Feb 22, 2023

Setting- Send validation button is non functional on mobile #15345

Closed

6 tasks

This comment was marked as off-topic.

Sign in to view

Beamanator mentioned this pull request Mar 10, 2023

Fix stuck navigation & Re-add Contact Method Details pages #15751

Merged

55 tasks

New Contact methods page linked from Profile page #15039

New Contact methods page linked from Profile page #15039

Conversation

Beamanator commented Feb 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 10, 2023

Beamanator commented Feb 10, 2023

Beamanator commented Feb 10, 2023

Beamanator commented Feb 14, 2023

aldo-expensify left a comment • edited Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Feb 15, 2023

Beamanator commented Feb 15, 2023

aldo-expensify commented Feb 15, 2023

Beamanator commented Feb 15, 2023

Santhosh-Sellavel commented Feb 15, 2023

Santhosh-Sellavel commented Feb 15, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel commented Feb 16, 2023

Beamanator commented Feb 16, 2023

aldo-expensify commented Feb 16, 2023

aldo-expensify commented Feb 16, 2023

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Beamanator commented Feb 20, 2023

Beamanator commented Feb 20, 2023

Julesssss left a comment

Choose a reason for hiding this comment

Beamanator commented Feb 21, 2023

OSBotify commented Feb 21, 2023

OSBotify commented Feb 22, 2023

This comment was marked as off-topic.

Beamanator commented Mar 10, 2023 • edited Loading

Beamanator commented Feb 10, 2023 •

edited

Loading

aldo-expensify left a comment •

edited

Loading

Santhosh-Sellavel commented Feb 15, 2023 •

edited

Loading

Beamanator commented Mar 10, 2023 •

edited

Loading