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

Add availibility property to ProductVariantView #508

Merged

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Aug 1, 2019

I misinterpredted how sylius checks the availibility of a product when implementing #503.

This will now use the correct availibilityChecker service of sylius.

@alexander-schranz alexander-schranz force-pushed the feature/product-availability-check branch 5 times, most recently from a3d79b2 to 4f26a16 Compare August 1, 2019 16:32
@alexander-schranz alexander-schranz marked this pull request as ready for review August 1, 2019 16:37
@alexander-schranz alexander-schranz requested a review from a team as a code owner August 1, 2019 16:37
@alexander-schranz alexander-schranz force-pushed the feature/product-availability-check branch from 165ec12 to ab5ca81 Compare August 1, 2019 18:47
@alexander-schranz
Copy link
Contributor Author

Anything missing from my side?

@lchrusciel
Copy link
Member

lchrusciel commented Aug 9, 2019

No, but both features were added after RC. I'm a little bit worried about adding too much of new functionalities before going to be stable. In default Sylius implementation, we are not presenting the available amount of products atm.

Also, onHand is just a part of the story, because we should take onHold into account as well.

To sum up, I would say, that #503 should be reverted and added after stable release. Probably just by adding available flag to products.

I have same problem with #502

@AndreasA
Copy link

AndreasA commented Nov 28, 2019

From what I can see at a quick glance this is still too simple because the availability checker has a second method to check for sufficient quantity. This has to be done when adding a product variant to the cart and when changing its quantity. There exists a constraint \Sylius\Bundle\CoreBundle\Validator\Constraints\CartItemAvailability but that one is only useful for `\Sylius\Bundle\OrderBundle\Controller\AddToCartCommandInterface' but it might be a good reference (as this is an add to cart the check has to take into account the existing quantity as that one gets added to it).

And \Sylius\Bundle\InventoryBundle\Validator\Constraints\InStock can be used for the quantity check in general which is already added to the Sylius\Component\Order\Model\OrderItem with the validation groups sylius and sylius_checkout_complete. Just not sure, if there is already a validation for this in the Shop API (would have to check).

@AndreasA
Copy link

One more suggestion: Instead of a boolean create a state, as users might want to differentiate between not in stock, low on stock and available.
In theory you could even add this information already by providing a low on stock parameter, then:
isStockAvailable with false would be not in stock, isStockAvailable with true and isSufficientQuantity called parameter value and returns falsewould below on stockand if both aretrueit would beavailable`.

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hey Alexander,

I think it is high time to merge it. Are you willing to rebase this branch?

@mamazu mamazu force-pushed the feature/product-availability-check branch from ab5ca81 to 5349869 Compare January 16, 2020 15:20
@mamazu
Copy link
Member

mamazu commented Jan 16, 2020

I have rebased the merge request to the master. This means that the onHand property was removed and replaced with the available property. For anyone watching please make sure to check the merge request for any references for the old property. (It was a big rebase with a lot of conflicts so I might have missed one or two instances)

@alexander-schranz

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up. Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "feature/product-availability-check" to update your local branch. Feel free to ask for assistance when you get stuck. +1

@mamazu mamazu force-pushed the feature/product-availability-check branch from 5349869 to 5a6b696 Compare January 16, 2020 15:33
@mamazu mamazu merged commit 2b36f2b into Sylius:master Feb 3, 2020
@mamazu
Copy link
Member

mamazu commented Feb 3, 2020

Thank you, Alexander! 🎉

@alexander-schranz alexander-schranz deleted the feature/product-availability-check branch February 3, 2020 08:56
@alexander-schranz
Copy link
Contributor Author

@mamazu Thank you for rebasing. Didn't got the time to fix the tests 🙈

@mamazu
Copy link
Member

mamazu commented Feb 3, 2020

No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants