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

[PWA-376] Cart page items show correct configurable options #2167

Merged
merged 4 commits into from
Feb 17, 2020

Conversation

tjwiebell
Copy link
Contributor

Description

Configurable Products listed in the MiniCart and new Cart page are showing different values for their configured properties (ex. Fashion Size, Fashion Color).

RCA: configurable_options.id is not unique based on selected values, so Apollo was not caching any other options after the first.

Related Issue

  • [PWA-376] Cart page items show correct configurable options

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Add two configurable products to your cart with different selected options
  2. Navigate to /cart page and verify that options are correctly displayed

Screenshots / Screen Captures (if appropriate)

Screen Shot 2020-02-13 at 10 12 10 AM

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@tjwiebell tjwiebell added the version: Patch This changeset includes backwards compatible bug fixes. label Feb 13, 2020
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Feb 13, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 13, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-376.

Generated by 🚫 dangerJS against 60bee68

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Add a comment and I'll approve.

@@ -42,7 +43,8 @@ export const cacheKeyFromType = object => {
return object.url_key
? `${MagentoGraphQLTypes.ProductInterface}:${object.url_key}`
: defaultDataIdFromObject(object);

case MagentoGraphQLTypes.SelectedConfigurableOption:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment here explaining why we have to do this is warranted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also looks like we are getting value_id from the getCartDetails query. I'm not sure that this is just some magical thing we have. We may want to make sure we add it to the product details query within the CartPage/ components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man, good catch! I had assumed Apollo automatically injected this key, but I was mistaken. The bug appeared to be fixed because we weren't caching any options, because object.value_id was undefined 🤔

I've now fixed this up in a187f13 so it uses that field if available, and explicitly won't cache if its left out.

@m2-community-project m2-community-project bot moved this from Ready for Review to Changes Requested in Pull Request Progress Feb 13, 2020
@m2-community-project m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Request Progress Feb 13, 2020
@@ -23,6 +23,7 @@ export const ProductListingFragment = gql`
configurable_options {
id
option_label
value_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly enough - we now have two "required" fields. id per validation although it's not actually guaranteed to be unique and value_id because we use it for caching though you'd find no code within CartPage/ that uses it.

@dpatil-magento dpatil-magento merged commit 4026d96 into develop Feb 17, 2020
@dpatil-magento dpatil-magento deleted the tommy/options-bug branch February 17, 2020 19:53
@m2-community-project m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants