Fix customer address form #17526

zuk3975 · 2020-02-06T10:18:35Z

Questions	Answers
Branch?	1.7.7.x
Description?	After trying to fix addresses required fields and format relation in #17293 we found out it needs more work than expected, so this pr only fixes mandatory bugs and leaves the BO address form simple, without considering the required fields(in customer->addresses settings) or Address format (in country settings). The above mentioned relation of addressFormat and fields is planned to be reworked in 1.7.8
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	part of #17282, finishes #10558, improve #17549, fix #17032 #17549
How to test?	Customer address form in BO now should always render all fields as optional (= you can leave blank). Only firstname, lastname, address1, country and city fields are mandatory. We dont try to guess what fields are required or not based on country on whatever. We keep it simple 😄 NOTE I had to leave some of required fields check in BO anyway to ensure that it works like before and nothing breaks in FO.

This change is

PierreRambaud · 2020-02-10T10:33:05Z

src/Adapter/Form/ChoiceProvider/CountryStateByIdChoiceProvider.php

+                return [];
+            }
+
+            $states = State::getStatesByIdCountry($countryId, $resolvedOptions['only_active']);
            $choices = [];


$choices must be decalred above. If there is an exception in new Country, $choices will be undefined

I get what you mean it would be cleaner, but it shoudn't be a problem here since an exception is thrown in the catch block It will never reach the return $choices;

What if it's not a PrestaShopException 🤔

Then it's not caught by this class and either the process crashes or it's caught by something higher In both cases it still won't reach the return

Could you just move the return $choices after the foreach to have a cleaner code :D 🙏

PierreRambaud

see my comment

jolelievre

Nothing blocking for me But I always have concerns regarding this "Address" feature, it is mixed/common with brand addresses, manufacturer addresses But each one has been developed separately, so we fix bugs separately as well And the improvement are never ported from one to another
Since you said some improvement should be made in 178 about the customer address, maybe it would be worth refactoring all the address management so all rules are clear everywhere?

jolelievre · 2020-02-10T13:11:16Z

src/Adapter/Address/CommandHandler/SetRequiredFieldsForAddressHandler.php

@@ -46,7 +46,7 @@ final class SetRequiredFieldsForAddressHandler implements SetRequiredFieldsForAd
     */
    public function handle(SetRequiredFieldsForAddressCommand $command)
    {
-        $address = new Address();
+        $address = new CustomerAddress();


One question, how does it work for brand or manufacturer address? Because I believe they all share the same Address class But using CustomerAddress here means it's only dedicated to customer addresses, but not the other ones? Then shouldn't the handler name and namespace reflect this limitation?
=> SetRequiredFieldsForCustomerAddressHandler::handle(SetRequiredFieldsForCustomerAddressCommand $command)

Only customer address is affected 🤷‍♂️
Maybe as you say it could be reworked in 178.
It came from legacy that in db required_fields are saved by classname of the object (in this case CustomerAddress. By providing Address it doesn't work at all).

jolelievre · 2020-02-10T13:12:57Z

src/Adapter/Form/ChoiceProvider/CountryStateByIdChoiceProvider.php

+                return [];
+            }
+
+            $states = State::getStatesByIdCountry($countryId, $resolvedOptions['only_active']);
            $choices = [];


I get what you mean it would be cleaner, but it shoudn't be a problem here since an exception is thrown in the catch block It will never reach the return $choices;

zuk3975 · 2020-02-12T09:46:09Z

see my comment

moved var declaration up before try-catch

zuk3975 · 2020-02-13T11:46:52Z

Related issue: #17647

zuk3975 · 2020-02-14T09:20:12Z

@jolelievre, @PierreRambaud addressed.
also i put back checks for required DNI and POSTCODE fields in js
31f6e85 . Somehow those checks are essential only for these 2 fields so those cannot be always optional, but no other configurable requiredFields need it 🤷‍♂️

Robin-Fischer-PS · 2020-03-26T07:17:46Z

I tested again, with France and Spain addresses, both BO & FO are OK (except the "issue" which was already there in 176) !

Thanks @zuk3975 :)

zuk3975 · 2020-03-26T07:32:07Z

I tested again, with France and Spain addresses, both BO & FO are OK (except the "issue" which was already there in 176) !

Thanks @zuk3975 :)

Thank you, doing a great job!

Progi1984 · 2020-03-26T08:09:52Z

Hello @zuk3975, thanks for your contribution, would you mind to rebase your pull request? Thanks

PierreRambaud · 2020-03-26T08:31:57Z

Thanks @zuk3975

zuk3975 requested a review from a team as a code owner February 6, 2020 10:18

prestonBot added the 1.7.7.x Branch label Feb 6, 2020

zuk3975 mentioned this pull request Feb 6, 2020

[POC] Fix address required field and country address format relation #17293

Closed

prestonBot added Bug Type: Bug Waiting for wording Status: action required, waiting for wording labels Feb 6, 2020

