Refactor close account page to use Form by Puneet-here · Pull Request #12534 · Expensify/App

Puneet-here · 2022-11-07T21:06:48Z

Details

We are refactoring close account page to use Form
#11341 (comment)

Fixed Issues

$ #11341
PROPOSAL: #11341 (comment)

Tests

Go to settings > Security > Close account
Try submitting the form without typing anything, verify you see error
Add correct contact method, verify the error goes away
Add wrong contact method and tap on screen to remove focus, verify you see error
Verify the button is red and is not disabled
Verify the button gets disabled when your are offline

Verify that no errors appear in the JS console

QA Steps

Go to settings > Security > Close account
Try submitting the form without typing anything, verify you see error
Add correct contact method, verify the error goes away
Add wrong contact method and tap on screen to remove focus, verify you see error
Verify the button is red and is not disabled
Verify the button gets disabled when your are offline

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

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

Screenshots

Web

Screen.Recording.2022-11-08.at.4.01.59.PM.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2022-11-08.at.4.10.41.PM.mov

iOS

Android

melvin-bot · 2022-11-08T10:29:58Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

src/pages/settings/Security/CloseAccountPage.js

Co-authored-by: Sahil <thesahindia@gmail.com>

thesahindia · 2022-11-08T16:54:48Z

When we get an error the error will be shown above the button which is expected in the Form refactor, the problem I see is that the error contains html tag (the error is from backend)

The other thing is that at mobile the modal covers the error above the button and it will be dismissed once user closes the modal
@Beamanator, what can we do here?

Puneet-here · 2022-11-08T17:25:08Z

src/languages/es.js

+            addressZipCode: 'Por favor ingresa un código postal válido',
+            debitCardNumber: 'Ingresa un número de tarjeta de débito válido',
            expirationDate: 'Por favor introduzca una fecha de vencimiento válida',
-            securityCode: 'Ingrese un código de seguridad válido',


This change is not related to this PR but added since it was asked here

…efactor-close-account-page

…/Puneet-here/App into form-refactor-close-account-page

Beamanator · 2022-11-10T18:49:09Z

@thesahindia

The other thing is that at mobile the modal covers the error above the button and it will be dismissed once user closes the modal
@Beamanator, what can we do here?

Ooh that's a very interesting point, it seems like we either need to do one of these:

Not show the error in the form (above the "Close account" button)
Not show the "Unable to close account" modal anymore

I call upon @shawnborton to assist with this question - @shawnborton on the "Close account" page we currently don't actually tell the user what error may have occurred when closing their account, we just show the "Unable to close account" modal (see this comment).

Is this a pattern we are ok with?

If yes, I assume we want to avoid showing the exact error in the "Close account" form (and just show the modal)
If not, maybe let's remove the modal & just show the error above the "Close account" button?

thesahindia · 2022-11-11T09:50:42Z

@shawnborton is OOO, should we tag the design team?

Beamanator · 2022-11-11T18:32:43Z

Aah shucks good point @thesahindia - currently @shawnborton is the whole @Expensify/design team 😅

@thesahindia do you mind posting in #expensify-open-source to get some feedback on this idea? I think we can pause the PR-merge-timer while we wait for feedback

shawnborton · 2022-11-15T00:17:25Z

Just confirming that I like where we ended up in Slack, where we can omit the modal and just use the error message above the form button.

Puneet-here · 2022-11-15T15:02:33Z

@Beamanator @thesahindia I have updated the code to remove the modal, we need to remove the <p> tag from the error message at backend.

Beamanator · 2022-11-15T20:00:52Z

@Puneet-here thanks for the reminder, made a follow-up issue for myself here: #12747

Beamanator · 2022-11-15T20:38:43Z

@thesahindia please feel free to test when you have time 👍

thesahindia · 2022-11-15T21:23:31Z

@Puneet-here, let's remove the modal part since we don't have a modal now

App/src/libs/actions/CloseAccount.js

Line 6 in 672dc9e

* Clear CloseAccount error message to hide modal

P.S. I have reviewed the code, it looks good. I will test it again in the morning. EOD for me 🌃

thesahindia · 2022-11-16T10:51:38Z

@Puneet-here, great work!
Looks good and works well 🎉

cc: @Beamanator

thesahindia · 2022-11-16T11:13:12Z

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

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Beamanator

A few small requested changes, happy to discuss :D

Also going to request @shawnborton check out the new Close Account page design, I don't know if I love the two error messages both showing in red, with red dots 🤔

src/ONYXKEYS.js

src/pages/settings/Security/CloseAccountPage.js

shawnborton · 2022-11-16T21:10:39Z

I agree that the red dot + red text is not my favorite, but I had posted some options in open source a while ago and this is what the group decided.

Puneet-here · 2022-11-16T21:49:00Z

Updated!
cc: @thesahindia @Beamanator

thesahindia · 2022-11-17T06:10:36Z

Thanks for the changes!

All yours @Beamanator

Beamanator

Sounds good, looks like we're good to go here!

OSBotify · 2022-11-18T17:20:34Z

✋ 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-11-22T16:37:03Z

🚀 Deployed to staging by @Beamanator in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-22T17:46:47Z

🚀 Deployed to staging by @Beamanator in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-24T00:05:09Z

🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀

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

marcaaron · 2022-12-30T07:01:38Z

src/components/Form.js

    /** Should the button be enabled when offline */
    enabledWhenOffline: PropTypes.bool,

+    /** Whether the action is dangerous */


I don't understand this - what does it mean for an action to be "dangerous"? 😕

Why was this added to such a low level component..

To me, "Dangerous" means if the user clicks the "Submit" button, the action being taken should only be taken with caution. I think it would be helpful if you can recommend a "better solution" or what you'd prefer seeing - I am guessing you are saying you'd like to see a more generic prop name, maybe something having to do with the submit button color?

To me, "Dangerous" means if the user clicks the "Submit" button, the action being taken should only be taken with caution.

I am guessing you are saying you'd like to see a more generic prop name

No, I think it's too generic. Which "action" is "dangerous"? It's a Form so the user can take many actions. It looks like this just controls the color of the button. So my suggestion would be something like isSubmitButtonDangerous, isSubmitActionDangerous etc.

Aah ok I'm glad we got that clarified, I see what you mean that technically there can be many "actions" taken in a form, though I thought it was a bit self explanatory that the main "action" is submitting the form - but it's good to be clear 👍 I like your suggestions, I think isSubmitActionDangerous is a bit more clear to me - there should only be 1 "submit action" (clicking on the submit button) so I'll go with that - follow-up PR incoming!

@marcaaron follow-up PR here: #13981

Puneet-here added 3 commits November 5, 2022 02:13

refactor to use form

cf6e656

use new key

0bbc47b

clean up

c8fa465

Puneet-here marked this pull request as ready for review November 8, 2022 11:10

Puneet-here requested a review from a team as a code owner November 8, 2022 11:10

melvin-bot bot requested review from Beamanator and thesahindia and removed request for a team November 8, 2022 11:11

thesahindia suggested changes Nov 8, 2022

View reviewed changes

src/pages/settings/Security/CloseAccountPage.js Show resolved Hide resolved

remove unnecessary code

0f40bff

Co-authored-by: Sahil <thesahindia@gmail.com>

Puneet-here commented Nov 8, 2022

View reviewed changes

Puneet-here added 7 commits November 8, 2022 14:05

fix typo

327241d

add isDangerousAction prop

b0684e1

Merge branch 'main' of https://github.com/Puneet-here/App into form-r…

83aae95

…efactor-close-account-page

resolve conflicts

85d5fc4

use ingresa instead of ingrese

a62334a

add spanish copy

99e0bd4

Merge branch 'form-refactor-close-account-page' of https://github.com…

9a5b602

…/Puneet-here/App into form-refactor-close-account-page

Beamanator requested a review from shawnborton November 10, 2022 18:49

Puneet-here added 3 commits November 15, 2022 20:22

remove close account error modal and unused code

be2f120

remove unused onyx prop

93c0564

clear error on refresh and closing page

b2b2eb2

Puneet-here mentioned this pull request Nov 15, 2022

[Hold for Payment 30-11-22][$500] Close account - Refactor to use Form component #11341

Closed

thesahindia approved these changes Nov 16, 2022

View reviewed changes

thesahindia previously approved these changes Nov 16, 2022

View reviewed changes

Beamanator requested changes Nov 16, 2022

View reviewed changes

src/ONYXKEYS.js Show resolved Hide resolved

src/pages/settings/Security/CloseAccountPage.js Show resolved Hide resolved

src/pages/settings/Security/CloseAccountPage.js Outdated Show resolved Hide resolved

add userEmailOrPhone inside validate function

003391b

Puneet-here dismissed thesahindia’s stale review via 003391b November 16, 2022 21:46

thesahindia approved these changes Nov 17, 2022

View reviewed changes

Beamanator approved these changes Nov 18, 2022

View reviewed changes

Beamanator merged commit 46c1c7b into Expensify:main Nov 18, 2022

OSBotify mentioned this pull request Nov 22, 2022

Deploy Checklist: New Expensify 2022-11-22 #12934

Closed

40 tasks

marcaaron reviewed Dec 30, 2022

View reviewed changes

Beamanator mentioned this pull request Jan 4, 2023

Update Form dangerous submit action prop #13981

Merged

53 tasks

Conversation

Puneet-here commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

melvin-bot bot commented Nov 8, 2022

Uh oh!

Uh oh!

thesahindia commented Nov 8, 2022

Uh oh!

Puneet-here Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Nov 10, 2022

Uh oh!

thesahindia commented Nov 11, 2022

Uh oh!

Beamanator commented Nov 11, 2022

Uh oh!

shawnborton commented Nov 15, 2022

Uh oh!

Puneet-here commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Beamanator commented Nov 15, 2022

Uh oh!

Beamanator commented Nov 15, 2022

Uh oh!

thesahindia commented Nov 15, 2022

Uh oh!

thesahindia commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesahindia commented Nov 16, 2022

Reviewer Checklist

Screenshots/Videos

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawnborton commented Nov 16, 2022

Uh oh!

Puneet-here commented Nov 16, 2022

Uh oh!

thesahindia commented Nov 17, 2022

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Nov 18, 2022

Uh oh!

OSBotify commented Nov 22, 2022

Uh oh!

OSBotify commented Nov 22, 2022

Uh oh!

OSBotify commented Nov 24, 2022

Uh oh!

marcaaron Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

marcaaron Jan 3, 2023

Choose a reason for hiding this comment

Puneet-here commented Nov 7, 2022 •

edited

Loading

Puneet-here commented Nov 15, 2022 •

edited

Loading

thesahindia commented Nov 16, 2022 •

edited

Loading