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

Fix a bug where changing quantity on product quickview adds products tags on ever products #16852

Merged
merged 8 commits into from Jan 10, 2020

Conversation

@NeOMakinG
Copy link
Contributor

NeOMakinG commented Dec 18, 2019

Questions Answers
Branch? 1.7.6.x
Description? When you were on product quickview, it was adding tags of the viewed product on every products on the page
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16633 and Fixes #9919
How to test? Go on the homepage, and click on quickview on a product with a promo tag for example, then change quantity and see if others product has the tag on the page

This change is Reviewable

@NeOMakinG NeOMakinG requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 18, 2019
@prestonBot prestonBot added the Bug label Dec 18, 2019
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>
@NeOMakinG

This comment has been minimized.

Copy link
Contributor Author

NeOMakinG commented Dec 19, 2019

I should rebuild assets, don't approve yet

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

khouloudbelguith commented Dec 19, 2019

Hi @NeOMakinG,

It is ok in the Categories page but if you follow these steps, the issue is reproduced:

  1. Create a product
  2. Add some related products, which they have some labels, to the last product created
  3. Go to the FO => Product details page
  4. YOU MIGHT ALSO LIKE section, click the "quick view" button for the "pack" or "on sale" labeled related product,
  5. See error

Before
image

After
image

https://drive.google.com/file/d/1XSu23m1bZeDPLysiMoMqpP1T7AvOt2h1/view

Thanks!

$('.quickview .product-discounts, .page-product:not(.modal-open) .row .product-discounts').replaceWith(data.product_discounts);
$('.quickview .product-additional-info, .page-product:not(.modal-open) .row .product-additional-info').replaceWith(data.product_additional_info);
$('.quickview #product-details, #product-details').replaceWith(data.product_details);
$('.quickview .product-flags, .page-product:not(.modal-open) .row .product-flags').replaceWith(data.product_flags)

This comment has been minimized.

Copy link
@arouiadib

arouiadib Dec 27, 2019

Contributor

I don't find any element with class .product-flags inside the quickview. Consequently, I think this line should only be: $('.page-product .product-flags').replaceWith(data.product_flags).

Same thing apply to #product-details, .product-discounts and .product-additionnal-info, because quickview doesn't contain them.

BUT this will not fully resolve the problem. One case is: when quantity is changed from a quickview inside a product page (when product miniatures exist in product page e.g similar products).

Let's take an example in detail:

  • user is in product X page and there are other similar products Y and Z as miniatures in sidebar.
  • user clicks in quickview of Y and changes quantity.

In this DOM, there are :

  • 2 elements with class product-variants , one in quickview of Y and the other in product page X.
  • 2 with .product-prices
  • 1 element with .product-customization but for product X
  • 1 element with #product-details but for product X
  • 1 element with .product-discounts but for product X
  • 1 element with .product-additionnal-info but for product X.

IMHO, the whole problem could be solved by re-naming classes (or adding discriminator ones) depending on the source (quickview or product page) and keeping javascript as it is for all classes.

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 27, 2019

Author Contributor

I agree for the class not contained in quickview, gonna add a commit for it, but for the rest, did you see that I select elements on product X page only if there are no modals opened? It means that product infos will change only if there are no quickview opened

This comment has been minimized.

Copy link
@arouiadib

arouiadib Dec 27, 2019

Contributor

Thank you for the time you gave to my comment, and I am sorry for wrongly expressing the second point.

I should have been only focusing on .product-flags when giving my example, so let's see an example with this class:

1- Install featured products module and hook it inside product page (footer for example).
2. From product page of product X, click on quickview of product Y( anyone from featured products and not necessarly one with special flags), change quantity...etc
3. Look at product flags of featured products and product X, they change to flags of product Y.

Now, let's keep modal closed and only change quantity for productX => all featured products have their flags changed.

I agree with you that :not(.open-modal) is the solution for the majority of classes, but not all of them.

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 30, 2019

Author Contributor

Hey, ok this time it looks like you didn't see the .row selector, featured products and product in the same category are not in a .row. I agree with you, thats not really solid, but atleast it avoid breaking changes, we are not able to change JS selectors for a new class because if we would do that, theme which herit these templates or rewrited these but didn't modify the JS would break with a new class, we're able to do it in a major version probably.

Thank you for your feedback and the time you take on this PR :)

This comment has been minimized.

Copy link
@arouiadib

arouiadib Dec 30, 2019

Contributor

You pointed me to subject I am not always aware of. Thank you very much for such an invaluable info.

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 31, 2019

Author Contributor

No problem, you'r welcome, thanks a lot for your feedback there, it was a pleasure !

@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Jan 3, 2020
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Jan 3, 2020

Hi @NeOMakinG !

The fix is OK for #16633, but I still have the bug for #9919 (comment)
Each time I modify in quickview a "You might also like" product (quantity, attribute...), an error is displayed on the product.

@NeOMakinG

This comment has been minimized.

Copy link
Contributor Author

NeOMakinG commented Jan 3, 2020

The error is now displayed in the right place, but the error is related to something else, submit an issue if this error should be fixed by a backend dev

@PierreRambaud PierreRambaud added this to the 1.7.6.3 milestone Jan 8, 2020
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 8, 2020

I don't know why PrettyCi is failing when there are only js files edited 😅

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Jan 8, 2020

Yep, that's weird.. @NeOMakinG maybe you can try to squash all the commits (this will make the PR cleaner) and re-push to relaunch the PrettyCi build

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Jan 10, 2020

Hi @NeOMakinG !

So, could you confirm me that the fact this error is still displayed (when changing combination of a "related product") should not be fixed by your PR ? If so, I would suggest to close the issue #16633 , which is totally fixed by your PR, but to keep #9919 (comment) open, since it's not totally fixed... WDYT ?

@NeOMakinG

This comment has been minimized.

Copy link
Contributor Author

NeOMakinG commented Jan 10, 2020

image

We can keep #9919 as it's related to the url called using ajax and not misplacement of ajax calls as this issue should fix, but 16633 is fixed, the error is even displayed on the product it should be displayed now

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Jan 10, 2020

Ok ! So since the error of #9919 is now OK (only displayed on the good product), it's QA approved :)

@PierreRambaud PierreRambaud merged commit 39e5e9e into PrestaShop:1.7.6.x Jan 10, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 10, 2020

Thanks @NeOMakinG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.