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

magento/magento2#13765 Excess requests 'customer data' on checkout cart page were fixed #14314

Merged
merged 2 commits into from Apr 19, 2018

Conversation

andrewbess
Copy link
Contributor

@andrewbess andrewbess commented Mar 24, 2018

Description

The customer/section/load gets called 4 times when loading the cart for the first time. 3 of those times, cart data gets returned. On Magento 2.1, I believe only two calls are made to customer/section/load.

Fixed Issues (if relevant)

  1. "cart" section data gets loaded 3 times on cart page (2.2.2) #13765: "cart" section data gets loaded 3 times on cart page (2.2.2)

Steps to reproduce

  1. Add any product to cart
  2. Go to cart page /checkout/cart with the "Network" panel open in DevTools.

Expected result

  1. The /customer/section/load url should only get called twice. For example, these are the two urls that get called on a Magento 2.1 install:
https://magento22.dev/customer/section/load/?sections=&update_section_id=false&_=1519190370042
https://magento22.dev/customer/section/load/?sections=directory-data&update_section_id=false&_=1519190370043

Actual result

  1. The /customer/section/load url will get called four times:
https://magento22.dev/customer/section/load/?sections=&update_section_id=false&_=1519190370042
https://magento22.dev/customer/section/load/?sections=directory-data&update_section_id=false&_=1519190370043
https://magento22.dev/customer/section/load/?sections=cart&update_section_id=false&_=1519190370045
https://magento22.dev/customer/section/load/?sections=cart&update_section_id=false&_=1519190370046

Note: if you refresh the cart, there will only be two calls made to /customer/section/load.

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)

…ere fixed:

- Excess request for 'directory-data' was fixed;
- Excess requests for 'cart' were fixed.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 24, 2018

CLA assistant check
All committers have signed the CLA.

@osrecio osrecio added the Event: distributed-cd Distributed Contribution Day label Mar 24, 2018
@magento-engcom-team magento-engcom-team added this to the March 2018 milestone Mar 24, 2018
….2.2):

- Blank line after variable declarations was added;
- Blank line before "if" was added.
@@ -94,17 +94,17 @@ define([
this.isLoading(addToCartCalls > 0);
sidebarInitialized = false;
this.update(updatedCart);

if (cartData()['website_id'] !== window.checkout.websiteId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this file is using not at checkout page only, so window.checkout will not be available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihor-sviziev
I checked your comment.
window.checkout is available on home page and category pages.
2018-03-26 09-51-13
Also, I want to tell you, that logic "cartData()['website_id'] !== window.checkout.websiteId" was earlier in Magento.
It was fixed it to remove excessed request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will review deeper little bit later

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that window.checkout property exist at any page where present minicart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, @sidolov
Please check and approve this PR.
Thanks for advance.

@andrewbess andrewbess closed this Mar 26, 2018
@andrewbess andrewbess reopened this Mar 26, 2018
@ihor-sviziev ihor-sviziev self-requested a review March 27, 2018 18:34
@ihor-sviziev ihor-sviziev self-assigned this Mar 28, 2018
@ihor-sviziev
Copy link
Contributor

@magento-engcom-team give me new test instance

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, here is your new test Magento instance

@magento-engcom-team
Copy link
Contributor

Hi @andrewbess. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

@xmav
Copy link
Contributor

xmav commented Jun 5, 2018

Hi @andrewbess
Changes from this PR cause an issue.
Here is steps to reproduce:

  • Create Simple product with price=100$ and Qty=100
  • Go to storefront and add that product to cart with Qty = 5
  • Notice cart Subtotal in cart and minicart (Both are 500$)
  • Open Backend in new window(tab) and add Tier price(on Advanced Pricing tab) for that product for Qty=5. You may set discount=5%
  • Refresh window(tab) with Cart page
  • Notice cart Subtotal in cart and minicart.
    You will see that Subtotal in sidebar is 475 and 500 in Minicart. Please check attached screenshot
    screen shot 2018-06-05 at 18 18 51

Would be great if you can adjust your changes to prevent this issue.

Feel free to contact me if you have questions.

Thanks,
Maksym

@andrewbess
Copy link
Contributor Author

Hello @xmav
Thank you.
I will check it.

@xmav
Copy link
Contributor

xmav commented Jun 25, 2018

Code changes has been reverted due to introduced issues

@ihor-sviziev
Copy link
Contributor

@xmav there also was forwardport to 2.3-develop #14782. Was it also reverted?

@ihor-sviziev ihor-sviziev removed their request for review June 26, 2018 12:36
@xmav
Copy link
Contributor

xmav commented Jun 26, 2018

@ihor-sviziev yes, it was reverted.

@sshymko
Copy link

sshymko commented Jun 28, 2018

@xmav
Is there an alternative fix to be used instead of the reverted one?

@elvinristi
Copy link
Contributor

Problem with this PR is that it probably works only then when you have to show prices excluding tax.

when I update to from "quote.totals().subtotal" to "quote.totals().subtotal_incl_tax" then this works with inclusive tax.

So there should be added some check what is the admin setting - inclusive or exclusive tax and using proper value based on it.

@Ctucker9233
Copy link

@xmav Is there an alternate to this fix? It is desperately needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Event: distributed-cd Distributed Contribution Day Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet