[Cart][Order] Merge Cart into Order #6134

Zales0123 · 2016-09-16T07:59:43Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Related tickets
License	MIT

It's a huge BC Break, I suppose one of the lasts before Sylius beta. It completely remove CartBundle and move its features into OrderBundle (and some to CoreBundle).

To-do:

fix some failing tests
remove/change documentation
rebase with master (it's going to be a nightmare 😄 💀 - EDIT: in fact, not at all)
some refactoring would be nice
consider naming (although we don't have Cart bundle and component, there are still some places where Cart concept is more adequate than Order)

pamil

There are still plenty of places where we are using "cart" in class names / methods names / services names. What should we do about them? I'm thinking about changing it to "order" where it makes sense. Would you like us to point them out to ease their renaming?

pamil · 2016-09-16T08:52:03Z

docs/bundles/SyliusCartBundle/models.rst

@@ -8,7 +8,7 @@ Cart

 You can access the cart total value using the ``->getTotal()`` method. The denormalized number of cart items is available via ``->getTotalItems()`` method.
 Recalculation of totals can happen by calling ``->calculateTotal()`` method, using the simplest possible math. It will also update the item totals.
-The carts have their expiration time - ``->getExpiresAt()`` returns that time and ``->incrementExpiresAt()`` sets it to +3 hours from now by default.
+The carts have their expiration time, which can be get with ``->getExpiresAt()`` method.


The documentation of Cart / CartBundle should be deleted as well.

I pointed it out in PR description, in To-do list ;)

pamil · 2016-09-16T08:57:19Z

src/Sylius/Bundle/CoreBundle/Controller/OrderController.php

+
+        $form = $this->resourceFormFactory->create($configuration, $resource);
+
+        if (in_array($request->getMethod(), ['POST', 'PUT', 'PATCH']) && $form->submit($request, !$request->isMethod('PATCH'))->isValid()) {


Probably just copied, but using true as third argument in in_array will make it use strict comparison (===) instead of the loose one (==).

Will be fixed in upcoming refactoring commits.

pamil · 2016-09-16T09:02:59Z

src/Sylius/Bundle/CoreBundle/spec/EventListener/CartBlamerListenerSpec.php

        UserEvent $userEvent,
        ShopUserInterface $user
    ) {
-        $cartContext->getCart()->willReturn($cart);
+        $cartContext->getCart()->willReturn('cart');


Returning a mock of Order's OrderInterface should work better.

pamil · 2016-09-16T09:03:08Z

src/Sylius/Bundle/CoreBundle/spec/EventListener/CartBlamerListenerSpec.php

        InteractiveLoginEvent $interactiveLoginEvent,
        TokenInterface $token,
        ShopUserInterface $user
    ) {
-        $cartContext->getCart()->willReturn($cart);
+        $cartContext->getCart()->willReturn('cart');


pamil · 2016-09-16T09:03:24Z

src/Sylius/Bundle/CoreBundle/spec/EventListener/UserCartRecalculationListenerSpec.php

@@ -54,14 +53,13 @@ function it_recalculates_cart_for_logged_in_user(
    function it_throws_exception_if_provided_cart_is_not_order(


Should be removed.

pamil · 2016-09-16T09:05:05Z

src/Sylius/Bundle/OrderBundle/Resources/config/services.xml

@@ -61,5 +63,27 @@
        </service>

        <service id="sylius.order_processing.order_processor" class="Sylius\Component\Order\Processor\CompositeOrderProcessor" />
+
+        <service id="sylius.listener.session_cart" class="%sylius.listener.session_cart.class%">


I'm wondering whether we should remove .class parameters in this PR or leave it for another one, it's up to you.

pamil · 2016-09-16T09:06:38Z

src/Sylius/Bundle/OrderBundle/spec/Form/Type/OrderTypeSpec.php

@@ -39,8 +39,19 @@ function it_is_a_form_type()

    function it_builds_form_with_items_collection_field(FormBuilderInterface $builder)
    {
-        $builder->add('items', 'collection', Argument::any())
-            ->willReturn($builder);
+        $builder


IMO writing specs for form types is just additional work without any profit, but I'm not gonna be lobbying it right now 🎉

You're without profit :(

lchrusciel · 2016-09-19T07:56:24Z

src/Sylius/Bundle/OrderBundle/Controller/OrderItemController.php

+            $cartManager->persist($cart);
+            $cartManager->flush();
+
+            $this->eventDispatcher->dispatchPostEvent('sylius.cart_item.post_create', $configuration, $newResource);


OrderItemController distpaches sylius.cart_item.post_create. Please, no.

Also, it would not be hardcoded

lchrusciel · 2016-09-19T07:59:36Z

src/Sylius/Bundle/OrderBundle/Controller/OrderItemController.php

+                return $this->redirectHandler->redirectToIndex($configuration, $newResource);
+            }
+
+            $cart = $this->getCurrentCart();


$cart -> $order

$this->getCurrentCart() -> $this->getCurrentOrder()

Not sure about it - we should consider naming, as in some points Cart is still more appropriate than Order - which is now means by Order in cart state, not by other entity.

lchrusciel

Finally, it would be united!

BTW. Shouldn't all translation file be removed and handled by Crowdin?

lchrusciel · 2016-09-19T08:34:02Z

src/Sylius/Bundle/OrderBundle/Controller/OrderItemController.php

     *
+     * @return Response
+     */
+    public function addAction(Request $request)


Is there any difference between cart item and an order item? If no, I would suggest to merge them together. So we should remove CartItemType and dispatch an order_item events here. We can stay with $cart and $order separation, but here I would vote to keep them together.

pamil · 2016-09-19T13:12:35Z

UPGRADE.md

@@ -3,6 +3,10 @@ UPGRADE

 ## From 0.19 to 1.0.0-alpha

+### CartBundle
+
+ * Merge ``Cart`` component and ``CartBundle`` into ``Order\Core`` component and ``Order\CoreBundle``.


Order\Core -> Order
Order\CoreBundle -> OrderBundle

In fact, some code must have been moved to Core also.

lchrusciel

Few things could be adjusted, but probably it would be easier to detect them in work.

Zales0123 · 2016-09-21T09:06:01Z

Ok, I would vote for merging this as it is now - it doesn't provide more mess than it was before, but at least we will have this huge BC Break behind 💃 . We should create an issue with all naming stuff that should be considered and change it step by step. This PR is already a big mess 😄

michalmarcinkowski · 2016-09-22T06:57:48Z

src/Sylius/Behat/Page/Shop/Cart/SummaryPage.php

@@ -264,7 +264,7 @@ protected function getDefinedElements()
            'cart_items' => '#sylius-cart-items',
            'cart_total' => '#sylius-cart-button',
            'clear_button' => '#sylius-cart-clear',
-            'coupon_field' => '#sylius_cart_promotionCoupon',
+            'coupon_field' => '#sylius_order_promotionCoupon',


Since all other selectors are using cart we should leave this one also, at least for now.

michalmarcinkowski · 2016-09-22T07:03:30Z

src/Sylius/Bundle/CoreBundle/Controller/OrderController.php

+            }
+
+            $this->getEventDispatcher()->dispatch(SyliusCartEvents::CART_CHANGE, new GenericEvent($resource));
+            $this->manager->flush();


These two lines should be moved above dispatchPostEvent

michalmarcinkowski · 2016-09-22T07:06:47Z

src/Sylius/Bundle/CoreBundle/Form/Type/OrderItemType.php

@@ -19,8 +19,6 @@
 use Symfony\Component\OptionsResolver\OptionsResolver;

 /**
- * Order item type.
- *
 * @author Paweł Jędrzejewski <pawel@sylius.org>
 */
 class OrderItemType extends BaseOrderItemType


Not sure if it's used anywhere, if not, it should be removed.

michalmarcinkowski · 2016-09-22T07:09:27Z

src/Sylius/Bundle/CoreBundle/Resources/config/services.xml

+            <argument type="service" id="sylius.custom_factory.order_item.inner" />
+            <argument type="service" id="sylius.variant_resolver.default" />
+        </service>
+        <service id="sylius.factory.cart_item" alias="sylius.custom_factory.order_item" />


We should consider if aliasing is a good option.

Probably not 😄 Done this way to make it mergable asap and should obviously considered in the nearest future.

michalmarcinkowski · 2016-09-22T07:14:50Z

src/Sylius/Bundle/OrderBundle/Controller/OrderItemController.php

+
+            $cartManager = $this->getCartManager();
+            $cartManager->persist($cart);
+            $cartManager->flush();


There should be also a post event dispatched

michalmarcinkowski · 2016-09-22T07:16:48Z

src/Sylius/Bundle/OrderBundle/Controller/OrderItemController.php

+        if ($request->isMethod('POST') && $form->submit($request)->isValid()) {
+            $newResource = $form->getData();
+
+            $event = $this->eventDispatcher->dispatchPreEvent('sylius.cart_item.pre_create', $configuration, $newResource);


There is no CartItem entity, so sticking to the Resource logic we should dispatch OrderItem pre_create event. Someone can have a listener to this event and it will not get triggered on add to cart action what is wrong.

michalmarcinkowski · 2016-09-22T07:19:16Z

src/Sylius/Bundle/OrderBundle/Form/Type/OrderItemType.php

@@ -44,10 +44,11 @@ public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('quantity', 'integer', [
-                'label' => 'sylius.form.order_item.quantity',
+                'attr' => ['min' => 1],
+                'label' => 'sylius.ui.quantity',


For forms labels we use sylius.form.form_type_name.field convention.

michalmarcinkowski · 2016-09-22T07:26:51Z

Good job Mateusz! 👍 Please apply my comments in a separate PR. Please also open an RFC where we will establish naming conventions for order and cart.

[Cart][Order] Merge Cart into Order

Zales0123 mentioned this pull request Sep 16, 2016

[Cart] Remove unneeded Cart methods #6024

Closed

pamil reviewed Sep 16, 2016

View reviewed changes

Zales0123 force-pushed the merge-cart-into-order branch 3 times, most recently from c4619ab to a87667b Compare September 16, 2016 11:53

Zales0123 added 8 commits September 16, 2016 14:00

[Cart] Remove unneeded Cart methods

6762e98

[Docs] Related docs changes

eedd707

[Cart][Order] Move all Cart classes to Order component

8de53eb

[Cart][Order] Move CartBundle classes to OrderBundle

a67f756

[Core][Order] Huge amount of fixes to make Sylius work

61c16f5

[Cart][Order] Move cart flashes translations

19694c1

[Core][Order] Some more small naming fixes

9757320

[Docs] Remove/change cart docs

e6ecad4

Zales0123 force-pushed the merge-cart-into-order branch 9 times, most recently from 421a974 to ea6d44a Compare September 16, 2016 14:54

Fix failing build

81babc8

Zales0123 force-pushed the merge-cart-into-order branch from ea6d44a to 81babc8 Compare September 17, 2016 11:49

Zales0123 changed the title ~~[WIP][Cart][Order] Merge Cart into Order~~ [Cart][Order] Merge Cart into Order Sep 19, 2016

Zales0123 mentioned this pull request Sep 19, 2016

[Cart] Small cart refactoring #6146

Closed

lchrusciel reviewed Sep 19, 2016

View reviewed changes

lchrusciel approved these changes Sep 19, 2016

View reviewed changes

Zales0123 force-pushed the merge-cart-into-order branch from 6fa631a to 8bbee29 Compare September 19, 2016 12:13

[Core][Order] Some more refactoring

238481b

Zales0123 force-pushed the merge-cart-into-order branch from 8bbee29 to 238481b Compare September 19, 2016 12:24

pamil reviewed Sep 19, 2016

View reviewed changes

lchrusciel approved these changes Sep 21, 2016

View reviewed changes

Arminek approved these changes Sep 21, 2016

View reviewed changes

michalmarcinkowski reviewed Sep 22, 2016

View reviewed changes

michalmarcinkowski merged commit cf65873 into Sylius:master Sep 22, 2016

Zales0123 mentioned this pull request Sep 22, 2016

[Core] Remove unneeded expired orders purger #6181

Merged

tchapi mentioned this pull request Oct 5, 2016

listeners for sylius.cart_item.pre_create are not called #6318

Closed

michalmarcinkowski mentioned this pull request Oct 6, 2016

[OrderBundle] Fix Add-to-cart Pre-create event #6323

Merged

Zales0123 deleted the merge-cart-into-order branch October 28, 2016 13:34

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#6134 from Zales0123/merge-cart-into-order

1740780

[Cart][Order] Merge Cart into Order

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cart][Order] Merge Cart into Order #6134

[Cart][Order] Merge Cart into Order #6134

Zales0123 commented Sep 16, 2016 •

edited

Loading

pamil left a comment

pamil Sep 16, 2016

Zales0123 Sep 16, 2016

pamil Sep 16, 2016

Zales0123 Sep 16, 2016

pamil Sep 16, 2016

pamil Sep 16, 2016

pamil Sep 16, 2016

pamil Sep 16, 2016

pamil Sep 16, 2016

Zales0123 Sep 19, 2016

lchrusciel Sep 19, 2016

lchrusciel Sep 19, 2016

lchrusciel Sep 19, 2016

lchrusciel Sep 19, 2016

Zales0123 Sep 19, 2016

lchrusciel left a comment

lchrusciel Sep 19, 2016

pamil Sep 19, 2016

Zales0123 Sep 21, 2016

lchrusciel left a comment

Zales0123 commented Sep 21, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski Sep 22, 2016

Zales0123 Sep 22, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski Sep 22, 2016

michalmarcinkowski commented Sep 22, 2016


		$form = $this->resourceFormFactory->create($configuration, $resource);

		if (in_array($request->getMethod(), ['POST', 'PUT', 'PATCH']) && $form->submit($request, !$request->isMethod('PATCH'))->isValid()) {

		@@ -54,14 +53,13 @@ function it_recalculates_cart_for_logged_in_user(
		function it_throws_exception_if_provided_cart_is_not_order(

[Cart][Order] Merge Cart into Order #6134

[Cart][Order] Merge Cart into Order #6134

Conversation

Zales0123 commented Sep 16, 2016 • edited Loading

pamil 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

lchrusciel 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

lchrusciel left a comment

Choose a reason for hiding this comment

Zales0123 commented Sep 21, 2016

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

michalmarcinkowski commented Sep 22, 2016

Zales0123 commented Sep 16, 2016 •

edited

Loading