Skip to content

#257: create new id_v2 option #28210

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

Conversation

mmezhensky
Copy link

@mmezhensky mmezhensky commented May 13, 2020

Description (*)

This PR extends product schema and added new identifier "id_v2"

Solution Architecture

Related Pull Requests

Fixed Issues (if relevant)

  1. Single mutation for adding products to cart

Manual testing scenarios (*)

query {
  products(filter: { price: { from: "0.00001" } }) {
    items {
      sku

      ... on CustomizableProductInterface {
        options {
          option_id
          title
          
          ... on CustomizableRadioOption {
            title
            option_id
            selected_option: value {
              option_type_id
              uid
            }
          }
          
          ... on CustomizableDropDownOption {
            title
            option_id
            selected_option: value {
              option_type_id
              uid
            }
          }
          
          ... on CustomizableMultipleOption {
            option_id
            title
            selected_option: value {
              option_type_id
              uid
            }
          }
          
          ... on CustomizableCheckboxOption {
            option_id
            title
            selected_option: value {
              option_type_id
              uid
            }
          }
          
          ... on CustomizableAreaOption {
            option_id
            title
            entered_option: value {
              uid
            }
          }
          
          ... on CustomizableFieldOption {
            option_id
            title
            entered_option: value {
              uid
            }
          }
          
          ... on CustomizableFileOption {
            title
            option_id
            entered_option: value {
              uid
            }
          }
            
          ... on CustomizableDateOption {
            title
            option_id
            entered_option: value {
              uid
            }
          }
        }
      }
      
      ... on ConfigurableProduct {
        variants {
          attributes {
            label
            value_index
            uid
          }
        }
      }
      
      ... on DownloadableProduct {
        downloadable_product_links {
          title
          uid
        }
      }
      
      ... on BundleProduct {
        items {
          title
          options {
            uid
          }
        }
      }
    }
  }
}

Questions or comments

⚠️ For your information, that we have to use aliases (selected_options, entered_options), because both cases have a conflicting return type for value.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented May 13, 2020

Hi @mmezhensky. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@eduard13
Copy link
Contributor

@magento run Static Tests

1 similar comment
@eduard13
Copy link
Contributor

@magento run Static Tests

@rogyar rogyar self-requested a review May 27, 2020 09:19
@eduard13 eduard13 force-pushed the 257-single-mutation-option-id-v2 branch from d5a6091 to d8d7d83 Compare May 28, 2020 06:28
@rogyar rogyar self-requested a review May 29, 2020 12:35
@rogyar rogyar marked this pull request as ready for review May 29, 2020 12:42
@rogyar rogyar added Award: category of expertise Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: test coverage labels May 29, 2020
@rogyar
Copy link
Contributor

rogyar commented May 29, 2020

Looks good to me. Let's wait for the final decision regarding the bundle product options with quantity

rogyar
rogyar previously approved these changes Jun 2, 2020
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-7608 has been created to process this Pull Request

@prabhuram93
Copy link
Contributor

Note: We can update the architecture document, but occurrences of id_v2 should be an ID type, not a String. This allows us to change the format of the ID later, and also signals to developers that they're getting an opaque value, rather than a string they can perform some operations on.

We're finishing up formalizing these guidelines, and would be great to get this in

On the same lines, ID should be a mandatory field. Would be great if we can update this on the schema.

@prabhuram93 prabhuram93 dismissed their stale review July 9, 2020 20:38

Need to update the field type from "string" to ID!. Also this deviates from the architecture document, but it is a valuable change.

@eduard13 eduard13 force-pushed the 257-single-mutation-option-id-v2 branch from b96a6da to f990c5d Compare July 14, 2020 06:47
@eduard13
Copy link
Contributor

@magento run WebAPI Tests, Static Tests, Integration Tests, Unit Tests

@eduard13
Copy link
Contributor

@magento run all tests
Hi @prabhuram93, @DrewML thank you for the suggestions, the implementation has been adjusted by renaming it to uid and making it a mandatory field. Please let me know if I missed anything.
Thank you.

@prabhuram93
Copy link
Contributor

@magento run all tests
Hi @prabhuram93, @DrewML thank you for the suggestions, the implementation has been adjusted by renaming it to uid and making it a mandatory field. Please let me know if I missed anything.
Thank you.

Looks good! Thank you @eduard13

@m2-assistant
Copy link

m2-assistant bot commented Jul 16, 2020

Hi @mmezhensky, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants