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

Patch 18017 magento 23 #19635

Merged

Conversation

niravkrish
Copy link
Contributor

Description (*)

When pre-selecting a configurable product swatch that has an image, Magento will try to load that image in the product gallery before it is initialized, throwing a JavaScript error.

Fixed Issues (if relevant)

  1. URL pre-selection of configurable product swatches with associated product images throws JavaScript error #18017: URL pre-selection of configurable product swatches with associated product images throws JavaScript error

Manual testing scenarios (*)

  1. Hit Url With Url domain.com/montana-wind-jacket.html#93=53
  2. Hit Url With Url domain.com/montana-wind-jacket.html#93=58
  3. Test with normal configurable product on listing page as well as on Product Details Page

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @niravkrish. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@@ -1223,7 +1223,14 @@ define([
}

imagesToUpdate = this._setImageIndex(imagesToUpdate);
gallery.updateData(imagesToUpdate);
if (gallery === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Such check is not correct, please use typeof instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the code.
Please review It.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.1 milestone Dec 11, 2018
@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, thank you for the review.
ENGCOM-3665 has been created to process this Pull Request

@orlangur
Copy link
Contributor

@VladimirZaets shouldn't this be typeof gallery === 'undefined' instead of typeof gallery === undefined?

@sdzhepa
Copy link
Contributor

sdzhepa commented Jan 17, 2019

Hello @niravkrish
During testing our QA engineer faced an issue.
Could you please have a look and fix it?

Please see the quote of QA comment after testing on branch with the PR

PR doesn't fix issue #18017

Manual testing scenario:

  1. Create two option for configurable product based on Visual Swatch attribute
  2. Set Use Web Server Rewrites=No
  3. Open the configurable product page, with the option pre-selected.
    magento_site/index.php/conf1.html#135=4

Actual Result: Not OK,
19635pr

Thank you for contribution and collaboration

@VladimirZaets
Copy link
Contributor

@niravkrish, I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for collaboration

@ghost
Copy link

ghost commented Jan 30, 2019

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

@niravkrish
Copy link
Contributor Author

niravkrish commented Jan 31, 2019

Hello,

Please review and test issue.

I have already resolve gallery image change issue in resolve conflicts my last commit ( resolve conflicts - bbceaab ).

@niravkrish niravkrish reopened this Jan 31, 2019
@VladimirZaets
Copy link
Contributor

Hi, @niravkrish. As I see the merge conflict exists. Please resolve it.

@VladimirZaets
Copy link
Contributor

@niravkrish thanks.

@VasylShvorak
Copy link
Contributor

✔️ QA passed

@ghost
Copy link

ghost commented Mar 14, 2019

Hi @niravkrish, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants