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

Xmlrpc login errors. #2280

Merged
merged 6 commits into from
Feb 28, 2022
Merged

Xmlrpc login errors. #2280

merged 6 commits into from
Feb 28, 2022

Conversation

develric
Copy link
Contributor

@develric develric commented Feb 9, 2022

IMPORTANT: this is ready to be reviewed; but since it has implications for Woo as well, keeping it as a draft so it is not accidentally merged before the validation from WPAndroid and WooAndroid point of view.

This is part of 12401 issue.

I tried to keep the current behaviors not affected, only adding a couple of enums mapping the various errors conditions for xml-rpc, that are used/parsed only in the login flow for self-hosted for now. This should reduce the possibility of regressions but appreciate testing of the various log-in/sign-up scenarios 🙇 .

See the companion WPMobile PR for more details.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 9, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@develric develric requested a review from khaykov February 9, 2022 23:23
@develric develric marked this pull request as ready for review February 9, 2022 23:23
@develric develric marked this pull request as draft February 10, 2022 07:14
@khaykov khaykov self-assigned this Feb 11, 2022
Copy link
Member

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Looks good to me! No issues when testing with wpandroid client.

@@ -14,6 +15,23 @@ public static SiteError genericToSiteError(BaseNetworkError error) {
break;
}
}
return new SiteError(errorType, error.message);

SiteError siteError = new SiteError(errorType, error.message);
Copy link
Member

Choose a reason for hiding this comment

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

A very minor NP - maybe we can avoid a double initialization of siteError here? If it makes sense :)

@jkmassel jkmassel marked this pull request as ready for review February 15, 2022 16:49
@jkmassel jkmassel marked this pull request as draft February 15, 2022 16:51
@develric develric marked this pull request as ready for review February 24, 2022 22:06
@khaykov
Copy link
Member

khaykov commented Feb 25, 2022

Looks like there are no changes since my review, so good to go :)

@develric develric merged commit 1436f46 into trunk Feb 28, 2022
@develric develric deleted the issue/12401-login-error-xmlrpc branch February 28, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants