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

Show only host when displaying URL of a site you are trying to log-in #14486

Merged

Conversation

khaykov
Copy link
Member

@khaykov khaykov commented Apr 19, 2021

Fixes #14319

This PR removes the path component of an URL that is displayed when you enter credentials for a self-hosted site.
Image from Gyazo

To test:

  • Log out from the app.
  • Access self-hosted login flow.
  • Use URL that has a path in it (eg. https://testsite.wordpress.com/wp-login.php).
  • On the screen where you enter login and password confirm that only the host part of URL is displayed in Enter your account information for xyz header.

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@peril-wordpress-mobile
Copy link

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout issue/14319-omit-path-when-logging-in-self-hosted-site
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/14486
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/14486 and open a new PR.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 19, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 19, 2021

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Hey @khaykov 👋 Thanks for the changes, they work pretty well.

Before we go ahead with this, I think we should just keep in mind that removing any paths from the address would probably not work so well for users that have their WordPress installation at subdirectories like https://mysite.com/blog. Even more so for users that have multisite installations at different subdirectories.

Although these scenarios are probably not the most common, I'd bet they still happen more often than the scenario this change is addressing, but it's hard to know for sure without looking at the data, and I could be totally wrong on this.

Another option would be to only strip paths that we know for sure won't be useful for the login process, but I can't say if the required effort is worth it.

@khaykov
Copy link
Member Author

khaykov commented Apr 21, 2021

@renanferrari That's a good point, thank you! Would do you think about using XMLRPC endpoint URL for this (when available)? In theory we can just strip xmlrpc.php and get a path to the root of the site.

@renanferrari
Copy link
Member

@khaykov I'm not sure if I understand your suggestion correctly, so let me check if I got it right: are you proposing to simply check if the URL string contains xmlrpc.php and then strip it if so? Or am I missing something?

@khaykov
Copy link
Member Author

khaykov commented Apr 26, 2021

@renanferrari Thanks for your help! I implemented your suggestions, as we discussed, and tested with regular and subdir based multisites, and they seems to work pretty good :)

Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, @khaykov!

LGTM 👍

@renanferrari renanferrari merged commit 50ec8a8 into develop Apr 27, 2021
@renanferrari renanferrari deleted the issue/14319-omit-path-when-logging-in-self-hosted-site branch April 27, 2021 00: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.

Log In screen displaying site address path components
2 participants