Migrate Shop parameters -> Order settings page #9007

rokaszygmantas · 2018-04-27T15:20:29Z

Questions	Answers
Branch?	develop
Description?	Migration of Shop parameters -> Order settings controller
Type?	new feature
Category?	CO
BC breaks?	no
Deprecations?	no
Fixed ticket?	n/a
How to test?	Compare with legacy controller

Important guidelines

Make sure your local branch is up to date before commiting your changes!
Your code MUST respect our Coding Standards (for code written in PHP, JavaScript, HTML/CSS/Smarty/Twig, SQL)!
Your commit name MUST respect our naming convention!

This change is

mickaelandrieu

Some few details needs to be fixed/improved, but it's good already

mickaelandrieu · 2018-04-30T12:56:48Z

src/Adapter/CMS/CMSDataProvider.php

+ */
+class CMSDataProvider
+{
+    public function getCMSPages($languageId)


would you mind to add PHPDoc for newly created classes? 👍

mickaelandrieu · 2018-04-30T13:00:04Z

src/PrestaShopBundle/Form/Admin/Configure/ShopParameters/OrderPreferences/GeneralType.php

+    {
+        $choices = [];
+
+        foreach ($this->cmsDataProvider->getCMSPages($languageId) as $cms) {


I'd like to see this function moved in CmsDataProvider instead, so you can inject the array instead of the full CmsDataProvider (like we already do to inject the locales).

mickaelandrieu · 2018-04-30T13:02:57Z

src/PrestaShopBundle/Form/Admin/Type/MoneyWithSuffixType.php

+use Symfony\Component\Form\FormView;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+/**


I believe this form type already exists... wdyt @Quetzacoalt91 ?

I couldn't find anything similar to this type - money type only adds the currency sign next to the input. This new type is used for situations like these: https://prnt.sc/jbyc7w

I'm pretty sure we already have this input type in Product page, isn't it?

how about this one? https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Form/Admin/Type/CustomMoneyType.php

@mickaelandrieu yes I've seen this one, but it's just an extension to money type, it's extending my new type as well. It doesn't have the functionality to enter a text suffix right after the currency sign so that's why I've created a new type

ok! fair enough then 👍

mickaelandrieu · 2018-05-03T08:56:17Z

src/PrestaShopBundle/Form/Admin/Type/MoneyWithSuffixType.php

+     */
+    private function applySuffix(&$value, $key, $suffix)
+    {
+        if (strlen($value) > 0) {


why do you need a $key here? You don't use it :)

It's because of array_walk where this method is used, array walk passes the value as the third argument so I must define the second argument as well. I don't like it, but is it worth changing array_walk to something custom because of that?

Add this explanation to PHPDoc please, this makes Codacy cry a river.

wdyt @PierreRambaud?

Yes, just add the explanation to PHPDoc.

mickaelandrieu

applySuffix have an extra parameter "key" which is not used.

mickaelandrieu · 2018-05-05T00:01:29Z

Hello, @rokaszygmantas this needs a rebase.

@PierreRambaud I chose you to fairly review work of @rokaszygmantas and @sarjon as I'm in holidays for 1 week!

done

src/Adapter/CMS/CMSDataProvider.php

@@ -0,0 +1,21 @@
+<?php


mickaelandrieu · 2018-05-10T15:25:27Z

src/PrestaShopBundle/Resources/config/services/form/form_type.yml

+        public: true
+        arguments:
+            - '@prestashop.adapter.data_provider.currency'
+            - '@=service("prestashop.adapter.data_provider.cms").getCmsChoices()'


don't we need to pass the language id to the function here?

the cms data provider internally uses CMS::listCms($idLang = null, $idBlock = false, $active = true),
it takes the default language if no argument is passed. In this case the default language should be used so there's no need to pass it as an argument
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/CMS.php#L166

rokaszygmantas · 2018-05-10T17:51:36Z

this PR is still work in progress - please don't merge it yet :)

mickaelandrieu · 2018-05-10T17:53:43Z

@rokaszygmantas what is missing here from a functional point of view?

rokaszygmantas · 2018-05-10T18:01:11Z

Gift options fieldset is still missing and also the links are still pointing to the legacy controller. I should finish it tomorrow :)

PierreRambaud · 2018-05-10T19:25:12Z

admin-dev/themes/new-theme/js/pages/order-preferences/terms-and-conditions-option-handler.js

+  handleTermsAndConditionsCmsSelect(isTosEnabled) {
+    const tosCmsSelect = $('#form_general_tos_cms_id');
+
+    if (isTosEnabled) {


Maybe you can replace the whole function by:

$('#form_general_tos_cms_id').prop('disabled', !isTosEnabled);

rokaszygmantas · 2018-05-11T13:54:28Z

@mickaelandrieu now all functionality of the legacy controller is migrated

mickaelandrieu

minor comment

mickaelandrieu · 2018-05-11T15:17:51Z

...le/Form/Admin/Configure/ShopParameters/OrderPreferences/OrderPreferencesFormDataProvider.php

@@ -82,11 +91,19 @@ public function setData(array $data)
            $data['general']['tos_cms_id'] = 0;
        }

+        // If gift wrapping tax rules group was not submitted - reset it to 0
+        if (!$data['gift_options']['gift_wrapping_tax_rules_group']) {


i prefer a condition using isset or array_key_exists please : more explicit

PierreRambaud · 2018-05-11T15:26:29Z

...le/Form/Admin/Configure/ShopParameters/OrderPreferences/OrderPreferencesFormDataProvider.php

+    {
+        $errors = [];
+        $invalidFields = [];
+        $purchaseMinimumValue = $data['general']['purchase_minimum_value'];


Maybe add some controls on $data['general'] and $data['gift_options']? wdyt @mickaelandrieu ?

yes, if these 2 keys don't exist or are null what happens?

anyway, this can be validated by QA.

sarjon · 2018-05-11T15:33:10Z

src/PrestaShopBundle/Form/Admin/Configure/ShopParameters/OrderPreferences/GeneralType.php

+        TranslatorInterface $translator,
+        array $locales,
+        CurrencyDataProvider $currencyDataProvider,
+        $tosCmsChoices


- $tosCmsChoices + array $tosCmsChoices

sarjon · 2018-05-11T15:34:12Z

src/PrestaShopBundle/Form/Admin/Configure/ShopParameters/OrderPreferences/GiftOptionsType.php

+        TranslatorInterface $translator,
+        array $locales,
+        CurrencyDataProvider $currencyDataProvider,
+        $taxChoices


- $taxChoices + array $taxChoices

marionf · 2018-05-16T08:34:30Z

Hello @rokaszygmantas

When I click on the "order" link in the breadcrumb

I have a 404 because it's the link of the legacy page

Otherwise everything is good :)

PierreRambaud · 2018-05-16T08:46:03Z

@marionf Bad link in breadcrumbs is maybe fixed in #9051 :)

marionf · 2018-05-16T08:50:38Z

Oh yes you're right, so it's all good for me :)

mickaelandrieu · 2018-05-16T09:17:31Z

and ... merged!

Good job @rokaszygmantas, and thanks everyone for reviews!

prestonBot added develop Branch Waiting for wording Status: action required, waiting for wording labels Apr 27, 2018

mickaelandrieu suggested changes Apr 30, 2018

View reviewed changes

mickaelandrieu added the Code reviewed label Apr 30, 2018

mickaelandrieu requested a review from Quetzacoalt91 April 30, 2018 13:05

mickaelandrieu added the Type: Feature label Apr 30, 2018

mickaelandrieu added this to the 1.7.5.0 milestone Apr 30, 2018

LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Apr 30, 2018

mickaelandrieu reviewed May 3, 2018

View reviewed changes

mickaelandrieu previously requested changes May 3, 2018

View reviewed changes

PierreRambaud reviewed May 5, 2018

View reviewed changes

src/Adapter/CMS/CMSDataProvider.php

@@ -0,0 +1,21 @@

<?php

This comment was marked as outdated.

Sign in to view

Migrate order preferences form's General fieldset

4b966c9

rokaszygmantas force-pushed the migration/shop_parameters-order_settings branch from 137d437 to 4b966c9 Compare May 10, 2018 11:41

rokaszygmantas added 4 commits May 10, 2018 14:55

Remove conflict syntax - oops

3fa13bb

Add missing docblocks and licenses

e92989d

Update method's php doc

536e119

Move TOS choices to CMS data provider adapter

fcb5bf4

mickaelandrieu reviewed May 10, 2018

View reviewed changes

mickaelandrieu previously approved these changes May 10, 2018

View reviewed changes

mickaelandrieu changed the title ~~[WIP] Migrate Shop parameters -> Order settings controller~~ Migrate Shop parameters -> Order settings page May 10, 2018

prestonBot added the Waiting for wording Status: action required, waiting for wording label May 10, 2018

mickaelandrieu added the Waiting for QA Status: action required, waiting for test feedback label May 10, 2018

mickaelandrieu added wip and removed Waiting for QA Status: action required, waiting for test feedback labels May 10, 2018

PierreRambaud reviewed May 10, 2018

View reviewed changes

rokaszygmantas added 3 commits May 11, 2018 16:43

Migrate gift options fieldset

3cb0fe1

Update link to the migrated controller

e2fec1e

Remove legacy order preferences controller

0ba2eeb

rokaszygmantas dismissed mickaelandrieu’s stale review via 0ba2eeb May 11, 2018 13:47

Remove deprecated parameter

3ea48d8

mickaelandrieu reviewed May 11, 2018

View reviewed changes

PierreRambaud reviewed May 11, 2018

View reviewed changes

sarjon reviewed May 11, 2018

View reviewed changes

rokaszygmantas added 2 commits May 14, 2018 09:56

Add array typehints

7586208

Change gift tax rules group validation condition to be more explicit

fe91e8d

mickaelandrieu removed the wip label May 15, 2018

mickaelandrieu approved these changes May 15, 2018

View reviewed changes

mickaelandrieu added the Waiting for QA Status: action required, waiting for test feedback label May 15, 2018

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 May 16, 2018

marionf added QA ✔️ Status: check done, code approved and removed Waiting for author Status: action required, waiting for author feedback labels May 16, 2018

mickaelandrieu approved these changes May 16, 2018

View reviewed changes

mickaelandrieu merged commit b14a802 into PrestaShop:develop May 16, 2018

rokaszygmantas deleted the migration/shop_parameters-order_settings branch May 16, 2018 10:52

mickaelandrieu mentioned this pull request Jun 26, 2018

Roadmap migration (1.7.5) PrestaShop/prestafony-project#10

Closed

40 tasks

mickaelandrieu mentioned this pull request Sep 5, 2018

Symfony migration roadmap for 1.7.5 #10301

Closed

40 tasks

matks added the migration symfony migration project label Sep 18, 2018

matks mentioned this pull request Sep 20, 2018

Migrate "Shop parameters > Order Settings > Order settings" page #10539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Shop parameters -> Order settings page #9007

Migrate Shop parameters -> Order settings page #9007

rokaszygmantas commented Apr 27, 2018 •

edited by eternoendless

Loading

mickaelandrieu left a comment

mickaelandrieu Apr 30, 2018

mickaelandrieu Apr 30, 2018

mickaelandrieu Apr 30, 2018

rokaszygmantas Apr 30, 2018

mickaelandrieu Apr 30, 2018

mickaelandrieu Apr 30, 2018

rokaszygmantas May 2, 2018

mickaelandrieu May 2, 2018

mickaelandrieu May 3, 2018

rokaszygmantas May 3, 2018

mickaelandrieu May 3, 2018

PierreRambaud May 5, 2018

mickaelandrieu left a comment

mickaelandrieu commented May 5, 2018

This comment was marked as outdated.

mickaelandrieu May 10, 2018

rokaszygmantas May 10, 2018

rokaszygmantas commented May 10, 2018

mickaelandrieu commented May 10, 2018

rokaszygmantas commented May 10, 2018

PierreRambaud May 10, 2018 •

edited

Loading

rokaszygmantas commented May 11, 2018

mickaelandrieu left a comment

mickaelandrieu May 11, 2018

PierreRambaud May 11, 2018

mickaelandrieu May 15, 2018

mickaelandrieu May 15, 2018

sarjon May 11, 2018

sarjon May 11, 2018

marionf commented May 16, 2018

PierreRambaud commented May 16, 2018

marionf commented May 16, 2018

mickaelandrieu commented May 16, 2018

Migrate Shop parameters -> Order settings page #9007

Migrate Shop parameters -> Order settings page #9007

Conversation

rokaszygmantas commented Apr 27, 2018 • edited by eternoendless Loading

Important guidelines

mickaelandrieu 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

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

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

mickaelandrieu left a comment

Choose a reason for hiding this comment

mickaelandrieu commented May 5, 2018

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokaszygmantas commented May 10, 2018

mickaelandrieu commented May 10, 2018

rokaszygmantas commented May 10, 2018

PierreRambaud May 10, 2018 • edited Loading

Choose a reason for hiding this comment

rokaszygmantas commented May 11, 2018

mickaelandrieu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marionf commented May 16, 2018

PierreRambaud commented May 16, 2018

marionf commented May 16, 2018

mickaelandrieu commented May 16, 2018

rokaszygmantas commented Apr 27, 2018 •

edited by eternoendless

Loading

PierreRambaud May 10, 2018 •

edited

Loading