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 check for required fields in BO > Country > Address format #15428

Merged

Conversation

@matthieu-rolland
Copy link
Contributor

matthieu-rolland commented Sep 5, 2019

Questions Answers
Branch? develop
Description? When editing the address format for a country, check that all required address fields are present. This prevents the address form in the frontend from throwing an exception because of missing required fields.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9812
How to test? In BO > International > Zones geo > Country, change address format, remove a required field (for ex: firstname, lastname, or Country:name), you should see an error message stating that this field is required.

This change is Reviewable

@matthieu-rolland matthieu-rolland requested a review from PrestaShop/prestashop-core-developers as a code owner Sep 5, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Sep 5, 2019

@LouiseBonnard , when removing a field that is required from the Address Format form, it will display this error message: The field %s is required.

This is the form I'm talking about:

Capture d’écran 2019-09-05 à 17 07 08

I used this message because it already exists and I didn't find anything more suitable, would you have any idea for a better message ?

Copy link
Contributor

jolelievre left a comment

Copy link
Contributor

PierreRambaud left a comment

Little lint issue

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-address-format branch from bed5a79 to 3bb5cc1 Sep 19, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Sep 23, 2019

Hello @matthieu-rolland
There is a little problem
When you delete for exemple firstname save it's Ok
When you add by clicking on firstname and save the message is still appear:

https://drive.google.com/file/d/1dtZ4dZk1sLztqo2FvfeaJ8ANdJ_SXh79/view

@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Sep 27, 2019

Hello @matthieu-rolland
There is a little problem
When you delete for exemple firstname save it's Ok
When you add by clicking on firstname and save the message is still appear:

https://drive.google.com/file/d/1dtZ4dZk1sLztqo2FvfeaJ8ANdJ_SXh79/view

That's normal behaviour, because customer:lastname is not the same as lastname, you can add lastname from the Address tab.

As seen with @sarahdib , that's not really clear for the end user.

Ping @colinegin @marionf , I suggest we add a little text above this form, stating the required fields from the Address tab, so that the user doesn't try to add the required lastname from another tab for example. What do you think ?

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-address-format branch from 3bb5cc1 to a7b7408 Sep 27, 2019
@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Sep 27, 2019

@matthieu-rolland

I suggest we add a little text above this form, stating the required fields from the Address tab

Why not, but we should also handle the case where the user add customer:lastname and also display an error if it's required

@marionf marionf removed the waiting for PM label Sep 27, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Sep 30, 2019

@matthieu-rolland

I suggest we add a little text above this form, stating the required fields from the Address tab

Why not, but we should also handle the case where the user add customer:lastname and also display an error if it's required

@marionf
customer:lastname is not in the list of required fields, it's just that since it's in the first tab, after seeing the error message saying that the address is required, people would click on it thinking that this is the required field. (this is what induced @sarahdib 's misunderstanding and I'm afraid the end users will have the same misunderstanding).

Ok, I'll do that !

@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Sep 30, 2019

As seen with @colinegin , we will precise the tab of the required field in the error message, and change the text above the address format field.

return $fieldTab[0];
}
return false;

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Sep 30, 2019

Author Contributor

@jolelievre @PierreRambaud

I have doubts on how to deal with this (when this function returns false).

"Normally" it can never happen, but I wanted the method to be abnormal-proof. Do you think I should throw some kind of exception, in the legacy?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 1, 2019

Member

As a general rule, methods should do what they are expected to do, or throw an exception otherwise. Return values should only reflect "correct" and "expected" states in normal execution, all other situations should be considered exceptions.

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Oct 1, 2019

Author Contributor

Thanks for your input @eternoendless 👍

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-address-format branch from e9b5ef8 to f9632ac Sep 30, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-address-format branch from a2976d5 to 449162e Oct 1, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 9, 2019

Hi,

This PR is working fine, but I'm not sure about the wording and/or position of this message :
"Some countries require different elements than others. Click on the button below to get the valid default address format for this country. "
Since there are no less than 4 buttons under this message, + the block to add parts of adress, I wonder if the end user will easily understand which button this message is pointing to...

pr15428 not clear 1

What do you think @TristanLDD @LouiseBonnard @colinegin ?

Thanks :)

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Oct 10, 2019

Hi @Robin-Fischer-PS, you're right and it is supposed to be corrected with the migration, cf. this PR.

@@ -32,7 +32,7 @@
<textarea id="ordered_fields" name="address_layout" style="height:150px;">{$input.address_layout}</textarea>
</div>
<div class="col-lg-8">
{l s='Required fields for the address (click for more details):' d='Admin.International.Feature'}
{l s='Some countries require different elements than others. Click on the button below to get the valid default address format for this country.' d='Admin.International.Feature'}

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Oct 14, 2019

Contributor
Suggested change
{l s='Some countries require different elements than others. Click on the button below to get the valid default address format for this country.' d='Admin.International.Feature'}
{l s='Some countries require different elements than others. Click the button below to get the valid default address format for this country.' d='Admin.International.Feature'}
@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Oct 14, 2019
@matthieu-rolland matthieu-rolland merged commit 83c088b into PrestaShop:develop Oct 14, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.