Shipping method with pickup places - zasilkovna #6

boris-brtan · 2018-07-26T15:02:49Z

Q	A
Description, reason for the PR	we want to try extensibility of shopsys framework
New feature	Yes
BC breaks	Yes
Fixes issues	#3
Standards and tests pass	Yes/No
Have you read and signed our License Agreement for contributions?	Yes

LukasHeinz · 2018-07-29T20:05:55Z

Please add entry to changelog...

Miroslav-Stopka

My review is done. It was a little bigger than I expected. Please check it :)

Miroslav-Stopka · 2018-07-31T13:28:16Z

tests/ShopBundle/Database/EntityExtension/EntityExtensionTest.php

+//        $this->doTestExtendedEntityInstantiation(OrderItem::class, ExtendedOrderTransport::class, self::ORDER_TRANSPORT_ID);
+//        $this->doTestExtendedEntityInstantiation(OrderPayment::class, ExtendedOrderPayment::class, self::ORDER_PAYMENT_ID);
+//        $this->doTestExtendedEntityInstantiation(OrderProduct::class, ExtendedOrderProduct::class, self::ORDER_PRODUCT_ID);
+//        $this->doTestExtendedEntityInstantiation(OrderTransport::class, ExtendedOrderTransport::class, self::ORDER_TRANSPORT_ID);


Why is this code commented out? Is it on purpose? Or just some forgotten change?

it was a big mistake, forgotten changes after searching for tests failing.

Miroslav-Stopka · 2018-07-31T14:57:18Z

src/Shopsys/ShopBundle/Resources/views/Front/Content/Order/step3.html.twig

-                    {{ form_row(form.companyTaxNumber, { label: 'Tax number'|trans }) }}
-                </div>
-            </fieldset>
+            <div id="js-company-and-delivery-address-fields">


Why did you add this new div? Id js-company-and-delivery-address-fields is never used.

Miroslav-Stopka · 2018-07-31T15:09:43Z

src/Shopsys/ShopBundle/Resources/views/Front/Content/Order/step3.html.twig

+                    {{ form_row(form.deliveryAddressFilled, {
+                        attr: { class: 'js-checkbox-toggle', 'data-checkbox-toggle-container-id': 'js-delivery-address-fields' },
+                        label: 'I want to deliver products to different address than the billing one'|trans,
+                        disabled: pickupPlaceDelivery}) }}


pickupPlaceDelivery contains information about the fact if transport is already checked and this checked transport is pickUpPlace type. Am I right? If so, please consider renaming the variable like "isSelectedPickupPlaceTypeTransport". I mean, you should be capable to recognize the content of the variable only by variable name. Don't you think?

Miroslav-Stopka · 2018-08-06T06:57:53Z

tests/ShopBundle/Database/Model/Transport/TransportDomainTest.php

-use Shopsys\FrameworkBundle\Model\Transport\Transport;
-use Shopsys\FrameworkBundle\Model\Transport\TransportDataFactory;
-use Shopsys\FrameworkBundle\Model\Transport\TransportFactory;
+use Shopsys\FrameworkBundle\Model\Transport\TransportDataFactoryInterface;


I am kind of confused with your usage of interfaces. In a class VatFacadeTest you changed Shopsys\FrameworkBundle\Model\Transport\TransportDataFactory => Shopsys\ShopBundle\Model\Transport\TransportDataFactory. In this class TransportDomainTest you changed Shopsys\FrameworkBundle\Model\Transport\TransportDataFactory => Shopsys\FrameworkBundle\Model\Transport\ TransportDataFactoryInterface . Please, explain it to me? Why did you choose TransportDataFactoryInterface and sometimes just TransportDataFactory from ShopBundle?

My mistake,
should be the key as in services_test.yml, so Shopsys\FrameworkBundle\Model\Transport\ TransportDataFactoryInterface is correct.

Miroslav-Stopka · 2018-08-06T08:07:31Z

src/Shopsys/ShopBundle/Resources/scripts/frontend/validation/form/order.js

-                if ($orderPersonalInfoForm.find('#order_personal_info_form_deliveryAddressFilled').is(':checked')) {
+                var pickUpPlaceTransport = $orderPersonalInfoForm.find('.js-pick-up-place-hide-if-chosen').hasClass('display-none');
+                var deliveryAddressFilled = $orderPersonalInfoForm.find('#order_personal_info_form_deliveryAddressFilled').is(':checked');
+                if (deliveryAddressFilled && !pickUpPlaceTransport) {


Is this condition part !pickUpPlaceTransport necessary? Explain it to me.

If the shipping address is filled just from backend system, there is no need for checking whether it is valid.

Im not sure, if checking selection of pickup place by class display-none is the correct way. There must be some other way how you can't decide whether pickup place was chosen or not. An finally change pickUpPlaceTransport to isSelectedPickUpPlaceTransport

Miroslav-Stopka · 2018-08-06T09:15:33Z

src/Shopsys/ShopBundle/Model/Order/OrderDataMapper.php

+        return $orderData;
+    }
+
+    private function fillPickUpPlaceDeliveryData(OrderData $orderData, FrontOrderData $frontOrderData)


I see, shipping address form is disabled and that is the reason why u need to fill the delivery address from chosen pickupPlace. Am I right? But if you set the shipping address form on readonly instead of disabled, this method will be unnecessary. Correct?

after consultation, no change needed

Miroslav-Stopka · 2018-08-06T09:18:01Z

src/Shopsys/ShopBundle/Model/Order/OrderDataMapper.php

+            $orderData->companyNumber = null;
+            $orderData->companyTaxNumber = null;
+        }
+


What about:
$orderData = new OrderData();
parent:: getOrderDataFromFrontOrderData($orderData);

parent:: getOrderDataFromFrontOrderData($orderData)

public function getOrderDataFromFrontOrderData(FrontOrderData $frontOrderData) { $orderData = $this->orderDataFactory->create();

i tried to use use

/** @var \Shopsys\ShopBundle\Model\Order\OrderData $orderData */ $orderData = parent:: getOrderDataFromFrontOrderData($orderData);

Miroslav-Stopka · 2018-08-06T09:18:42Z

src/Shopsys/ShopBundle/Model/Order/OrderData.php

+    /**
+     * @var \Shopsys\ShopBundle\Model\PickUpPlace\PickUpPlace|null
+     */
+    public $pickUpPlace;


Missing constructor with default value.

Miroslav-Stopka · 2018-08-06T09:22:01Z

src/Shopsys/ShopBundle/Model/Order/Order.php

+        }
+        parent::__construct($orderData, $orderNumber, $urlHash, $user);
+    }
+


What about edit() method?

Miroslav-Stopka · 2018-08-06T09:23:45Z

CHANGELOG.md

