-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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. #15919
Xmlrpc login errors. #15919
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement, @develric ! I tested with a variety of sites, and so far everything worked as expected, and I got all the correct error messages.
Since xmlrp issues is a big chunk of errors our support tickets, do you think it makes sense to get an editorial review of error messages?
Hi @develric, yes, it would be better to sync and create a single Woo PR for both changes, let's sync on Wednesday to work on this together. |
This and linked login and fluxc lib PRs have been tested/reviewed and also confirmed by woo here. Since it's involving shared libs, to make it stay in trunk for a bit, we will merge everything in 19.4 soon after code freeze (cc @hichamboushaba ). |
Fixes #12401
Needed PR to merge before:
This PR aims to improve error messages that can occur during login for self-hosted sites (see this comment for more details).
NOTE 1: for both FluxC and Login libs I tried to keep the current behaviors 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. This should reduce the possibility of regressions but appreciate testing of the various log-in/sign-up scenarios 🙇 .
NOTE 2: hi @hichamboushaba 👋 , as for another recent PR set, also this one implies a few changes in the FluxC and Login libs (see linked PRs at the beginning of this description). Not sure if they could have an impact on Woo app, but maybe we can sync the testing/merge of this PRs set with the testing/merge of this other PR (and related fluxc) so to have only 1 PR for Woo. Let me know 🙇
To test
As per this comment install the xml-rpc-tweak.zip plugin in a self-hosted site; once done that you will have a settings page for the plugin in the wp-admin that allows to set the various test MODEs. Here below the expected results while logging in:
Smoke test login/signup
Using wpcom/jp/self-hosted/atomic sites smoke test the various login/signup flows. Test also magic links, 2FA, reset password, or anything else you could think of 🙇 .
Regression Notes
Potential unintended areas of impact
Based on how the modifications were done, effects could be in case present only on self-hosted sites.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing.
What automated tests I added (or what prevented me from doing so)
Autometed tests for login flows.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.