Introduce Reservation API #60

aleron75 · 2017-08-10T05:52:57Z

Description

Add Reservation entity and related APIs
Update Install Schema for Reservation table creation
Add GetReservationsForProduct domain service
Add ReservationAppend domain service

Note

Since the Install Schema has been updated, you need to re-install the Inventory module, wiping out existing data structure and content.

A more practical approach I followed:

remove Magento_Inventory entry from setup_module table but keep module's tables and data
comment the execution of existing tables creation in Magento\Inventory\Setup\InstallSchema::install() leaving only $this->createReservationTable->execute($setup);
run the bin/magento setup:upgrade
undo the changes made to Magento\Inventory\Setup\InstallSchema::install()

- Update comment to better clarify the definition of Reservation Domain services - Add GetReservationsForProduct api class implementation - Add ReservationAppend api class implementation - Update Install Schema for Reservation table creation - Remove useless parameter from getter method - Add multiple save for Reservation - Fix typo in const declaration - Add CreateReservationTable operation class - Fix param type in DocBlock for ReservationAppendInterface::execute() - Fix return type in DocBlock for GetReservationsForProductInterface::execute() - Rename Reservation Append service interface - Rename Get Reservations for Product service interface - Add Reservation resource model - Add Reservation Collection resource model - Add Reservation model - Add Reservation interface - Add Reservation Append service interface - Add Get Reservation for Product service interface

naydav · 2017-08-10T14:31:22Z

app/code/Magento/Inventory/Model/ReservationAppend.php

+ SaveMultiple $saveMultiple,
+ LoggerInterface $logger
+ ) {
+ $this->saveMultiple = $saveMultiple;


Missed private properties

naydav · 2017-08-10T14:44:58Z

app/code/Magento/InventoryApi/Api/Data/ReservationInterface.php

+ /**
+ * Get Product SKU
+ *
+ * @return string|null


Sku can not be null, it is required field

naydav · 2017-08-10T14:45:38Z

app/code/Magento/InventoryApi/Api/Data/ReservationInterface.php

+ /**
+ * Get Product Qty
+ *
+ * @return float|null


Can not be null, it is required field
Need to add description that this field can be negative

I am not sure this can be negative.

I understand from here that qty is always > 0 and it is subtracted or added depending on Reservation status.

If it is so (always >0) it could be a good thing to add validation and throw an exception if the API call tries to pass a negative value.

Let me know, thank you!

@aleron75
Correct, in the document I described it as qty is always > 0 and it is subtracted or added depending on Reservation status.

But thinking about an efficient way how can we get Sum of Grouped Reservation, made me think that it would be more performance efficient to have both negative and positive values for Reservations.

Because from the calculation point of view for us it would be easier to have both negative (<0) and positive (>0) Qty values.

Like when we placed an order -> we created Reservation with Qty -30,
when we processed the Order and deducted SourceItems -> we created a reservation with Qty +30

For example, executing this query

select SUM(r.qty) as total_reservation_qty from Reservations as r where stockId = {%id%} and sku = {%sku%}

pretty simple query, and we've given total reservation qty.

if we would have always Qty > 0, we need more sophisticated logic i.e.

process aggregation query for Open Reservation

process aggregation query for Closed Reservation

subtract 1. from 2.

@aleron75 updated Design Document for Reservation with this explanation

naydav · 2017-08-10T14:46:32Z

app/code/Magento/InventoryApi/Api/Data/ReservationInterface.php

+ /**#@+
+ * Reservation possible statuses. Maybe make sense to introduce extension point for Reservation Open-Close statuses
+ */
+ const STATUS_ORDER_CREATED = 1; // For Order Placed


Maybe need to move to some "statuses provider" object

Is there an example in M2 I can look up? Otherwise, I need to discuss with some of you to understand what you mean.

@aleron75 let's leave it as-is now. When all constants are in this class.
And will discuss how it can be improved later.
Because for now, we don't have a clear vision how this should look like

naydav · 2017-08-10T14:49:26Z

app/code/Magento/Inventory/Model/ReservationAppend.php

+ {
+ /** @var ReservationInterface $reservation */
+ foreach ($reservations as $reservation) {
+ if (!is_null($reservation->getReservationId())) {


Why during creation reservation must have id?
When I create reservations I pass data with sku, qty, stock id. But we do not know about id.

Also, need to use null !== $var instead of is_null

@naydav ReservationAppend service is used to add newly created reservations.
Here we @aleron75 added a check which prevents that someone retrieves a reservation, change some data, and will try to save again using ReservationAppend service.

Current code considers that Database Generated Ids are used, so that entity which has an ReservationId considered as already created, thus can't be appended one more time.

So, the logic is correct as for me

The code checks whether reservation is not null; Reservations can only be appended so if you try to update an existing one (that is an entity with a not null ID) you get an exception.

Re-reading your comment:

Why during creation reservation must have id?

maybe you mean we can assume that nobody will call the API passing a reservation with not null id?

I applied what was written here:

"we could check the ReservationId which is passed in the scope of ReservationInterface is nullified"

Let me know, thank you!

Sorry, it was my mistake.

naydav · 2017-08-10T14:57:41Z

app/code/Magento/Inventory/Model/ReservationRepository.php

+/**
+ * @inheritdoc
+ */
+class ReservationRepository implements ReservationRepositoryInterface


Now we have chain of calls
GetReservationsForProductInterface -> ReservationRepository -> GetListInterface
Looks like ReservationRepository and GetListInterface are redundant objects.

GetReservationsForProductInterface it is service like as GetAssignedSourcesForStockInterface (interface and implementation)

Reading the API and SPI paragraph of Service Contracts page, it seems to me that Repository and GetList are different objects; Repository is an API implementing facade pattern and proxying to GetList, that is an SPI class command.
Let me know.

@aleron75 you are right. Regarding the API and SPI, and that Repository should play the role of API.

From my point of view, @naydav meant that we already have an API for Reservation Retrieving: GetReservationsForProductInterface
And we don't have another Business Use-Cases for Reservation Search (with Filtering) where Repository::getList would be needed for us.

That's why business logic would not use ReservationRepository as an API (Business Logic will not work with ReservationRepository directly, calling its methods).
It will always work indirectly through the GetReservationsForProductInterface API call.
Thus, in this case, ReservationRepository become an SPI, but not API.

But that's not a desirable state for us, as it overcomplicates object dependencies:
GetReservationsForProductInterface (API) -> ReservationRepository (SPI) -> GetListCommandInterface (SPI)
Too many extension points

naydav · 2017-08-10T14:58:47Z

app/code/Magento/Inventory/Model/ResourceModel/Reservation/SaveMultiple.php

+ /** @var ReservationInterface $reservation */
+ foreach ($reservations as $reservation) {
+ $data[] = [
+ $reservation->getReservationId(),


It is redundant data.

I looked up to Magento\Inventory\Model\ResourceModel\StockSourceLink\SaveMultiple which does the same; can you please clarify?

@aleron75 I think @naydav means that you no need to pass ReservationId , because it's supposed to be empty, because we rely on database generated ID

naydav · 2017-08-10T15:03:15Z

app/code/Magento/InventoryApi/Api/ReservationAppendInterface.php

+
+/**
+ * Domain service used to appends reservations when order is placed or canceled
+ *


Can be used not only when order is placed or canceled

Yes, agreed we will use this service and RMA (Returns) would be created and handled as well

Or when order would be split, or partial refund made etc.

naydav · 2017-08-10T15:26:09Z

app/code/Magento/Inventory/Setup/Operation/CreateReservationTable.php

+ 'Quantity'
+ )->addColumn(
+ ReservationInterface::STATUS,
+ Table::TYPE_INTEGER,


Pls, use Table::TYPE_SMALLINT

maghamed

Good Start @aleron75 ! Congratulations on your 1st PR.
We've made Code Review, and you can find out our feedback.

maghamed · 2017-08-10T15:20:08Z

app/code/Magento/Inventory/Model/GetReservationsForProduct.php

+ public function execute($sku, $stockId)
+ {
+ if (!is_numeric($stockId)) {
+ throw new InputException(__('Input data is invalid'));


Btw very soon (I think from the next week) we could use PHP 7 syntax (return types, and type hinting for simple types).
So that, you no need such check in nearest future

maghamed · 2017-08-10T15:45:01Z

app/code/Magento/InventoryApi/Api/ReservationRepositoryInterface.php

+/**
+ * @api
+ */
+interface ReservationRepositoryInterface


I don't think we need to provide API for ReservationRepository.
Because we are not going to expose Reservation entities via WebAPI .
All reservations would be created as supplementary operations along with some business process (like order placement).

What we need is to provide GetReservationsForProduct interface which retrieves reservations by $sku and $stockId. Except that we no need to apply another filtering, so that no need to introduce ReservationRepository

maghamed · 2017-08-10T15:46:21Z

app/code/Magento/Inventory/Model/GetReservationsForProduct.php

+
+ public function __construct(
+ SearchCriteriaBuilder $searchCriteriaBuilder,
+ ReservationRepositoryInterface $reservationRepository,


No need to use ReservationRepository, explanation provided below

maghamed · 2017-08-10T15:58:03Z

app/code/Magento/Inventory/Model/GetReservationsForProduct.php

+ $searchCriteria = $this->searchCriteriaBuilder
+ ->addFilter(ReservationInterface::SKU, $sku)
+ ->addFilter(ReservationInterface::STOCK_ID, $stockId)
+ ->create();


if we don't have generic Filtering Logic, no need to use genetic SearchCritetia and Filter objects.
We could do that same way we did for the SaveMultiple SourceItem object.

In this case we will have :

GetReservationsForProductInterface::execute($sku, $stockId);

the model which implements interface 1. (current file)

And Resource Model which executes SQL query - ResourceModel\Reservation\GetReservationsForProduct

This will simplify Filtering logic. We no need over-complicate filtering, because we have the only use-case for filtering - by StockId and SKU there are no other filter queries like time, Qty and their possible combinations

maghamed · 2017-08-10T16:01:46Z

app/code/Magento/Inventory/Model/Reservation/Command/GetList.php

+/**
+ * @inheritdoc
+ */
+class GetList implements GetListInterface


Propose to remove this command, and simplify filtering logic

maghamed · 2017-08-10T16:18:50Z

app/code/Magento/Inventory/Setup/Operation/CreateReservationTable.php

+ $reservationTable,
+ [
+ ReservationInterface::STOCK_ID,
+ ReservationInterface::SKU,


I propose to extend this index and add Qty as well.

because executing such query

select MAX(r.status) as grouped_status, group_concat(reservation_id) as reservations from Reservations as r group by stock_id, sku, qty having grouped_status > 100

MySQL could use such index for Groupping operation , which is made by (stock_id, sku, qty)

Also this index would be used for Search lookup by (stock_id, sku)

Do you mean extending the existing index on (stock_id, sku) or adding a new one on (stock_id, sku, qty)?

@aleron75 correct, extend existing index and add one more column to it.

maghamed · 2017-08-10T16:23:37Z

app/code/Magento/InventoryApi/Api/Data/ReservationSearchResultsInterface.php

+
+use Magento\Framework\Api\SearchResultsInterface;
+
+interface ReservationSearchResultsInterface extends SearchResultsInterface


if we remove Repostiory , this interface could be eliminated as well

maghamed · 2017-08-10T16:24:29Z

app/code/Magento/InventoryApi/Api/ReservationAppendInterface.php

+
+/**
+ * Domain service used to appends reservations when order is placed or canceled
+ *


Yes, agreed we will use this service and RMA (Returns) would be created and handled as well

maghamed · 2017-08-10T16:25:22Z

app/code/Magento/InventoryApi/Api/ReservationAppendInterface.php

+
+/**
+ * Domain service used to appends reservations when order is placed or canceled
+ *


Or when order would be split, or partial refund made etc.

maghamed · 2017-08-10T16:34:53Z

app/code/Magento/InventoryApi/Api/Data/ReservationInterface.php

+ *
+ * @api
+ */
+interface ReservationInterface extends ExtensibleDataInterface


Looks like we no need to expose Reservation API for WebAPI (REST and SOAP).
Currently, in Magento 2 WebAPI imposes some restrictions for entity interfaces (existence getter and setter methods).
But because we would not expose reservations over web (REST , SOAP) - we could use any interface we want.
And because we agreed that Reservation is append-only immutable entity we could eliminate all the setter methods.

So, we will end-up with ReservationInterface consisting of just getter methods.
And we need to introduce ReservationBuilderInterface which will allow the possibility to set data into the reservation when we need to create one.
after that we could build Reservation entity.

$reservationBuilder->setStockId(1); $reservationBuilder->setSku('sku'); $reservationBuilder->setQty(10); $newReservation = $reservationBuilder->build(); //now we could save Reservation entity $reservationAppend->execute([$newReservation]);

Doing so, we could ensure immutability on the level of Reservation interface

@maghamed

- Simplify Reservation filtering logic using GetReservationsForProduct resource model and removing ReservationRepository, GetList Command and ReservationSearchResultInterface - Change private member name in SaveMultiple resource model from $connection to $resource - Use PHP7's typehints as suggested by @maghamed - Improve documentation block for ReservationInterface - Improve documentation block for ReservationAppendInterface - Remove cross-module foreign key from Reservation Table - Update index on Reservation Table from (stock_id, sku) to (stock_id, sky, quantity) - Remove redundant ID field when saving multiple Reservations - Change DB column type to TYPE_SMALLINT for ReservationInterface::STATUS - Fix/rename some constants - Change documented return value for getSku(): cannot be null - Change documented return value for getQuantity(): cannot be null - Add missing private properties - Change is_null() into null !== $var

- Fix wrong reference from ReservationInterface::QTY to ReservationInterface::QUANTITY

Introduce Reservation Api - Merge branch 'develop' into msi-reservations - Remove status column and add metadata column in Reservation table - Update ReservationInterface and Reservation according to design changes (remove status, add metadata, achieve immutability) - Add GetProductQuantityInStockInterface - Add IsProductInStockInterface - Perform general refactoring to introduce usage of PHP7 type hinting and return types - Add ReservationBuilderInterface and ReservationBuilder - Add class preference for ReservationBuilderInterface in di.xml - Update GetReservationsForProduct to use ReservationBuilder - Update SaveMultiple according to Reservation design changes

maghamed · 2017-09-08T07:55:59Z

Current PR is closed as all the changes exist in PR 64
https://github.com/magento-engcom/magento2/pull/64

[Arrows] MC-40439: Update 2.4.3 branches with 2.4.2 branches

naydav reviewed Aug 10, 2017

View reviewed changes

magento deleted a comment from naydav Aug 10, 2017

maghamed suggested changes Aug 10, 2017

View reviewed changes

aleron75 added 3 commits August 13, 2017 02:32

Introduce Reservation Api

20d9104

- Fix wrong reference from ReservationInterface::QTY to ReservationInterface::QUANTITY

naydav force-pushed the develop branch from 9627b00 to a64ea9b Compare August 22, 2017 09:53

aleron75 mentioned this pull request Aug 23, 2017

Introduce Reservation Api #64

Merged

maghamed closed this Sep 8, 2017

smoskaluk mentioned this pull request Mar 23, 2018

Exception in Low stock report for product with another name on additional storeview #737

Closed

mmansoor-magento pushed a commit that referenced this pull request Feb 9, 2021

Merge pull request #60 from magento-tsg/MC-40439

0ffa84e

[Arrows] MC-40439: Update 2.4.3 branches with 2.4.2 branches


		use Magento\Framework\Api\SearchResultsInterface;

		interface ReservationSearchResultsInterface extends SearchResultsInterface

Introduce Reservation API #60

Introduce Reservation API #60

Conversation

aleron75 commented Aug 10, 2017

Description

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maghamed Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maghamed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maghamed commented Sep 8, 2017

maghamed Aug 11, 2017 •

edited

Loading