Add error-specific validation message in site identification page #91748

Imran92 · 2024-06-13T07:08:12Z

Related to #91479

Proposed Changes

Added error-specific validation messages for the following scenarios.

Input is an Email
Input has invalid characters
Wrong Protocol (FTP://, file:// etc.)
Missing TLD
IDN URLs (Either has non ASCII characters in URL or hostname starts with xn--)
General error message for fallback

I have replaced "Incomplete URL Error" from the issue with Wrong Protocol because otherwise the error is too ambiguous and broad to detect. But I am a bit unsure about using the word "Protocol" in the error message, looks too technical. I only kept it because we show it only when the user has already used the wrong protocol and it conveys the exact reason. But LMK if you think a different and less technical message should be shown.

Also, detecting missing TLD is super tricky. For example, "test.com" is valid but "test.wpcomstaging" is not even though both of them has the same structure as a string. So it's not too correct when implemented with regex or some other way through code. But it should be helpful because our error log says users mostly write URLs like this "myblog" instead of "myblog.com" in which case, it'll be helpful.

Also, I've included an example of a valid URL with each error message to better convey to the user how a URL looks.

Why are these changes being made?

To enable the user to better understand what's wrong with the URL they have input.

Screenshots

Testing Instructions

Make sure your language is English in https://wordpress.com/me/account
In the migration flow (take any migration path, for example http://calypso.localhost:3000/start), navigate to the Site Identification step
In the URL field, try different wrong URLs (Please check the Proposed Changes section above). For example - myblog, https://myblog, https://myblog^, email@myblog.com etc.
You should see different error messages depending on the URL
Change the language to some other language in https://wordpress.com/me/account
Go to the site identification page and try different wrong URLs again
Make sure you see the old error message

Screen.Recording.2024-06-14.at.4.32.22.PM.mov

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-06-13T07:08:23Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-112023

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-112023&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-112023&env=a8c-for-agencies

matticbot · 2024-06-13T07:09:19Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1615 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
import-hosted-site-flow      +1221 B  (+0.1%)     +551 B  (+0.2%)
import-flow                  +1221 B  (+0.0%)     +511 B  (+0.1%)
import                       +1221 B  (+0.2%)     +553 B  (+0.3%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

donnapep · 2024-06-13T16:21:33Z

WDYT about requesting a copy review (pfGA22-2-p2) of these error messages first?

And it looks like there are some test failures.

Imran92 · 2024-06-13T18:38:39Z

Thanks @donnapep! All tests are green now.

WDYT about requesting a copy review (pfGA22-2-p2) of these error messages first?

This is a TIL, thanks for sharing it! Just wondering if it would be worth running these through Bohdan/Jana first before submitting for review, WDYT?

donnapep · 2024-06-13T18:43:39Z

This is a TIL, thanks for sharing it! Just wondering if it would be worth running these through Bohdan/Jana first before submitting for review, WDYT?

We can just go straight to the copy review folks as they're the copy experts. 🙂

Imran92 · 2024-06-13T18:44:23Z

We can just go straight to the copy review folks as they're the copy experts. 🙂

Thanks, on it!

donnapep · 2024-06-13T20:16:21Z

👋🏻 I did a quick test of the behavior. I found it somewhat distracting to see the error message update as I typed. Could we change it so that the message only changes after clicking the button?

Imran92 · 2024-06-14T11:07:41Z

👋🏻 I did a quick test of the behavior. I found it somewhat distracting to see the error message update as I typed. Could we change it so that the message only changes after clicking the button?

Thanks for checking @donnapep, I've changed the behavior here f5b85cf so that it only appears when you click the button and message disappears when you change something in the input field. I've seen this behavior on many login pages. In case you prefer this message to stay until the next click, LMK!

gabrielcaires

I left some suggestions

gabrielcaires · 2024-06-14T17:00:25Z

client/blocks/import/capture/capture-input.tsx

@@ -69,13 +72,124 @@ const CaptureInput: FunctionComponent< Props > = ( props ) => {
 		}
 	}

+	function isIDN( url: string ) {


Can we have this function outside the component body?
Each render is recreating the function.

Thanks, I've handled it here 228d668

gabrielcaires · 2024-06-14T17:04:15Z

client/blocks/import/capture/capture-input.tsx

+	}
+
+	// TODO: Just return the translated string directly from getValidationMessage once we have translations for all messages.
+	const errorMessages = {


Suggestion: Move all error message logic to a private component, since is this pretty big.

Thank you, done here 228d668

gabrielcaires · 2024-06-14T17:06:11Z

client/blocks/import/capture/test/index.tsx

+		);
+	} );
+
+	test( 'should show error for missing top-level domain', async () => {


Usually, we use the it instead of test the file already has scenarios with it.

Suggested change

test( 'should show error for missing top-level domain', async () => {

it.( 'shows error for missing top-level domain', async () => {

Done under it.each here 3447cd3

gabrielcaires · 2024-06-14T17:11:26Z

client/blocks/import/capture/test/index.tsx

+	} );
+
+	test( 'should show error for missing top-level domain event when protocol is present', async () => {
+		await enterUrlAndContinue( 'https://myblog' );


You can use dinamic to test all scenarios:

Example:

it.each([ {scenario: 'A', input: 'XXX', error: 'YYY'}, {scenario: 'B', input: 'BB', error: 'ZZZ'}, ])(' show error ', ({scenario, input, expected}) => { await enterUrlAndContinue( input ); expect( screen.getByText( error ) ).toBeInTheDocument(); });

Docs here https://jestjs.io/docs/api#testeachtablename-fn-timeout

Very good suggestion, thank you! Done here 3447cd3

client/blocks/import/capture/url-validation-message-helper.ts

donnapep · 2024-06-17T19:41:29Z

I couldn't get the Looks like your URL has some invalid characters (like ~ or ^). message to trigger with
the following:

Also, I had some strange behavior when testing that is not related to this PR, but perhaps you may still have insight into. I added supsup.com as the URL, and on the next screen I clicked the Back button. When I went to the previous screen, my URL was now set to https://www.brandbucket.com/names/supsup?source=ext/, which is really weird. Wondering if we saw a URL like that at all in the error data? Seems like a potential bug to me:

donnapep · 2024-06-17T19:43:20Z

@Imran92 Given the strings have already been approved in 1NK0CW5QEL336zXL4QfQdiNCDNQhVO2uWJbjEap3rLNM-gdoc, I'm going ahead and adding the "String Freeze" label. Hopefully they'll be translated by the time you're back, and then we can get rid of the additional complexity / duplicated strings prior to merge.

a8ci18n · 2024-06-17T20:20:19Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/14781025

Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday.

Thank you @Imran92 for including a screenshot in the description! This is really helpful for our translators.

a8ci18n · 2024-06-23T18:31:57Z

Translation for this Pull Request has now been finished.

Imran92 · 2024-06-24T05:56:46Z

Is there any chance we would show the message even if the URL was valid, and so the user wouldn't be able to move on? Because if so, that would be bad. 🙅🏻

Very good and valid question, yep that'll be bad if that happens. Actually, this was my first fear as well, so to completely avoid that, I only try to detect the type of error when the existing main validation regex detects an error. Only when the isError flag is true, then we try to figure out the proper message for it -

const tempValidationMessage = isValid
			? ''
			: getValidationMessage( url, translate, hasEnTranslation );

But I'm thinking that it would be more the opposite case where we wouldn't catch some missing TLDs, so the user would see the default error message instead. Does that all sound correct?

Yap, Right, that will probably be the case

Imran92 · 2024-06-24T06:57:20Z

Translation for this Pull Request has now been finished.

I am seeing no associated translation strings here https://translate.wordpress.com/deliverables/overview/14781025 .

We are using the string freeze label instead

donnapep · 2024-06-24T15:39:02Z

I re-tested and can confirm that the internationalized domain name message and the generic error message are showing in English, so there's some problem there. Maybe we should check with the i18n team.

Also, any thoughts on #91748 (comment)?

donnapep · 2024-06-24T15:44:08Z

For French, none of the error messages are translated.

Imran92 · 2024-06-25T12:27:04Z

I couldn't get the Looks like your URL has some invalid characters (like ~ or ^). message to trigger with the following:

Thanks for finding it Donna, fixed it here

Also, I had some strange behavior when testing that is not related to this PR, but perhaps you may still have insight into. I added supsup.com as the URL, and on the next screen I clicked the Back button. When I went to the previous screen, my URL was now set to https://www.brandbucket.com/names/supsup?source=ext/, which is really weird. Wondering if we saw a URL like that at all in the error data? Seems like a potential bug to me:

Nice finding! I wasn't sure what was happening here, so I investigated the issue and found the reason. Apparently, if the URL that is put in the input field redirects to some other URL, we replace the input URL with the final redirected URL. The URL you put there (supsup.com) is on sell by brandbucket.com, so if anyone tries to access the URL, they redirect it to the purchase page. The same will happen if you try with "dubico.com" or "yazani.com" etc. Same happens if we try to access a WPCOM site with the wpcomstaging URL when it has a different primary domain. I'm not sure if it is an expected behavior or a bug though :p

donnapep · 2024-06-25T15:25:12Z

I'm not sure if it is an expected behavior or a bug though

I'd consider this a bug as it was pretty confusing for me as a user. I'm going to create an issue for it.

donnapep · 2024-06-25T19:35:39Z

@Imran92 What's the plan for figuring out the translation issue? If we can't solve it in a timely manner, then let's revert the change that removed the use of useHasEnTranslation and ship for English users only. Either way, let's please ship by the end of the week. 🙇🏻‍♀️

Imran92 · 2024-06-25T19:53:55Z

@Imran92 What's the plan for figuring out the translation issue? If we can't solve it in a timely manner, then let's revert the change that removed the use of useHasEnTranslation and ship for English users only. Either way, let's please ship by the end of the week. 🙇🏻‍♀️

Yap falling back to the useHasEnTranslation is my plan if it doesn't get sorted out soon. I've done some investigation already and found that even though the translations are marked as completed in the deliverables for all locales, the strings didn't get included in all translation bundles. I've commented with my findings in the i18n internal channel and they said they'll look into the deployed translations. Hopefully, that'll sort it. p1719343082673409-slack-C02AED43D

update: I did a quick test and seems the issue is resolved. But I'll test again tomorrow

donnapep

Everything looks good! Let's

Imran92 added 4 commits June 13, 2024 11:20

Show dynamic validation message for URL

e8a27a6

Should show the TLD error when applicable

a4460e1

Should show TLD error when protocol is given

a3149ef

Add test for all error messages

b38434b

Imran92 requested a review from a team June 13, 2024 07:08

Imran92 self-assigned this Jun 13, 2024

Imran92 requested a review from a team as a code owner June 13, 2024 07:08

matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 13, 2024

Imran92 added 2 commits June 13, 2024 13:42

Update test URL for default case

aff936c

Fix type for url

6c29802

Imran92 added 2 commits June 13, 2024 23:55

Fix e2e error

51e3369

Add period at the end of the copies

6bd7289

Imran92 linked an issue Jun 14, 2024 that may be closed by this pull request

Task: Show more informative error messages for commonly made URL mistakes #91479

Closed

Show error message only after submission

f5b85cf

Imran92 added 3 commits June 14, 2024 21:58

Detect IDN and show proper validation message

758766e

Add test for IDN urls

d8e027a

Fix url type

8ea6cd5

gabrielcaires requested changes Jun 14, 2024

View reviewed changes

Imran92 added 2 commits June 14, 2024 23:21

Move validation logic to separate file

228d668

Change tests to use it.each

3447cd3

donnapep reviewed Jun 17, 2024

View reviewed changes

client/blocks/import/capture/url-validation-message-helper.ts Outdated Show resolved Hide resolved

donnapep reviewed Jun 17, 2024

View reviewed changes

client/blocks/import/capture/url-validation-message-helper.ts Outdated Show resolved Hide resolved

donnapep added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Jun 17, 2024

Imran92 closed this Jun 24, 2024

Imran92 reopened this Jun 24, 2024

github-actions bot removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging labels Jun 24, 2024

Remove extra strings for translation check

2659b50

We are using the string freeze label instead

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 24, 2024

Imran92 added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Jun 24, 2024

Remove tilde character from accepted list

aac5931

donnapep mentioned this pull request Jun 25, 2024

Incorrect URL shown in migration flow site identification step for some URLs #92089

Open

renatho mentioned this pull request Jun 25, 2024

Site Profiler: Extract the hosting provider details to a separate hook #91989

Merged

7 tasks

donnapep approved these changes Jun 26, 2024

View reviewed changes

Imran92 merged commit 0d7c7bd into trunk Jun 26, 2024
10 of 11 checks passed

Imran92 deleted the add/validation-message-for-url-identification branch June 26, 2024 13:03

github-actions bot removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging labels Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error-specific validation message in site identification page #91748

Add error-specific validation message in site identification page #91748

Imran92 commented Jun 13, 2024 •

edited by donnapep

Loading

github-actions bot commented Jun 13, 2024 •

edited by matticbot

Loading

matticbot commented Jun 13, 2024 •

edited

Loading

donnapep commented Jun 13, 2024 •

edited

Loading

Imran92 commented Jun 13, 2024

donnapep commented Jun 13, 2024

Imran92 commented Jun 13, 2024

donnapep commented Jun 13, 2024

Imran92 commented Jun 14, 2024

gabrielcaires left a comment

gabrielcaires Jun 14, 2024

Imran92 Jun 14, 2024 •

edited

Loading

gabrielcaires Jun 14, 2024

Imran92 Jun 14, 2024

gabrielcaires Jun 14, 2024

Imran92 Jun 14, 2024

gabrielcaires Jun 14, 2024

Imran92 Jun 14, 2024

donnapep commented Jun 17, 2024 •

edited

Loading

donnapep commented Jun 17, 2024

a8ci18n commented Jun 17, 2024 •

edited

Loading

a8ci18n commented Jun 23, 2024

Imran92 commented Jun 24, 2024

Imran92 commented Jun 24, 2024

donnapep commented Jun 24, 2024 •

edited

Loading

donnapep commented Jun 24, 2024

Imran92 commented Jun 25, 2024

donnapep commented Jun 25, 2024

donnapep commented Jun 25, 2024

Imran92 commented Jun 25, 2024 •

edited

Loading

donnapep left a comment

	test( 'should show error for missing top-level domain', async () => {
	it.( 'shows error for missing top-level domain', async () => {

Add error-specific validation message in site identification page #91748

Add error-specific validation message in site identification page #91748

Conversation

Imran92 commented Jun 13, 2024 • edited by donnapep Loading

Proposed Changes

Why are these changes being made?

Screenshots

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jun 13, 2024 • edited by matticbot Loading

matticbot commented Jun 13, 2024 • edited Loading

donnapep commented Jun 13, 2024 • edited Loading

Imran92 commented Jun 13, 2024

donnapep commented Jun 13, 2024

Imran92 commented Jun 13, 2024

donnapep commented Jun 13, 2024

Imran92 commented Jun 14, 2024

gabrielcaires left a comment

Choose a reason for hiding this comment

gabrielcaires Jun 14, 2024

Choose a reason for hiding this comment

Imran92 Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

gabrielcaires Jun 14, 2024

Choose a reason for hiding this comment

Imran92 Jun 14, 2024

Choose a reason for hiding this comment

gabrielcaires Jun 14, 2024

Choose a reason for hiding this comment

Imran92 Jun 14, 2024

Choose a reason for hiding this comment

gabrielcaires Jun 14, 2024

Choose a reason for hiding this comment

Imran92 Jun 14, 2024

Choose a reason for hiding this comment

donnapep commented Jun 17, 2024 • edited Loading

donnapep commented Jun 17, 2024

a8ci18n commented Jun 17, 2024 • edited Loading

a8ci18n commented Jun 23, 2024

Imran92 commented Jun 24, 2024

Imran92 commented Jun 24, 2024

donnapep commented Jun 24, 2024 • edited Loading

donnapep commented Jun 24, 2024

Imran92 commented Jun 25, 2024

donnapep commented Jun 25, 2024

donnapep commented Jun 25, 2024

Imran92 commented Jun 25, 2024 • edited Loading

donnapep left a comment

Choose a reason for hiding this comment

Imran92 commented Jun 13, 2024 •

edited by donnapep

Loading

github-actions bot commented Jun 13, 2024 •

edited by matticbot

Loading

matticbot commented Jun 13, 2024 •

edited

Loading

donnapep commented Jun 13, 2024 •

edited

Loading

Imran92 Jun 14, 2024 •

edited

Loading

donnapep commented Jun 17, 2024 •

edited

Loading

a8ci18n commented Jun 17, 2024 •

edited

Loading

donnapep commented Jun 24, 2024 •

edited

Loading

Imran92 commented Jun 25, 2024 •

edited

Loading