Bugfix InventoryHandler to remove units of items that were removed from cart #842

Richtermeister · 2014-01-15T02:19:11Z

Currently the inventory handler only adjusts the units of items that are still in the cart,
but when an item is removed from the cart, the inventory unit remains. This PR fixes this.

…ed from the cart. Currently the inventory handler only adjusts the units of items that are still in the cart, but when an item is removed from the cart, the inventory unit remains. This PR fixes this.

winzou · 2014-01-15T03:20:48Z

Partially solves #567
It will fully solve it if you take into account that a same variant can appear in several OrderItems =)

Richtermeister · 2014-01-15T05:17:17Z

@winzou Sorry, I forgot all about your ticket.. see if the new implementation solves things better.

winzou · 2014-01-15T05:21:30Z

src/Sylius/Bundle/CoreBundle/OrderProcessing/InventoryHandler.php

@@ -113,9 +124,15 @@ protected function addInventoryUnits(OrderInterface $order, VariantInterface $va
    protected function removeInventoryUnits(OrderInterface $order, VariantInterface $variant, $quantity)
    {
        $units = $order->getInventoryUnitsByVariant($variant);
+        $counter = 0;
+
+        foreach ($units as $unit) {


You should use $units->slice(0, $quantity) to stay elegant here.

I was wondering that too.. can I always count on collections though? The interface only hints at an array..

@Richtermeister I guess you can, the Collection implements \Countable interface, so it's supported.

@stloyd \Countable is not what's needed. The slice() method is on the ArrayCollection class.. but I guess we should use collections properly when we have them.. I'll make that change.

@Richtermeister Your question was:

can I always count on collections though?

So my answer was correct =) But additionally, all Collections must implement Collection interface, which requires: slice() to be implemented.

@stloyd Ah, sorry, what that is meant to say is "Can I always rely on Collections being returned".. as in "I'm counting on you!" :)

When you look at the specs, for example, they work with arrays, so we'll have to switch those to Collections, if we want to go that route..

@Richtermeister Why still not using slice? Any real reason, or you just forgot =)

@stloyd Did not forget, but it requires updating all the specs to use ArrayCollection instead of array, and currently it's making the specs fail for me when I do that.. and, I'd love @pjedrzejewski to chime in, because he wrote the original specs with arrays..

Ok, let's skip it, I will create PR about this & fix specs then.

I'm trying to recall why I have used the arrays instead of the ArrayCollection. I'd prefer to use the Collection interface of course, but I think there was a reason why I did not in the first place...

Richtermeister · 2014-01-15T18:00:39Z

Just ran into the "hidden feature" where all the units passed to InventoryOperator::decrease() must belong to the same variant.. @pjedrzejewski do you think I can clean that up a little to accept a mixed collection? When you look at my previous commits here you see how easy it is to use this method incorrectly..

Richtermeister · 2014-01-15T18:42:25Z

@stloyd How about them apples? :)

Richtermeister · 2014-01-15T18:45:53Z

@pjedrzejewski Just for my better understanding, what is the purpose of creating inventory units before an order is completed?

stloyd · 2014-01-15T18:53:46Z

src/Sylius/Bundle/CoreBundle/OrderProcessing/InventoryHandler.php

+     */
+    private function updateVariantUnits(OrderInterface $order, VariantInterface $variant, $quantity)
+    {
+        $units = $order->getInventoryUnitsByVariant($variant);


As you get always Collection object, you can just call:

$quantityDifference = $quantity - $order->getInventoryUnitsByVariant($variant)->count();

winzou · 2014-01-16T02:15:50Z

Big 👍 on this PR, thanks @Richtermeister

Regarding our interfaces, I think yes it will be much easier if we can rely on a getId method! If one day a model actually uses another identifier, it will just have to implement the getId method to just return something unique. I see no issue here.

Richtermeister · 2014-01-16T19:48:15Z

@pjedrzejewski If there are no technical issues here, could we get this merged? The bug is otherwise affecting shipping calculations.. By the way, that is the reason for the early creation of inventory units, right? Being able to calculate shipping?

Also, can you please kick off the subtree split update again (ideally after merging this :).. I'm trying to deploy an app that has some dependencies.. Thanks!

Bugfix InventoryHandler to remove units of items that were removed from cart

pjedrzejewski · 2014-01-16T20:26:14Z

I'm fine with this solution, but we should somehow resolve the getId() method issue in another PR. And now @stloyd reminded me why we had the issue in the first place. There will be conflicts if 2 interfaces implement the same method. We could bump the requirement... but I'd prefer not to if there is any other solution.

@Richtermeister Thanks for the patch and I just started the sync. My script is crashing for some reason and I did not have time to figure it out. (I run it manually on my local)

Richtermeister · 2014-01-16T20:44:23Z

Thanks man! 😘

Bugfix InventoryHandler to remove units of items that have been remov…

35c9da6

…ed from the cart. Currently the inventory handler only adjusts the units of items that are still in the cart, but when an item is removed from the cart, the inventory unit remains. This PR fixes this.

Take into account multiple items with same variant.

930017e

winzou reviewed Jan 15, 2014
View reviewed changes

Reverting a change that broke the tests.

33c908c

Refactoring code to make it easier to follow.

f74b406

stloyd reviewed Jan 15, 2014
View reviewed changes

pjedrzejewski pushed a commit that referenced this pull request Jan 16, 2014

Merge pull request #842 from Richtermeister/bugfix-inventory-handler

ab5693b

Bugfix InventoryHandler to remove units of items that were removed from cart

pjedrzejewski merged commit ab5693b into Sylius:master Jan 16, 2014

Richtermeister deleted the bugfix-inventory-handler branch January 16, 2014 20:44

Richtermeister mentioned this pull request Jan 16, 2014

[Core] Handle better inventory units on cart change #567

Closed

winzou mentioned this pull request Jan 21, 2014

[RFC] Incorrect inventory tracking record #874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix InventoryHandler to remove units of items that were removed from cart #842

Bugfix InventoryHandler to remove units of items that were removed from cart #842

Richtermeister commented Jan 15, 2014

winzou commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

winzou Jan 15, 2014

Richtermeister Jan 15, 2014

stloyd Jan 15, 2014

Richtermeister Jan 15, 2014

stloyd Jan 15, 2014

Richtermeister Jan 15, 2014

stloyd Jan 15, 2014

Richtermeister Jan 15, 2014

stloyd Jan 15, 2014

pjedrzejewski Jan 15, 2014

Richtermeister commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

stloyd Jan 15, 2014

winzou commented Jan 16, 2014

Richtermeister commented Jan 16, 2014

pjedrzejewski commented Jan 16, 2014

Richtermeister commented Jan 16, 2014

Bugfix InventoryHandler to remove units of items that were removed from cart #842

Bugfix InventoryHandler to remove units of items that were removed from cart #842

Conversation

Richtermeister commented Jan 15, 2014

winzou commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

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

Richtermeister commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

Richtermeister commented Jan 15, 2014

Choose a reason for hiding this comment

winzou commented Jan 16, 2014

Richtermeister commented Jan 16, 2014

pjedrzejewski commented Jan 16, 2014

Richtermeister commented Jan 16, 2014