Add ResendResetPassword action to break up ResetPassword #10798

bondydaa · 2022-09-02T13:45:26Z

Details

We were re-using an api action in two different places when we wanted different behavior so this splits them so that we can do different things when Forgot? is clicked vs Resend Link is clicked.

Fixed Issues

$ #10785

Tests

Verify that no errors appear in the JS console
- This is reported here https://expensify.slack.com/archives/C01GTK53T8Q/p1663264353337809?thread_ts=1662053176.714619&cid=C01GTK53T8Q and I bumped it.

Non SAML account test

Enter an email for an existing, non-saml account and click continue
Click the Forgot? button when prompted for you password.
Ensure that the new page that appears doesn't have a loader spinner appear and that no Brick Road messages appear.
Click the Resend link button and ensure the success Brick Road message shows

SAML account test

Enter an email that is on a SAML required domain (if you don't have this you can test with expensifail.com but ensure you make QA aware you're doing this on staging/production) and click continue
Click the Forgot? button when prompted for you password ensure the next page doesn't show a loader or any Brick Road messages
Ensure you get the proper error Brick Road message like so

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Non SAML account test

Enter an email for an existing, non-saml account and click continue
Click the Forgot? button when prompted for you password.
Ensure that the new page that appears doesn't have a loader spinner appear and that no Brick Road messages appear.
Click the Resend link button and ensure the success Brick Road message shows

SAML account test

Enter an email that is on a SAML required domain (if you don't have this you can test with expensifail.com but ensure you make QA aware you're doing this on staging/production) and click continue
Click the Forgot? button when prompted for you password ensure the next page doesn't show a loader or any Brick Road messages
Ensure you get the proper error Brick Road message like so

Screenshots

Web

Mobile Web

Desktop

No message or loader shows when you first click Forgot?

Message appears properly when you click resend

Error message is properly shown for SAML users

iOS

Android

…estPasswordReset

luacmartins · 2022-09-15T16:31:49Z

src/libs/actions/Session/index.js

+                key: ONYXKEYS.ACCOUNT,
+                value: {
+                    isLoading: false,
+                    message: null,


NAB I'm wondering if we actually need to set message: null here. We already clear message in optimisticData so it should already be empty. This applies for failureData and for resendResetPassword too.

Yeah my only thought was if something else would have set this message key previously?

Though I just tested this and don't think it's possible, here's what I did

sign up with a new account, click resend to resend a 2nd validation link and get the success message which also uses account.message.

validate the account outside of going through new dot (I simulated this with cli tools to validate for me)

go back to "start" page and enter email again and then click forgot?. Here is where I was expecting the message previous to persist but it didn't so I think we're okay. There is probably a corner case somewhere lurking if we don't probably clear out account.message but we can fix that if it ever shows up easily.

Whenever we tap Forgot? we trigger resetPassword and we clear message and error in optimisticData, so I think that we should be ok.

luacmartins · 2022-09-15T16:33:58Z

src/libs/actions/Session/index.js

+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.ACCOUNT,
+                value: {
+                    isLoading: true,


NAB maybe this is just me, but it looks a bit odd to be redirected to the Forgot page and see the loading indicator when the message above says we sent a link already. Should we just remove the loading indicator from the first load? 🤔

Yep agreed.

…o set it in success/failure data

bondydaa · 2022-09-15T18:02:59Z

Updated and ready for review.

Though there is one thing I'm not sure about, we didn't want the Brick Road messaging showing up when the page first loaded but in the case for SAML required users it actually is and I think that's okay since it then gives a heads up that we didn't actually send the email.

luacmartins · 2022-09-15T20:08:28Z

I think the SAML case makes sense.

luacmartins · 2022-09-15T20:09:33Z

src/libs/actions/Session/index.js

+        successData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.ACCOUNT,
+                value: {
+                    isLoading: false,
+                },
+            },
+        ],
+        failureData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.ACCOUNT,
+                value: {
+                    isLoading: false,
+                },
+            },
+        ],


Hmm now that we don't set isLoading in optimisticData, there's no need to set it to false here. So we can just get rid of both successData and failureData

mananjadhav · 2022-09-16T04:33:04Z

The code changes look fine but the test didn't work for me. Also I don't have access to a SAML account.

@bondydaa @luacmartins Do I need any additional setup here? I am getting 404 errors in the network tab on clicking Resend Link

requestresendpassword.mp4

luacmartins · 2022-09-16T13:23:53Z

@mananjadhav the API changes that introduce this command are only in staging at the moment. You can either wait until these changes make it to prod (probably on Monday) or point your App to the staging API to test.

mananjadhav · 2022-09-16T13:26:08Z

the API changes that introduce this command are only in staging

@luacmartins I thought so, I copied .env.staging into a .env but it gives me CORS error. I used to use a Chrome web plugin for CORS but that doesn't seem to work here. Do I need to do something additional for staging APIs to work?

luacmartins · 2022-09-16T13:35:17Z

@mananjadhav web might be a problem, but I don't have issues on desktop or mobile. Maybe you can test those?

mananjadhav · 2022-09-16T13:41:29Z

Yeah App and Desktop I'll sort out. Then I'll test on these platforms and Monday if it is pushed I'll test the Web too.

mananjadhav · 2022-09-16T14:03:42Z

Changes look good and I tested them on Desktop, iOS and Android App. The only corner case that might not be related to the PR is, if I press Resend Link and immediately I go offline (before the request completes), the button gets disabled and is in loading state.

🎀 👀 🎀
C+ reviewed

I haven't tested on Web and mWeb.

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
- Any functional components have the displayName property
- 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

luacmartins · 2022-09-16T14:40:36Z

@mananjadhav thanks for the review! I think that edge case exists for all commands that set a loading state, so I wouldn't block this PR on that. That being said, we should probably address the general issue since it could happen quite often with spotty connections.

luacmartins · 2022-09-19T19:25:24Z

Web-E PR is in prod. Removing hold and merging!

luacmartins · 2022-09-19T19:28:07Z

I edited the checklists so that tests pass.

melvin-bot · 2022-09-19T19:28:16Z

@luacmartins looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

luacmartins · 2022-09-19T19:29:19Z

Tests passed. Removed Emergency label.

OSBotify · 2022-09-19T19:32:25Z

✋ 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 · 2022-09-20T15:12:34Z

🚀 Deployed to staging by @luacmartins in version: 1.2.3-0 🚀

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

OSBotify · 2022-09-21T19:17:21Z

🚀 Deployed to production by @luacmartins in version: 1.2.3-1 🚀

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

bondydaa added 2 commits September 2, 2022 15:42

add new action for resendresetpassword

a86ee91

use new action

0980486

bondydaa self-assigned this Sep 2, 2022

bondydaa mentioned this pull request Sep 2, 2022

[$250] Create ResendRequestPasswordReset #10785

Closed

luacmartins self-requested a review September 8, 2022 08:48

bondydaa added 3 commits September 8, 2022 12:27

create unique command for different usage of resend pw reset email

d868cf1

Merge branch 'main' of github.com:Expensify/App into bondy-split-Requ…

cda145d

…estPasswordReset

Merge branch 'main' of github.com:Expensify/App into bondy-split-Requ…

4d13c68

…estPasswordReset

luacmartins reviewed Sep 15, 2022

View reviewed changes

since optimistic data resets message and errors for us we dont need t…

dd2ad85

…o set it in success/failure data

bondydaa marked this pull request as ready for review September 15, 2022 18:02

bondydaa requested a review from a team as a code owner September 15, 2022 18:02

melvin-bot bot requested review from Justicea83 and removed request for a team September 15, 2022 18:03

puneetlath requested a review from mananjadhav September 15, 2022 18:52

luacmartins reviewed Sep 15, 2022

View reviewed changes

get rid of success and failure data since theyre no longer needed

a67dcdc

luacmartins previously approved these changes Sep 15, 2022

View reviewed changes

fix style

76e9167

bondydaa dismissed luacmartins’s stale review via 76e9167 September 15, 2022 20:52

luacmartins approved these changes Sep 15, 2022

