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

Migrate "Improve > International > Localization > Geolocation" page #9176

Merged
merged 22 commits into from Jun 21, 2018

Conversation

Projects
None yet
7 participants
@sarjon
Member

sarjon commented Jun 8, 2018

Questions Answers
Branch? develop
Description? This PR migrates Geolocation controller to Symfony.
Type? improvement
Category? CO
BC breaks? yes (controllers)
Deprecations? no
Fixed ticket? no
How to test? Compare with legacy Geolocation page and it should work the same.

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 8, 2018

@mickaelandrieu or maybe @PierreRambaud waiting for you reviews. 👍

/**
* @param Configuration $configuration
*/
public function __construct(Configuration $configuration)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

Don't we have an interface for this one? If we have one, please use it and move this final class into Core.

This comment has been minimized.

@sarjon

sarjon Jun 12, 2018

Member

we do, but at the moment it it has only 2 methods get($key) and set($key, $value). In this case, im using getBoolean($key) method.

@mickaelandrieu

Good job so far, some refactoring expected

/**
* @param ConfigurationInterface $configuration
*/
public function __construct(ConfigurationInterface $configuration)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

same here, we may declare an interface in Core/Configuration and already move this to Core as we don't depend on anything related to legacy directly

if ($this->validateConfiguration($config)) {
$this->configuration->set(
'PS_GEOLOCATION_WHITELIST',
//preg_replace('/\r\n|\r|\n/', ';', $config['geolocation_whitelist'])

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

I don't like commented code... do you need to filter value from the request?

namespace PrestaShop\PrestaShop\Core\Geolocation\GeoLite;
use PrestaShop\PrestaShop\Core\ConfigurationInterface;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

ah! we have it :-)

/**
* @param ConfigurationInterface $configuration
*/
public function __construct(ConfigurationInterface $configuration)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

if you have an interface, always rely on it it's far better :)

*
* @return array
*/
public function getChoices()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

I think we have a pattern here :-) Go for the interface you have suggested last week ;)

DataConfigurationInterface $geolocationIpAddressWhitelistConfiguration,
DataConfigurationInterface $geolocationOptionsConfiguration,
GeoLiteCityCheckerInterface $geoLiteCityChecker,
Validate $validate

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

Would you mind to create an empty interface called ValidatorInterface into src/Core/Validation and make the adapter implements it?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

and rename the validate property to validator

}
return array_merge(
$this->geolocationByIpAddressConfiguration->updateConfiguration($data['geolocation_by_id_address']),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

the tests you are doing here... should be done in geolocationByIpAddressConfiguration class, am I right?

so in setData here, you should only have

return array_merge(
    $this->geolocationByIpAddressConfiguration->updateConfiguration($data['geolocation_by_id_address']),
    ...
);

This comment has been minimized.

@sarjon

sarjon Jun 13, 2018

Member

Well, setData() handles all form data, so if at least one field is invalid I don't want to save only valid fields but show error instead. If I were to delegate saving to each configuration service, there can be situation when 2 data configurators saves data and 1 fails due to validation.

Another option would be to use something like:

$errors = $this->geolocationByIpAddressConfiguration->validateConfiguration($data['geolocation_by_id_address']);
$errors += $this->geolocationOptionsConfiguration->validateConfiguration($data['geolocation_options']);

if (empty($errors)) {
    return array_merge(
          $this->geolocationByIpAddressConfiguration->updateConfiguration($data['geolocation_by_id_address']),
        ...
    );
}

return $errors;

but the method validateConfiguration() of contract DataConfigurationInterface does not really allow that:

    /**
     * @return bool Returns true if no exception are thrown
     */
    public function validateConfiguration(array $configuration);

We can get only true or false after validation, but not error messages. Could we update contract to this?

+ @return array Errors if any or empty array
- @return bool Returns true if no exception are thrown

What do you think?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

We don't want to change the interface here, or to be more precise, this won't really help.

The issue here is that the setData can save all valid operation until he get an invalid operation when we want to invalid all the operation if we have an error.

We can't update the interface we need to rely on another one which extends the first one. I don't see - for now - how we could use a new interface while keeping compatibility with current behavior :/

Let's keep it this way for now, I need to think about it.. we probably need an abstract class that implement the interface FormDataProviderInterface and add a fonction getErrors($config1, $config2, $config3) : array (empty or errors) without "update" the ones which are not valid ^^

*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

would you mind to disable the translator?

you can add choice_translation_domain => false for each ChoiceType or Symfony will translate the label twice (this is why we have a lot of missing translations in Product Page for instance)

use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\OptionsResolver\OptionsResolver;
class MaterialChoiceTableType extends AbstractType

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

add PHPDoc please

{% trans_default_domain 'Admin.International.Feature' %}
{% block geolocation_options %}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

if your block have the same name everywhere, how PrestaShop developers can override only one?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

it should probably be geolocation_ip_adress_whitelist, am I right?

/**
* Class CountryChoiceProvider is responsible for providing both enabled/disabled country choices with ISO code values
*/
final class CountryByIsoCodeChoiceProvider implements FormChoiceProviderInterface

This comment has been minimized.

@sarjon

sarjon Jun 13, 2018

Member

@mickaelandrieu not sure if it's the best place for choice providers, but i think think that developers should find all predefined choice providers in one place.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 13, 2018

Contributor

in PrestaShop\PrestaShop\Core\Form\ChoiceProviderplease :)

