Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/code/Magento/Inventory/Model/Reservation.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ class Reservation implements ReservationInterface
*/
private $metadata;

/**
* Reservation constructor.
*
* @param int|null $reservationId
* @param int $stockId
* @param string $sku
* @param float $quantity
* @param null $metadata
*/
public function __construct(
$reservationId,
int $stockId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public function setMetadata(string $metadata = null): ReservationBuilderInterfac
public function build(): ReservationInterface
{
$data = [
ReservationInterface::RESERVATION_ID => null,
Copy link

Choose a reason for hiding this comment

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

Why do you add this string? By default reservation id is null
But maybe I do not take into account some cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that If we don't pass the reservation ID, the constructor (used by the object manager) will throw an exception because it's a constructor mandatory parameter

ReservationInterface::STOCK_ID => $this->stockId,
ReservationInterface::SKU => $this->sku,
ReservationInterface::QUANTITY => $this->quantity,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Model\Stock\Command;
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to put this implementation upper level,
as it's going to be one of the most-used services provided by Inventory
(as all the front-end code needs Get Product Qty to render some data on front-end, make checkout etc.)

So, I propose to put it under this namespace:
Magento\Inventory\Model


use Magento\Framework\App\ResourceConnection;
use Magento\Inventory\Indexer\Alias;
use Magento\Inventory\Indexer\IndexNameBuilder;
use Magento\Inventory\Indexer\IndexNameResolverInterface;
use Magento\Inventory\Indexer\StockItem\IndexStructure as StockItemIndex;
use Magento\Inventory\Indexer\StockItemIndexerInterface;
use Magento\Inventory\Setup\Operation\CreateReservationTable;
use Magento\InventoryApi\Api\Data\ReservationInterface;
use Magento\InventoryApi\Api\GetProductQuantityInStockInterface;

/**
* Return Quantity of products available to be sold by Product SKU and Stock Id
*
* @see \Magento\InventoryApi\Api\GetProductQuantityInStockInterface
* @api
*/
class GetProductQuantityInStock implements GetProductQuantityInStockInterface
{
/**
* @var ResourceConnection
*/
private $resource;

/**
* @var IndexNameBuilder
*/
private $indexNameBuilder;

/**
* @var IndexNameResolverInterface
*/
private $indexNameResolver;

/**
* GetProductQuantityInStock constructor.
*
* @param ResourceConnection $resource
* @param IndexNameBuilder $indexNameBuilder
* @param IndexNameResolverInterface $indexNameResolver
*/
public function __construct(
ResourceConnection $resource,
IndexNameBuilder $indexNameBuilder,
IndexNameResolverInterface $indexNameResolver
) {
$this->resource = $resource;
$this->indexNameBuilder = $indexNameBuilder;
$this->indexNameResolver = $indexNameResolver;
}

/**
* @inheritdoc
*/
public function execute(string $sku, int $stockId): float
{
$productQtyInStock = $this->getStockItemQty($sku, $stockId) - $this->getReservationQty($sku, $stockId);
return (float) $productQtyInStock;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@aleron75 you raised a correct question.
Whether MySQL logic should be placed in the Domain Service implementation.

we usually don't do that introducing intermediate level of abstraction.
For example, Repository does not make direct requests to MySQL table, what it does - it uses ResourceModels to get needed data.

Ideally, Implementations of Service Contracts should be agnostic to the Data Access Layer.

What I recommend to do in this particular case - I propose to introduce two Models and inject them into this Service.

  • The first model would be responsible for retrieving StockItem Quantity (implementation of your getStockItemQty method).
  • The second one would be responsible for getting Reservation Quantity (getReservationQty in current code)

So, you would end up in current service with next code:

$productQtyInStock = $this->stockItemQty->execute($sku, $stockId) - 
$this->reservationQty->execute($sku, $stockId);
return (float) $productQtyInStock;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, just to be sure, by "I propose to introduce two Models" you mean Resource Models, is it right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aleron75 Correct, Resource models


/**
* Return product quantity in stock.
*
* @param string $sku
* @param int $stockId
* @return float
*/
private function getStockItemQty(string $sku, int $stockId): float
{
$indexName = $this->indexNameBuilder
->setIndexId(StockItemIndexerInterface::INDEXER_ID)
->addDimension('stock_', $stockId)
->setAlias(Alias::ALIAS_MAIN)
->create();

$stockItemTableName = $this->indexNameResolver->resolveName($indexName);

$connection = $this->resource->getConnection();

$select = $connection->select()
->from($stockItemTableName, [StockItemIndex::QUANTITY])
->where(StockItemIndex::SKU . '=?', $sku)
->limit(1);

$stockItemQty = $connection->fetchOne($select);
if (false === $stockItemQty) {
$stockItemQty = 0;
}

return (float) $stockItemQty;
}

/**
* Return the sum of all product reservations in stock.
*
* @param string $sku
* @param int $stockId
* @return float
*/
private function getReservationQty(string $sku, int $stockId): float
{
$connection = $this->resource->getConnection();

$reservationTableName = $connection->getTableName(CreateReservationTable::TABLE_NAME_RESERVATION);

$select = $connection->select()
->from($reservationTableName, [ReservationInterface::QUANTITY => 'sum(' . ReservationInterface::QUANTITY . ')'])
->where(ReservationInterface::SKU . '=?', $sku)
->where(ReservationInterface::STOCK_ID . '=?', $stockId)
->limit(1);

$reservationQty = $connection->fetchOne($select);
if (false === $reservationQty) {
$reservationQty = 0;
}

return (float) $reservationQty;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* 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.

whether namespace under which I put the service is correct;

Yes, the namespace is correct.
As we are going to expose this service as a Public API being used in client code - it should be placed in the Magento\InventoryApi\Api


/**
* Service which returns Quantity of products available to be sold by Product SKU and Stock Id
*
* @api
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, we need to add this service to Web API webapi.xml

in
https://github.com/magento-engcom/msi/blob/develop/app/code/Magento/InventoryApi/etc/webapi.xml
to make it reachable from outside of Magento using "headless" approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, wondering if I can use the Magento_InventoryApi::stock ACL for that API method or should create a new one for reading stock data

Copy link

@naydav naydav Sep 25, 2017

Choose a reason for hiding this comment

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

We have open task for refactoring of our ACL rules
Generally, I believe that we need to simplify our resource counts.
Single resource Magento_InventoryApi::stock is enough for all operation with stock

But in this case need to discuss maybe need to relate on some product resource

*/
interface GetProductQuantityInStockInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

whether naming is correct;

It's a good question
When I proposed a name for this service - GetProductQuantityInStockInterface
I meant that this service will "retrieve product quantity which is available (in stock)"
We use this naming currently (In stock/ Out of stock).

But, having "Stock" entity introduced by MSI.
The service name could be "read" differently. Like - get product quantity for specified Stock (Stock entity).

Currently, this service accepts $stockId as one of the parameters
public function execute(string $sku, int $stockId): float;
Thus, following this signature - service returns Qty for provided $stockId

Based on the above IMO current naming - GetProductQuantityInStockInterface is appropriate.
But if you have objections - let's discuss and re-name one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, here we have a term "stock" that violates the "ubiquitous language" DDD principle because in the same context it can mean both the "available quantity" (more high-level) and the "virtual aggregation" (more low-level);

probably we should think about it;

what's more, as I introduced during our last weekly meeting, in the future, when we will have the sales channel connected to our stock (virtual aggregation), maybe this service could be better named GetProductQuantityInSalesChannelInterface but at the moment it is good enough to leave it as is

Copy link
Contributor

Choose a reason for hiding this comment

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

@aleron75 you made a very true statement regarding violation of "ubiquitous language".
There is definitely an issue with this naming.

Regarding GetProductQuantityInSalesChannelInterface I thought about this.
But came up to the conclusion that naming like InSalesChannelInterface sounds like a "leaky abstraction".
As I am not sure that on the level of this interface we should expose a knowledge that we have a relationship SalesChannel to Stock and that it's enough just to provide $stockId to get Product Qty for your Sales Channel.
Moreover potentially we could have the single Stock being re-usable by different Sales channels.
For example, there are 3 websites:
website A, website B, website C
and we have the only stock - Stock A
Thus, all of those websites will use the same stock

So, semantically it's incorrect to say - give me product quantity for current Sales Channel providing $stockId as the parameter.

Does it make sense?

Copy link
Contributor Author

@aleron75 aleron75 Sep 19, 2017

Choose a reason for hiding this comment

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

semantically it's incorrect to say - give me product quantity for current Sales Channel providing $stockId as the parameter

you're perfectly right, it should be instead:

"give me product quantity for current Sales Channel providing some channel id as the parameter"

where the $channelId should be derived from the website in the current context;

by the way at the moment let's leave the service as is and just postpone this discussion when we'll work on the channel entity; thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

@aleron75 100% agree, let's see how our interfaces will evolve and later align naming accordingly.

Copy link

@naydav naydav Sep 25, 2017

Choose a reason for hiding this comment

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

My opinion is to follow YAGNI principle.
Now we do not have the clear vision about sales channel abstraction. How to do this extensible and clear...
And we have chance to create service which will not be related to our future implementation

So we should not create service related to Sales Channels. We will do it when we will work on this task.
I believe that we could create some prototype (how to work with Sales Channels) in separate branch or provide some interfaces on our wiki page
But we need to keep our develop branch in clear state

{
/**
* Get Product Quantity for given SKU in a given Stock
*
* @param string $sku
* @param int $stockId
* @return float
*/
public function execute(string $sku, int $stockId): float;
}