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

Refactor Session.resendValidationLink and the ResendValidationForm to use the new API #10556

Merged
merged 35 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
6ed249a
add back changes from other branch
jasperhuangg Aug 25, 2022
fc01f7f
add back missing change
jasperhuangg Aug 25, 2022
1444f37
remove unused imports
jasperhuangg Aug 25, 2022
7edabc4
refactor into function component
jasperhuangg Aug 25, 2022
ae97682
Add/use the DotIndicatorMessage component
jasperhuangg Aug 25, 2022
1ade4cd
remove unused imports
jasperhuangg Aug 25, 2022
2ef66fd
fix styles
jasperhuangg Aug 25, 2022
1d17c28
fix styles
jasperhuangg Aug 25, 2022
4672cd2
refactor styles
jasperhuangg Aug 25, 2022
db9ea54
Update src/pages/signin/ResendValidationForm.js
jasperhuangg Aug 29, 2022
de7b94c
access from props
jasperhuangg Aug 29, 2022
eeb48e9
Merge remote-tracking branch 'origin/jasper-resendValidateCodeRefacto…
jasperhuangg Aug 29, 2022
840aaac
clear errors when submitting form
jasperhuangg Aug 29, 2022
3e33ea3
move errors/message into value
jasperhuangg Aug 30, 2022
4f6987e
fix propTypes
jasperhuangg Aug 31, 2022
33028f5
fix propTypes
jasperhuangg Aug 31, 2022
9576ee0
fix propTypes
jasperhuangg Aug 31, 2022
04d7fe1
early return null if no messages to display
jasperhuangg Aug 31, 2022
e9920b7
move sorting logic into DotIndicatorMessage
jasperhuangg Aug 31, 2022
f76df2b
clear out by setting to null
jasperhuangg Aug 31, 2022
ec3b6c2
style
jasperhuangg Aug 31, 2022
5154fe0
Merge branch 'main' of github.com:Expensify/App into jasper-resendVal…
bondydaa Aug 31, 2022
ce3061a
update props to only expect object of strings, fix comments
bondydaa Aug 31, 2022
e9f90bb
fix default prop
bondydaa Aug 31, 2022
6f80da2
fix typo
bondydaa Aug 31, 2022
f5bed5e
remove extra line
bondydaa Aug 31, 2022
070113f
dont bother sorting messages for now
bondydaa Aug 31, 2022
6ba7db4
ensure we pass the message in a similar format to how errors are save…
bondydaa Aug 31, 2022
44de7e3
just pass errors to the brick road style component
bondydaa Aug 31, 2022
53de0a3
remove sorting logic bc its not very helpful and use .each instead of…
bondydaa Aug 31, 2022
2c0ffff
adding sorting back b/c ioni said so
bondydaa Aug 31, 2022
de9a109
use map bc it did not like .each
bondydaa Aug 31, 2022
a814cc5
add comment to warn about potential performance issues
bondydaa Sep 1, 2022
b47e419
simplify logic to not sure microtimestamp/not import the lib since it…
bondydaa Sep 1, 2022
be3212d
fix comment / line length
bondydaa Sep 1, 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
68 changes: 68 additions & 0 deletions src/components/DotIndicatorMessage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import React from 'react';
import _ from 'underscore';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import styles from '../styles/styles';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import colors from '../styles/colors';
import variables from '../styles/variables';
import Text from './Text';

const propTypes = {
/**
* In most cases this should just be errors from onxyData
* if you are not passing that data then this needs to be in a similar shape like
* {
* timestamp: 'message',
* }
*/
messages: PropTypes.objectOf(PropTypes.string),

// The type of message, 'error' shows a red dot, 'success' shows a green dot
type: PropTypes.oneOf(['error', 'success']).isRequired,

// Additional styles to apply to the container */
// eslint-disable-next-line react/forbid-prop-types
style: PropTypes.arrayOf(PropTypes.object),
};

const defaultProps = {
messages: {},
style: [],
};

