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

Why fetch every time? #2355

Closed
wants to merge 3 commits into from
Closed

Conversation

mrn-rigtved
Copy link

@mrn-rigtved mrn-rigtved commented Feb 23, 2022

1. Why is this change necessary?

We might have the data in local storage then there is no reason to fetch it.
If we don't have the content we can just get it.

Why have the comments say session storage when it's local storage?

2. What does this change do, exactly?

We only fetch if we don't have any data matching the key in local storage.
So we reduce the fetch requests.

3. Describe each step to reproduce the issue or behaviour.

I just made a page load on the landing page and some category pages then I saw we always call from this plugin.

5. Checklist

  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

We might have the data in local storage then there is no reason to fetch it.
When we add to cart then we update the local storage with the new information.
If we don't have the content we can just get it.

Why have the comments say session storage when it's local storage?
@CLAassistant
Copy link

CLAassistant commented Feb 23, 2022

CLA assistant check
All committers have signed the CLA.

@keulinho
Copy link
Contributor

Please add a little changelog entry as described here.

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #2355 (1b8fa07) into trunk (515c1dc) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 1b8fa07 differs from pull request most recent head 36ab3c1. Consider uploading reports for the commit 36ab3c1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #2355      +/-   ##
==========================================
+ Coverage   61.43%   61.46%   +0.03%     
==========================================
  Files        3328     3329       +1     
  Lines       81910    81928      +18     
==========================================
+ Hits        50323    50359      +36     
+ Misses      31587    31569      -18     
Impacted Files Coverage Δ
src/Core/Checkout/Cart/PriceDefinitionFactory.php 40.00% <0.00%> (-10.00%) ⬇️
...torefront/Framework/Twig/TemplateDataExtension.php 96.22% <0.00%> (-3.78%) ⬇️
src/Core/Content/Seo/SeoUrlGenerator.php 87.67% <0.00%> (-1.81%) ⬇️
...out/Promotion/Cart/PromotionDeliveryCalculator.php 82.68% <0.00%> (-0.56%) ⬇️
...amework/Seo/SeoUrlRoute/ProductPageSeoUrlRoute.php 93.10% <0.00%> (-0.23%) ⬇️
...Flow/Exception/GenerateDocumentActionException.php 66.66% <0.00%> (ø)
...csearch/Product/ElasticsearchProductDefinition.php 97.26% <0.00%> (+0.45%) ⬆️
...tractionLayer/Dbal/EntityDefinitionQueryHelper.php 89.85% <0.00%> (+0.72%) ⬆️
...c/Core/Content/Sitemap/Service/SitemapExporter.php 97.14% <0.00%> (+1.42%) ⬆️
.../PropertyGroupOption/PropertyGroupOptionEntity.php 48.71% <0.00%> (+2.56%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 515c1dc...36ab3c1. Read the comment docs.

@tinect
Copy link
Contributor

tinect commented Mar 7, 2022

I like the try to save requests and traffic.

But, I'm really not happy with this solution :-(

This will lead to confused customers, when they are working through more than one device, while the cart is saved on account not browser :-)
Additionally, the widget will stay "0,00 €" when a customers logs in with empty cart, but had items in cart when he was loggedin.

Short: not requesting the current cart regularly will result in wrong data shown in widget.

EDIT: @raknison any reaction? :-)

@raknison
Copy link

Hey @mrn-rigtved

Sorry for the long delay and thank you for your contribution, but I am closing this PR, because as @tinect says, incorrect data is displayed if the current shopping cart is not queried regularly.

@raknison raknison closed this May 17, 2022
@mrn-rigtved mrn-rigtved deleted the patch-1 branch November 14, 2022 12:58
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