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

Mni/source selection #33

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
68 changes: 68 additions & 0 deletions app/code/Magento/Inventory/Model/Package.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Model;

class Package extends \Magento\Framework\Api\AbstractExtensibleObject
implements PackageInterface
{

/**
* @inheritdoc
*/
public function getSource()
{
return $this->_get('source');
}

public function getQty()
{
return $this->_get('qty');
}

public function setAddress($address)
{
return $this->_set('address', $address);
}

public function getAddress()
{
return $this->_get('address');
}

/**
* @return \Magento\Quote\Model\Quote\Item[]
*/
public function getItems()
{
return $this->_get('items');
}

public function getBaseSubtotal()
{
return $this->_get('base_subtotal');
}

public function getBaseSubtotalWithDiscount()
{
return $this->_get('base_subtotal_with_discount');
}

public function getWeight()
{
return $this->_get('weight');
}

public function getItemQty()
{
return $this->_get('item_qty');
}

public function getPhysicalValue()
{
return $this->_get('physical_value');
}
}
43 changes: 43 additions & 0 deletions app/code/Magento/Inventory/Model/PackageInterface.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;

interface PackageInterface
{
/**
* @return \Magento\InventoryApi\Api\Data\SourceInterface
*/
public function getSource();
Copy link
Contributor

Choose a reason for hiding this comment

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

we no need to return SourceInterface in this interface, because Package can't contain the whole Source entity, it's not an Aggregate Root for the source.

What it can contain is a reference to SourceId.


/**
* @return \Magento\Quote\Model\Quote\Item[]
*/
public function getItems();

/**
* @return \Magento\Quote\Model\Quote\Address
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be Magento\Quote\Api\Data\AddressInterface
not the implementation

*/
public function getAddress();

/**
* @param $address \Magento\Quote\Model\Quote\Address
* @return $this
*/
public function setAddress($address);

public function getQty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need qty here as it is already part of \Magento\Quote\Api\Data\CartItemInterface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, because qty here is the number of the product in this particular package. It can be less or equal to the qty of the product in the cart. I.e. 10 products with sku1 were requested, 6 fulfilled from source 1, 4 from source 2.

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Jun 23, 2017

Choose a reason for hiding this comment

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

Doesn't it make sense to just have different quote items in different packages? As it is already implemented even in the current version of the Shipping Assignments

Duplication of the Qty in multiple places leads to complicated code which would be potentially hard to maintain

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @ishakhsuvarov here.
Duplication of QTYs is misleading.
As QuoteItem (which represents single product SKU) has Qty
and Package which could consist of several SKUs has Qty (btw, why single, but not an array of quantities?)

So, it's hard to maintain and keep in consistent state.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's why I proposed to consider usage of Visitor pattern here,
to separate Data from Algorithm implementation.
That could help us to live with current data structure of Quote


public function getBaseSubtotal();

public function getBaseSubtotalWithDiscount();

public function getWeight();

public function getItemQty();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between Qty and ItemQuantity?


public function getPhysicalValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

what is business data represented with this Interface method?

PhysicalValue of the package?

}
88 changes: 88 additions & 0 deletions app/code/Magento/Inventory/Model/SourceSelection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Model;

use \Magento\Framework\App\Config\ScopeConfigInterface;
use \Magento\Sales\Model\Order\Shipment;

class SourceSelection implements SourceSelectionInterface
{
/** @var PackageFactory */
private $packageFactory;

/** @var SourceFactory */
private $sourceFactory;

/** @var ScopeConfigInterface */
private $scopeConfig;

/**
* @param PackageFactory $packageFactory
* @param SourceFactory $sourceFactory
* @param ScopeConfigInterface $scopeConfig
*/
public function __construct(
PackageFactory $packageFactory,
SourceFactory $sourceFactory,
ScopeConfigInterface $scopeConfig
) {
$this->packageFactory = $packageFactory;
$this->sourceFactory = $sourceFactory;
$this->scopeConfig = $scopeConfig;
}

/**
* @inheritdoc
*/
public function getPackages($store, $items, $destinationAddress)
{
$countryId = $this->scopeConfig->getValue(
Shipment::XML_PATH_STORE_COUNTRY_ID,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE,
$store
);
$regionId = $this->scopeConfig->getValue(
Shipment::XML_PATH_STORE_REGION_ID,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE,
$store
);
$city = $this->scopeConfig->getValue(
Shipment::XML_PATH_STORE_CITY,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE,
$store
);
$postcode = $this->scopeConfig->getValue(
Shipment::XML_PATH_STORE_ZIP,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE,
$store
);

$source = $this->sourceFactory->create(
[
'data' => [
'source_id' => 0,
'name' => 'Default',
'country_id' => $countryId,
'region_id' => $regionId,
'city' => $city,
'postcode' => $postcode
]
]
);
return [
$this->packageFactory->create(
[
'data' => [
'items' => $items,
'source' => $source,
'address' => $destinationAddress
]
]
)
];
}
}
18 changes: 18 additions & 0 deletions app/code/Magento/Inventory/Model/SourceSelectionInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Model;

interface SourceSelectionInterface
{
/**
* @param $store
* @param $items
* @param \Magento\Quote\Model\Quote\Address $destinationAddress
* @return Package[]
*/
public function getPackages($store, $items, $destinationAddress);
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 accept $store parameter here?
Just to read the configuration data? Why can't we resolve it?

in any case if we want introduce scoping , which we usually not do - it should be called $scope , but not $store.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this method supposed to be called several times in case of multi-shipping delivery?

}
1 change: 1 addition & 0 deletions app/code/Magento/Inventory/etc/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@
<preference for="Magento\InventoryApi\Api\Data\SourceCarrierLinkInterface" type="Magento\Inventory\Model\SourceCarrierLink" />
<preference for="Magento\InventoryApi\Api\Data\SourceSearchResultsInterface" type="Magento\Framework\Api\SearchResults" />
<preference for="Magento\Inventory\Model\SourceCarrierLinkManagementInterface" type="Magento\Inventory\Model\SourceCarrierLinkManagement" />
<preference for="Magento\Inventory\Model\SourceSelectionInterface" type="Magento\Inventory\Model\SourceSelection" />
</config>
12 changes: 12 additions & 0 deletions app/code/Magento/Quote/Api/Data/ShippingAssignmentInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ public function getItems();
*/
public function setItems($value);


Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest refactoring this interface in case we are already introducing BiC, as it does not reflect the desired state described in the latest version of the Multishipping design

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me see

Copy link
Contributor

Choose a reason for hiding this comment

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

We had a discussion regarding implementation of the Visitor pattern to integrate MSI with Quote. Maybe it is better to research that solution?

/**
* @param \Magento\Inventory\Model\PackageInterface[]
* @return $this
*/
public function setPackages($packages);

/**
* @return \Magento\Inventory\Model\PackageInterface[]
*/
public function getPackages();
Copy link
Contributor

Choose a reason for hiding this comment

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

again ShippingAssignment interface has get/setQuoteItems
you also intorduce get/setPackages which also have get/setQuoteItems

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover there is a get/setShipping method here which returns
\Magento\Quote\Api\Data\ShippingInterface
so, you have also duplication of the shipping address here and in Package.


/**
* Retrieve existing extension attributes object or create a new one.
*
Expand Down
Loading