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

Cleanup any promise chains off of actions inside views #6092

Closed
Tracked by #5984
marcaaron opened this issue Oct 28, 2021 · 0 comments
Closed
Tracked by #5984

Cleanup any promise chains off of actions inside views #6092

marcaaron opened this issue Oct 28, 2021 · 0 comments
Assignees
Labels

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Oct 28, 2021

Good news here but there really aren't that many cases where we're doing this...

Just gonna kinda break down each one explain what the alternative should be...

Using component state for loading state instead of Onyx

This one is using a loading state locally in a component. Probably, it would be more ideal to find an Onyx key to put that loading state on. But seeing these kind of things does beg the question of where that "loading" state belongs. We've mostly defaulted to putting these on some Onyx key so that's what we should try to do here and move the error handling logic to the action.

requestInboxCall(this.props.route.params.taskID, personalPolicy.id, this.state.firstName, this.state.lastName, this.state.phoneNumber)
.then((result) => {
this.setState({isLoading: false});
if (result.jsonCode === 200) {
Growl.success(this.props.translate('requestCallPage.growlMessageOnSave'));
fetchOrCreateChatReport([this.props.session.email, CONST.EMAIL.CONCIERGE], true);
return;
}
// Phone number validation is handled by the API
Growl.error(result.message, 3000);
});

Another loading issue

fetchActions(this.props.reportID, offset)
.then(() => this.setState({isLoadingMoreChats: false}));

Chaining actions together that could be combined into a single action and storing errors in component state

This one is a few different actions chained together which seems fine it should just be its own action. The error message should be set somewhere else and not in the component state.

API.ValidateEmail({
accountID,
validateCode,
}).then((responseValidate) => {
if (responseValidate.jsonCode === 200) {
API.ChangePassword({
authToken: responseValidate.authToken,
password: this.state.password,
}).then((responsePassword) => {
if (responsePassword.jsonCode === 200) {
signIn(this.state.password);
} else {
this.setState({
error: this.props.translate('setPasswordPage.passwordNotSet'),
});
}
});
} else if (responseValidate.title === CONST.PASSWORD_PAGE.ERROR.ALREADY_VALIDATED) {

Navigation logic

This next two are kind of tricky because I don't think we should have a "navigate" logic added to a generic action that does not navigate when used in other places. But the easy solution is to take this and make it setSecondaryLoginAndNavigate() then refactor later if we need a version that does not navigate

setSecondaryLogin(this.state.login, this.state.password)
.then((response) => {
if (response.jsonCode === 200) {
Navigation.navigate(ROUTES.SETTINGS_PROFILE);
}
});

changePassword(this.state.currentPassword, this.state.newPassword)
.then((response) => {
if (response.jsonCode === 200) {
Navigation.navigate(ROUTES.SETTINGS);
}
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants