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.

Sign up for GitHub

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

Display message to user when account is closed #11665

Merged
merged 4 commits into from
Oct 18, 2022
Merged

Display message to user when account is closed #11665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffc627e
Display message to user if action succeeded
MonilBhavsar Oct 7, 2022
244863a
Use closeAccount Onyx key
MonilBhavsar Oct 7, 2022
2a1b0f7
Add offline support and offline indicator
MonilBhavsar Oct 10, 2022
6d8af41
Address reviews and clear success message
MonilBhavsar Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ const CONST = {
},
DEFAULT_TIME_ZONE: {automatic: true, selected: 'America/Los_Angeles'},
DEFAULT_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
DEFAULT_CLOSE_ACCOUNT_DATA: {error: '', success: '', isLoading: false},
APP_STATE: {
ACTIVE: 'active',
BACKGROUND: 'background',
Expand Down
9 changes: 9 additions & 0 deletions src/libs/actions/CloseAccount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';

/**
* Clear CloseAccount error message to hide modal
Expand All @@ -8,7 +9,15 @@ function clearError() {
Onyx.merge(ONYXKEYS.CLOSE_ACCOUNT, {error: ''});
}

/**
* Set default Onyx data
*/
function setDefaultData() {
Onyx.merge(ONYXKEYS.CLOSE_ACCOUNT, {...CONST.DEFAULT_CLOSE_ACCOUNT_DATA});
}

export {
// eslint-disable-next-line import/prefer-default-export
clearError,
setDefaultData,
};
11 changes: 10 additions & 1 deletion src/pages/settings/Security/CloseAccountPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import withLocalize, {withLocalizePropTypes} from '../../../components/withLocal
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import * as CloseAccount from '../../../libs/actions/CloseAccount';
import ONYXKEYS from '../../../ONYXKEYS';
import OfflineIndicator from '../../../components/OfflineIndicator';
import {withNetwork} from '../../../components/OnyxProvider';
import networkPropTypes from '../../../components/networkPropTypes';

const propTypes = {
/** Onyx Props */
Expand All @@ -37,6 +40,9 @@ const propTypes = {
email: PropTypes.string.isRequired,
}).isRequired,

/** Information about the network */
network: networkPropTypes.isRequired,

...windowDimensionsPropTypes,
...withLocalizePropTypes,
};
Expand Down Expand Up @@ -110,9 +116,11 @@ class CloseAccountPage extends Component {
text={this.props.translate('closeAccountPage.closeAccount')}
isLoading={this.props.closeAccount.isLoading}
onPress={() => User.closeAccount(this.state.reasonForLeaving)}
isDisabled={Str.removeSMSDomain(userEmailOrPhone).toLowerCase() !== this.state.phoneOrEmail.toLowerCase()}
isDisabled={Str.removeSMSDomain(userEmailOrPhone).toLowerCase() !== this.state.phoneOrEmail.toLowerCase() || this.props.network.isOffline}
style={[styles.mt5]}
/>
{!this.props.isSmallScreenWidth
&& <OfflineIndicator containerStyles={[styles.mt2]} />}
Comment on lines +122 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

OH. Whoops.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

</ScrollView>
<ConfirmModal
title={this.props.translate('closeAccountPage.closeAccountError')}
Expand Down Expand Up @@ -147,6 +155,7 @@ CloseAccountPage.defaultProps = defaultProps;
export default compose(
withLocalize,
withWindowDimensions,
withNetwork(),
withOnyx({
closeAccount: {
key: ONYXKEYS.CLOSE_ACCOUNT,
Expand Down
19 changes: 19 additions & 0 deletions src/pages/signin/LoginForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import OfflineIndicator from '../../components/OfflineIndicator';
import {withNetwork} from '../../components/OnyxProvider';
import networkPropTypes from '../../components/networkPropTypes';
import * as ErrorUtils from '../../libs/ErrorUtils';
import DotIndicatorMessage from '../../components/DotIndicatorMessage';
import * as CloseAccount from '../../libs/actions/CloseAccount';

const propTypes = {
/** Should we dismiss the keyboard when transitioning away from the page? */
Expand All @@ -41,6 +43,11 @@ const propTypes = {
isLoading: PropTypes.bool,
}),

closeAccount: PropTypes.shape({
/** Message to display when user successfully closed their account */
success: PropTypes.string,
}),

/** Props to detect online status */
network: networkPropTypes.isRequired,

Expand All @@ -53,6 +60,7 @@ const propTypes = {

const defaultProps = {
account: {},
closeAccount: {},
blurOnSubmit: false,
};

Expand Down Expand Up @@ -124,6 +132,11 @@ class LoginForm extends React.Component {
return;
}

// If account was closed and have success message in Onyx, we clear it here
if (!_.isEmpty(this.props.closeAccount.success)) {
CloseAccount.setDefaultData();
}

const login = this.state.login.trim();
if (!login) {
this.setState({formError: 'common.pleaseEnterEmailOrPhoneNumber'});
Expand Down Expand Up @@ -176,6 +189,11 @@ class LoginForm extends React.Component {
{this.props.account.success}
</Text>
)}
{!_.isEmpty(this.props.closeAccount.success) && (

// DotIndicatorMessage mostly expects onyxData errors, so we need to mock an object so that the messages looks similar to prop.account.errors
<DotIndicatorMessage style={[styles.mv2]} type="success" messages={{0: this.props.closeAccount.success}} />
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
)}
{ // We need to unmount the submit button when the component is not visible so that the Enter button
// key handler gets unsubscribed and does not conflict with the Password Form
this.props.isVisible && (
Expand Down Expand Up @@ -203,6 +221,7 @@ LoginForm.defaultProps = defaultProps;
export default compose(
withOnyx({
account: {key: ONYXKEYS.ACCOUNT},
closeAccount: {key: ONYXKEYS.CLOSE_ACCOUNT},
}),
withWindowDimensions,
withLocalize,
Expand Down