Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
43 changes: 43 additions & 0 deletions app/code/Magento/Inventory/Model/GetProductQuantityInStock.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Model;

use Magento\Inventory\Model\ResourceModel\Reservation\ReservationQuantity;
use Magento\Inventory\Model\ResourceModel\Stock\StockItemQuantity;
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
Copy link
Contributor

Choose a reason for hiding this comment

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

This service looks like a perfect candidate to be covered with both:

  • Integration
    and
  • Web API
    testing

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.

Looks like to have two separate interfaces for API and SPI is redundant
Implementation of API is only proxy to SPI

We have the same situation with SourceItemsSave operation
And we decided to have only one interface (which presents API and SPI in one interface)

Interface
https://github.com/magento-engcom/msi/blob/develop/app/code/Magento/InventoryApi/Api/SourceItemsSaveInterface.php

Implementation
https://github.com/magento-engcom/msi/blob/develop/app/code/Magento/Inventory/Model/SourceItem/Command/SourceItemsSave.php

For Repository we need to extract SPI parts in separate set of interfaces due Repository represents Facade pattern (accumulate some functionality together)
But for single command, it looks like overhead (after creating we need to maintain this code)

Thus now looks like we have some mess in our interfaces:
GetProductQuantityInStockInterface
GetProductQuantityInterface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As proposed by @naydav and discussed with @maghamed we will simplify the implementation getting rid of the command layer;
thus we will delegate effective computation to resource model, injected into the services in place of the command.

{
/**
* GetProductQuantityInStock constructor.
*
* @param StockItemQuantity $stockItemQty
* @param ReservationQuantity $reservationQty
*/
public function __construct(
StockItemQuantity $stockItemQty,
ReservationQuantity $reservationQty
) {
$this->stockItemQty = $stockItemQty;
$this->reservationQty = $reservationQty;
}

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

Choose a reason for hiding this comment

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

if we are putting both positive and negative values to the Reservation.
Looks like we need to Add $this->reservationQty->execute($sku, $stockId);
But not Substract

For example,
Stock is 100 products.
We have Reservation for -10 products

100 - (-10) will lead to increase of stock but not deducation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because I intended a "reserved quantity" (positive number) as something I don't have to consider as available because it's "reserved", thus not available and thus something that has to be subtracted from total quantity.

Your interpretation is opposite and it's ok to use it but that also means that there is space for interpretation that is error prone.

I will fix as you suggest but probably we should find a naming that's less subject to interpretation.

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've just re-read your spec here and you were perfectly clear on the meaning of adding (+) and subtracting (-) but for some reason, I inverted it in my mind

return (float) $productQtyInStock;
}
}
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,60 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Model\ResourceModel\Reservation;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add declare(strict_types=1); for all newly created files in the scope of MSI project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on which line do you prefer to add this directive?

  • just after <?php, before copyrignt disclaimer comment
  • just after copyrignt disclaimer comment
  • just afternamespace directive
  • just afternamespace and use directives

Copy link
Contributor Author

@aleron75 aleron75 Sep 22, 2017

Choose a reason for hiding this comment

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

just find this tool: https://github.com/dypa/declare-strict-types
didn't use it yet but maybe can be useful

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 choose is
just after copyrignt disclaimer comment
It should be before any code ... but copyright looks like some additional meta data for class (also it can be generated via tools automatically)


use Magento\Framework\App\ResourceConnection;
use Magento\Inventory\Setup\Operation\CreateReservationTable;
use Magento\InventoryApi\Api\Data\ReservationInterface;

/**
* The resource model responsible for retrieving Reservation Quantity.
* Used by Service Contracts that are agnostic to the Data Access Layer.
*/
class ReservationQuantity
{
/**
* @var ResourceConnection
*/
private $resource;

/**
* ReservationQuantity constructor.
*
* @param ResourceConnection $resource
*/
public function __construct(
ResourceConnection $resource
) {
$this->resource = $resource;
}

/**
* Given a product sku and a stock id, return reservation quantity.
*
* @param string $sku
* @param int $stockId
* @return float
*/
public function execute(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,84 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Model\ResourceModel\Stock;

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;

/**
* The resource model responsible for retrieving StockItem Quantity.
* Used by Service Contracts that are agnostic to the Data Access Layer.
*/
class StockItemQuantity
{
/**
* @var ResourceConnection
*/
private $resource;

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

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

/**
* StockItemQuantity 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;
}

/**
* Given a product sku and a stock id, return stock item quantity.
*
* @param string $sku
* @param int $stockId
* @return float
*/
public function execute(string $sku, int $stockId): float
{
$indexName = $this->indexNameBuilder
->setIndexId(StockItemIndexerInterface::INDEXER_ID)
->addDimension('stock_', $stockId)
->setAlias(Alias::ALIAS_MAIN)
->create();
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW , why do we have a builder with method create, but not build
@naydav that's probably question to you

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.

Yes, we can rename method to build

Also, if I do not mistake we decided to introduce some sugar service which must encapsulate logic of table name resolving


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

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

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

Limit is redundant because SKU is unique key
So we have only one record per product in this table


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

return (float) $stockItemQty;
}
}
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;
}