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

MSI source Import #116

Merged
merged 44 commits into from
Oct 29, 2017
Merged

MSI source Import #116

merged 44 commits into from
Oct 29, 2017

Conversation

larsroettig
Copy link
Member

@larsroettig larsroettig commented Oct 3, 2017

Description

Import module for Multi Source Inventory

Manual testing scenarios

  1. Create a CSV file with e.g. following content
"source","sku","qty","status"
1,"test",60,1
2,"test",2,0
3,"test",34,1
1,"foobar",55,1
2,"foobar",44,1
  1. Go to System > Import page in Magento backend
  2. Choose Entity Type Stock Sources and import the CSV with append behavior

TODOs

  • Implement Import Entity for Stock Sources
    • File validation
    • DELETE behavior
    • REPLACE behavior
    • APPEND behavior
  • Integration tests for Import

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 on Travis CI are green)

- Add Documentation
- phpcs fixes
- Impelement phpunit test for ValidatorChain
@naydav naydav added the WIP label Oct 11, 2017
larsroettig added 6 commits October 13, 2017 17:31
- 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
* Implementation of SourceItem delete multiple operation for specific db layer
* Delete Multiple used here for performance efficient purposes over single delete operation
*/
class DeleteMultiple
Copy link
Contributor

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

Copy link
Member Author

@larsroettig larsroettig Oct 22, 2017

Choose a reason for hiding this comment

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

Hi @maghamed if we remove we will break the current delete function
behaviour. Because the current Delete Multiple removes all entries from the Database by only matching SKU.

If you mean it is okay I open for this an extra PR:
#140

* @param SourceItemInterface[] $sourceItems
* @return int[]
*/
private function getSourceIds($sourceItems): array
Copy link
Contributor

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'));
}
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

larsroettig added 2 commits October 20, 2017 19:42
- declare strict_types fot Import/Export Module
- Add Replace Comand
- Improve Coverrage
- phpmd und phpcs
- declare strict_types fot Import/Export Module
- phpmd and phpunit fix
- review fixes
@larsroettig larsroettig changed the title WIP: MSI source import/export MSI source Import Oct 26, 2017
@larsroettig larsroettig removed the WIP label Oct 26, 2017
@maghamed maghamed merged commit 307de4f into develop Oct 29, 2017
@maghamed maghamed deleted the msi-source-import-export branch December 11, 2018 18:13
magento-cicd2 pushed a commit that referenced this pull request May 12, 2021
MC-41874: Out of Stock Product is back in stock after shipping an order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants