Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Confirm that site is indeed not a WordPress site after XML-RPC validation fails#796

Merged
staskus merged 9 commits into
trunkfrom
fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly
Dec 20, 2023
Merged

Confirm that site is indeed not a WordPress site after XML-RPC validation fails#796
staskus merged 9 commits into
trunkfrom
fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly

Conversation

@staskus

@staskus staskus commented Oct 27, 2023

Copy link
Copy Markdown
Contributor

More context: wordpress-mobile/WordPress-iOS#18497 (comment)
Related PR wordpress-mobile/WordPress-iOS#21904

In rare cases, XML-RPC validation can fail and indicate that the site is not a WordPress site when it actually is.

To avoid this corner case, I added an additional isWPSite check after guessXMLRPCURL and before showing errors to the users:

  • If WordPressOrgXMLRPCValidator determines that it's not a valid WordPress site and isWPSite (connect/site-info) API call confirms it, then we inform the user that the site is invalid
  • If WordPressOrgXMLRPCValidator determines that it's not a valid WordPress site but isWPSite (connect/site-info) API suggests otherwise, we inform users about XML-RPC issues which they can then look for further assistance

Implementation details

I didn't want to change this fragile code before writing tests to it and to write tests I needed to split business logic from UI. I recommend reviewing commit by commit:

  1. Extract error handling code - 9c14098
  2. Move it to ViewModel - b62db73
  3. Add unit tests - 76925ba
  4. Add isWPSite check - a27ac06
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@staskus
staskus force-pushed the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch from dfc746c to f69793a Compare October 27, 2023 10:41
@staskus
staskus marked this pull request as ready for review October 27, 2023 10:41
@staskus
staskus requested a review from guarani October 27, 2023 10:42
@staskus

staskus commented Oct 27, 2023

Copy link
Copy Markdown
Contributor Author

@guarani the code can be reviewed but ideally, I would do some more manual testing myself, so I'm not targeting 23.6 for sure. If you'll have time to review the code or critique the solution overall - great, I'll finish after I come back. Thanks! 🙇

@staskus
staskus force-pushed the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch from f69793a to 9db1d08 Compare November 13, 2023 11:18
In rare cases, XMLRPC validation can fail for unexpected reasons. Make a fallback check to determine if site is WP site before concluding to the user that the site is not WordPress site
@staskus
staskus force-pushed the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch from 9db1d08 to a27ac06 Compare November 13, 2023 12:55

@SiobhyB SiobhyB left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@staskus, thank you for working on this! 🙇‍♀️ I verified that the misleading "not a WordPress site" error is resolved with these changes applied:

Before After

In addition, I went through the steps at wordpress-mobile/WordPress-iOS#17724 and tried a few ways to see if I could break login, but couldn't.

The only note I have is that, although less confusing than before, I feel some users won't know what next steps to take or perhaps even what XML-RPC is. How would you feel about adding a sentence like Please check with your hosting provider to find out if they are blocking XML-RPC access? Or perhaps even linking to our support doc? I understand that this might be out of the scope of this PR though. Let me know if I should open a separate issue regarding the wording!

I went through each commit as well as your descriptions, and the logic seems sound to me. That said, I'm not an iOS developer, so it might be wise to have @guarani or another iOS developer do another pass.

Approving based on my testing and understanding of the logic. :)

@guarani guarani left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved via wordpress-mobile/WordPress-iOS#17724.

The code change looks good and I ran this repos tests locally.

@staskus
staskus merged commit b51b4b2 into trunk Dec 20, 2023
@staskus
staskus deleted the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch December 20, 2023 08:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants