-
Notifications
You must be signed in to change notification settings - Fork 18
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 OLD PR #400
Product variants OLD PR #400
Conversation
-Added string type or message entity id to accomodate product variants
@magento run all tests |
1 similar comment
@magento run all tests |
@magento run Static Tests |
@magento run Unit Tests |
@magento run all tests |
@magento run Integration Tests |
-Added product variant write and delete functionality to Catalog Storefront
@magento run all tests |
@magento run all tests with editions EE |
1 similar comment
@magento run all tests with editions EE |
@magento run Unit Tests |
-Modified GetProductVariants and DeleteProductVariants to the VariantService
@magento run all tests with editions EE |
-static fixes
@magento run all tests with editions EE |
1 similar comment
@magento run all tests with editions EE |
@magento run Static Tests |
-Added test -Improved elasticsearch deleteByQuery -Added some exceptions for empty variant response
@magento run all tests with editions EE |
-Test fix
-Removed old ConfigurableVariantsExtractor
@@ -14,7 +14,7 @@ | |||
class Entity | |||
{ | |||
/** | |||
* @var int | |||
* @var int|string | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in which case entityId is a string?
Can we use string
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using string only. Id is a string for product variants.
@@ -49,7 +49,7 @@ public function __construct( | |||
/** | |||
* @inheritdoc | |||
*/ | |||
public function execute(array $entities, string $scope): void | |||
public function execute(array $entities, string $scope = null): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use nullable annotation ?string $scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
public function execute(array $entities, string $scope): void | ||
public function execute(array $entities, string $scope = null): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use nullable annotation ?string $scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
* | ||
* @return void | ||
*/ | ||
public function execute(array $entities, string $scope): void; | ||
public function execute(array $entities, string $scope = null): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use nullable annotation ?string $scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
@@ -49,7 +49,7 @@ public function __construct( | |||
/** | |||
* @inheritdoc | |||
*/ | |||
public function execute(array $entities, string $scope): void | |||
public function execute(array $entities, string $scope = null): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use nullable annotation ?string $scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
'product_variant' => [ | ||
//TODO: Adapt to work without store code | ||
self::EMPTY_STORE_CODE => [ | ||
'delete_by_query' => $deleteFields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use created constant here \Magento\CatalogStorefront\Model\CatalogRepository::DELETE_BY_QUERY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
} | ||
|
||
/** | ||
* Validate something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add better description to this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method description is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
$this->variantService->GetProductVariants($this->variantsGetRequestInterface); | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method description is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method description is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
-Changed entity_id type to string in order to pass product variant message correctly
use Psr\Log\LoggerInterface; | ||
|
||
/** | ||
* @inheritdoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inheritdoc should be used only on class members
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
-Fixed and added tests -Other minor fixes and improvements like syntax, docBlock, constants, etc.
-Added string type or message entity id to accommodate for product variants
Description (*)
PR moved to #413
Related Pull Requests
Fixed Issues (if relevant)
Questions or comments
Code Review Checklist (*)
See dataied checklist