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

Add realtime site address validation #10255

Merged
merged 9 commits into from Jul 22, 2019

Conversation

@shiki
Copy link
Member

commented Jul 19, 2019

Closes #9720.

This adds realtime validation for the site address in Login or Add Site. This is based on the iOS implementation described in wordpress-mobile/WordPress-iOS#10294 (comment).

  • The error is only shown if the user has not typed anything after 2 seconds
  • The Next button is only enabled if the URL is valid

To implement this, I moved all the site address logic to a new class, LoginSiteAddressValidator. The class encapsulates the validation, cleaning, and error reporting using debounce. A corresponding unit test, LoginSiteAddressValidatorTest, has also been added.

Known Issues

The LoginSiteAddressFragment doesn't retain any error message shown below the text field after configuration changes. I didn't attempt to fix this. I thought it would be a huge change.

Testing

Adding a Site

  1. Click on Sites → Switch Site.
  2. Click on the + button at the top right.
  3. Enter a site address.

Logging in with a self-hosted site

  1. On Login page, click on Login.
  2. Click on Log in by entering your site address.
  3. Enter a self-hosted site address.

On both scenarios, please test against the specs described above. Please also test that there are no regressions in logging in with a valid site.

Reviewing

Only 1 reviewer is needed but anyone can review.

Editorial

We're hoping to get a review for the error message that is shown when the URL is invalid:

Enter a valid URL eg. example.com.

Release Notes

  • If there are user-facing changes, I have added an item to RELEASE-NOTES.txt.

Tasks

shiki added some commits Jul 19, 2019

Move LoginSiteAddress validation to a class
This time, the _Next_ button is not enabled until the user has entered an invalid site address.
Site Address: Show an error message after 2 sec
The invalid address error message will only be shown 2 seconds after the user stopped typing.
Replace login_invalid_site_url
This is to match the new iOS error message.

@shiki shiki added this to the 13.0 milestone Jul 19, 2019

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Jul 19, 2019

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into 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/9720-site-url-validation
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/10255
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/10255 and open a new PR.

Generated by 🚫 dangerJS

@@ -359,7 +369,7 @@ public void onDiscoverySucceeded(OnDiscoveryResponse event) {
return;
} else {
AppLog.e(T.API, "onDiscoveryResponse has error: " + event.error.name()
+ " - " + event.error.toString());
+ " - " + event.error.toString());

This comment has been minimized.

Copy link
@shiki

shiki Jul 19, 2019

Author Member

Auto-formatted. ¯\_(ツ)_/¯

@shiki shiki changed the title Issue/9720 site url validation Add realtime site address validation Jul 19, 2019

@shiki shiki marked this pull request as ready for review Jul 19, 2019

@shiki shiki requested review from AmandaRiu and aforcier Jul 19, 2019

@benhuberman

This comment has been minimized.

Copy link

commented Jul 20, 2019

For the error message, I'd go with something that explains the error a bit more fully and avoids abbreviations (like e.g.). For example:

Please enter a complete website address, like example.com.

This doesn't strike me as significantly longer than the current version, but let me know if it creates a length issue.

Update login_invalid_site_url
This is based on Editorial’s advice.
@shiki

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

Thank you, @benhuberman. I've applied your suggestion. This is what it looks like now:

@benhuberman

This comment has been minimized.

Copy link

commented Jul 22, 2019

I like how it looks, @shiki.

I'm sorry I didn't catch this when reviewing this the first time, but I just saw that the first sentence on that screen could also use a small tweak -- the your/you sounds redundant (and it's strongly assumed that you'd only ever connect your own site). I'd go with this instead:

Enter the address of the WordPress site you'd like to connect.

@shiki

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

No worries at all @benhuberman. Updated:

@AmandaRiu AmandaRiu assigned AmandaRiu and unassigned AmandaRiu Jul 22, 2019

@AmandaRiu
Copy link
Contributor

left a comment

Tested through the noted scenarios and reviewed the code. LGTM :shipit:

@shiki shiki merged commit 2a0c965 into develop Jul 22, 2019

4 checks passed

Peril All green. Jolly good show.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.