-
Notifications
You must be signed in to change notification settings - Fork 248
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
MSI source Import #116
MSI source Import #116
Conversation
- Add Documentation - phpcs fixes - Impelement phpunit test for ValidatorChain
- Add SPI for Delete Multiple - Improve Delte Comand - Implement SPI
- Bugfix for Intigrationstest to new source_ids - Impelment new test case for delete behavior
- Refactoring: Add Helperfunction for convert - Impelment Replace Comand - Impelment Replace Comand Test TODO: - Implement clenup for Replace Comand
- phpcs fixes - phpmd fixes
* Implementation of SourceItem delete multiple operation for specific db layer | ||
* Delete Multiple used here for performance efficient purposes over single delete operation | ||
*/ | ||
class DeleteMultiple |
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.
Looks like we need to remove
SourceItemRepositoryInterface::delete(SourceItemInterface $sourceItem);
method, as we have
SourceItemsDeleteInterface::execute(array $sourceItems);
which do the same
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.
* @param SourceItemInterface[] $sourceItems | ||
* @return int[] | ||
*/ | ||
private function getSourceIds($sourceItems): array |
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.
name of the method is not correct, as it returns source_id and sku list assigned to it
{ | ||
if (empty($sourceItems)) { | ||
throw new InputException(__('Input data is empty')); | ||
} |
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.
not sure that we need to throw an exception when someone tries to delete empty array of source Items.
For example, you have a replace strategy and you want to delete existing SourceItems and substitute with new ones, but on clean Magento installation with empty DB - delete would have an empty set
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\Inventory\Model\ResourceModel\SourceItem; |
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.
all the tests should be stored under the namespace
Magento\Inventory\Test\Unit|Integration\Model
not mixed with code. Such tests would not be even run by Travis
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\InventoryApi\Api; |
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.
don't forget to use strict typing for newly created code
* | ||
* @api | ||
*/ | ||
interface LegacyJsonHelperInterface |
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.
why do we need to introduce this interface? especially if it's already legacy?
If you want to make Backward Compatible change, legacy class
Magento\Framework\Json\Helper\Data
should be adapted as well.
Because currently, you are breaking the contract, as AbstractEntity accept
\Magento\Framework\Json\Helper\Data $jsonHelper,
But you pass
SerializerInterface $jsonHelper,
instead
*/ | ||
public function jsonEncode($valueToEncode) | ||
{ | ||
return $this->serializer->serialize($valueToEncode); |
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.
this method should be implemented proxying call to
$this->serialize($valueToEncode)
*/ | ||
public function jsonDecode($encodedValue) | ||
{ | ||
return $this->serializer->unserialize($encodedValue); |
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.
this method should be implemented proxying call to
$this->unserialize($valueToEncode)
to make all Pluginization of SerializerInterface work correctly
/** | ||
* Extension point for row validation | ||
* | ||
* @api |
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.
why do we have @api on class implementation?
/** | ||
* Extension point for source validation | ||
* | ||
* @api |
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.
no need to use @api here
…2 into msi-source-export
…o msi-source-export
Msi source export
MC-41874: Out of Stock Product is back in stock after shipping an order
Description
Import module for Multi Source Inventory
Manual testing scenarios
System > Import
page in Magento backendStock Sources
and import the CSV with append behaviorTODOs
Contribution checklist