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

Migration for Sell > Customers > Addresses create/edit forms #15300

Merged

Conversation

@RaimondasSapola
Copy link
Contributor

RaimondasSapola commented Aug 29, 2019

Questions Answers
Branch? develop
Description? Migrated for Sell > Customers > Addresses create/edit forms. Added new validators for customer email and country states and zip code format
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10558
How to test? You should see the migrated pages in Sell > Customers > Addresses and this link should be using the migrated page as well.

This change is Reviewable

@RaimondasSapola RaimondasSapola requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 29, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Aug 29, 2019

Hello @RaimondasSapola!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@RaimondasSapola

This comment has been minimized.

Copy link
Contributor Author

RaimondasSapola commented Aug 29, 2019

Waiting for #15229 to be merged for address required fields provider

@RaimondasSapola

This comment has been minimized.

Copy link
Contributor Author

RaimondasSapola commented Aug 29, 2019

Also question for @matks remains about if something should be done about #15180 (comment) issue

'validation_groups' => function () {
$groups = ['Default'];

//TODO add required fields as validation groups

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 15, 2019

Author Contributor

@matks i have a question about adding new dataProvider for this case. In controller i use queryBus to fetch address required fields. It is a simple array of strings with names of required fields. For this form i need this array to add validation groups and required flags based on the same array.

Would it be a good idea to create a service RequiredFieldsProvider with interface witch runs the same query and this service here and at the Controller level to get required fields?

Or get rid of validation groups and pass required values as form options and use if statements to determine if fields are required?

What do you think?

This comment has been minimized.

Copy link
@matks

matks Nov 15, 2019

Contributor

So, we should try to avoid dispatching too many queries for the same HTTP request.

CQRS ideal usecase is: "you receive one HTTP request, you dispatch 1 query and/or 1 command". That's the ideal usecase. One usecase = 1 query and/or 1 command. So if it's 1 query, then we're supposed to ask, in the query, for all the data we need to display and get it all together.

That is why I suggest the following, which is based on your idea:

  • create this RequiredFieldsProvider
  • use this RequiredFieldsProvider inside GetCustomerAddressForEditingHandler and return the required fields data into the EditableCustomerAddress so that AddressFormDataProvider can retrieve it and pass this data to the form so it can be validated correctly
  • you can also use this RequiredFieldsProvider for the 2nd query (the ones you use in controller to get the data)

Is this alright for you ? If we perform a GetCustomerAddressForEditing query, it means we're asking the Domain for "all the data I need to perform an editing operation for customer address". If the "required fields" are needed to perform an editing operation they should be part of the Query response.

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 15, 2019

Author Contributor

Required fields are also needed for create operation so returning them with EditableCustomerAddress only solves half the problem. Or maybe for edit form i could get all required fields with EditableCustomerAddress and for create i can pass required fields as data to form?

@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:M/Customers/AddressesForms branch from 3a067a9 to f2e01a6 Nov 15, 2019
@RaimondasSapola RaimondasSapola changed the title [WIP] Migration for Sell > Customers > Addresses create/edit forms Migration for Sell > Customers > Addresses create/edit forms Nov 15, 2019
$dni = isset($value['dni']) ? $value['dni'] : null;

/** @var CountryRequiredFields $requiredFields */
$requiredFields = $this->queryBus->handle(new GetCountryRequiredFields($countryId));

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 18, 2019

Author Contributor

So @matks we talked about required fields provider service and we rejected it because for create and edit cases data could be passed in data providers but i totally forgot abut validation case. So here i see two solutions. Use required fields provider service or pass required fields from form as constraint option. Wdyt?

This comment has been minimized.

Copy link
@matks

matks Nov 18, 2019

Contributor

What about this:

  • edit case: we retrieve the required fields from Query
  • create case: we retrieve the required fields from Query too
  • validation: we use a Provider

And internally, the QueryHandlers use the Provider that has been injected into them. Does it make sense ?

I say this because I think so far, all of the usecases where we needed data to perform validation was retrieved through an Adapter 🤔 I hope I'm not wrong ?

}