zuk3975 changed the title ~~[WIP] Fix customer address form~~ Fix customer address form Feb 6, 2020

LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Feb 6, 2020

zuk3975 mentioned this pull request Feb 6, 2020

Finish @todo in 1.7.7 migrated code #17282

Closed

19 tasks

prestonBot added the Waiting for wording Status: action required, waiting for wording label Feb 7, 2020

MatShir mentioned this pull request Feb 7, 2020

Adresses page - Required Fields cannot be selected which occurs issues in the BO & the FO #17032

Closed

matks added the migration symfony migration project label Feb 7, 2020

matks previously approved these changes Feb 7, 2020

View reviewed changes

LouiseBonnard added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Feb 7, 2020

PierreRambaud reviewed Feb 10, 2020

View reviewed changes

PierreRambaud suggested changes Feb 10, 2020

View reviewed changes

jolelievre reviewed Feb 10, 2020

View reviewed changes

zuk3975 dismissed matks’s stale review via 48c5894 February 12, 2020 09:45

prestonBot added the Waiting for wording Status: action required, waiting for wording label Feb 12, 2020

zuk3975 mentioned this pull request Feb 12, 2020

The fields in the BO address form must be optional. #17549

Closed

LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Feb 13, 2020

zuk3975 force-pushed the address-form-fix branch from 48c5894 to e0ccead Compare February 14, 2020 08:52

PierreRambaud previously approved these changes Feb 14, 2020

View reviewed changes

zuk3975 dismissed PierreRambaud’s stale review via b917140 February 19, 2020 14:43

zuk3975 force-pushed the address-form-fix branch 3 times, most recently from 4e9edb7 to 90283c0 Compare February 19, 2020 14:51

PierreRambaud previously approved these changes Mar 24, 2020

View reviewed changes

Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label Mar 25, 2020

Robin-Fischer-PS added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 26, 2020

zuk3975 added 13 commits March 26, 2020 10:11

Add command validation for required fields

89882f3

Fix required fields saving object name

21fa8f6

Fix always rendering phone_mobile field, just checking if its required

891b92d

fix states provider to consider Country->contains_state

ce5fa23

remove fields togglers that depends from country

79efe5c

remove configurable requiredFields check from AddressType

1d94f32

apply cs fixer

e794626

declare var before trycatch

f63231e

revert js postcode and dni fields required checker

985e23d

fix wrong rendering after rebase

310dd1e

minor optimization

85b572e

remove unused attribute in form type

4858502

rebuild assets

8657aab

zuk3975 dismissed stale reviews from PierreRambaud and Progi1984 via 8657aab March 26, 2020 08:18

zuk3975 force-pushed the address-form-fix branch from bc20fdb to 8657aab Compare March 26, 2020 08:18

Progi1984 approved these changes Mar 26, 2020

View reviewed changes

PierreRambaud approved these changes Mar 26, 2020

View reviewed changes

PierreRambaud merged commit 7e8e1b7 into PrestaShop:1.7.7.x Mar 26, 2020

matks approved these changes Apr 2, 2020

View reviewed changes

khouloudbelguith mentioned this pull request Apr 3, 2020

[BOOM-4120] Required field for customer address are not in BO #9845

Closed

matks mentioned this pull request May 21, 2021

Migrate "Customers > Addresses > Add new / edit address" page #10558

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix customer address form #17526

Fix customer address form #17526

zuk3975 commented Feb 6, 2020 •

edited

Loading

PierreRambaud Feb 10, 2020

jolelievre Feb 10, 2020

PierreRambaud Feb 10, 2020

jolelievre Feb 10, 2020

PierreRambaud Feb 11, 2020

PierreRambaud left a comment

jolelievre left a comment

jolelievre Feb 10, 2020

zuk3975 Feb 12, 2020

jolelievre Feb 10, 2020

zuk3975 commented Feb 12, 2020

zuk3975 commented Feb 13, 2020

zuk3975 commented Feb 14, 2020 •

edited

Loading

Robin-Fischer-PS commented Mar 26, 2020 •

edited

Loading

zuk3975 commented Mar 26, 2020

Progi1984 commented Mar 26, 2020

PierreRambaud commented Mar 26, 2020

Fix customer address form #17526

Fix customer address form #17526

Conversation

zuk3975 commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreRambaud left a comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 commented Feb 12, 2020

zuk3975 commented Feb 13, 2020

zuk3975 commented Feb 14, 2020 • edited Loading

Robin-Fischer-PS commented Mar 26, 2020 • edited Loading

zuk3975 commented Mar 26, 2020

Progi1984 commented Mar 26, 2020

PierreRambaud commented Mar 26, 2020

zuk3975 commented Feb 6, 2020 •

edited

Loading

zuk3975 commented Feb 14, 2020 •

edited

Loading

Robin-Fischer-PS commented Mar 26, 2020 •

edited

Loading