-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
magento/magento2#22856: Catalog price rules are not working with custom options as expected. #22917
magento/magento2#22856: Catalog price rules are not working with custom options as expected. #22917
Conversation
Hi @p-bystritsky. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
27aa7d1
to
48df4f9
Compare
48df4f9
to
171dc02
Compare
171dc02
to
1719f53
Compare
* return converted percent to price | ||
* Return price. | ||
* | ||
* If $flag is true and price is percent return converted percent to price | ||
* |
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.
I take this comment as an example of very good and less good use of it at the same time.
When you say something like "if $flag is true..." it is very valuable but at the same time saying "Return price" on a method named getPrice()
doesn't add any additional value.
Next time, I would recommend avoiding comments that repeat the name of the method, just use a comment to explain something that's not very explicit, that way you will add valuable info.
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.
@aleron75 thank you for the review! Unfortunately, code sniffers are forcing us to add some description to method annotation, so sometimes we have no other way then add some absolutely useless comment to old methods.
* @param ProductOptionValue $optionValue | ||
* @return float | ||
*/ | ||
public function execute( |
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.
The execute()
method underscores the functional usage of a service class, that's a correct approach.
For this reason, a better name for the class would be one that suggests the command that is executed, like CalculateCustomOptionCatalogRule
.
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.
@aleron75 done.
1719f53
to
8064f3e
Compare
Hi @aleron75, thank you for the review.
|
Hi @p-bystritsky issue is also reproducible with custom option type Field. Could you take a look?
Result: |
22d752e
to
e767643
Compare
@magento run all tests |
bb39c32
to
abca0b8
Compare
abca0b8
to
fbea751
Compare
guys where are we at with this? I cannot believe that no one else is making a stink about this issue. |
…options as expected. #22917
Hi @p-bystritsky, thank you for your contribution! |
so what release will this be in? |
@johncarlson21 I guess it is going to be in 2.4 as it is in 2.4-develop branch. Not sure... I'm having this issue in M 1.9.3.1 too. |
@kunzi thanks.. I never had this problem in 1.9.x and that is why when I updated I didn't notice it right away, but our site relys on this function, can't believe it was missed. P.S. if anyone needs a quick fix for the above issue before the release, let me know as I've taken this code and put it into an extension for now. |
@johncarlson21 We actually do have an extension in M 1.9.3.1 that sets absolute price for each custom option. Maybe that causes it. Thanks for the hint. |
Can you please send me extension for now as I need this now. |
@johncarlson21 @sdzhepa @p-bystritsky This ticket says the issue is fixed but i am bit confused after reading this https://experienceleague.adobe.com/docs/commerce-knowledge-base/kb/support-tools/patches/v1-1-23/acsd-47107-catalog-price-rule-is-applied-to-custom-options.html?lang=en |
Description (*)
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)