try {
$this->queryBus->handle(new GetCustomerForAddressCreation($value));

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 22, 2019

Author Contributor

@matks another situation with queryBus in validator :( To have a core interface for this one case seems a bit useless. Maybe queryBus is not so bad here?

$country = new Country($countryIdValue);
} catch (PrestaShopException $e) {
throw new CountryNotFoundException(
sprintf('Country with id "%s" was not found.', $countryIdValue)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Nov 22, 2019

Contributor

Could be another exception, the message must be more generic or use the $e->getMessage().
Otherwise you can do this to prevent duplicate messages.

try {
    $country = new Country($countryIdValue);
} catch (PrestaShopException $e) {
}

if (empty($country->id) || $country->id !== $countryIdValue) {
    throw new CountryNotFoundException(
        sprintf('Country with id "%s" was not found.', $countryIdValue)
    );
}
*
* @return NotBlank|null
*/
private function getNotBlackOrNull(string $field, array $requiredFields): ?NotBlank

This comment has been minimized.

Copy link
@matks

matks Nov 28, 2019

Contributor
Suggested change
private function getNotBlackOrNull(string $field, array $requiredFields): ?NotBlank
private function getNotBlanckOrNull(string $field, array $requiredFields): ?NotBlank

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Nov 29, 2019

Contributor

getNotBlankOrNull is the correct wording 🙂

This comment has been minimized.

Copy link
@matks

matks Nov 29, 2019

Contributor

Indeed 😅

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 28, 2019

OK @RaimondasSapola I've discussed this issue with @eternoendless

Summary

In this PR, there is 3 custom validators used to validate the Symfony form:

  • CustomerAddressCountryRequiredFieldsValidator which uses an adapter CountryRequiredFieldsProviderInterface to get the data it needs to validate the input data it is given
  • CustomerAddressZipCodeValidator which uses an adapter CountryZipCodeRequirementsProviderInterface to get the data it needs to validate the input data it is given
  • ExistingCustomerEmailValidator which (currently) uses a dispatched GetCustomerForAddressCreation Query to get the data it needs to validate the input data it is given

About the usage of CQRS in the app layer

I confirmed with @eternoendless that Commands and Queries must be used in order to address user-oriented usecases. This means "user needs to see a dashboard with data A, B and C" => it's a Query. "User needs to modify data X state" => it's a Command.

However here the user does not want to validate the form (when the form is submitted). The system needs to validate the form prior to executing the true intent of the user which is to persist the data provided by the user.

So the answer is: we will not use Queries nor Commands to retrieve the data needed by the validators, but instead some classes, we can label them as data providers in charge of providing this data. These data providers are read-only.

About this PR and the implementation

For this PR, it means:

  • CustomerAddressCountryRequiredFieldsValidator => it's good as it relies on an interface with an adapter implementation (the adapter will later be removed and replaced by a doctrine repository implementation)
  • CustomerAddressZipCodeValidator => it's good as it relies on an interface with an adapter implementation (the adapter will later be removed and replaced by a doctrine repository implementation)
  • ExistingCustomerEmailValidator => we need to remove the use of the QueryBus here and replace it with a call to a data provider

The data provider must be an interface. See examples PrestaShop\PrestaShop\Core\Localization\Currency\RepositoryInterface, PrestaShop\PrestaShop\Core\Localization\Currency\DataSourceInterface.

And the interface can be implemented by either

  • an adapter class that rely on ObjectModel (this solution must be chosen if 1) there is already an ObjectModel that can provide this data with an existing function or 2) using the ObjectModel allows us to implement the function with very few lines of code)
  • a Core class that rely on Doctrine DBAL to perform SQL queries (this solution must be chosen if previous usecase does not apply)
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 28, 2019

@RaimondasSapola Apart from that, I also reviewed the PR and I found only 1 issue (see above): getNotBlackOrNull => getNotBlanckOrNull

@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:M/Customers/AddressesForms branch from e573aff to cc7d738 Nov 29, 2019
Copy link
Contributor

jolelievre left a comment

Quite a lot of feedbacks (sorry ^^) I like many of those modifications though, I'm sure most of them could be useful for manufacturer and supplier addresses (any adresses actually) This would probably be worth it to have a generic AddressFormType with a consistent management around it
To avoid having to duplicate the code and I'm sure that with this separated implementation when one of the form is fixed the other ones are not.. that's too bad

*/
protected function getAddress(AddressId $addressId)
protected function getAddress(AddressId $addressId): Address

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 9, 2019

Contributor

Since this class was introduced in 176 you shouldn't change the type here, or this will cause a BC for classes that would have inherited it without the strong typing
We only use strong typing with new classes/methods

src/Adapter/Address/AbstractAddressHandler.php Outdated Show resolved Hide resolved
@atomiix atomiix force-pushed the RaimondasSapola:M/Customers/AddressesForms branch from cc7d738 to a0eece6 Dec 17, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 18, 2019

@atomiix please tell us when we can review again ?

@atomiix

This comment has been minimized.

Copy link
Contributor

atomiix commented Dec 19, 2019

@atomiix please tell us when we can review again ?

@matks It's ready to be reviewed! The only part that may be useless when create order will be merged is that file.

@atomiix atomiix force-pushed the RaimondasSapola:M/Customers/AddressesForms branch from a0eece6 to 05ed82a Dec 20, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 2, 2020

I checked, all review feedbacks have been adresses excluding #15300 (review)

As #15300 (review) is in discussion, I think we can move on.

@matks matks added the waiting for QA label Jan 2, 2020
@matks
matks approved these changes Jan 2, 2020
@sarahdib sarahdib removed the waiting for QA label Jan 3, 2020
@matks matks dismissed jolelievre’s stale review Jan 3, 2020

Requested changes have been applied

@matks matks added the waiting for QA label Jan 3, 2020
@sarahdib sarahdib self-assigned this Jan 3, 2020
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jan 6, 2020
@sarahdib sarahdib added this to the 1.7.7.0 milestone Jan 6, 2020
@jolelievre jolelievre merged commit d9d146d into PrestaShop:develop Jan 6, 2020
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.