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

Product variants #413

Merged
merged 26 commits into from
Nov 13, 2020
Merged

Conversation

bricht
Copy link
Contributor

@bricht bricht commented Nov 5, 2020

Description (*)

Related Pull Requests

magento/commerce-data-export#8
https://github.com/magento-commerce/graphql/pull/4
https://github.com/magento-commerce/magento2-infrastructure/pull/1368
https://git.corp.adobe.com/ecp/api-protos/pull/22
duhon/magento-docker#30

Fixed Issues (if relevant)

  1. Fixes [357.3] Import data to SF Application  #331
  2. Fixes [357.4] GetProductVariants service #332
  3. Fixes [357.6] DeleteProductVariants service #335

Questions or comments

Code Review Checklist (*)

See detailed checklist

  • Story AC is completed
  • proposed changes correspond to Magento Technical Vision
  • new or changed code is covered with web-api/integration tests (if applicable)
    • expected results in test verified with data from fixture
  • no backward incompatible changes
  • Export API (et_schema.xml) and SF API schemas (proto schema) are reflected in the codebase
    • prerequisite: story branch created with all needed generated classes according to proposes schema-changes
    • DTO classes do not contain any manual changes (Magento\CatalogExportApi*, Magento\CatalogStorefrontApi*)
  • Class usage: magento/catalog-storefront repo don't use directly classes from magento/saas-export repo and vise-verse
    • Check composer.json dependencies
  • Legacy code is deleted
    • Any Data Providers present in Connector part (Magento\CatalogStorefrontConnector, Magento*Extractor modules)
    • And Data Providers from Export API (magento/saas-export repo) that is not relevant anymore
    • Any DTO for Export API/SF API which does not reflect current schema: et_schema, proto schema
    • Any “mapper” on Message Broker (between Export API and SF API)
      • if mapper still needed, verify fields used in mapping, remove not relevant fields

jekabs and others added 13 commits October 21, 2020 16:06
-Added string type or message entity id to accomodate product variants
-Added product variant write and delete functionality to Catalog Storefront
-Modified GetProductVariants and DeleteProductVariants to the VariantService
-static fixes
-Added test
-Improved elasticsearch deleteByQuery
-Added some exceptions for empty variant response
-Test fix
-Removed old ConfigurableVariantsExtractor
-Changed entity_id type to string in order to pass product variant message correctly
-Fixed and added tests
-Other minor fixes and improvements like syntax, docBlock, constants, etc.
@bricht
Copy link
Contributor Author

bricht commented Nov 5, 2020

@magento run all tests
without extensions magento/saas-export, magento/data-solutions-services-id, magento/magento-services-connector

-Test fix
@bricht
Copy link
Contributor Author

bricht commented Nov 6, 2020

@magento run all tests
without extensions magento/saas-export, magento/data-solutions-services-id, magento/magento-services-connector

-Extended searchEntries and other small fixes
@bricht
Copy link
Contributor Author

bricht commented Nov 9, 2020

@magento run all tests
without extensions magento/saas-export, magento/data-solutions-services-id, magento/magento-services-connector

@bricht
Copy link
Contributor Author

bricht commented Nov 9, 2020

@magento run Integration Tests
without extensions magento/saas-export, magento/data-solutions-services-id, magento/magento-services-connector

@@ -41,6 +41,8 @@ public function getAliasName(): string
/**
* Get current data source name of storage taking into account version of the data source.
*
* TODO: Adapt to work without store code
Copy link
Contributor

Choose a reason for hiding this comment

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

please create a separate ticket for this and mention id here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 72 to 73
$this->productRepository->get('simple_10'),
$this->productRepository->get('simple_20')
Copy link
Contributor

Choose a reason for hiding this comment

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

add product variants that represent 2 configurable options:
size: s, m, l
color: red, green, blue
so you'll have 9 simple products (variants):

s-red
s-green
s-blue
m-red
m-green
m-blue
l-red
l-green
l-blue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* @throws RuntimeException
* @throws \Throwable
*/
public function GetProductVariants(ProductVariantRequestInterface $request): ProductVariantResponseInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I have created configurable product with 2 options:
Color: red, green, blue
Size: s, m, l

result of this method is incorrect

image

Each variation should have 2 option values, but as we see on the screenshot only half of them have 2 options, the rest of them have 1 option value for some reason.

For id configurable/32/36 there's only one option value 32:color/Y29uZmlndXJhYmxlLzkzLzE0 however in storage we have 2 records

image

Copy link
Contributor Author

@bricht bricht Nov 10, 2020

Choose a reason for hiding this comment

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

Was a bug with max search size. Changed it to 5000 and added a ticket for pagination.

Comment on lines 104 to 107
$simple1 = $this->getProduct(self::SIMPLE1_SKU);
$simple1Id = $simple1->getId();
$simple2 = $this->getProduct(self::SIMPLE2_SKU);
$simple2Id = $simple2->getId();
Copy link
Contributor

Choose a reason for hiding this comment

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

add more variants to configurable product

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@RuslanKostiv1
Copy link
Contributor

@magento run all tests

-Added more variants to tests
-Fixed search bug
@bricht
Copy link
Contributor Author

bricht commented Nov 10, 2020

@magento run all tests

@RuslanKostiv1
Copy link
Contributor

@magento run all tests

@RuslanKostiv1
Copy link
Contributor

@magento run all tests

@bricht
Copy link
Contributor Author

bricht commented Nov 12, 2020

@magento run WebApi Tests

@bricht
Copy link
Contributor Author

bricht commented Nov 12, 2020

@magento run WebAPI Tests

@RuslanKostiv1
Copy link
Contributor

@magento run all tests

1 similar comment
@RuslanKostiv1
Copy link
Contributor

@magento run all tests

@RuslanKostiv1
Copy link
Contributor

@magento run Static tests

@RuslanKostiv1
Copy link
Contributor

@magento run Static Tests

-Added debug statement to a test
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run WebAPI Tests

This reverts commit 3ab8815.
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run WebAPI Tests

-Added temp code for testing
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run WebAPI Tests

@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run Integration Tests

@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run Unit Tests

jekabs added 2 commits November 13, 2020 13:54
This reverts commit 0c0fa11.
-Added debug code
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run all tests

This reverts commit 7775563.
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run all tests

2 similar comments
@bricht
Copy link
Contributor Author

bricht commented Nov 13, 2020

@magento run all tests

@RuslanKostiv1
Copy link
Contributor

@magento run all tests

@RuslanKostiv1
Copy link
Contributor

@magento run Static Tests

1 similar comment
@RuslanKostiv1
Copy link
Contributor

@magento run Static Tests

@RuslanKostiv1
Copy link
Contributor

@magento import pr to magento-commerce/catalog-storefront-ce

@magento-engcom-team
Copy link

@RuslanKostiv1 the pull request successfully imported.

@mmansoor-magento mmansoor-magento merged commit 30e0b62 into magento:develop Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants