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

Fixes #12584 Bundle Item price cannot differ per website #27315

Merged
merged 31 commits into from
Feb 18, 2021

Conversation

rain2o
Copy link
Contributor

@rain2o rain2o commented Mar 17, 2020

Description

Bundle Item prices are missing their website id which is checked for in \Magento\Bundle\Model\Selection::afterSave. This check always failed.

I started with the changes from PR #12586 and updated it to fix the issue of the website-scoped price overriding the global price.

Related Pull Requests

#12586

Fixed Issues

  1. Bundle Item price cannot differ per website #12584: Bundle Item price cannot differ per website

Manual testing scenarios

  1. Set Stores | Configuration | Catalog | Catalog | Price | Catalog Price Scope to 'Website'
  2. Create a second website, store and store view
  3. Create a bundle product with option 'Dynamic Price': 'No'
  4. Add it to both websites
  5. Add a bundle item with price type 'fixed' and price '12'
  6. Save the product
  7. Use the dropdown on the top left to change scope to your second store view
  8. Set the Bundle Item price to '10'
  9. Save the product

Expected result
The bundle item should have price '12' in the default scope and in the first store view
The bundle item should have price '10' in the second store view

Questions or comments

I tested this in multiple environments running Magento 2.4. Two out of three tested successfully. One of those environments throws an error when saving stock items (see below). Because this only happened in one environment I couldn't figure out if it was environmental or if there is another underlying issue related to this PR. It seems as though saving the Stock Item, even when updating an existing one, throws an error for a duplicate PRIMARY index. Which is strange as it is updating an existing entry.

The stock item was unable to be saved. Please try again. {"exception":"[object] (Magento\\Framework\\Exception\\CouldNotSaveException(code: 0): The stock item was unable to be saved. Please try again. at /var/www/html/app/code/Magento/CatalogInventory/Model/Stock/StockItemRepository.php:193, Magento\\Framework\\Exception\\LocalizedException(code: 0): SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2047-0-1-5-9' for key 'PRIMARY', query was: UPDATE `catalog_product_index_price_bundle_sel_tmp`
 INNER JOIN `catalog_product_index_price_bundle_tmp` AS `i`
 INNER JOIN `catalog_product_entity` AS `parent_product` ON parent_product.entity_id = i.entity_id
 INNER JOIN `catalog_product_bundle_option` AS `bo` ON bo.parent_id = parent_product.entity_id
 INNER JOIN `catalog_product_bundle_selection` AS `bs` ON bs.option_id = bo.option_id
 INNER JOIN `catalog_product_bundle_selection_price` AS `bsp` ON bs.selection_id = bsp.selection_id AND bsp.website_id = i.website_id
SET `catalog_product_index_price_bundle_sel_tmp`.`entity_id` = `i`.`entity_id`, `catalog_product_index_price_bundle_sel_tmp`.`customer_group_id` = `i`.`customer_group_id`, `catalog_product_index_price_bundle_sel_tmp`.`website_id` = `i`.`website_id`, `catalog_product_index_price_bundle_sel_tmp`.`option_id` = `bo`.`option_id`, `catalog_product_index_price_bundle_sel_tmp`.`selection_id` = `bs`.`selection_id`, `catalog_product_index_price_bundle_sel_tmp`.`group_type` = IF(bo.type = 'select' OR bo.type = 'radio', 0, 1), `catalog_product_index_price_bundle_sel_tmp`.`is_required` = `bo`.`required`, `catalog_product_index_price_bundle_sel_tmp`.`price` = LEAST(IF(bsp.selection_price_type = 1, ROUND(i.price * (bsp.selection_price_value / 100),4), IF(i.special_price > 0 AND i.special_price < 100, ROUND(bsp.selection_price_value * (i.special_price / 100),4), bsp.selection_price_value))* bs.selection_qty, IFNULL((IF(i.base_tier IS NOT NULL, IF(bsp.selection_price_type = 1, ROUND(i.base_tier - (i.base_tier * (bsp.selection_price_value / 100)),4), IF(i.tier_percent > 0, ROUND((1 - i.tier_percent / 100) * bsp.selection_price_value,4), bsp.selection_price_value)) * bs.selection_qty, NULL)), IF(bsp.selection_price_type = 1, ROUND(i.price * (bsp.selection_price_value / 100),4), IF(i.special_price > 0 AND i.special_price < 100, ROUND(bsp.selection_price_value * (i.special_price / 100),4), bsp.selection_price_value))* bs.selection_qty)), `catalog_product_index_price_bundle_sel_tmp`.`tier_price` = IF(i.base_tier IS NOT NULL, IF(bsp.selection_price_type = 1, ROUND(i.base_tier - (i.base_tier * (bsp.selection_price_value / 100)),4), IF(i.tier_percent > 0, ROUND((1 - i.tier_percent / 100) * bsp.selection_price_value,4), bsp.selection_price_value)) * bs.selection_qty, NULL)
WHERE (i.price_type=1) at /var/www/html/app/code/Magento/Catalog/Model/Indexer/Product/Price/Action/Row.php:32

Removing the following lines from \Magento\Bundle\Model\LinkManagement causes this error to go away, so it seems related to it. I just can't figure out how/why.

if ($productLink->getWebsiteId() !== null) {
    $selectionModel->setWebsiteId($productLink->getWebsiteId());
}

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 Mar 17, 2020

Hi @rain2o. 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.4-develop instance - deploy vanilla Magento instance

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

@rain2o
Copy link
Contributor Author

rain2o commented Mar 17, 2020

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @rain2o. Thank you for your request. I'm working on Magento instance for you

@lbajsarowicz lbajsarowicz changed the title Fixes #12584 Fixes #12584 Bundle Item price cannot differ per website Mar 17, 2020
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

Could you provide Integration Tests that will also clarify the overview of the change?
Introduction of Automated Tests is part of our Definition of Done:
https://devdocs.magento.com/guides/v2.3/contributor-guide/contributing_dod.html

app/code/Magento/Bundle/Model/Selection.php Outdated Show resolved Hide resolved
@rain2o
Copy link
Contributor Author

rain2o commented Mar 18, 2020

Could you provide Integration Tests that will also clarify the overview of the change?
Introduction of Automated Tests is part of our Definition of Done:
https://devdocs.magento.com/guides/v2.3/contributor-guide/contributing_dod.html

@lbajsarowicz I am a little unfamiliar with the Integration tests. I will attempt to do so. I don't see any integration tests in the Bundle module, but I see some in dev/tests/integration/testsuite/Magento/Bundle. Should I be adding these tests inside the Bundle module directly, or in the testsuite under dev/tests?

@lbajsarowicz
Copy link
Contributor

@rain2o Fix Static Tests then.
We will find help with Integration Tests.

@rain2o
Copy link
Contributor Author

rain2o commented Mar 18, 2020

@lbajsarowicz Sure thing, thanks for the assist. I noticed the static tests that are failing also fail on the 2.4-develop branch as well. Most seem to be things like code duplication between models and their respective test classes (e.g. .../app/code/Magento/Customer/Model/Data/Address.php and .../app/code/Magento/TestModuleExtensionAttributes/Model/Data/FakeAddress.php).
Are these the tests I need to fix, as they are pre-existing? Just trying to make sure I'm clear on what I need to do.

@rain2o
Copy link
Contributor Author

rain2o commented Mar 18, 2020

Sorry, I see the Allure test results now and it's more clear to me. I just can't seem to successfully run these tests locally to get the same results.

lbajsarowicz
lbajsarowicz previously approved these changes Mar 18, 2020
@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7141 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@rain2o rain2o linked an issue Mar 19, 2020 that may be closed by this pull request
@engcom-Bravo engcom-Bravo self-assigned this Mar 20, 2020
@gabrieldagama
Copy link
Contributor

@magento run all tests

@magento-engcom-team
Copy link
Contributor

Hi @gabrieldagama, thank you for the review.
ENGCOM-7141 has been created to process this Pull Request
✳️ @gabrieldagama, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@gabrieldagama gabrieldagama added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Jan 27, 2021
@engcom-Bravo engcom-Bravo moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Jan 28, 2021
@gabrieldagama
Copy link
Contributor

@magento run all tests

@engcom-Bravo
Copy link
Contributor

engcom-Bravo commented Jan 28, 2021

✔️ QA Passed

Manual testing scenarios

  1. Go to Stores - Configuration - Catalog - Catalog - Price and set Catalog Price Scope to Website
    per_website
  2. Create a second Website, Store and Store view
  3. Go to Catalog - Products and Add a new bundle product
  4. Set the following settings:
  • Dynamic Price - No
    dynam_no

  • Products in Website - select all the available websites
    websites

  1. Add a bundle item with price type fixed and price 12 and Save the product
    bundle_item
  2. Change the Scope to the second store view.
  3. Change the Bundle item price to 10, and Save the product
    bundle_ite_10
  4. Change the Scope to the third store view and check the Bundle Item price

BEFORE applying changes provided in the PR, the Bundle item price has been changed for all store views, which in this case was 10

AFTER switching to the PR, the Bundle item price is changed only in the scope of a particular store view. In this case, the price remained 12
bundle_ite_12
The entity is added to catalog_product_bundle_selection_price table in the database
entity

@engcom-Bravo engcom-Bravo added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Jan 28, 2021
@m2-community-project m2-community-project bot moved this from Testing in Progress to Ready for Testing in High Priority Pull Requests Dashboard Jan 28, 2021
@engcom-Bravo engcom-Bravo moved this from Ready for Testing to Extended Testing (optional) in High Priority Pull Requests Dashboard Jan 28, 2021
@engcom-Foxtrot engcom-Foxtrot moved this from Extended Testing (optional) to Merge in Progress in High Priority Pull Requests Dashboard Feb 1, 2021
@magento-engcom-team magento-engcom-team merged commit 15015fa into magento:2.4-develop Feb 18, 2021
@m2-assistant
Copy link

m2-assistant bot commented Feb 18, 2021

Hi @rain2o, 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.

@gabrieldagama gabrieldagama moved this from Merge in Progress to Recently Merged in High Priority Pull Requests Dashboard Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Bundle Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle Item price cannot differ per website
10 participants