View reviewed changes

luacmartins changed the title ~~Add ResendResetPassword action to break up ResetPassword~~ [HOLD Web-E #34769 ] Add ResendResetPassword action to break up ResetPassword Sep 16, 2022

luacmartins changed the title ~~[HOLD Web-E #34769 ] Add ResendResetPassword action to break up ResetPassword~~ Add ResendResetPassword action to break up ResetPassword Sep 19, 2022

luacmartins merged commit e093383 into main Sep 19, 2022

luacmartins deleted the bondy-split-RequestPasswordReset branch September 19, 2022 19:28

melvin-bot bot added the Emergency label Sep 19, 2022

luacmartins removed the Emergency label Sep 19, 2022

OSBotify mentioned this pull request Sep 20, 2022

Deploy Checklist: New Expensify 2022-09-20 #11139

Closed

10 tasks

kbecciv mentioned this pull request Sep 20, 2022

IOS - Signin - Page looks broken for SAML account after tap on forgot button #11148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ResendResetPassword action to break up ResetPassword #10798

Add ResendResetPassword action to break up ResetPassword #10798

bondydaa commented Sep 2, 2022 •

edited by luacmartins

Loading

luacmartins Sep 15, 2022

bondydaa Sep 15, 2022

luacmartins Sep 15, 2022

luacmartins Sep 15, 2022

bondydaa Sep 15, 2022

bondydaa commented Sep 15, 2022

luacmartins commented Sep 15, 2022

luacmartins Sep 15, 2022

mananjadhav commented Sep 16, 2022 •

edited

Loading

luacmartins commented Sep 16, 2022

mananjadhav commented Sep 16, 2022

luacmartins commented Sep 16, 2022

mananjadhav commented Sep 16, 2022

mananjadhav commented Sep 16, 2022 •

edited by luacmartins

Loading

PR Reviewer Checklist

luacmartins commented Sep 16, 2022

luacmartins commented Sep 19, 2022

luacmartins commented Sep 19, 2022

melvin-bot bot commented Sep 19, 2022

luacmartins commented Sep 19, 2022

OSBotify commented Sep 19, 2022

OSBotify commented Sep 20, 2022

OSBotify commented Sep 21, 2022

Add ResendResetPassword action to break up ResetPassword #10798

Add ResendResetPassword action to break up ResetPassword #10798

Conversation

bondydaa commented Sep 2, 2022 • edited by luacmartins Loading

Details

Fixed Issues

Tests

Non SAML account test

SAML account test

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Non SAML account test

SAML account test

Screenshots

Web

Mobile Web

Desktop

iOS

Android

luacmartins Sep 15, 2022

Choose a reason for hiding this comment

bondydaa Sep 15, 2022

Choose a reason for hiding this comment

luacmartins Sep 15, 2022

Choose a reason for hiding this comment

luacmartins Sep 15, 2022

Choose a reason for hiding this comment

bondydaa Sep 15, 2022

Choose a reason for hiding this comment

bondydaa commented Sep 15, 2022

luacmartins commented Sep 15, 2022

luacmartins Sep 15, 2022

Choose a reason for hiding this comment

mananjadhav commented Sep 16, 2022 • edited Loading

luacmartins commented Sep 16, 2022

mananjadhav commented Sep 16, 2022

luacmartins commented Sep 16, 2022

mananjadhav commented Sep 16, 2022

mananjadhav commented Sep 16, 2022 • edited by luacmartins Loading

PR Reviewer Checklist

luacmartins commented Sep 16, 2022

luacmartins commented Sep 19, 2022

luacmartins commented Sep 19, 2022

melvin-bot bot commented Sep 19, 2022

luacmartins commented Sep 19, 2022

OSBotify commented Sep 19, 2022

OSBotify commented Sep 20, 2022

OSBotify commented Sep 21, 2022

bondydaa commented Sep 2, 2022 •

edited by luacmartins

Loading

mananjadhav commented Sep 16, 2022 •

edited

Loading

mananjadhav commented Sep 16, 2022 •

edited by luacmartins

Loading