* @param {Event} event
*/
handleSelectAll(event) {
const $selectAllChecbox = $(event.target);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor
- $selectAllChecbox
+ $selectAllCheckboxes
* @param Configuration $configuration
*/
public function __construct(
Configuration $configuration

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

type hint on PrestaShop\PrestaShop\Core\ConfigurationInterface instead please :)

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

👍 use interface please

This comment has been minimized.

@sarjon

sarjon Jun 20, 2018

Member

i cant use ConfigurationInterface as it lacks some methods. :/
this is PR #9193 that we working on to add these methods. When this PR gets merged, ill update it to depend on interface.

/**
* @param Configuration $configuration
*/
public function __construct(Configuration $configuration)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

use PrestaShop\PrestaShop\Core\ConfigurationInterface instead.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

also I've noticed this interface doesn't implements all needed function... would you mind to:

  • create a new interface that extends the previous one, named AdvancedConfigurationInterface with the following functions: has, getInt, getBool, keys, all, remove, count.
  • deprecate ConfigurationInterface and remove all uses of it in the core, replace it by AdvancedConfigurationInterface
  • Remove all calls of Configuraration of Core namespace and replace it by the new interface.

Please do it in a specific commit, so it's easier to review :)

This comment has been minimized.

@sarjon

sarjon Jun 15, 2018

Member

no problem, ill do that. 👍

but why cant we extend current interface with new methods? Is there particular reason why AdvancedConfigurationInterface would be better ConfigurationInterface?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

because if someone is already using the current interface, once we will update it his code will break :D

This comment has been minimized.

@sarjon

sarjon Jun 15, 2018

Member

oh yeah. but maybe not deprecating ConfigurationInterface is also an option, since we dont always need these 'advanced' methods? anyway, we can discuss it on another PR. 👍

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

Yes, maybe... it's only a PHPDoc tag, don't add an error_log (you cant do it in interfaces anyway)

* @param CountryDataProvider $countryDataProvider
*/
public function __construct(
LegacyContext $legacyContext,

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

can you inject languageId instead?

}
/**
* @return FormHandler

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

type hint on interface

}
return array_merge(
$this->geolocationByIpAddressConfiguration->updateConfiguration($data['geolocation_by_id_address']),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 15, 2018

Contributor

We don't want to change the interface here, or to be more precise, this won't really help.

The issue here is that the setData can save all valid operation until he get an invalid operation when we want to invalid all the operation if we have an error.

We can't update the interface we need to rely on another one which extends the first one. I don't see - for now - how we could use a new interface while keeping compatibility with current behavior :/

Let's keep it this way for now, I need to think about it.. we probably need an abstract class that implement the interface FormDataProviderInterface and add a fonction getErrors($config1, $config2, $config3) : array (empty or errors) without "update" the ones which are not valid ^^

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

Also, you need to remove AdminGeolocationController, isn't it?

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 18, 2018

it is removed if you mean AdminGeolocationControllerCore.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

@sarjon yup! so you also need to remove the following unit test like described in migration guide => https://travis-ci.org/PrestaShop/PrestaShop/jobs/391733594#L942

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 18, 2018

@mickaelandrieu done! :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

Hello @sarjon I'll finish this one, thank you!

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 19, 2018

I'll finish this one

are you sure? i can rebase it and do other changes if needed.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

@sarjon it's only rebase stuff, as you like :)

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 19, 2018

waiting for QA?

/**
* Initialize component
*/
init() {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

use constructor instead of init call

],
'choice_translation_domain' => false,
])
->add('geolocation_countries', MaterialChoiceTableType::class, [

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

Why using a custom MaterialChoiceTableType and not ChoiceType like above?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 19, 2018

Contributor

It was discussed with @eternoendless, take a look at the discussion on Gitter :)

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 20, 2018

Contributor

Don't find anything on gitter :/

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 20, 2018

Contributor

aww maybe you don't have access to the history :/

{% include '@PrestaShop/Admin/Improve/International/Geolocation/Blocks/geolocation_ip_address_whitelist.html.twig' %}
</div>
</div>
{{ form_end(geolocationForm) }}

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

missing form_rest in case we extend form

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 19, 2018

Contributor

nice spotted, I always forgot them :/

@PierreRambaud

See my comments ;)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

One more rebase, comments to be adressed and go for QA testing 👍

Good job @sarjon !

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 20, 2018

rebased and @PierreRambaud comments addressed 👍

@PierreRambaud

This comment has been minimized.

This comment has been minimized.

Owner

sarjon replied Jun 20, 2018

🤦‍♂️ thanks. 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 20, 2018

form_rest and not form_reset or form_end.

in fact, I'm not sure this is required => see https://symfony.com/doc/3.4/reference/forms/twig_reference.html#form-end-view-variables

Looks like calling form_end calls form_rest ;)

But in this context I think it's a good idea 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 20, 2018

Dunno why but tests are broken ^^

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 20, 2018

Yes but we need to add form_rest, 1) for doing the same thing everywhere, 2) using it in block and sometimes proper div, in case form_rest doesn't contain only hidden fields :)

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 20, 2018

Dunno why but tests are broken ^^

can it fail just by accident? :/

comments have been fixed my friend

@prasanthSelvar

This comment has been minimized.

prasanthSelvar commented Jun 21, 2018

Hello @sarjon the help tool no longer appears when I click on it. It is link to this PR?
It's also appears for Delivery Slips page #9139 and Payment Method page #9192.

Thanks

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 21, 2018

it's not related at all, you can check that help poppin works well here => #9201

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 21, 2018

Thanks @sarjon !

@mickaelandrieu mickaelandrieu merged commit 1d812e8 into PrestaShop:develop Jun 21, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Jun 21, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment