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 stock ui aggregation #35

Merged

Conversation

jonfres
Copy link
Contributor

@jonfres jonfres commented Jun 26, 2017

  • created a new page for stock administration and all needed repositories, models, resource models needed for stock management
  • created API for stock management

Description

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)

vrann and others added 21 commits June 26, 2017 13:01
- created Resource models, models and APIs
- created StockRepositoryInterface and StockRepository
    - created Stock UI files and view ui layout and component for adminhtml

Merge branch 'msi-stock-ui-aggregation' of https://github.com/jonfres/magento2 into msi-stock-ui-aggregation
        - controller Stock and menu option for Manage Stock added
  - controller Stock and menu option for Manage Stock added2
- changed implementation of StockRepository
…/magento2 into msi-stock-ui-aggregation

# Conflicts:
#	app/code/Magento/Inventory/Ui/DataProvider/StockDataProvider.php
use Magento\InventoryApi\Api\Data\StockInterface;

/**
* Class Edit
Copy link
Member

Choose a reason for hiding this comment

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

remove class comment

use Magento\Framework\Controller\ResultFactory;

/**
* Class Index
Copy link
Member

Choose a reason for hiding this comment

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

remove class comment

use Magento\InventoryApi\Api\StockRepositoryInterface;

/**
* Class Save
Copy link
Member

Choose a reason for hiding this comment

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

remove class comment

}

/**
* {@inheritdoc}
Copy link
Member

Choose a reason for hiding this comment

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

use pls @inheritdoc without {}

$requestData = $this->getRequest()->getParam('general');
if ($this->getRequest()->isPost() && $requestData) {
try {
$stockId = !empty($requestData[StockInterface::STOCK_ID])
Copy link
Member

Choose a reason for hiding this comment

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

non-readable negative short if improve this for better readable code

}

/**
* @param int $stockId
Copy link
Member

Choose a reason for hiding this comment

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

add some function description

</editor>
</settings>
</column>
<!--<column name="contact_name" sortOrder="40">
Copy link
Member

Choose a reason for hiding this comment

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

Comment code pls remove

@naydav naydav changed the base branch from develop to msi-stock-ui-aggregation July 4, 2017 16:06
@naydav naydav merged commit 629bfa8 into magento:msi-stock-ui-aggregation Jul 4, 2017
$connection,
$resource
);
}
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 constructor, if all the parameters are just proxying to parent constructor

use Magento\InventoryApi\Api\Data\StockInterface;

/**
* Class Source
Copy link
Contributor

Choose a reason for hiding this comment

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

please provide more meaningful description

@@ -0,0 +1,134 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to have delete()
operation in Stock repository.

When Stock is removed, we need to remove all stock items related to it (could be done using Foreign Key on DB level)

use Magento\Framework\Api\ExtensibleDataInterface;

/**
* SourceCarrierLink interface
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to provide more meaningful description to this interface.

* SourceCarrierLink interface
* @api
*/
interface SourceStockLinkInterface extends ExtensibleDataInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no class which implements this interface

* @param string $name
* @return void
*/
public function setName($name);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to specify
getExtensionAttributes/setExtensionAttributes methods and specify own types they accept

use Magento\Framework\Api\SearchResultsInterface;

/**
* @api
Copy link
Contributor

Choose a reason for hiding this comment

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

please add description to all interfaces marked as api

namespace Magento\InventoryApi\Api;

/**
* @api
Copy link
Contributor

Choose a reason for hiding this comment

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

meaningful description should be added

*/
public function getList(
\Magento\Framework\Api\SearchCriteriaInterface $searchCriteria = null
);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to add deleteById method to the interface

* @param \Magento\Framework\Api\SearchCriteriaInterface $searchCriteria
* @return \Magento\InventoryApi\Api\Data\StockSearchResultsInterface
*/
public function getList(
Copy link
Contributor

Choose a reason for hiding this comment

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

we have the only field (Name) by which we could make filtering, don't think that we need to over complicate our API and accept SearchCriteria in this case.
$name parameter should be okay

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