Skip to content

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

Merged
staskus merged 4 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#21904
staskus merged 4 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

Partially addresses #18497
Related WordPressAuthenticator PR: wordpress-mobile/WordPressAuthenticator-iOS#796
Related WordPressKit PR: wordpress-mobile/WordPressKit-iOS#655

To test:

A URL for testing can be found at pbArwn-4xZ-p2#comment-5712 and/or 5170539-zd-a8c (not included in this ticket to protect user privacy):

  1. Navigate to the app's login screen and select the "Log in by entering site address" option.
  2. Submit the test URL.
  3. Confirm that XML-RPC error is shown instead of invalid WordPress site error

Regression

Regression Notes

  1. Potential unintended areas of impact

Breaking site address login

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Manual testing, existing tests

  1. What automated tests I added (or what prevented me from doing so)

Added new tests into WordPressAuthenticator-iOS

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@staskus
staskus force-pushed the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch from f10cbfd to 5997b3b Compare November 13, 2023 12:57
@staskus staskus modified the milestones: Pending, 23.8 Nov 13, 2023
@staskus
staskus requested a review from guarani November 13, 2023 12:59
@staskus
staskus marked this pull request as ready for review November 13, 2023 13:00
@wpmobilebot

wpmobilebot commented Nov 13, 2023

Copy link
Copy Markdown
Contributor
WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21904-45dcc79
Version23.9
Bundle IDorg.wordpress.alpha
Commit45dcc79
App Center BuildWPiOS - One-Offs #8187
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

wpmobilebot commented Nov 13, 2023

Copy link
Copy Markdown
Contributor
Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21904-45dcc79
Version23.9
Bundle IDcom.jetpack.alpha
Commit45dcc79
App Center Buildjetpack-installable-builds #7209
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus modified the milestones: 23.8, Pending Nov 23, 2023
@staskus

staskus commented Nov 23, 2023

Copy link
Copy Markdown
Contributor Author

Automattic/peril-settings#118 and Automattic/dangermattic#30 will resolve Danger warnings.

@staskus

staskus commented Nov 23, 2023

Copy link
Copy Markdown
Contributor Author

Hey @SiobhyB, I saw you originally reported this issue #18497 maybe you could to take a look at the solution? This is not urgent.

My proposal was to do an additional check to determine if a site is a WP site if XML-RPC validation fails, just so we wouldn't confuse users with incorrect error messages.

@SiobhyB
SiobhyB self-requested a review November 24, 2023 16:59

@SiobhyB SiobhyB left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving via wordpress-mobile/WordPressAuthenticator-iOS#796 (review), with the note that I think it'd be wise for @guarani or another iOS developer to also do a review, based on my lower level of experience in iOS. (I also see a couple of failing tests, which I assume are the ones referenced here.)

Thanks again for working on this @staskus! 🙌

@staskus

staskus commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

@SiobhyB thank you very much for your testing and review! 🙇

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?
wordpress-mobile/WordPressAuthenticator-iOS#796 (review)

I think that's a very good point. Given the goal of this fix is to reduce the number of confused and misled users. I will see what could be the improvement!

with the note that I think it'd be wise for @guarani or another iOS developer to also do a review

👍

@staskus

staskus commented Dec 1, 2023

Copy link
Copy Markdown
Contributor Author

Improved error messages to suggest contacting the host: wordpress-mobile/WordPressKit-iOS#655

@guarani guarani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the long delay reviewing this, @staskus!

Tested:

  • I see the new "Couldn't connect. Required XML-RPC methods are missing on the server. Please contact your hosting provider to solve this problem." message as expected when using the test URL "https://www.inter..." (redacted)
  • Using the XMLRPC tweak tool and Mode 4: Block All Calls, the new & updated message is also shown (as above)
  • The other XMLRPC tweak tool scenarios also work as expected
  • Login to a functional self-hosted site continues to work

@staskus staskus modified the milestones: Pending, 24.0 Dec 20, 2023
@staskus
staskus merged commit 4fc94b6 into trunk Dec 20, 2023
@staskus
staskus deleted the fix/18497-login-not-a-wordpress-site-error-sometimes-displays-incorrectly branch December 20, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants