Display message to user when account is closed by MonilBhavsar · Pull Request #11665 · Expensify/App

MonilBhavsar · 2022-10-07T11:24:09Z

Details

Display success message when account is closed
Supports Offline functionality

Fixed Issues

$ #10224

Tests

Tap on profile
Go to Security > Close Account
Enter email
Click Close Account
Verify "Account closed successfully" message appears on Login page as in screenshot
Enter a new email in the login page
Click "Go Back" from the password page. Verify that "Account closed successfully" message is not there

Offline mode

Tap on profile
Go to Security > Close Account
Enter email
Close Account should be disabled

Verify that no errors appear in the JS console

PR Review Checklist

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

QA Steps

Tap on profile
Go to Security > Close Account
Enter email
Click Close Account
Verify "Account closed successfully" message appears on Login page as in screenshot
Enter a new email in the login page
Click "Go Back" from the password page. Verify that "Account closed successfully" message is not there

Offline mode

Tap on profile
Go to Security > Close Account
Enter email
Close Account should be disabled

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-10-12.at.10.01.36.PM.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

cc @Expensify/design

Beamanator

So far looking great! A few comments, and also it looks like we don't clear the success message in this flow but maybe should:

Log in with account, close it
Verify you're back on the login page, and you see the close success message
Enter another account's email address, click "Continue"
Don't sign in, then click "Go back"
You now still see the "close account success message", but I don't think you should

Beamanator · 2022-10-11T10:55:25Z

src/pages/settings/Security/CloseAccountPage.js

+                    {!this.props.isSmallScreenWidth
+                        && <OfflineIndicator containerStyles={[styles.mt2]} />}


While this is definitely useful, I wonder if we can generalize it for all drawer pages / modal pages since it looks like this doesn't exist in many of those pages (like the profile page)

What exactly do you mean by that Alex? Just want to make sure I understand as a non-engineer 😄

Yeah good question, so my train of thought is:

If we're adding the Offline Indicator to the "Close Account" page, why wasn't it there before?

Then I checked if the Offline Indicator is present on other form pages (example: the Profile Page) and I don't see the offline indicator there

It seems weird to me to show the user they're offline in the Close Account page w/out also showing they're offline in the Profile page, so I'm basically asking here if we want to show the Offline Indicator on all pages, or only on pages with forms, or something else

One other idea I had is: In web / desktop full screen mode, it may not be necessary to show the Offline Indicator twice (you can see that it's already showing in the chat report page) so maybe we only show the Offline Indicator on sub-pages on small screens (including mobile devices)?

It seems weird to me to show the user they're offline in the Close Account page w/out also showing they're offline in the Profile page, so I'm basically asking here if we want to show the Offline Indicator on all pages, or only on pages with forms, or something else

Maybe I misunderstood. But aren't these different offline patterns? We let users modify their profiles offline, but we shouldn't let users delete accounts while offline.

Hmm true true, but doesn't it feel a bit weird seeing an offline status in some pages and not in all?

IMO, the message You appear to be offline is good to know everywhere or nowhere, not "at the bottom of some page but not all".

I don't think we should care about a consistent offline behavior on a feature that lets users stop being our customer

I'm a bit confused how this would let users stop being our customer, do you mean if users see You appear to be offline they'll assume the form they're seeing won't be submittable, so they'll think our app doesn't work offline even though some pages will and some won't?

OH. Whoops.

I was mistaken and thought this was regarding disabling the Close Account button while offline... not the indicator. Ignore me

Agree, it is the case of mobile but not for web.
May be worth a discussion in channel, why it was only implemented for mobile and not web and then we can implement it.

Aahhhh ok that makes more sense @Julesssss 😅

May be worth a discussion in channel, why it was only implemented for mobile and not web and then we can implement it.

Ooh that's a good idea 👍

Shall we leave that discussion for now? I think we could merge this today and revisit.

why it was only implemented for mobile and not web and then we can implement it.

As for this, I think it works now because the page is shown as a RightDockedModal, right?

Beamanator · 2022-10-11T10:56:01Z

src/pages/signin/LoginForm.js

    }),

+    closeAccount: PropTypes.shape({
+        /** Success message to display when necessary */


Maybe this is a bit clearer

Suggested change

/** Success message to display when necessary */

/** Message to display when user successfully closed their account */

src/pages/signin/LoginForm.js

Julesssss

The code looks good, and the first test passed. But confusingly I'm not getting the offline state on web 😕

shawnborton · 2022-10-11T15:31:19Z

Style-wise the error/succes dot-indicator messages look good to me.

MonilBhavsar · 2022-10-13T09:35:43Z

This is ready for a re review. Updated testing and QA steps and added screen recording of the edge case @Beamanator pointed out. Thanks for pointing it out and helping with it!

MonilBhavsar · 2022-10-13T09:37:18Z

The code looks good, and the first test passed. But confusingly I'm not getting the offline state on web 😕

@Julesssss hope this was not consistent and something flakey with network?

Julesssss · 2022-10-13T11:30:33Z

Hey @MonilBhavsar, actually yeah. I'm pretty sure now that this is an unrelated issue on my side. But for that reason, I'm unable to verify the tests 😞

Julesssss

It looks like we'll take the discussion to Slack. But once that is resolved I'll be happy to retest on iOS/Android/mWeb. Unfortunately someone else will need to verify web, due to my current issue.

techievivek · 2022-10-13T16:06:31Z

I unassigned myself because I won't be able to review it by tomorrow. If this continues for next week, I will jump in again 😄

MonilBhavsar · 2022-10-14T06:37:22Z

Off hold

Beamanator · 2022-10-14T11:26:39Z

Honestly I'm having lotsss of trouble with my VM today so I think it's up to you @Julesssss 😅

Julesssss · 2022-10-14T14:06:00Z

Okay, offline functionality is finally working for me. Tested on all platforms:

Desktop

Web

Android

iOS

Julesssss

Tested successfully on all platforms ✅

Julesssss · 2022-10-14T14:16:29Z

PR 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 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 */
- 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.

MonilBhavsar · 2022-10-17T05:01:34Z

Thank you @Julesssss for testing and reviewing!
@Beamanator all yours

Beamanator · 2022-10-18T12:31:43Z

Sorry all, was battling VM issues the last few days, but I think I'm back and ready to review!

Beamanator

melvin-bot · 2022-10-18T12:47:10Z

@Beamanator 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.

OSBotify · 2022-10-18T12:51:28Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Beamanator · 2022-10-18T13:28:35Z

I'm 99.9% sure I saw all tests were passing before merging 🤔

OSBotify · 2022-10-19T12:22:45Z

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

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

OSBotify · 2022-10-19T13:45:47Z

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

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

OSBotify · 2022-10-24T23:33:52Z

🚀 Deployed to production by @chiragsalian in version: 1.2.18-10 🚀

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

Gonals · 2022-12-15T09:43:59Z

Heads up @MonilBhavsar @Beamanator @Julesssss ! This PR caused a bug here: #13506

Not a big deal, though. I think is more a change in approach than a bug.

Beamanator · 2022-12-15T11:53:41Z

Yeahhhh I agree it's more of a change in approach - I think I argued that it was better to keep the closed account message for longer, but no big arguments / opinions either way :D 👍

MonilBhavsar added 2 commits October 7, 2022 16:43

Display message to user if action succeeded

ffc627e

Use closeAccount Onyx key

244863a

MonilBhavsar self-assigned this Oct 7, 2022

MonilBhavsar force-pushed the monil-closeAccountSuccessMessage branch from 1d98d15 to 61297c5 Compare October 10, 2022 15:32

Add offline support and offline indicator

2a1b0f7

MonilBhavsar force-pushed the monil-closeAccountSuccessMessage branch from 61297c5 to 2a1b0f7 Compare October 10, 2022 17:01

MonilBhavsar changed the title ~~Display message to user when account is closed~~ [HOLD Web-E 35085] Display message to user when account is closed Oct 10, 2022

MonilBhavsar marked this pull request as ready for review October 10, 2022 17:46

MonilBhavsar requested a review from a team as a code owner October 10, 2022 17:46

melvin-bot bot requested review from techievivek and removed request for a team October 10, 2022 17:47

MonilBhavsar requested review from a team, Beamanator and Julesssss and removed request for techievivek October 10, 2022 17:47

melvin-bot bot requested review from techievivek and removed request for a team October 10, 2022 17:47

Beamanator requested changes Oct 11, 2022

View reviewed changes

Julesssss reviewed Oct 11, 2022

View reviewed changes

shawnborton self-requested a review October 11, 2022 15:31

Julesssss previously approved these changes Oct 12, 2022

View reviewed changes

Address reviews and clear success message

6d8af41

MonilBhavsar dismissed Julesssss’s stale review via 6d8af41 October 12, 2022 16:40

MonilBhavsar requested review from Beamanator and Julesssss October 13, 2022 09:37

Julesssss reviewed Oct 13, 2022

View reviewed changes

techievivek removed their request for review October 13, 2022 16:05

MonilBhavsar changed the title ~~[HOLD Web-E 35085] Display message to user when account is closed~~ Display message to user when account is closed Oct 14, 2022

Julesssss approved these changes Oct 14, 2022

View reviewed changes

Beamanator approved these changes Oct 18, 2022

View reviewed changes

Beamanator merged commit e3fd14b into main Oct 18, 2022

Beamanator deleted the monil-closeAccountSuccessMessage branch October 18, 2022 12:47

melvin-bot bot added the Emergency label Oct 18, 2022

Beamanator removed the Emergency label Oct 18, 2022

OSBotify mentioned this pull request Oct 19, 2022

Deploy Checklist: New Expensify 2022-10-19 #11979

Closed

36 tasks

Beamanator mentioned this pull request Dec 6, 2022

Account - “Close account” button isn’t disabled if nothing is written in the second input #13065

Closed

Gonals mentioned this pull request Dec 15, 2022

Log in - "Account closed successfully" message does not disappear until put your log in and click continue. #13506

Closed

		{!this.props.isSmallScreenWidth
		&& <OfflineIndicator containerStyles={[styles.mt2]} />}

	/** Success message to display when necessary */
	/** Message to display when user successfully closed their account */

Conversation

MonilBhavsar commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

shawnborton commented Oct 11, 2022

Uh oh!

MonilBhavsar commented Oct 13, 2022

Uh oh!

MonilBhavsar commented Oct 13, 2022

Uh oh!

Julesssss commented Oct 13, 2022

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

techievivek commented Oct 13, 2022

Uh oh!

MonilBhavsar commented Oct 14, 2022

Uh oh!

Beamanator commented Oct 14, 2022

Uh oh!

Julesssss commented Oct 14, 2022

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

Julesssss commented Oct 14, 2022

Uh oh!

MonilBhavsar commented Oct 17, 2022

Uh oh!

Beamanator commented Oct 18, 2022

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

melvin-bot bot commented Oct 18, 2022

Uh oh!

MonilBhavsar commented Oct 7, 2022 •

edited

Loading