+- [#6 - Shipping method with pickup places](https://github.com/shopsys/demoshop/pull/6)
+    - transport, Order entities and forms are extended
+    - PickUpPlace entity is added
+    - cronmodule for download of pick up places is created 


I think the description "Added new shipping method Zasilkovna. Pick up places downloaded by cron." would be enough. Im not sure that "transport, Order entities and forms are extended" is not too detailed for CHANGELOG.md

PetrHeinz · 2018-08-08T06:53:31Z

migrations-lock.yml

+    class: Shopsys\FrameworkBundle\Migrations\Version20180724105136
+    skip: false
+20180724060204:
+    class: Shopsys\FrameworkBundle\Migrations\Version20180724060204


Why add two migrations in a single commit?

Miroslav-Stopka · 2018-08-08T10:55:37Z

src/Shopsys/ShopBundle/Resources/views/Front/Content/Order/step3.html.twig

-                    {{ form_row(form.deliveryCountry, { label: 'Country'|trans }) }}
-                </div>
-            </fieldset>
+                <fieldset>


Indentation

Miroslav-Stopka · 2018-08-08T11:19:41Z

src/Shopsys/ShopBundle/Model/Transport/TransportDataFactory.php

+
+    /**
+     * @param \Shopsys\ShopBundle\Model\Transport\TransportData $transportData
+     * @param \Shopsys\ShopBundle\Model\Transport\Transport $transport


I think one of this param shoud be return.

Miroslav-Stopka · 2018-08-08T11:42:08Z

src/Shopsys/ShopBundle/Model/Order/OrderDataMapper.php

+        } else {
+            $orderData->companyName = null;
+            $orderData->companyNumber = null;
+            $orderData->companyTaxNumber = null;


Almost done, probably this "company" part is not necessary too.

Miroslav-Stopka · 2018-08-09T09:53:18Z

Hi @boris-brtan,

tests and review are done. I like it. Thank you for this new feature :)

- transport, Order entities and forms are extended - PickUpPlace entity is added - cronmodule for downloading of pick up places is created

boris-brtan force-pushed the bb-zasilkovna branch 5 times, most recently from 4b31c93 to f64a89f Compare July 27, 2018 08:25

boris-brtan force-pushed the bb-zasilkovna branch from f64a89f to dce2b8d Compare July 30, 2018 06:48

boris-brtan changed the title ~~shipping method with pickup places~~ Shipping method with pickup places Jul 30, 2018

boris-brtan force-pushed the bb-zasilkovna branch 4 times, most recently from 58c3b7e to 18ab5c9 Compare July 31, 2018 11:43

Miroslav-Stopka self-assigned this Jul 31, 2018

Miroslav-Stopka self-requested a review July 31, 2018 12:44

Miroslav-Stopka reviewed Aug 6, 2018

View reviewed changes

Miroslav-Stopka assigned boris-brtan and unassigned Miroslav-Stopka Aug 6, 2018

PetrHeinz reviewed Aug 8, 2018

View reviewed changes

boris-brtan force-pushed the bb-zasilkovna branch 5 times, most recently from 247c21a to 6e08203 Compare August 8, 2018 09:41

Miroslav-Stopka reviewed Aug 8, 2018

View reviewed changes

boris-brtan force-pushed the bb-zasilkovna branch 2 times, most recently from ad76f04 to e832f5f Compare August 8, 2018 13:27

boris-brtan force-pushed the bb-zasilkovna branch from e832f5f to 1b8f445 Compare August 9, 2018 14:02

shipping method functionality for pick up places is added

f992809

- transport, Order entities and forms are extended - PickUpPlace entity is added - cronmodule for downloading of pick up places is created

boris-brtan force-pushed the bb-zasilkovna branch from 1b8f445 to f992809 Compare August 9, 2018 14:08

boris-brtan merged commit b42e244 into master Aug 9, 2018

boris-brtan deleted the bb-zasilkovna branch August 9, 2018 14:20

boris-brtan mentioned this pull request Aug 9, 2018

Demoshop: Zasilkovna - dispensing points #3

Closed

LukasHeinz changed the title ~~Shipping method with pickup places~~ Shipping method with pickup places - zasilkovna Sep 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shipping method with pickup places - zasilkovna #6

Shipping method with pickup places - zasilkovna #6

boris-brtan commented Jul 26, 2018

LukasHeinz commented Jul 29, 2018

Miroslav-Stopka left a comment

Miroslav-Stopka Jul 31, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Jul 31, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Jul 31, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

boris-brtan Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 6, 2018

Miroslav-Stopka Aug 8, 2018

PetrHeinz Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka Aug 8, 2018

Miroslav-Stopka commented Aug 9, 2018

Shipping method with pickup places - zasilkovna #6

Shipping method with pickup places - zasilkovna #6

Conversation

boris-brtan commented Jul 26, 2018

LukasHeinz commented Jul 29, 2018

Miroslav-Stopka 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miroslav-Stopka commented Aug 9, 2018