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

Revert "Reset cart quantity to 0 if we get a 404 for the cart" #1920

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rowleyaj
Copy link
Contributor

@rowleyaj rowleyaj commented Dec 3, 2020

What?

This reverts commit caa1b30.

I noticed issues on my site where the cart quantity was not updating correctly. I dug into this and found that this is likely due to the 404 for the cart page no longer being handled correctly after the move from jquery to fetch.

fetch does not handle 404 as an error and requires this to be handled by consumer code. this looks to have been added in stencil utils bigcommerce/stencil-utils#137 as part of STRF-8771 so this code in cornerstone is no longer required. the stencil-utils change should fix the type error that occurs when trying to read physicalItems of a non-existent cart. this is the error that was making it's way through to this code which is why I noticed the 404 no longer makes it through to here.

This PR has pre-requisites:

  • release 6.7.0 of stencil-utils including STRF-8771
  • incorporate 6.7.0 stencil-utils into cornerstone
  • test any other changes thar are brought in alongside this

I'm unsure of the release process for the other libs so wasn't sure if making a PR to bump the package.json was useful or not.

Hope this helps. I'd really appreciate if we can get this into a cornerstone release soon so that our cart quantity works again!

Tickets / Documentation

Add links to any relevant tickets and documentation.

  • Original PR
  • Dependency in stencil-utils
  • BC Support Case - 04584972 - There is a BCTHEME ticket related to this but unfortunately, the live chat log hasn't come through yet and I didn't note the Jira ticket number.

This reverts commit caa1b30.

This will now be handled upstream in stencil-utils as a 404 is not an err
from fetch JakeChampion/fetch#155
@bigbot
Copy link

bigbot commented Dec 3, 2020

Autotagging @bigcommerce/themes-team

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Dec 3, 2020

cc @jairo-bc @bookernath as you were both involved in the linked PRs. would appreciate you taking a look at this :)

@jairo-bc
Copy link
Contributor

jairo-bc commented Dec 3, 2020

@rowleyaj LGTM. I think you can bump stencil-utils version in this PR. I will handle releasing stencil-utils.
cc @BC-tymurbiedukhin

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Dec 3, 2020

@jairo-bc thanks! i've added the package.json change for that, however I won't be able to update the lock file until I can do an NPM install with the real package

@Mikhail-MM
Copy link

Is this related?

On multiple themes, the scripts responsible for fetching cart metadata cart-preview.js is returning an error.

It's called in global.js, a CartID is injected from context and passed in to stencil utils.

The problem is - the CartID is occasionally stale if you've previously used stencil-utils to delete the cart.

This means that the call to stencil-utils getCartQuantity fails, and the stale value in the count-pill is not replaced, since it's initially set by pulling from localStorage.

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Dec 5, 2020

@Mikhail-MM yup that's the same issue I was having. After falling down the 🐰 🕳️ an digging through the code this is where i ended up. Essentially it's fixed in stencil-utils once we import the newer version to cornerstone, this PR is just cleaning up after that fix.

@jairo-bc
Copy link
Contributor

jairo-bc commented Dec 7, 2020

@rowleyaj stencil-utils 6.7.0. was released. Now you can update your PR

@rowleyaj
Copy link
Contributor Author

@jairo-bc sorry for the delay - PR updated with 6.7.0 in the lockfile now and CI is green

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Jan 8, 2021

@jairo-bc @BC-tymurbiedukhin @bookernath happy new year - just checking in on this one to see if we can get it merged 🤞

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Feb 9, 2021

@jairo-bc @BC-tymurbiedukhin @bookernath fixed the conflict. a higher version of stencil-utils has been included now so the main issue should be resolved. i think this should still be merged though as it cleans up code that isn't needed anymore.

@rowleyaj
Copy link
Contributor Author

@jairo-bc @BC-tymurbiedukhin @bookernath anything else needed on this before it can be merged?

@BC-tymurbiedukhin
Copy link
Contributor

@rowleyaj Could you please add changelod item as well?

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Jun 17, 2021

@BC-tymurbiedukhin this has been added to the changelog now. would you accept a PR to add this requirement to the pull request template and contributing guide? i've created 2080 for this

it seems that this PR was held up for quite a long time just because of this small missing piece

@rowleyaj
Copy link
Contributor Author

rowleyaj commented Jul 2, 2021

@BC-tymurbiedukhin any updates on this one?

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.

5 participants