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

Checking if the variant belongs to current product #3267

Merged

Conversation

Akarshit
Copy link
Contributor

@Akarshit Akarshit commented Nov 9, 2017

Fix for #3227

Problem

The variantId was not getting updated when selecting a new product.

Solution

In the check for updating the product/variant instead of only checking if variantId == null also check that variantId belong to the selected product

Another solution

Maybe a better solution is to call ReactionProduct.setCurrentVariant(null) as soon as some PDP page is opened.

To test

  1. Run reaction
  2. Add image to example-product and publish.
  3. Add another product with image(say test-product).
  4. Go to home and then select example-product(image should show).
  5. Go to home and select test-product(image should show).
  6. Do a refresh(image should show).

@brent-hoover brent-hoover self-requested a review November 17, 2017 02:12
@brent-hoover brent-hoover changed the base branch from master to meteor-1.6 November 17, 2017 06:56
@brent-hoover brent-hoover changed the base branch from meteor-1.6 to master November 17, 2017 06:56
@brent-hoover
Copy link
Collaborator

I think this is a workaround, as we shouldn't be getting a selectedVariant that's not associated with the product, but this fixes several pretty serious bug and fixing the source of the issue may involve rewriting some core PDP stuff.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Tested. Verified fixed.

@spencern spencern changed the base branch from master to release-1.5.9 November 20, 2017 23:44
@spencern spencern merged commit ef1c49c into reactioncommerce:release-1.5.9 Nov 20, 2017
@spencern spencern mentioned this pull request Nov 21, 2017
@janus-reith
Copy link
Collaborator

Could you test again?
This is broken for me, but could also be related to my pdp variant modifications.
Accesing the main page works, but if I try to directly browse to a variant by handle, like
http://localhost:3000/product/handle/id, the pdp just shows the loading spinner and console throws:

meteor.js?hash=cbcc712d51de4298c275e8dcf25c66c29914f19a:992 TypeError: Cannot read property 'indexOf' of undefined
    at _variantIsSelected (products.js:84)
    at ReactiveDict.ReactionProduct.setProduct (products.js:169)
    at composer (productDetail.js:652)
    at tracker.js:37
    at Tracker.Computation._compute (tracker.js?hash=997515fa2d5b0530ba07741da556c4b36963ef3b:339)
    at Tracker.Computation._recompute (tracker.js?hash=997515fa2d5b0530ba07741da556c4b36963ef3b:358)
    at Object.Tracker._runFlush (tracker.js?hash=997515fa2d5b0530ba07741da556c4b36963ef3b:532)
    at onGlobalMessage (meteor.js?hash=cbcc712d51de4298c275e8dcf25c66c29914f19a:448)

@brent-hoover
Copy link
Collaborator

@janus-reith I believe we dropped support for browsing to particular variant by URL a while ago. Is that what you mean?

Since this is already released, if you have a problem can you open a new issue?

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

Successfully merging this pull request may close these issues.

4 participants