Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checking stock levels on cart update, fixes issue #1190 #1221

Merged
merged 3 commits into from
Mar 19, 2014

Conversation

umpirsky
Copy link
Contributor

Fixes #1190.

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoooooooooooooooooooL.

@stloyd stloyd added this to the 1.0.0-BETA1 milestone Mar 18, 2014
<?php

/*
* This file is part of the Symfony package.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really man :trollface:

@winzou
Copy link
Contributor

winzou commented Mar 18, 2014

+1

{
public $message = '%stockable% does not have sufficient stock.';
public $stockablePath = 'stockable';
public $quantitytPath = 'quantity';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

@pjedrzejewski
Copy link
Member

@umpirsky This is quite important, would you mind adding a scenario or two to verify it works?

@pjedrzejewski
Copy link
Member

Checking for flash message on cart update would be enough!

@umpirsky
Copy link
Contributor Author

@pjedrzejewski I was thinking about that. But in order to reproduce this flash message, backorders need to be disabled. Is there a way to disable them?

@pjedrzejewski
Copy link
Member

We could disable it for test env, but this would require adjustments to other scenarios which assume backorders are enabled... Changing parameter in frozen container is not an option...

@umpirsky
Copy link
Contributor Author

@pjedrzejewski I know, but disabling it would require lot of work reworking other scenarios. Can we somehow allow disabling it dynamically?

@stloyd stloyd mentioned this pull request Mar 18, 2014
@stloyd
Copy link
Contributor

stloyd commented Mar 18, 2014

@umpirsky You can't do this easily via direct calls on container, you could try by introducing CompilerPass... but, why not simply create ENV variable and use it? =)

@@ -161,4 +161,11 @@
</property>
</class>

<class name="Sylius\Bundle\CartBundle\Model\CartItem">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be on CartBundle instead of CoreBundle ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it is inventory validation constraint. Cart bundle itself does not know about inventory at all. They are connected inside CoreBundle Otherwise we would couple Cart & Inventory.

@umpirsky
Copy link
Contributor Author

@stloyd Like this? Where did I go wrong?

@stloyd
Copy link
Contributor

stloyd commented Mar 19, 2014

@umpirsky Shouldn't this be (__ is replaced with .):

SetEnv SYMFONY__SYLIUS__INVENTORY__BACKORDERS_ENABLED true

@umpirsky
Copy link
Contributor Author

@stloyd Note that parameter in https://gist.github.com/umpirsky/a181b0f07dfa4bff3382#file-a-L11 is sylius.inventory.backorders.enabled.

@stloyd
Copy link
Contributor

stloyd commented Mar 19, 2014

@umpirsky TBH. I have never used this stuff, but maybe documentation give you a hint: http://symfony.com/doc/current/cookbook/configuration/external_parameters.html#environment-variables

@umpirsky
Copy link
Contributor Author

@stloyd I read it before. Cannot figure why this does not work. Maybe I put setEnv on wrong place.

@umpirsky
Copy link
Contributor Author

@pjedrzejewski We need this fix asap ^^

pjedrzejewski pushed a commit that referenced this pull request Mar 19, 2014
Checking stock levels on cart update, fixes issue #1190
@pjedrzejewski pjedrzejewski merged commit 36acfb3 into Sylius:master Mar 19, 2014
@pjedrzejewski
Copy link
Member

Shouldn't even have conflicts with the components branch. Thanks Sasha!

@umpirsky
Copy link
Contributor Author

Many thanks @pjedrzejewski!

Yes, about components, we now want to merge some changes into our project, but want to avoid merging components for some time. Can you give any advice on this?

@kayue
Copy link
Contributor

kayue commented Mar 19, 2014

Would cherry pick work?

@umpirsky
Copy link
Contributor Author

I guess, not sure how complicate will it be to merge components later. I gues no need to care.

@pjedrzejewski
Copy link
Member

Components are only namespace changes, but I understand it may be annoying to upgrade at specific stage of the project... I'll try to figure some solution for you (except tagging a release) before merging components.

@umpirsky umpirsky deleted the fix/issue-1190 branch March 21, 2014 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checking stock levels on cart update
6 participants