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

Update proposal for custom attributes schema #456

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

Conversation

melnikovi
Copy link
Member

Problem

Current schema for custom attributes doesn't support attributes that can have multiple values (checkbox, multiselect).

Solution

Requested Reviewers

@nrkapoor
Copy link
Contributor

@melnikovi Does this align with the custom attribute proposal from @akaplya for Product? #429

@@ -8,13 +8,42 @@ One workaround for "getting all fields" is based on schema introspection, it all

# Proposed solution

To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries, `custom_attributes: [CustomAttribute]!` container will be introduced.
To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries,we can introduce `custom_attributes: [CustomAttribute]!` container.

```graphql
type CustomAttribute {
Copy link
Member

@mslabko mslabko Oct 20, 2020

Choose a reason for hiding this comment

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

I think we have to add "type" opinion here to be able to render the appropriate template (datetime, text, multi-select, checkbox ...)

E.g.

"attributes":[
    {
      "code":"multiselect_attribute",
      "type":"multiselect",
      "values":[
            "Option 1",
            "Option 2"
      ]
    }
  ]

Copy link
Member Author

Choose a reason for hiding this comment

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

Attributes are going to be displayed as information and not as fields you can control, so I don't think it's necessary.

Copy link
Member

@mslabko mslabko Oct 22, 2020

Choose a reason for hiding this comment

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

it will be useful to render field properly on UI without complex logic. E.g. for "Date" field - "10-11-2020"

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, they are needed for the forms

Copy link
Contributor

Choose a reason for hiding this comment

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

We have this data already available in the scope of customAttributeMetadata query. Since update frequency for attribute metadata and product attribute values is different, it makes sense to separate them to avoid excessive payload size.

@melnikovi melnikovi added the needs update Author should update the proposal based on the feedback label Oct 22, 2020
code: String!
}

type SingleValueCustomAttribute extends CustomAttributeInterface {
Copy link
Contributor

@cpartica cpartica Oct 22, 2020

Choose a reason for hiding this comment

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

even if this adds more complexity, it's the graphql way to do it and I prefer it

@@ -8,13 +8,42 @@ One workaround for "getting all fields" is based on schema introspection, it all

# Proposed solution

To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries, `custom_attributes: [CustomAttribute]!` container will be introduced.
To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries, we can introduce `custom_attributes: [CustomAttribute]!` container (recommended approach).
Copy link
Contributor

Choose a reason for hiding this comment

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

do fragments bring any value for PWA since we still prefer these flat single structure arrays vs infterfaces?

@@ -0,0 +1,222 @@
type Query {
customAttributeMetadataV2(attributes: [AttributeMetadataInput!]!): CustomAttributeMetadata
customAttributesLists(listType: CustomAttributesListsEnum): CustomAttributeMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

customAttributesLists - do we need a better name for this

@cpartica cpartica requested a review from prabhuram93 March 10, 2021 17:13
@paliarush
Copy link
Contributor

paliarush commented Mar 29, 2021

  1. We need to make sure that data and metadata is not mixed in the same queries because they have different life cycles. Metadata can be cached on the client and reused between queries for data.
  2. Each schema modification must be covered with the example of usage. Suggestion is to split the original proposal into smaller ones, which address one problem at a time.
  3. We need to add proposed schema to the running server to make sure it is syntactically correct. For interfaces we can temporarily use an existing type resolver to bypass errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-guild GraphQL needs update Author should update the proposal based on the feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants