-
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
Introduce Stock quantity calculation #94
Conversation
- Add DocBlock to Reservtion constructor - Fix ReservationBuilder not passing Reservation ID
- Add GetProductQuantityInStock service interface and implementation
{ | ||
$productQtyInStock = $this->getStockItemQty($sku, $stockId) - $this->getReservationQty($sku, $stockId); | ||
return (float) $productQtyInStock; | ||
} |
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.
@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;
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.
sounds good, just to be sure, by "I propose to introduce two Models" you mean Resource Models, is it right?
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.
@aleron75 Correct, Resource models
* Copyright © Magento, Inc. All rights reserved. | ||
* 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.
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
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Inventory\Model\Stock\Command; |
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.
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
* | ||
* @api | ||
*/ | ||
interface GetProductQuantityInStockInterface |
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.
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
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.
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
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.
@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?
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.
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
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.
@aleron75 100% agree, let's see how our interfaces will evolve and later align naming accordingly.
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.
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
- Move GetProductQuantityInStock service to upper namespace - Implement GetProductQuantityInStock and ReservationQuantity resource models - Refactor GetProductQuantityInStock service to use GetProductQuantityInStock and ReservationQuantity resource models
I have a question regarding the Since at this abstraction level we don't deal with "status" (indeed, disabled sources and disabled quantities have already been taken into account by indexation), is it a simple check that product qty in stock is greater than zero? If so, is it correct to inject the The first solution seems correct to me since I wouldn't repeat the same code but I want to be sure that using a service in a service is an accepted practice in M2. Thank you |
public function execute(string $sku, int $stockId): float | ||
{ | ||
$productQtyInStock = $this->stockItemQty->execute($sku, $stockId) - | ||
$this->reservationQty->execute($sku, $stockId); |
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.
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
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.
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.
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.
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
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Inventory\Model\ResourceModel\Reservation; |
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.
let's add declare(strict_types=1);
for all newly created files in the scope of MSI project
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.
on which line do you prefer to add this directive?
- just after
<?php
, before copyrignt disclaimer comment - just after copyrignt disclaimer comment
- just after
namespace
directive - just after
namespace
anduse
directives
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.
just find this tool: https://github.com/dypa/declare-strict-types
didn't use it yet but maybe can be useful
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.
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)
->setIndexId(StockItemIndexerInterface::INDEXER_ID) | ||
->addDimension('stock_', $stockId) | ||
->setAlias(Alias::ALIAS_MAIN) | ||
->create(); |
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.
BTW , why do we have a builder with method create
, but not build
@naydav that's probably question to you
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.
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
* | ||
* @api | ||
*/ | ||
interface GetProductQuantityInStockInterface |
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.
@aleron75 100% agree, let's see how our interfaces will evolve and later align naming accordingly.
* @see \Magento\InventoryApi\Api\GetProductQuantityInStockInterface | ||
* @api | ||
*/ | ||
class GetProductQuantityInStock implements GetProductQuantityInStockInterface |
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 service looks like a perfect candidate to be covered with both:
- Integration
and - Web API
testing
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 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)
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
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.
/** | ||
* Service which returns Quantity of products available to be sold by Product SKU and Stock Id | ||
* | ||
* @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.
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
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.
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
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.
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
@aleron75 Regarding your question:
Very valid question! Based on the above I propose to introduce SPI interface responsible for this calculation. And out of the box it would be simple and straightforward (simple check that product qty in stock is greater than zero). Pretty similar what we have with Repositories when we have an API and SPI with command. And as you proposed further. you can re-use this command in the service As you correctly mentioned:
Usage of one service inside the implementation of another one - is not recommended (this is no strict rule, but it's considered as a good practice). |
Hi, regarding the suggestion below:
If I understand correctly, here is a prototype of what I should implement:
is the above correct or am I missing something? thank you |
- Add webapi declaration for GetAssignedSourcesForStockInterface service - Fix quantity calculation by adding (not subtracting) reservation qty
- Introduce the GetProductQuantity command - Change GetProductQuantityInStock and IsProductInStock service implementation to use GetProductQuantity command - Add webapi declaration for IsProductInStockInterface service
- Add Cleanup resource model
- Fix coding style issues
* @see \Magento\InventoryApi\Api\GetProductQuantityInStockInterface | ||
* @api | ||
*/ | ||
class GetProductQuantityInStock implements GetProductQuantityInStockInterface |
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 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)
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
private $commandGetProductQuantity; | ||
|
||
/** | ||
* IsProductInStock constructor. |
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.
Extra comments
Now we have open PRs with removing this comments
https://github.com/magento-engcom/msi/pull/103/files
Generally, we need to expand static tests. We need to add one more rule to our current rules set.
But now to do this task in scope of MSI looks like overhead
@@ -109,6 +109,7 @@ public function setMetadata(string $metadata = null): ReservationBuilderInterfac | |||
public function build(): ReservationInterface | |||
{ | |||
$data = [ | |||
ReservationInterface::RESERVATION_ID => null, |
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 you add this string? By default reservation id is null
But maybe I do not take into account some cause
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.
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
->having('sum(' . ReservationInterface::QUANTITY . ') = 0'); | ||
$groupedReservationIds = $connection->fetchCol($select, 'grouped_reservation_ids'); | ||
|
||
foreach ($groupedReservationIds as $reservationIds) { |
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.
Maybe it is better to collect ids and perform single query instead to perform query in cycle
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Inventory\Model\ResourceModel\Reservation; |
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.
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)
$select = $connection->select() | ||
->from($stockItemTableName, [StockItemIndex::QUANTITY]) | ||
->where(StockItemIndex::SKU . '=?', $sku) | ||
->limit(1); |
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.
Limit is redundant because SKU is unique key
So we have only one record per product in this table
{ | ||
$productQtyInStock = $this->stockItemQty->execute($sku, $stockId) + | ||
$this->reservationQty->execute($sku, $stockId); | ||
return (float) $productQtyInStock; |
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.
(float)
Looks like is redundant typecasting
Now we have clear declaration of return type in StockItemQuantity and ReservationQuantity
So there is no possibility to get some type different from float
/** | ||
* Service which returns Quantity of products available to be sold by Product SKU and Stock Id | ||
* | ||
* @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.
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
* | ||
* @api | ||
*/ | ||
interface GetProductQuantityInStockInterface |
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.
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
* | ||
* @package Magento\Inventory\Model\ResourceModel\Reservation | ||
*/ | ||
class Cleanup |
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.
Maybe we need to provide SPI interface for this action
For example, we have SPI for creation of reservations and I could replace implementation on my own (for example I would like to save reservation in some key-value storage)
But we don't provide the possibility to replace the implementation of reservation deleting.
It is a little strange for me
- Remove command layer for GetProductQuantityInStock and IsProductInStock and use resource model - Add declare(strict_types=1); directive after copyrignt disclaimer comment - Delete some extra comments - Provide ReservationCleanup SPI interface and concrete implementation - Remove useless limit(1) from query - Remove some redundant typecasting
- Delete some extra comments
- Fix wrong reservation table quantity field specification - Fix preferences in di.xml - Add integration test for GetProductQuantityInStock service - Add integration test for IsProductInStock service
- Remove useless dependency from Checker in GetProductQuantityInStockTest
{ | ||
$productQtyInStock = $this->stockItemQty->execute($sku, $stockId) + | ||
$this->reservationQty->execute($sku, $stockId); | ||
return $productQtyInStock > 0; |
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.
I believe we could leave as-is current implementation for now.
But a little bit further we will have a story from our Backlog
https://github.com/magento-engcom/msi/wiki/MSI-Roadmap
Make Stock calculation and check availability services to support Backorders functionality (M)
where we would need to add a support of Backorders. Also Dropshipping functionality could also affect this service
['grouped_reservation_ids' => 'group_concat(' . ReservationInterface::RESERVATION_ID . ')'] | ||
) | ||
->group([ReservationInterface::STOCK_ID, ReservationInterface::SKU]) | ||
->having('sum(' . ReservationInterface::QUANTITY . ') = 0'); |
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.
It's not the most effective way of cleaning up reservations because we will remove all reservations (for given stock_id and sku) just in case - all the reservations quantities summing up will result 0
For example, if we have
SKU STOCK_ID Reservation
sku-1 1 -10
sku-1 1 5
sku-1 1 2
sku-1 1 3
sku-1 1 -1
we will leave it as-is, but potentially already in this case we could remove everything except
sku-1 1 -1
But I agree that currently we could leave it as-is
interface ReservationCleanupInterface | ||
{ | ||
/** | ||
* Clean reservation table to prevent overloading. |
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.
I would not expose that reservation table is used on the level of this service, because potentially it could be Redis queue
Don't introduce a leaky abstraction , as we need to be agnostic to the DAL at this level of abstraction
$reservations = [ | ||
$this->reservationBuilder->setStockId(1)->setSku('SKU-1')->setQuantity(3.5)->build(), // unreserve 3.5 units | ||
]; | ||
$this->reservationsAppend->execute($reservations); |
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 this code
+ $reservations = [
+ $this->reservationBuilder->setStockId(1)->setSku('SKU-1')->setQuantity(3.5)->build(), // unreserve 3.5 units
+ ];
+ $this->reservationsAppend->execute($reservations);
without any asserts afterwords ?
is this kind of rollback action? which should prevent to affect further calculations?
->addDimension('stock_', $stockId) | ||
->setAlias(Alias::ALIAS_MAIN) | ||
->create(); | ||
$indexStructure->delete($indexName); |
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.
it's better to make such operation in Rollback Fixture than in tearDown
because you will copy-paste this tear down code among other test classes which use the same fixtures
->addDimension('stock_', $stockId) | ||
->setAlias(Alias::ALIAS_MAIN) | ||
->create(); | ||
$indexStructure->delete($indexName); |
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.
that's the copy-paste I was talking above
$reservations = [ | ||
$this->reservationBuilder->setStockId(1)->setSku('SKU-1')->setQuantity(8.5)->build(), // unreserve 8.5 units | ||
]; | ||
$this->reservationsAppend->execute($reservations); |
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.
is this kind of rollback action? which should prevent to affect further calculations?
…ulation # Conflicts: # app/code/Magento/Inventory/Model/Reservation/Validator/QuantityValidator.php # app/code/Magento/Inventory/Model/Reservation/Validator/ReservationValidatorInterface.php # app/code/Magento/Inventory/Model/Reservation/Validator/SkuValidator.php # app/code/Magento/Inventory/Model/Reservation/Validator/StockIdValidator.php # app/code/Magento/Inventory/Model/Reservation/Validator/ValidatorChain.php # app/code/Magento/Inventory/etc/di.xml
-- refactoring
-- refactoring
This is not a complete PR but I'd like to start having a feedback on what implemented so far, that is
GetProductQuantityInStock
service.In particular, I'd like to ask:
GetProductQuantityInStock
class;