Add on-hold inventory quantity #938
Conversation
| /** | ||
| * Set stock on hold. | ||
| * | ||
| * @return integer |
There was a problem hiding this comment.
Should be @param integer $onHold not @return =)
| ); | ||
| } | ||
|
|
||
| if ($payment::STATE_COMPLETED !== $payment->getState()) { |
There was a problem hiding this comment.
Send event only if the payment was completed?
ghost
assigned 
|
Looks great so far! |
|
Thanks for following this PR @winzou! Really appreciate it. |
| @@ -19,8 +19,9 @@ | |||
| <mapped-superclass name="Sylius\Bundle\CoreBundle\Model\Variant" table="sylius_variant"> | |||
| <field name="sku" column="sku" type="string" nullable="true" /> | |||
| <field name="price" column="price" type="integer" /> | |||
| <field name="onHold" column="on_hold" type="integer" nullable="true" /> | |||
There was a problem hiding this comment.
Please remove this, and put simple 0 in model class.
There was a problem hiding this comment.
Yes, I think we should have this defined as in other columns, so instead of nullable give 0 in model definition (actual line 45).
|
@winzou @stloyd Fixed reported comments, thanks. Now I think on my todo list is:
Sounds good? Did I miss something? /cc @kayue @pjedrzejewski |
|
I think the process of releasing inventory units can stay in the service Then the cron itself + the conditions to trigger it should be done in another PR. |
|
@winzou Yes, I was thinking about implementing |
| $units = $order->getInventoryUnitsByVariant($item->getVariant()); | ||
|
|
||
| foreach ($units as $unit) { | ||
| $unit->setInventoryState(InventoryUnitInterface::STATE_ONHOLD); |
There was a problem hiding this comment.
Uhm, it may actually be interesting to check state before setting it to on-hold.
I would add a condition STATE_CHECKOUT === $unit->getInventoryState() before changing the state + for counting the quantity for the inventoryOperator::hold just after.
What do you think?
The aim is to make it possible to trigger more than once this method without any risk (I'm thinking of viewing more than once the finalize step for example).
There was a problem hiding this comment.
Good catch. I think this can fix it https://gist.github.com/umpirsky/7bce762a0e8858a22aa4
It is just quantity that need to take care of, all items are going to on-hold anyway.
|
@winzou Not changing state, but removing inventory units. |
| * @return Symfony\Component\HttpFoundation\Response | ||
| * @throws Symfony\Component\HttpKernel\Exception\NotFoundHttpException | ||
| */ | ||
| public function releaseInventory() |
There was a problem hiding this comment.
@winzou I added this action for manual inventory release for order. But it removes onhold items, now I think according to https://github.com/Sylius/Sylius/wiki/Status it is not right. Should it update status from onhold to checkout? But it looks odd to have order in backend with inventory units in checkout status.
There was a problem hiding this comment.
@pjedrzejewski Except last question everything is pretty finished. Please review.
There was a problem hiding this comment.
Inventory units can be releases only if order is not paid yet, so so issue to set it back to check out
| $quantity--; | ||
| } | ||
|
|
||
| $unit->setInventoryState(InventoryUnitInterface::STATE_ONHOLD); |
There was a problem hiding this comment.
This is not good I think, this setter must be in the if. Imagine there are item already shipped (unit sold), then this would set it back to on hold...
Behavior is: we set to on hold every units that was checkout before.
Add on-hold inventory quantity
|
Thanks Sasha and company! 👍 |
|
@winzou @pjedrzejewski Last comments fixed in #952 |
#923