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

#26314: fixed logic for updating map price for selected swatch #26317

Conversation

sergiy-v
Copy link
Contributor

@sergiy-v sergiy-v commented Jan 8, 2020

Description

Fixed logic for updating map price for selected swatch option on product list

Fixed Issues

  1. Minimum Advertised Prices duplicates for all configurable products with price from selected swatch #26314

Manual testing scenarios

  1. Enable MAP (Stores -> Configuration -> Sales -> Sales -> Minimum Advertised Price -> Enable MAP).
  2. Enable 'Show Swatches in Product List', enabled by default (Stores -> Configuration -> Catalog -> Catalog -> Storefront -> Show Swatches in Product List).
  3. The product attribute uses for configurable products should be visible on Product List (Stores -> Attributes -> Product -> select attribute -> Storefront Properties -> Used in Product Listing) in order to show attribute swatches for Product List.
  4. Create few configurable products for the attribute with 'swatch' (Visual Swatch, Text Swatch) type with associated products:
    • Configurable A:
      • Black A: price 1, MAP 11. (ex: 'black' color)
      • White A: price 2, MAP 12. (ex: 'white' color)
    • Configurable B:
      • Black B: price 10, MAP 110. (ex: 'black' color)
      • White B: price 20, MAP 120. (ex: 'white' color)
  5. Create category, assign created products.
    Expected result

Questions or comments

Better to test once the changes from the #26240 will be merged.

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)

@sergiy-v sergiy-v requested a review from DrewML as a code owner January 8, 2020 18:18
@m2-assistant
Copy link

m2-assistant bot commented Jan 8, 2020

Hi @sergiy-v. 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.

@magento-engcom-team magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Jan 8, 2020
@dmytro-ch dmytro-ch self-assigned this Jan 9, 2020
Copy link
Contributor

@dmytro-ch dmytro-ch left a comment

Choose a reason for hiding this comment

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

Hello @sergiy-v, could you please check the failing unit test tests/js/jasmine/tests/app/code/Magento/Msrp/frontend/js/msrp.test.js?
Thank you! :)

@sergiy-v
Copy link
Contributor Author

Hello @sergiy-v, could you please check the failing unit test tests/js/jasmine/tests/app/code/Magento/Msrp/frontend/js/msrp.test.js?
Thank you! :)

Hello @dmytro-ch, I'll check it, thank you!

@dmytro-ch dmytro-ch added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix labels Jan 24, 2020
@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-6685 has been created to process this Pull Request

@engcom-Alfa
Copy link
Contributor

Hi @sergiy-v,

During testing, we faced the issue

Problem: The minimum advertised price does not change after the swatches have been selected

Manual testing scenario:

  1. Enable MAP (Stores -> Configuration -> Sales -> Sales -> Minimum Advertised Price -> Enable MAP).
  2. Enable 'Show Swatches in Product List', enabled by default (Stores -> Configuration -> Catalog -> Catalog -> Storefront -> Show Swatches in Product List).
  3. The product attribute uses for configurable products should be visible on Product List (Stores -> Attributes -> Product -> select attribute -> Storefront Properties -> Used in Product Listing) in order to show attribute swatches for Product List.
  4. Create few configurable products for the attribute with 'swatch' (Visual Swatch, Text Swatch) type with associated products and assign created products to category :
    Configurable A:
    Black A: price 1, MAP 11. (ex: 'black' color)
    White A: price 2, MAP 12. (ex: 'white' color)
    Configurable B:
    Black B: price 10, MAP 110. (ex: 'black' color)
    White B: price 20, MAP 120. (ex: 'white' color)

Actual Result:
When we switch between swatches, MAP should change too
after

@sergiy-v Could you take a look?

Thanks!

@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-6685 has been created to process this Pull Request

@engcom-Alfa
Copy link
Contributor

Hi @sergiy-v.
The issue described in my comment can still be reproduced with PR changes

Actual Result:
after3

And the problem #26240 that has been resolved in #26241 is returned

Actual Result:
after4

@sergiy-v Could you take a look?

Thanks!

@sergiy-v
Copy link
Contributor Author

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @sergiy-v. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @sergiy-v, here is your new Magento instance.
Admin access: https://pr-26317.instances.magento-community.engineering/admin_ad24
Login: ea183482 Password: b85c18f4d7d2
Instance will be terminated in up to 3 hours.

@sergiy-v
Copy link
Contributor Author

sergiy-v commented Feb 12, 2020

Hi @sergiy-v.
The issue described in my comment can still be reproduced with PR changes

Actual Result:
after3

And the problem #26240 that has been resolved in #26241 is returned

Actual Result:
after4

@sergiy-v Could you take a look?

Thanks!

Hello @engcom-Alfa,
The changes works fine on my the local environment and created test environment (see credentials above). You can check the result on the test environment. Maybe I missed something.
Could you please check it one more time?

Thank you.

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Actual Result:
screen222

@m2-assistant
Copy link

m2-assistant bot commented Feb 20, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants