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

optimized configurable product indexing #24962

Conversation

ravi-chandra3197
Copy link
Contributor

@ravi-chandra3197 ravi-chandra3197 commented Oct 10, 2019

Description (*)

optimized configurable product indexing (Fixed Negative min_price and max_price issue after migration)

Fixed Issues (if relevant)

Fixes #24535
Fixes #15609
Fixes #14443
Fixes #24953

Manual testing scenarios (*)

Questions or comments

Testing with sample data min_price equation calculate like this

min_price = i.min_price - i.price + io.min_price
which become 44 = 0 - 0 + 44

i.max_price - i.price + io.max_price
44 = 0 - 0 + 44
there is no meaning to use configurable price and min_price in the calculation
(Magento2 Not allow to add price or special price it's by default 0 )

Fixed Negative min_price and max_price issue after migration
Magento1 allows adding price and special prices for the configurable product which create an issue for calculating min_price and max_price in Magento2

with the above solution, indexing becomes faster to reduce calculation for each product min and max price and fixed

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Oct 10, 2019

Hi @ravi-chandra3197. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@ravi-chandra3197
Copy link
Contributor Author

Resolve this issue using single PR #24535, #15609, #14443

Negative prices in layered navigation #24535

Magento 2.2.4 Product sorting list by price on category page wrong when configurable product has special price set by mass action. #15609

Final min_price and max_price calculation on configurable product with special price #14443

@dhorytskyi
Copy link
Contributor

The mentioned expression doesn't result in "0 - 0 + X" every time, i.price field contains base product price, but i.min_price field contains base price plus price of required custom options.
With those changes, minimal price of configurable product will not contain price of required custom options. As a result, minimal price will not be actual minimal price of configurable product and it will be inconsistent with minimal price of other product types.

@ravi-chandra3197
Copy link
Contributor Author

The mentioned expression doesn't result in "0 - 0 + X" every time, i.price field contains base product price, but i.min_price field contains base price plus price of required custom options.
With those changes, minimal price of configurable product will not contain price of required custom options. As a result, minimal price will not be actual minimal price of configurable product and it will be inconsistent with minimal price of other product types.

Hello @dhorytskyi
in Magento2 there is no base price option for a configurable product, old expression conflict when the configurable product has a base price.
as a normal situation, the configurable product has no price option in M2 but when you migration with M1 Data it will create an issue.

step-1: create any configurable product in M1 set price and special price
step-2: perform migration in M2 so we get the same product in M2
step-3: run reindexing
step-4: check the 'catalog_product_index_price' table in the database it has a
negative value for configurable products.
in frontend filter by price not working as expected due to negative price

negative-price

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:18
@MichaelThessel
Copy link

MichaelThessel commented Jan 27, 2020

@akaplya, any chance you could review this? It would be great to get this resolved.

At least in my case this resolved the issue (#24535)

@merance2000
Copy link

merance2000 commented Feb 2, 2020

We are having the same issue - I thought it was because of M1 import but other items updated via api are also showing the issue .

@MichaelThessel
Copy link

This has been pending for 6 months now. Any chance this can be merged?

@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev ihor-sviziev self-assigned this Aug 19, 2020
@ihor-sviziev ihor-sviziev added the Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. label Aug 19, 2020
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @ravi-chandra3197,

  1. Could you review failing integration tests?
    According to test results - seems like your change does some regression in specific cases and solution should be improved.

  2. After fixing failing existing tests, could you cover your changes with some kind of test?

@ghost ghost moved this from Pending Review to Changes Requested in Pull Requests Dashboard Aug 19, 2020
@ravi-chandra3197
Copy link
Contributor Author

Hello @ihor-sviziev

I am working on failing integration tests

@ihor-sviziev
Copy link
Contributor

@ravi-chandra3197 will you be able to update your PR? Or maybe you have some questions / need some help?

@ravi-chandra3197
Copy link
Contributor Author

@ihor-sviziev I have need help to fix integration tests.

@ihor-sviziev
Copy link
Contributor

@engcom-Charlie could you help with failing tests?

@engcom-Hotel
Copy link
Contributor

Hello @ravi-chandra3197, will try to help you with Integration Tests.

@ihor-sviziev
Copy link
Contributor

@engcom-Hotel make sure that these changes didn't break some functionality.

@engcom-Hotel
Copy link
Contributor

@magento run all tests

@engcom-Hotel
Copy link
Contributor

Hello @ravi-chandra3197 @ihor-sviziev.
I have checked the integration test and it is fine.
Unfortunately, the offered solution is not perfect. As a result prices calculation process doesn't work properly. It is quite risky optimization.

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @ravi-chandra3197,
Could you update your solution to be compatible with the cases when tests are failing?

@sidolov sidolov added Priority: P3 May be fixed according to the position in the backlog. Risk: high Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Sep 16, 2020
@sidolov
Copy link
Contributor

sidolov commented Sep 16, 2020

Risk: high since the configurable indexation process is affected

@ihor-sviziev
Copy link
Contributor

Looks like we have another PR that fixes the same issue. #26810

@ihor-sviziev
Copy link
Contributor

@ravi-chandra3197 will you be able to update your PR?

@ihor-sviziev
Copy link
Contributor

@ravi-chandra3197 I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@m2-assistant
Copy link

m2-assistant bot commented Oct 2, 2020

Hi @ravi-chandra3197, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ghost ghost removed this from Changes Requested in Pull Requests Dashboard Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ConfigurableProduct help wanted Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner Priority: P3 May be fixed according to the position in the backlog. Progress: needs update Release Line: 2.4 Risk: high Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Squashtoberfest 2019
Projects
None yet
9 participants