const DotIndicatorMessage = (props) => {
if (_.isEmpty(props.messages)) {
return null;
}

// To ensure messages are presented in order we are sort of destroying the data we are given
// and rebuilding as an array so we can render the messages in order. We don't really care about
// the microtime timestamps anyways so isn't the end of the world that we sort of lose them here.
// BEWARE: if you decide to refactor this and keep the microtime keys it could cause performance issues
const sortedMessages = _.chain(props.messages)
.keys()
.sortBy()
.map(key => props.messages[key])
.value();

return (
<View style={[styles.dotIndicatorMessage, ...props.style]}>
<View style={styles.offlineFeedback.errorDot}>
<Icon src={Expensicons.DotIndicator} fill={props.type === 'error' ? colors.red : colors.green} height={variables.iconSizeSmall} width={variables.iconSizeSmall} />
</View>
<View style={styles.offlineFeedback.textContainer}>
{_.map(sortedMessages, (message, i) => (
<Text key={i} style={styles.offlineFeedback.text}>{message}</Text>
))}
</View>
</View>
);
};

DotIndicatorMessage.propTypes = propTypes;
DotIndicatorMessage.defaultProps = defaultProps;

export default DotIndicatorMessage;

18 changes: 2 additions & 16 deletions src/components/OfflineWithFeedback.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ import withLocalize, {withLocalizePropTypes} from './withLocalize';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
import stylePropTypes from '../styles/stylePropTypes';
import Text from './Text';
import styles from '../styles/styles';
import Tooltip from './Tooltip';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import * as StyleUtils from '../styles/StyleUtils';
import colors from '../styles/colors';
import variables from '../styles/variables';
import DotIndicatorMessage from './DotIndicatorMessage';

/**
* This component should be used when we are using the offline pattern B (offline with feedback).
Expand Down Expand Up @@ -83,11 +81,6 @@ const OfflineWithFeedback = (props) => {
const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors;
let children = props.children;
const sortedErrors = _.chain(props.errors)
.keys()
.sortBy()
.map(key => props.errors[key])
.value();

// Apply strikethrough to children if needed, but skip it if we are not going to render them
if (needsStrikeThrough && !hideChildren) {
Expand All @@ -102,14 +95,7 @@ const OfflineWithFeedback = (props) => {
)}
{hasErrors && (
<View style={StyleUtils.combineStyles(styles.offlineFeedback.error, props.errorRowStyles)}>
<View style={styles.offlineFeedback.errorDot}>
<Icon src={Expensicons.DotIndicator} fill={colors.red} height={variables.iconSizeSmall} width={variables.iconSizeSmall} />
</View>
<View style={styles.offlineFeedback.textContainer}>
{_.map(sortedErrors, (error, i) => (
<Text key={i} style={styles.offlineFeedback.text}>{error}</Text>
))}
</View>
<DotIndicatorMessage messages={props.errors} type="error" />
<Tooltip text={props.translate('common.close')}>
<Pressable
onPress={props.onClose}
Expand Down
17 changes: 17 additions & 0 deletions src/libs/ErrorUtils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import _ from 'underscore';
import CONST from '../CONST';

/**
Expand Down Expand Up @@ -35,7 +36,23 @@ function getAuthenticateErrorMessage(response) {
}
}

/**
* @param {Object} onyxData
* @param {Object} onyxData.errors
* @returns {String}
*/
function getLatestErrorMessage(onyxData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we did this before here, but I don't get why would we only care about one error message

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yeah I was wondering about this too actually... won't errors also be keyed by microtime so we should be able to sort based on those values - not that we need to necessarily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I actually took this logic from the LoginForm here, which displays the latest error. I simply moved that logic into getLatestErrorMessage since I thought it made sense to reuse them here, since they're all part of the same flow.

Doesn't it make sense to only display the latest error though? Why would a user want to know about the other previous errors if they're unrelated to the last action they took?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return _.chain(onyxData.errors || [])
.keys()
.sortBy()
.reverse()
.map(key => onyxData.errors[key])
.first()
.value();
}

