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: reducer removes guestCartId when it fails #314

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Sep 28, 2018

This PR is a:

[ ] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[x] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

When this pull request is merged, it will fix an issue where a stale guestCartId in local storage will cause an infinite loop, as the app repeatedly attempts to load an expired cart and tries again when it fails.

Image from Gyazo

Fixed this by making sure that the reducer deletes a known-bad guest cart ID.

Additional information

@zetlen zetlen added bug Something isn't working 2.x release labels Sep 28, 2018
@zetlen zetlen requested review from jimbo, pcvonz, Serunde, artKozinets, JStein92 and a team September 28, 2018 20:26
@PWAStudioBot
Copy link
Contributor

Fails
🚫

The following file(s) did not pass ESLint. Execute npm run lint locally for more details

packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js

Generated by 🚫 dangerJS

chore: add test coverage too
@zetlen zetlen force-pushed the zetlen/fix-infinite-cart-loop branch from 3f6c891 to 0266350 Compare September 28, 2018 20:30
@coveralls
Copy link

coveralls commented Sep 28, 2018

Pull Request Test Coverage Report for Build 1056

  • 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
  • 536 unchanged lines in 21 files lost coverage.
  • Overall coverage decreased (-15.1%) to 15.937%

Files with Coverage Reduction New Missed Lines %
packages/pwa-buildpack/src/WebpackTools/index.js 1 0.0%
packages/pwa-buildpack/src/util/promisified/openport.js 3 0.0%
packages/pwa-buildpack/src/util/promisified/child_process.js 3 0.0%
packages/pwa-buildpack/src/util/promisified/dns.js 3 0.0%
packages/pwa-buildpack/src/util/promisified/fs.js 3 0.0%
packages/pwa-buildpack/src/WebpackTools/MagentoResolver.js 5 0.0%
packages/pwa-buildpack/src/WebpackTools/middlewares/StaticRootRoute.js 7 0.0%
packages/pwa-buildpack/src/magento-layout-loader/validateConfig.js 13 0.0%
packages/pwa-buildpack/src/WebpackTools/plugins/DevServerReadyNotifierPlugin.js 13 0.0%
packages/pwa-buildpack/src/WebpackTools/plugins/ServiceWorkerPlugin.js 16 0.0%
Totals Coverage Status
Change from base Build 1047: -15.1%
Covered Lines: 258
Relevant Lines: 1733

💛 - Coveralls

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Some of these tests are pretty gross—inline snapshots are particularly bad. But happy path works and I'm not able to reproduce the bug.

@zetlen zetlen merged commit 91e49a6 into release/2.0 Oct 1, 2018
@zetlen zetlen deleted the zetlen/fix-infinite-cart-loop branch October 1, 2018 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants