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

Add behat test scenarios for wholesale_price change when assigning supplier #22380

Merged

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Dec 11, 2020

Questions Answers
Branch? develop
Description? Adds missing behat tests scenario to support some logic when supplier is assigned to product. When default supplier is assigned to standard product, then product->wholesale_price should be copied from that supplier price. read more #20544 (comment) and #20198 (comment)
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? related to #19683
How to test? Travis ✔️

This change is Reviewable

@zuk3975 zuk3975 requested a review from a team as a code owner December 11, 2020 19:51
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Dec 11, 2020
@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 11, 2020

The test is constantly failing due duplicate key in ps_layered_price_index. It might be a good chance to tackle down that problem, as it might not only be a caching issue, but actual bug with combinations and faceted search module 🤔

@Progi1984 Progi1984 changed the title Add behat test scenarios for wholesale_price change when assigning supplier [product page migration] Add behat test scenarios for wholesale_price change when assigning supplier Dec 14, 2020
@Progi1984 Progi1984 added the Waiting for author Status: action required, waiting for author feedback label Dec 14, 2020
@matks matks added the migration symfony migration project label Dec 14, 2020
@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 14, 2020

ps_facetedsearch.php indexProductPrices() the final result of values to be inserted are (in ps_facetedsearch.php L551)

[
    "(23,2,1, ,21)"
    "(23,7,1, ,21)"
]

Im not actually sure its bug in module or it is test fault, maybe some undesired stuff is happening because there is 7 currencies created (5 of which are USD) during tests 😓

@matks
Copy link
Contributor

matks commented Dec 15, 2020

@zuk3975 Did you find the answer about ps_facetedsearch?

@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 15, 2020

| @zuk3975 Did you find the answer about ps_facetedsearch?

yep It was caching issue related to combination, this seemed to help 1e94d99
this is how prices are related to supplier:
When supplier is updated, product->id_supplier (along with supplier_reference and wholesale_price) must be updated too, so then we invoke Product->update() , which triggers hook actionProductSave, which is used by facetedsearch to invoke Product::priceCalculation.
priceCalculation used to return null because cache was not empty (but not filled by needed attributes too).
Screenshot from 2020-12-15 11-01-06
based on screenshot the $_pricesLevel2[$cache_id_2] was set, but [$id_product_attribute] wasn't.

@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 15, 2020

clearing cache manually, doesn't seem like a good solution yet, its still under investigation 🕵️ , so don't rush to merge 😅

@jolelievre
Copy link
Contributor

@zuk3975 after a few search on my side I did notice the same problems you mentioned, the root problem indeed came from the Product::priceCalculation method and especially its cache which:

  • is filled based on productId (cache key)
  • stored all the attributes prices
  • returns null (not even zero) when it doesn't find the price for request attribute

The problem here is that the cache was filled on product creation, then combinations were created but the cache didn't know them so it didn't know their prices and returned null..

I added two fixes for this:

  • most important now Product::priceCalculation checks if the specified attribute cache is set, if not it rebuilds it for all attributes, this should prevent a lot of problems
  • when combinations are created by behat tests, the context calls Product::resetStaticCache() for extra safety to avoid encountering similar problems related to combination cache in the future

@matks matks removed the Waiting for author Status: action required, waiting for author feedback label Dec 17, 2020
@matks matks added this to the 1.7.8.0 milestone Dec 17, 2020
@matks matks merged commit 0019845 into PrestaShop:develop Dec 17, 2020
@matks matks deleted the m/product/support-supplier-wholesale branch December 17, 2020 17:33
@matks
Copy link
Contributor

matks commented Dec 17, 2020

Thank you @zuk3975 and @jolelievre

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch migration symfony migration project Refactoring Type: Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants