-
Notifications
You must be signed in to change notification settings - Fork 9.4k
#14020-Cart-Sales-Rule-with-negated-condition-over-special-price-does… #16342
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
Conversation
…ce-does-not-work-for-configurable-products. Use configurable product`s children for shopping cart rules validation for cases when attribute exists for children only (E.g. special_price)
Hi @novikor. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @novikor ,
thank you for your contribution.
We appreciate your support.
Hi @phoenix128, thank you for the review. |
@phoenix128 , please revert d2a0de8 This functionality have been implemented for configurable products only, not for all composite types. Moreover, Unit tests will not fail if this commit is reverted. Thank you. |
* @return \Magento\Catalog\Api\Data\ProductInterface|\Magento\Catalog\Model\Product | ||
* @throws \Magento\Framework\Exception\NoSuchEntityException | ||
*/ | ||
protected function getProductToValidate(\Magento\Framework\Model\AbstractModel $model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make this method private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ce-does-not-work-for-configurable-products. Revert "Removed dependency to configurable product" This reverts commit d2a0de8.
…ce-does-not-work-for-configurable-products. Made getProductToValidate method private according to Technical Guideline (2.7)
…ce-does-not-work-for-configurable-products. Added configurable product to dependencies
@sidolov , are there any updates? |
Hi @novikor , we are trying to avoid new dependencies across Magento modules and define another one it's not a good idea. If you have the specific case for the configurable product during the rule processing it would be better to use default Magento ways for extensibility (plugins, DI configuration, etc). |
…cial-price-does-not-work-for-configurable-products. Added configurable product to dependencies" This reverts commit 887ee4a
…gated-condition-over-special-price-does-not-work-for-configurable-products # Conflicts: # app/code/Magento/SalesRule/Model/Rule/Condition/Product.php
…ce-does-not-work-for-configurable-products. Restored \Magento\SalesRule\Model\Rule\Condition\Product to its previous state except PHPDocs.
c1a780e
to
618f408
Compare
{ | ||
/** | ||
* @param \Magento\SalesRule\Model\Rule\Condition\Product $subject | ||
* @param \Magento\Framework\Model\AbstractModel $model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove extra spaces
|
||
/** | ||
* @param \Magento\SalesRule\Model\Rule\Condition\Product $subject | ||
* @param \Magento\Framework\Model\AbstractModel $model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove extra spaces
@@ -0,0 +1,53 @@ | |||
<?php declare(strict_types=1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, cover new code at least by the unit test. It would be great if scenario will be covered with integration test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests coverage have been implemented.
@sidolov, please check.
…Cart-Sales-Rule-with-negated-condition-over-special-price-does-not-work-for-configurable-products
…th-negated-condition-over-special-price-does-not-work-for-configurable-products' into magento#14020-Cart-Sales-Rule-with-negated-condition-over-special-price-does-not-work-for-configurable-products
…ce-does-not-work-for-configurable-products. Unit tests coverage: case when child should be used
…ce-does-not-work-for-configurable-products. Fixed mock objects comparison
02bf4e0
to
870faf1
Compare
870faf1
to
c52e0e8
Compare
…ce-does-not-work-for-configurable-products. Fixed static tests.
…Cart-Sales-Rule-with-negated-condition-over-special-price-does-not-work-for-configurable-products
@sidolov , any updates? |
Hi @novikor , we found the problems with this solution on the B2B edition and trying to figure it out right now. |
Hi @novikor. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
…-not-work-for-configurable-products.
Use configurable product`s children for shopping cart rules validation
for cases when attribute exists for children only (E.g. special_price)
Description
Fixed Issues (if relevant)
Manual testing scenarios
special_price
for promo rule conditions:special_price
special
Contribution checklist