export {
// eslint-disable-next-line import/prefer-default-export
getAuthenticateErrorMessage,
getLatestErrorMessage,
};
32 changes: 27 additions & 5 deletions src/libs/actions/Session/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,33 @@ function signOutAndRedirectToSignIn() {
* @param {String} [login]
*/
function resendValidationLink(login = credentials.login) {
Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: true});
DeprecatedAPI.ResendValidateCode({email: login})
.finally(() => {
Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: false});
});
const optimisticData = [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: true,
errors: null,
message: null,
},
}];
const successData = [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
message: Localize.translateLocal('resendValidationForm.linkHasBeenResent'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi ✋ coming from #17216, where this message is not translated when a user changes the language.

Localized text saved in Onyx is not translated when the locale is changed. So instead, we should save the key and translate it at the component level.

},
}];
const failureData = [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
message: null,
},
}];

API.write('RequestAccountValidationLink', {email: login}, {optimisticData, successData, failureData});
}

/**
Expand Down
9 changes: 2 additions & 7 deletions src/pages/signin/LoginForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import FormAlertWithSubmitButton from '../../components/FormAlertWithSubmitButto
import OfflineIndicator from '../../components/OfflineIndicator';
import {withNetwork} from '../../components/OnyxProvider';
import networkPropTypes from '../../components/networkPropTypes';
import * as ErrorUtils from '../../libs/ErrorUtils';

const propTypes = {
/** Should we dismiss the keyboard when transitioning away from the page? */
Expand Down Expand Up @@ -151,13 +152,7 @@ class LoginForm extends React.Component {

render() {
const formErrorTranslated = this.state.formError && this.props.translate(this.state.formError);
const error = formErrorTranslated || _.chain(this.props.account.errors || [])
.keys()
.sortBy()
.reverse()
.map(key => this.props.account.errors[key])
.first()
.value();
const error = formErrorTranslated || ErrorUtils.getLatestErrorMessage(this.props.account);
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't get why we only care about one error... NAB though since this existed before

return (
<>
<View style={[styles.mt3]}>
Expand Down
133 changes: 49 additions & 84 deletions src/pages/signin/ResendValidationForm.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React from 'react';
import _ from 'underscore';
import {TouchableOpacity, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import _ from 'underscore';
import Str from 'expensify-common/lib/str';
import styles from '../../styles/styles';
import Button from '../../components/Button';
Expand All @@ -17,6 +17,7 @@ import * as ReportUtils from '../../libs/ReportUtils';
import OfflineIndicator from '../../components/OfflineIndicator';
import networkPropTypes from '../../components/networkPropTypes';
import {withNetwork} from '../../components/OnyxProvider';
import DotIndicatorMessage from '../../components/DotIndicatorMessage';

const propTypes = {
/* Onyx Props */
Expand Down Expand Up @@ -46,92 +47,56 @@ const defaultProps = {
account: {},
};

class ResendValidationForm extends React.Component {
constructor(props) {
super(props);

this.validateAndSubmitForm = this.validateAndSubmitForm.bind(this);

this.state = {
formSuccess: '',
};
}

componentWillUnmount() {
if (!this.successMessageTimer) {
return;
}

clearTimeout(this.successMessageTimer);
}

/**
* Check that all the form fields are valid, then trigger the submit callback
*/
validateAndSubmitForm() {
this.setState({
formSuccess: this.props.translate('resendValidationForm.linkHasBeenResent'),
});

if (!this.props.account.validated) {
Session.resendValidationLink();
} else {
Session.resetPassword();
}

this.successMessageTimer = setTimeout(() => {
this.setState({formSuccess: ''});
}, 5000);
}

render() {
const isSMSLogin = Str.isSMSLogin(this.props.credentials.login);
const login = isSMSLogin ? this.props.toLocalPhone(Str.removeSMSDomain(this.props.credentials.login)) : this.props.credentials.login;
const loginType = (isSMSLogin ? this.props.translate('common.phone') : this.props.translate('common.email')).toLowerCase();

return (
<>
<View style={[styles.mt3, styles.flexRow, styles.alignItemsCenter, styles.justifyContentStart]}>
<Avatar
source={ReportUtils.getDefaultAvatar(this.props.credentials.login)}
imageStyles={[styles.mr2]}
/>
<View style={[styles.flex1]}>
<Text style={[styles.textStrong]}>
{login}
</Text>
</View>
</View>
<View style={[styles.mv5]}>
<Text>
{this.props.translate('resendValidationForm.weSentYouMagicSignInLink', {login, loginType})}
const ResendValidationForm = (props) => {
const isSMSLogin = Str.isSMSLogin(props.credentials.login);
const login = isSMSLogin ? props.toLocalPhone(Str.removeSMSDomain(props.credentials.login)) : props.credentials.login;
const loginType = (isSMSLogin ? props.translate('common.phone') : props.translate('common.email')).toLowerCase();

return (
<>
<View style={[styles.mt3, styles.flexRow, styles.alignItemsCenter, styles.justifyContentStart]}>
<Avatar
source={ReportUtils.getDefaultAvatar(props.credentials.login)}
imageStyles={[styles.mr2]}
/>
<View style={[styles.flex1]}>
<Text style={[styles.textStrong]}>
{login}
</Text>
</View>
{!_.isEmpty(this.state.formSuccess) && (
<Text style={[styles.formSuccess]}>
{this.state.formSuccess}
</View>
<View style={[styles.mv5]}>
<Text>
{props.translate('resendValidationForm.weSentYouMagicSignInLink', {login, loginType})}
</Text>
</View>
{!_.isEmpty(props.account.message) && (

// 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.mb5]} type="success" messages={{0: props.account.message}} />
)}
{!_.isEmpty(props.account.errors) && (
<DotIndicatorMessage style={[styles.mb5]} type="error" messages={props.account.errors} />
)}
<View style={[styles.mb4, styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}>
<TouchableOpacity onPress={() => redirectToSignIn()}>
<Text>
{props.translate('common.back')}
</Text>
)}
<View style={[styles.mb4, styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}>
<TouchableOpacity onPress={() => redirectToSignIn()}>
<Text>
{this.props.translate('common.back')}
</Text>
</TouchableOpacity>
<Button
medium
success
text={this.props.translate('resendValidationForm.resendLink')}
isLoading={this.props.account.loading}
onPress={this.validateAndSubmitForm}
isDisabled={this.props.network.isOffline}
/>
</View>
<OfflineIndicator containerStyles={[styles.mv1]} />
</>
);
}
}
</TouchableOpacity>
<Button
medium
success
text={props.translate('resendValidationForm.resendLink')}
isLoading={props.account.isLoading}
onPress={() => (props.account.validated ? Session.resetPassword() : Session.resendValidationLink())}
isDisabled={props.network.isOffline}
/>
</View>
<OfflineIndicator containerStyles={[styles.mv1]} />
</>
);
};

ResendValidationForm.propTypes = propTypes;
ResendValidationForm.defaultProps = defaultProps;
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/WorkspaceMembersPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,10 @@ class WorkspaceMembersPage extends React.Component {
}) {
const canBeRemoved = this.props.policy.owner !== item.login && this.props.session.email !== item.login;
return (
<OfflineWithFeedback onClose={() => this.dismissError(item)} pendingAction={item.pendingAction} errors={item.errors}>
<OfflineWithFeedback errorRowStyles={[styles.peopleRowBorderBottom]} onClose={() => this.dismissError(item)} pendingAction={item.pendingAction} errors={item.errors}>
<Hoverable onHoverIn={() => this.willTooltipShowForLogin(item.login, true)} onHoverOut={() => this.setState({showTooltipForLogin: ''})}>
<TouchableOpacity
style={[styles.peopleRow, !canBeRemoved && styles.cursorDisabled]}
style={[styles.peopleRow, !item.errors && styles.peopleRowBorderBottom, !canBeRemoved && styles.cursorDisabled]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check that there are no errors to add the border bottom?

Copy link
Contributor Author

@jasperhuangg jasperhuangg Aug 31, 2022

Choose a reason for hiding this comment

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

Because if there are errors, then the border bottom style is also added to the error row above. If we always add it here then we'd end up in a situation like this:

Screen Shot 2022-08-31 at 5 38 40 PM

With the condition that fixes the problem:

Screen Shot 2022-08-31 at 5 39 46 PM

onPress={() => this.toggleUser(item.login)}
activeOpacity={0.7}
>
Expand Down
Loading