search

Sylius / ShopApiPlugin

Shop API for Sylius.
https://sylius.com
129 stars 89 forks source link

Add validation for disabled product or product variant on ShopApiPlugin #684

Closed NenadStef closed 3 years ago

NenadStef commented 4 years ago

@lchrusciel @TiMESPLiNTER

Hi Lukasz

As we talked some time ago I implemented two new constraints on ShopApiPlugin.

First constraint is for checking wether Product is enabled or disabled and the second one is doing the same for product variant. They are triggered on adding item in the cart. I also added two spec tests for this two new validators.

NenadStef commented 4 years ago

@lchrusciel Thanks, I will add tests and for that second PR on which response do you think?

diimpp commented 4 years ago

Another case -- when product was added to cart as eligible, but become ineligible over time and customer went to checkout/complete step.

Those cases are the same as out of stock and require same implementation. Might as well check for them too

NenadStef commented 4 years ago

@lchrusciel @diimpp I have a bit of situation with running integration tests.

Because in composer.json is set that sylius/sylius must be ˆ1.4 running composer install will install sylius version 1.8 which will cause a lot of errors because of SonataCoreBundle which don't exists in 1.8 version. This can be fix by modifying test/Application/config/bundles.php by adding new bundles but in that case this pull request should be strictly for versions 1.8 and higher.

If I run integration tests with sylius version 1.4 I don't have bundles problem but this pull request has a problem because in 1.4 version there is no method isEnabled on ProductVariant.

What would be your suggestion?

mamazu commented 4 years ago

It indeed looks like we don't have any tests in the matrix for the most recent version of ShopApi only up to 1.6. We probably should update it from the Plugin Skeleton. But this is a different issue.

Hm, if it is not defined in 1.4 we might need to raise the minimum required version of the sylius/sylius package.

NenadStef commented 4 years ago

@lchrusciel @diimpp @mamazu

This pull request is set strictly to run with sylius/sylius ˆ1.8 so I did some modification to tests/Application/config files and bundles.php. All unit and integration tests are run successfully for this pull request.

As for product and product variant enabled/disabled functionality next cases are covered:

  1. On adding item to the cart validation will be triggered to check if product or product variant is enabled or disabled
  2. On changing quantity for order item validation will be triggered to check if product or product variant is enabled or disabled
  3. On cart checkout validation will be triggered to check if product or product variant is enabled or disabled

This validators are covered with unit and integration tests.

In each of this new validators there must be a check to see if object for which we are running query is found otherwise exception would be thrown. This validators do not handle missing objects because that is a job for other validators.

Take a look at this pull request when you got time and tell me what you think about it.

mamazu commented 4 years ago

We are currently planning a release for ShopAPI with support for 1.8 and onwards (#688). If you can wait with this pull request then you should be able to just rebase the branch onto the new master and it should work.

And this would make it easier to review your pull request as then it only contains the code for the validation.

NenadStef commented 4 years ago

@mamazu When do you expect to release new version of the ShopApiPlugin that will be compatible with sylius/sylius 1.8 so we can plan whether to wait for the release and this pull request or just to use our fork.

mamazu commented 4 years ago

As far as I know @lchrusciel is working on it currently with an estimated release date to be next week-ish.

NenadStef commented 3 years ago

@TiMESPLiNTER @lchrusciel @mamazu @diimpp

Regarding previous work that was done on this branch and also comments on https://github.com/Sylius/ShopApiPlugin/issues/683 this is a quick summary what is done so far:

New constraints

  1. On AddCouponRequest

    • CartEmpty constraint

  2. On ChangeItemQuantityRequest

    • CartEmpty constraint
    • CartItemEligibility constraint

  3. On EstimateShippingCoastRequest

    • CartEmpty constraint

  4. On PutOptionBasedConfigutableItemToCartRequest

    • ProductEligibility constraint

  5. On PutSimpleItemToCartRequest

    • ProductEligibility constrain

  6. On PutVariantBasedConfigutableItemToCartRequest

    • ProductEligibility constrain
    • ProductVariantEligibility constrain

  7. On AddressOrderRequest

    • CartEmpty constraint

  8. On ChoosePaymentMethodRequest

    • CartEmpty constraint

  9. On CompleteOrderRequest

    • CartEmpty constraint
    • CartEligibility constraint

All this validators and cases are covered with unit and integration tests.

TODO: Filter disabled products and product variants from responses on

  1. sylius_shop_api_product_show_latest
  2. sylius_shop_api_product_show_catalog_by_slug
  3. sylius_shop_api_product_show_catalog_by_code

Once this branch is updated with updated version that is compatible with sylius 1.8 you will be able to do a proper code review.

lchrusciel commented 3 years ago

v1.2 was just released, I will try to review this PR in the upcoming days.

mamazu commented 3 years ago

The new version is out. Please rebase your branch to the new master.

About your change log, I would say that adding a CartNotEmpty shouldn't be part of this pull request and rather a separate one. (It shouldn't be much to change anyways I suppose)

NenadStef commented 3 years ago

@lchrusciel Travis is failing because this pull request should be for sylius 1.8 and higher (it fails for sylius 1.7). Product variant isEnabled method was added in sylius 1.8.

NenadStef commented 3 years ago

@lchrusciel @TiMESPLiNTER @mamazu @diimpp

Regarding filtration of the disabled product or product variants I need some clarification.

  1. If product is disabled it should not be in shown product list
  2. If is a simple product (1 product and 1 variant) and product is enabled but product variant is disabled product should not be shown in product list
  3. If product has more then one variant and product is enabled but one of the variant is disabled product should be shown with only enabled variants

Are this assumptions correct?

Additional validation

Is there a need to add additional product and product variant eligibility validation on request such as add product review or show product details...

mamazu commented 3 years ago

Sounds good to me.

lchrusciel commented 3 years ago

Sorry, for the late feedback loop, I will try to jump into this PR next week.

According to supported versions, the last release supported 1.7 & 1.8. As 1.7 has security fixes only, I would vote for bumping minimal requirement to 1.8.

NenadStef commented 3 years ago

@lchrusciel

Hi Lukasz

I added filtration for disabled product and product variants on routes for latest products, product catalogs and product details.

Disabled products are handled automatically in Sylius database queries but for disabled product variants I added filtration so now only enabled product variants will be shown.

Regarding disabled product variant I have a question how this works in Sylius core. In case of simple product when product is set to disabled related product variant is also set to disabled, but when product variant is set to disabled related product is not set as disabled. In case when product has multiple variants we can set all product variants to disabled and product will be left as enabled. From the shop api plugin this will be manifest in form that enabled products will be shown only with enabled variants and if all variants are disabled it will be shown as product without variants.

I don’t know if this behaviour for disabled products and product variants is what was intended but if not it can be change with another pull request on Sylius core.

NenadStef commented 3 years ago

@lchrusciel Hi Lukasz

So I wanted to inform you about progress on this pull request.

  1. That 500 error is solved by adding additional validator for checking if option base product variant exists.
  2. Also additional validator is added for checking if option base product variant is eligible
  3. All code is refactor so now it can work both with Sylius 1.7 and Sylius 1.8
  4. All spec test are refactored so now thy are working both on 1.7 and 1.8
  5. All integration tests are working on 1.7 and 1.8
  6. Static analysis is without errors

Help needed

One last thing that is still failing on Travis (see stats and build) is coding standard. I could really use your help to solving this.

lchrusciel commented 3 years ago

Hey Nenad, I'm hard for me to say this, but I've never asked you to support both versions at the same time...

Your build was blocked for a really long time and I'm sorry for that. Today, I finally dropped support for Sylius 1.7 and this PR could go on. Nonetheless, all integration code has to be removed. Also, the build should be green now, as we moved to GitHub actions. In terms of ECS problems, it was removed from the CI pipeline.

I've tried to make a quick fix to your PR: https://github.com/lchrusciel/ShopApiPlugin/tree/disabled-product-in-shop-api but still things like additional config should be removed.

NenadStef commented 3 years ago

@lchrusciel I saw that you did update for Travis and I merge it on this pull request. Regarding versions compatibility I did that on my own because there was no answer about version compatibility for a long time but I can change it back so it can only be run on version 1.8.

lchrusciel commented 3 years ago

Once again, I'm sorry for being a bottleneck in this manner. I truly appreciate your work and would like to merge it to the core, once we will remove the 1.7 compatibility layer

NenadStef commented 3 years ago

@lchrusciel It's no problem I will remove that compatibility stuff in next couple of days

NenadStef commented 3 years ago

@lchrusciel I removed all Sylius 1.7 compatibility files and now pull-request is only for 1.8 version, so take a look and tell me what you think.

NenadStef commented 3 years ago

@lchrusciel Hi Lukasz

I was wondering is there any problem with Travis? It's been pending for couple of days.

mamazu commented 3 years ago

We moved from Travis to github actions. If you rebase your branch to the current master you will get that too.

NenadStef commented 3 years ago

@mamazu Hi mamazu, thanks for all the feedback.

I updated my fork with your's master branch but still having Travis CI pending as required check.

I know that this is a huge PR but it can be summarize to this

  1. Cart not empty validation
  2. Option base product exists validation
  3. Product eligibility (simple, variant, option base) validation and build view

I understand need to split this PR in small chunks, it would be easy to review it but thing is that this PR is mostly intended to be used by the company that I work for and I was hoping that it will be approved soon.

If you insist to go with separating PR to small chunks I will have to talk with my team at company to see how much time I will have to work on this because we are having very intense sprints in next couple of weeks.

@lchrusciel What do you think about splitting this PR in small chunks?

NenadStef commented 3 years ago

@lchrusciel Hi Lukasz

Do you have any feedback about how are we going to handle this pull request?

lchrusciel commented 3 years ago

Hey Nenad,

thanks a lot for keeping this PR alive and sorry for my late feedback. In addition, thanks Max and Dmitri for your early review.

NenadStef commented 3 years ago

@lchrusciel Hey Lukasz, thanks for approving this