Updated GraphQl Compare List Proposal #430

prabhuram93 · 2020-09-03T00:30:51Z

Problem

In Existing Implementation, Compare Lis doesn't have an id on its own. Its state is tied to Luma's session which should not be the case for GraphQl. Every new list entry expects a visitor_id in the database which is from Visitor session and cannot be mocked.
Existing proposal expects clients to generate list_id (Compare list id). This idea is debatable and can lead to data inconsistency.

Solution

Establish a list_id for each of the compare list. Clients will be able to create compare list to get a list_id. list_id can be used for future operations on the compare list

Requested Reviewers

@akaplya @melnikovi @cpartica @DrewML @nrkapoor @mauragcyrus

melnikovi · 2020-09-04T16:01:20Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls


 type CompareList {
-    list_id: ID! @doc(description: " Compare list id")
+    list_id: ID! @doc(description: "Compare list id")


Suggested change

list_id: ID! @doc(description: "Compare list id")

uid: ID! @doc(description: "Compare list uid")

melnikovi · 2020-09-04T16:03:07Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls


 type Customer {
-    compare_list: CompareList @doc(description: "Active customers compare list")
+    compareList(id: ID): [CompareList] @doc(description: "Active customers compare list") #id is optional. Introduces the possibility of more than 1 compare list.


Suggested change

compareList(id: ID): [CompareList] @doc(description: "Active customers compare list") #id is optional. Introduces the possibility of more than 1 compare list.

compare_list(uid: ID!): [CompareList]! @doc(description: "Active customers compare list") #id is optional. Introduces the possibility of more than 1 compare list.

melnikovi · 2020-09-04T16:03:33Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls


 type Query {
-    compareList(id: ID!): CompareList @doc(description: "Compare list")
+    compareList(id: ID!): [CompareList] @doc(description: "Compare list")


Suggested change

compareList(id: ID!): [CompareList] @doc(description: "Compare list")

compare_list(uid: ID!): [CompareList]! @doc(description: "Compare list")

melnikovi · 2020-09-04T16:05:36Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls

 }

 type Mutation {
+    createCompareList(items: [ID!]): CompareList @doc(description: "Creates a new compare list. For a logged in user, the created list is assigned to the user")


Suggested change

createCompareList(items: [ID!]): CompareList @doc(description: "Creates a new compare list. For a logged in user, the created list is assigned to the user")

createCompareList(product_id: [ID!]): CompareList @doc(description: "Creates a new compare list. For a logged in user, the created list is assigned to the user")

melnikovi · 2020-09-04T16:28:12Z

design-documents/graph-ql/coverage/catalog/compare-list.md

+
+Existing table structure for compare list
+```
+catalog_compare_item


Add new field catalog_compare_list_id.

melnikovi · 2020-09-04T16:29:49Z

design-documents/graph-ql/coverage/catalog/compare-list.md

+
+This dependency can be solved by managing the compare list state in a new table
+```
+catalog_compare_list


Will only have the following fields: list_id, visitor_id. Not sure we can avoid having here visitor_id, we need somehow to make sure that catalog_compare_item.visitor_id is the same for all items related to a list.

visitor_id is generated based on Luma's session. so we cannot mock it. But it can be 0, which will imply the record is not created via Luma. And I see your point.We limit the new table to have two columns list_id, additional_data (json) and we can refer it back to the original table.

design-documents/graph-ql/coverage/catalog/compare-list.graphqls

melnikovi · 2020-10-15T19:58:35Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls

 type CompareList {
-    list_id: ID! @doc(description: " Compare list id")
+    uid: ID! @doc(description: "Compare list id")
    items: [ComparableItem] @doc(description: "Comparable products")


Would be good to make the following changes to ComparableItem in this PR

Add uid field, so that clients can understand when they receive new items

Rename priceRange to price_range

Can we have media_gallery: [MediaGalleryInterface]! instead of images: [ProductImage]!?

Why do we need productId? If we expect that in some scenarios clients may need to load more product fields, we should probably expose ProductInterface.

Agree on 2 and 3. I will update the document. I think @paliarush exposed the id for the same reason on #4. I am okay with exposing ProductInterface, as I don't foresee N + 1 concerns here.

Figured that the reason for #3 ProductImage is, MediaGalleryInterface encompasses even videos. Videos are not supported in Product compare.

Co-authored-by: Igor Melnikov <imelnikov@magento.com>

Will have to decide between product_id and exposing product interface

…ace at the ComparableItem Simplifies implementation and usage.

prabhuram93 · 2020-10-22T17:47:44Z

design-documents/graph-ql/coverage/catalog/compare-list.graphqls

-    images: [ProductImage]! @doc(description: "Product Images")
-    values: [ProductAttribute]! @doc(description: "Product comparable attributes")
+    uid: ID!
+    product: ProductInterface!


After discussion with @cpartica and @melnikovi, decided to return ProductInterface here.

Prabhu Ram added 6 commits September 2, 2020 19:14

Updated GraphQl Compare List Proposal

9a1fd86

minor fix.

d5bac14

Removing the additional id field, based on Anton's suggestion.

d3d2642

Update compare-list.md

45fb01d

Reflecting schema changes based on the new proposal.

97fa6b2

Update compare-list.graphqls

2868872

melnikovi requested changes Sep 4, 2020

View reviewed changes

melnikovi reviewed Sep 4, 2020

View reviewed changes

melnikovi added GraphQL needs update Author should update the proposal based on the feedback labels Sep 9, 2020

Prabhu Ram added 7 commits September 15, 2020 16:00

Update compare-list.graphqls

52e8c18

Update compare-list.md

5ca38b9

Update compare-list.md

8cf881a

Update compare-list.graphqls

b6e841a

Update compare-list.graphqls

28fd892

Update compare-list.md

b5ac23d

Update compare-list.md

036a5f2

melnikovi reviewed Oct 15, 2020

View reviewed changes

design-documents/graph-ql/coverage/catalog/compare-list.graphqls Outdated Show resolved Hide resolved

melnikovi reviewed Oct 15, 2020

View reviewed changes

Prabhu Ram and others added 4 commits October 15, 2020 15:29

Update design-documents/graph-ql/coverage/catalog/compare-list.graphqls

4c7db6c

Co-authored-by: Igor Melnikov <imelnikov@magento.com>

Update compare-list.graphqls

b9679f5

Removing product_id for now, since its not in scope.

bd8e5b6

Will have to decide between product_id and exposing product interface

After internal discussion, decided its better to return ProductInterf…

2f70255

…ace at the ComparableItem Simplifies implementation and usage.

prabhuram93 commented Oct 22, 2020

View reviewed changes

Prabhu Ram added 2 commits October 22, 2020 13:07

Update compare-list.graphqls

faadf50

Update compare-list.graphqls

a67bd69

melnikovi self-requested a review October 22, 2020 18:16

melnikovi approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'master' into patch-6

236d319

melnikovi merged commit 1110740 into magento:master Oct 22, 2020

melnikovi removed the needs update Author should update the proposal based on the feedback label Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Updated GraphQl Compare List Proposal #430

Updated GraphQl Compare List Proposal #430

Uh oh!

prabhuram93 commented Sep 3, 2020 •

edited

Loading

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

melnikovi Sep 4, 2020

Uh oh!

prabhuram93 Sep 4, 2020

Uh oh!

Uh oh!

melnikovi Oct 15, 2020 •

edited

Loading

Uh oh!

prabhuram93 Oct 15, 2020

Uh oh!

prabhuram93 Oct 15, 2020

Uh oh!

prabhuram93 Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	list_id: ID! @doc(description: "Compare list id")
	uid: ID! @doc(description: "Compare list uid")

	compareList(id: ID): [CompareList] @doc(description: "Active customers compare list") #id is optional. Introduces the possibility of more than 1 compare list.
	compare_list(uid: ID!): [CompareList]! @doc(description: "Active customers compare list") #id is optional. Introduces the possibility of more than 1 compare list.

	compareList(id: ID!): [CompareList] @doc(description: "Compare list")
	compare_list(uid: ID!): [CompareList]! @doc(description: "Compare list")

	createCompareList(items: [ID!]): CompareList @doc(description: "Creates a new compare list. For a logged in user, the created list is assigned to the user")
	createCompareList(product_id: [ID!]): CompareList @doc(description: "Creates a new compare list. For a logged in user, the created list is assigned to the user")

Updated GraphQl Compare List Proposal #430

Updated GraphQl Compare List Proposal #430

Uh oh!

Conversation

prabhuram93 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Requested Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melnikovi Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prabhuram93 commented Sep 3, 2020 •

edited

Loading

melnikovi Oct 15, 2020 •

edited

Loading