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 comment in Customer registration form, add better error message #14138

Merged
merged 1 commit into from Jun 12, 2019

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Jun 10, 2019

Questions Answers
Branch? 1.7.6.x
Description? Add comment in Customer registration form, add better error message
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #13640
How to test? Try to register a new customer, you should see comments under Firstname and Lastname fields If trying to send an invalid value John R.Hamond a more detailed error message is displayed

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.6.0 milestone Jun 10, 2019
@jolelievre jolelievre requested a review from a team as a code owner June 10, 2019 12:14
@prestonBot prestonBot added 1.7.6.x Branch Improvement Type: Improvement Waiting for wording Status: action required, waiting for wording Needs port on StarterTheme labels Jun 10, 2019
@@ -27,7 +27,7 @@
{block name='form_errors'}
<ul>
{foreach $errors as $error}
<li class="alert alert-danger">{$error}</li>
<li class="alert alert-danger">{$error|strip_tags|nl2br nofilter}</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

why strip_tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be sure no html tag is inserted (since I use nofilter) only the converted \n remain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it, it will still work but I was worried of XSS injection 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

only if someone include xss in translations, but we should be worried if it happens ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright I'll remove it then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -170,7 +170,7 @@
>
{if isset($field.availableValues.comment)}
<span class="form-control-comment">
{$field.availableValues.comment}
{$field.availableValues.comment|nl2br nofilter}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PierreRambaud I also removed this strip_tag

PierreRambaud
PierreRambaud previously approved these changes Jun 11, 2019
Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

I don't like the concat but not other way from now 😭

@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Jun 11, 2019
@jolelievre
Copy link
Contributor Author

I don't like the concat but not other way from now 😭

Me neither, but if I return an array the error blocks are separated.. This would require to change too many templates and error management..

'Shop.Forms.Errors'
) . PHP_EOL .
$this->translator->trans(
'The following characters are not accepted: / \ ^ * `',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should tend towards shorter wordings, otherwise error notifications will be too present and discouraging. What about Invalid characters: /^*` instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems good to me, @Samuel-Pires wdyt?

'Shop.Forms.Errors'
) . PHP_EOL .
$this->translator->trans(
'The characters . and 。 are allowed only if they are directly followed by a space or the end of the string.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about the "。" character... it seems pretty specific, is it an absolute necessity to mention it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this character is indeed forbidden, but maybe you're right If someone tries to use this character it is probably in order to use the breach so maybe it's not useful to indicate it for usual users

@marionf
Copy link
Contributor

marionf commented Jun 11, 2019

@jolelievre some returns, seen with @Samuel-Pires and @TristanLDD

  1. Delete the message under the fields

capture d'écran_1664

  1. There are some other prohibited characters that are not accepted and not displayed in the message like: 0-9!<>,;?=+()@#"°{}_$%: we should add them

  2. Replace "The following characters are not accepted: " by "Invalid characters: "

  3. Replace "The characters . and 。 are allowed only if they are directly followed by a space or the end of the string." by "A space is needed after "." and "。""

@marionf marionf added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 11, 2019
@LouiseBonnard
Copy link
Contributor

@marionf, about the wording, you can also write A space is required after "." and "。" to give a sense of obligation.

@jolelievre
Copy link
Contributor Author

@marionf @LouiseBonnard @Samuel-Pires @TristanLDD
All your modifications have been addressed, this should be good unless you find other issues?

'Shop.Forms.Errors'
) . PHP_EOL .
$this->translator->trans(
'A space is required after . and 。',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be written with quotation marks? Example: A space is required after "." and "。"

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what is the best practice.

Copy link

Choose a reason for hiding this comment

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

Tristan was for the ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you mean using 'A space is required after "." and "。"'
(Isn't the space after 。 weird in this case?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: the space can't be removed, it's part of the character

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, then @PierreRambaud can you re-approve this PR please? ^^

Copy link
Contributor

@matks matks Jun 12, 2019

Choose a reason for hiding this comment

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

I think maybe some email browsers simplify them and convert them into regular . thus creating a clickable url

I confirm, try visiting https://www.apple。com 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

But... this PR has been merged without the fix on the wording?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, then @PierreRambaud can you re-approve this PR please? ^^

@LouiseBonnard we've been tricked !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups my bad, I was too quick and thought I had already added it
Here's a PR to fix this ^^
#14163

@LouiseBonnard LouiseBonnard added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Jun 11, 2019
@marionf marionf removed the Waiting for author Status: action required, waiting for author feedback label Jun 12, 2019
@marionf marionf added the QA ✔️ Status: check done, code approved label Jun 12, 2019
@Quetzacoalt91 Quetzacoalt91 merged commit 110e7da into PrestaShop:1.7.6.x Jun 12, 2019
@jolelievre jolelievre deleted the customer-name-infos branch July 12, 2019 00:06
@ghost ghost added the Front-end Category: Front end label Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.6.x Branch Front-end Category: Front end Improvement Type: Improvement QA ✔️ Status: check done, code approved Wording ✔️ Status: check done, wording approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants