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

Issues 6729, 6457, 7362: Configurable product price options provider #9796

Merged

Conversation

simpleadm
Copy link
Contributor

@simpleadm simpleadm commented May 30, 2017

Fix for lowest price options provider to retrieve products with additional attributes

Description

LowestPriceOptionsProvider returns products without attributes which are used for price calculation (e.g. tax adjustment)

Fixed Issues

  1. Configurable product old price with taxes displayed wrong #6729: Configurable product old price with taxes displayed wrong
  2. Expired special_price is still shown for configurable products when no variant is selected #6457: Expired special_price is still shown for configurable products when no variant is selected
  3. Special price vigency for configurable childs (simple products associated) doesn´t work #7362: Special price vigency for configurable childs (simple products associated) doesn´t work

Manual testing scenarios

  1. Create configurable product with tax, price is defined for each sku (Stores > Configuration > Sales > Tax > Catalog Prices = Including Tax)
  2. Create tax rule 25%
  3. Apply this tax rule to the product
  4. In tax settings set all values to display prices without tax (Stores > Configuration > Sales > Tax > Display Prices = Excluding Tax)
  5. Check old configurable product price

@okorshenko
Copy link
Contributor

Hi @simpleadm Thank you for your pull request. Could you please resolve the conflicts the double-check if the fix is still needed. Please check this commit: 1ab9b26

@simpleadm
Copy link
Contributor Author

simpleadm commented Jul 4, 2017

Hi @okorshenko.

Pull Request is still relevant. Commit: 1ab9b26 is fixing only 6457 and 7362 issues.
Conflicts were resolved, please take a look.

P.S. code style was changed to prevent "Line exceeds maximum limit of 120 characters; contains 122 characters" error.

@okorshenko
Copy link
Contributor

Hi @simpleadm thank you for update

@ishakhsuvarov ishakhsuvarov added this to the July 2017 milestone Jul 6, 2017
@ishakhsuvarov
Copy link
Contributor

@simpleadm
I have tried to reproduce the issue with and without the fix, but I do not notice any difference in the behavior. Could you please be more specific with the preconditions, steps to reproduce and actual/expected result?
Thanks!

@ishakhsuvarov
Copy link
Contributor

Hi @simpleadm
Are there any updates regarding manual testing steps?
Thanks!

@simpleadm
Copy link
Contributor Author

simpleadm commented Jul 19, 2017

Hi @ishakhsuvarov,

Please accept my apologies for the delayed response.

Issue with price was fixed. But my pull request is still useful and i will try to explain use cases.

  • Update tax configuration. Products price should include tax, but it should be displayed without tax (Stores > Configuration > Sales > Tax [Catalog Prices = Including Tax, Display Product Prices In Catalog = Excluding Tax, Shopping Cart Display Settings = Excluding Tax, Orders, Invoices, Credit Memos Display Settings = Excluding Tax]) - http://prntscr.com/fxhog8 http://prntscr.com/fxholh
  • Create configurable product with tax, price is defined for each sku (e.g. 100$)
  • Add special price for a child simple product (e.g. 60$) http://prntscr.com/fxhprq
  • Create tax rule 25%
  • Apply this tax rule to the product
  • Check "Regular Price" will be displayed with taxes - http://prntscr.com/fxhrlu (custom popup with product information)

In earlier Magento builds regular price used to display on configurable products and it was wrong. Now it renders by JS. Also in product listing default magento doesn't display regular price (<?php if (!$block->isProductList() && $block->hasSpecialPrice()): ?> app/code/Magento/ConfigurableProduct/view/base/templates/product/price/final_price.phtml ) for configurable products (if it will display, the regular price will be wrong).

But the bug can be very painful for third party developers. For example we need to have own block or widget with configurable product listing. So we will:

  • extends from \Magento\Catalog\Block\Product\AbstractProduct
  • create own template were use getProductPrice method echo $block->getProductPrice($block->getProduct())
  • get wrong "Regular Price"

Hope now it's more clear:)
Thanks.

@okorshenko
Copy link
Contributor

Hi @simpleadm could you please accept the invite on GitHub to join magento team?

@simpleadm simpleadm deleted the fix-configurable-lowest-price-provider branch October 27, 2017 06:01
@simpleadm
Copy link
Contributor Author

Hi @okorshenko, invite have been accepted.
Thanks, i appreciate it.

magento-engcom-team added a commit that referenced this pull request Feb 28, 2018
…s provider #13490

 - Merge Pull Request #13490 from simpleadm/magento2:backport/fix-configurable-lowest-price-provider
 - Merged commits:
   1. 40ddfc0
magento-engcom-team added a commit that referenced this pull request Feb 28, 2018
Accepted Public Pull Requests:
 - #13490: [Backport 2.1] #9796 configurable product price options provider (by @simpleadm)


Fixed GitHub Issues:
 - #6457: Expired special_price is still shown for configurable products when no variant is selected (reported by @heldchen) has been fixed in #13490 by @simpleadm in 2.1-develop branch
   Related commits:
     1. 40ddfc0

 - #6729: Configurable product old price with taxes displayed wrong (reported by @kandrejevs) has been fixed in #13490 by @simpleadm in 2.1-develop branch
   Related commits:
     1. 40ddfc0

 - #7362: Special price vigency for configurable childs (simple products associated) doesn´t work (reported by @valenciaisaza) has been fixed in #13490 by @simpleadm in 2.1-develop branch
   Related commits:
     1. 40ddfc0
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