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

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

Merged
merged 64 commits into from
Mar 7, 2023

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Feb 16, 2023

cc @cristipaval since you're helping with all of these related account settings / contact methods pages

Details

Nothing too fishy, but I left a few comments about places we have temporary logic that we'll remove soon - the next phase in this rollout is to create the "Add new contact method" page. At this point, we'll remove the old LoginField component & the old AddSecondaryLoginPage page and route

Fixed Issues

$ #15203
$ #11575

Tests

Notes:

  1. If you hit a rate limiting / throttling error when adding secondary logins, use this to delete the throttle:
    • DELETE FROM nameValuePairs WHERE accountID = <accountID> and name glob 'private_throttleAttempts_*';
  2. This web-e PR is needed for testing: https://github.com/Expensify/Web-Expensify/pull/36541

Default contact method

  1. Navigate to Settings -> Profile -> Contact method
  2. Select your default / primary contact method
  3. Verify you only see appropriate text on that page

Screenshot 2023-03-01 at 5 05 37 PM

Still able to add new contact methods

  1. Log in to an account that only has an email login OR only has a phone number login
  2. Navigate to Settings -> Profile -> Contact method
  3. Verify you can see your default contact method AND:
    1. If your default contact method is an email address, you can see a place to add a phone number login
    2. Vise versa
    • Screenshot 2023-03-01 at 5 06 25 PM
  4. Verify adding a new contact method still works, with your old password (we're removing the password flow soon)
  5. Verify you now see this unverified login in the "Contact methods" page, with a green dot next to it
    • Screenshot 2023-03-01 at 5 07 45 PM
  6. Verify you also see a green dot in these places:
    • Main Settings button On Settings page On Profile page
      Screenshot 2023-02-27 at 6 26 24 PM Screenshot 2023-02-27 at 6 27 03 PM Screenshot 2023-02-27 at 6 27 28 PM

Resend magic code to unvalidated contact method & validate it

  1. Navigate to Settings -> Profile -> Contact method
  2. View the unverified contact method you just added in the previous step
  3. Verify you see a page like this:
    • Screenshot 2023-03-01 at 5 08 16 PM
  4. Click "Resend magic code"
  5. Verify you can now see a green checkmark next to the "Resend magic code" link
    • Screenshot 2023-03-01 at 5 08 37 PM
  6. Verify you can successfully validate the login by entering 000000 magic code (@cristipaval will make the link work in a future PR)
    • Note: You should get automatically navigated back to the contact methods page
  7. Navigate back to the login you just validated, verify you can only see a "Remove" button on that page now
    • Note: In the near future we'll also add a "Set contact method as default" button, which will make the page look less dreary
    • (this is the view for a verified contact method that's not your default contact method)

Resend magic code, and observe errors

  1. Navigate to Settings -> Profile -> Contact method
  2. View an unverified contact method
  3. Click "Resend magic code" many times. Verify you eventually see this error:
    • Screenshot 2023-03-01 at 5 09 18 PM
  4. Navigate back to "Contact methods" and verify a red dot now shows up on that contact method row
    • You should also see a red dot in the main Settings indicator in the LHN, on the Settings page, and on the Profile page
  5. Navigate to that contact method details page, click the red "X", verify the error goes away

Validate secondary login, and observe errors

  1. Navigate to Settings -> Profile -> Contact method
  2. View an unverified contact method
  3. Enter an invalid magic code like 12345. Click "Verify".
  4. Verify you are redirected back to the Contact methods page and you quickly (once the request fails) see a red dot on the secondary login you tried to verify
  5. Open that contact method detail page, verify you see an error like this:
    • Screenshot 2023-03-01 at 5 09 49 PM
    • You should also see a red dot in the main Settings indicator in the LHN, on the Settings page, and on the Profile page
  6. Verify clicking the "X" clears the error

Remove contact method

  1. Navigate to Settings -> Profile -> Contact method
  2. View a non-default contact method (doesn't matter if it's verified yet or not)
  3. Click "Remove", verify a popup shows up
    • Screenshot 2023-02-23 at 5 10 19 PM
  4. Click "No", verify you go back to the contact method detail screen as before.
  5. Try again, but click "Yes"
  6. Verify you're taken back to the Contact methods page, and that contact method was removed.
  • Verify that no errors appear in the JS console

Offline tests

Send magic code

  1. Navigate to Settings -> Profile -> Contact method
  2. View an unverified contact method
  3. Click "Resend magic code"
  4. Verify the UI looks like this while offline:

Screenshot 2023-03-01 at 5 11 19 PM

Validate a secondary login

  1. Navigate to Settings -> Profile -> Contact method
  2. View an unverified contact method
  3. Enter any magic code, then click "Verify"
  4. You should get routed back to the contact methods page. Navigate in to that contact method again
  5. Verify the UI looks like this while offline:

Screenshot 2023-03-01 at 5 11 49 PM

Delete a secondary login

  1. Navigate to Settings -> Profile -> Contact method
  2. View an unverified contact method
  3. Click "Remove" while offline
  4. Verify you're back on the "Contact methods" page, and the login you're trying to delete looks like this (not necessarily with the red dot, if your contact method was validated previoiusly):

Screenshot 2023-03-01 at 5 12 05 PM

QA Steps

Same as above, except:

  1. When validating a contact method with a magic code, don't use 000000 - use the code that is sent to your email
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-27.at.7.27.38.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-03-01.at.1.59.44.PM.mov
Mobile Web - Safari
Screen.Recording.2023-03-01.at.2.17.43.PM.mov
Desktop
Screen.Recording.2023-02-27.at.7.43.14.PM.mov
iOS
Screen.Recording.2023-03-01.at.2.17.43.PM.mov
Android
Screen.Recording.2023-03-01.at.1.55.45.PM.mov

@Beamanator Beamanator self-assigned this Feb 16, 2023
@Beamanator Beamanator changed the title Show all Expensify logins in Contact Methods page & view each in new Contact Method Details page [HOLD #15039] Show all Expensify logins in Contact Methods page & view each in new Contact Method Details page Feb 16, 2023
@Beamanator Beamanator changed the title [HOLD #15039] Show all Expensify logins in Contact Methods page & view each in new Contact Method Details page Show all Expensify logins in Contact Methods page & view each in new Contact Method Details page Feb 21, 2023
@Beamanator
Copy link
Contributor Author

@mollfpr thanks for the comments, I addressed them! 👍

About that weird issue when adding a secondary login, I won't worry about that for now since I'm not modifying that page (AddSecondaryLoginPage) in this PR, but maybe you can try to reproduce on other pages and report a separate bug if it's reproducible!

@mollfpr
Copy link
Contributor

mollfpr commented Mar 6, 2023

@Beamanator Yeah, I feel this PR has nothing to do about the issue. I'll take a look more at issue and report it. Besides that, everything looks good to me 👍

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

Well done @Beamanator 🚀

@Beamanator
Copy link
Contributor Author

Groovy, thanks @mollfpr 👍

@cristipaval @shawnborton all y'all! 👍 😈

@shawnborton
Copy link
Contributor

Looks good to me!

@cristipaval cristipaval merged commit a7b6838 into main Mar 7, 2023
@cristipaval cristipaval deleted the beaman-showAllExpensifyLogins branch March 7, 2023 12:10
@OSBotify
Copy link
Contributor

OSBotify commented Mar 7, 2023

✋ 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 Mar 7, 2023

🚀 Deployed to staging by https://github.com/cristipaval in version: 1.2.80-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 9, 2023

🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.80-2 🚀

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

@Beamanator
Copy link
Contributor Author

Beamanator commented Mar 10, 2023

FYI this PR got reverted in this PR (#15761) due to this bug that it caused: #15725

Working on the fix here: #15751

Comment on lines +170 to +177
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mb1, styles.mt3]}>
<Icon src={Expensicons.DotIndicator} fill={colors.green} />
<View style={[styles.flex1, styles.ml4]}>
<Text>
{this.props.translate('contacts.enterMagicCode', {contactMethod})}
</Text>
</View>
</View>
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #18544:
For consistency, we should have reused DotIndicatorMessage component here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawnborton approved the designs of this PR, are you sure we wanted the other designs there? It looks like we didn't wait for Shawn to respond in Slack - https://expensify.slack.com/archives/C049HHMV9SM/p1683310488445319

And he wasn't ping'ed in that issue 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh perfecto, thanks @0xmiroslav !

[contactMethod]: {
pendingFields: {
validateLogin: null,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

✋ Coming from #21220

We need to clear the errorFields.validateCodeSent from the requestContactMethodValidateCode method, otherwise the error still visible after validating the contact method.

[contactMethod]: {
partnerUserID: '',
errorFields: {
deletedLogin: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #33021
We can clear all errors here, allowing the indicator to display the correct color. :)

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.

10 participants