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

Update SourceRepository.php to return stock to relevant stock source #113

Merged
merged 17 commits into from
May 30, 2023

Conversation

peekarboo
Copy link
Contributor

@peekarboo peekarboo commented May 15, 2023

In case of multiple msi sources, stock is not returned to the relevant stock source.
This PR

  1. Updates SourcesConverter to return all sources associated with order (Update SourcesConverter.php #102)
  2. Returns stock to relevant stock source when there are different sources

Assign multiple sources to a stock

Screenshot 2023-05-15 at 08 08 19

Stock A (BE) = 2
Stock B (NL) = 2

mysql> select * from inventory_source_item where sku ='64411d69-0361-49f7-bb85-4a30bb75bbeb';
+----------------+-------------+--------------------------------------+----------+--------+
| source_item_id | source_code | sku                                  | quantity | status |
+----------------+-------------+--------------------------------------+----------+--------+
|       10241017 | BE          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
|       10201427 | default     | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
|       10232247 | NL          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
+----------------+-------------+--------------------------------------+----------+--------+

Create a pending order with qty 5

Stock A (BE) = 0
Stock B (NL) = 0

mysql> mysql> select * from inventory_source_item where sku ='64411d69-0361-49f7-bb85-4a30bb75bbeb';
+----------------+-------------+--------------------------------------+----------+--------+
| source_item_id | source_code | sku                                  | quantity | status |
+----------------+-------------+--------------------------------------+----------+--------+
|       10241017 | BE          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   0.0000 |      0 |
|       10201427 | default     | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
|       10232247 | NL          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   0.0000 |      0 |
+----------------+-------------+--------------------------------------+----------+--------+

Cancel the order

Stock A (BE) = 2
Stock B (NL) = 2

mysql> mysql> select * from inventory_source_item where sku ='64411d69-0361-49f7-bb85-4a30bb75bbeb';
+----------------+-------------+--------------------------------------+----------+--------+
| source_item_id | source_code | sku                                  | quantity | status |
+----------------+-------------+--------------------------------------+----------+--------+
|       10241017 | BE          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
|       10201427 | default     | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
|       10232247 | NL          | 64411d69-0361-49f7-bb85-4a30bb75bbeb |   2.0000 |      1 |
+----------------+-------------+--------------------------------------+----------+--------+

Checklist

  • Pull request has a meaningful description of its purpose, include affected Magento versions if it is a bug.
  • All commits are accompanied by meaningful commit messages
  • Tests have been ran / updated (see ./dev/README.md for how to run tests)

@peekarboo peekarboo self-assigned this May 15, 2023
@convenient
Copy link
Contributor

@peekarboo this includes the changes in #102 ye?

@peekarboo
Copy link
Contributor Author

@convenient Yes it does

@convenient convenient linked an issue May 15, 2023 that may be closed by this pull request
@barryvdh
Copy link
Contributor

At first sight this seems to fix my issue indeed. The stock is returned to the original source.

@barryvdh
Copy link
Contributor

Wait I think this doesn't go well with multiple products. It seems to add the stock to all sources

Product 1
Stock A: 3
Stock B: 2

Product 2:
Stock A: 5
Stock B: 5

Order 5x1 and 10x 2

Product 1
Stock A: 0
Stock B: 1

Product 2
Stock A: 0
Stock B: 0

Cancel order

Product 1
Stock A: 8
Stock B: 7
-> 15 instead of 5, so 10 from product 2 are added also.

Product 2
Stock A: 8
Stock B: 6
-> 15 instead of 10, so 5 from Product 1 are added here also

@barryvdh
Copy link
Contributor

This looks good, but don't know why some tests are failing. Is there anything I can do to help this along?

Update `MultipleSourceInventoryTest.php` handling

- Patch a bug in 2.4.4 that stops fixtures working in integration tests because tables are prefixed with trv_. I want to keep the versions under test as clear as possible, but this wont affect most use cases and will allow us to get the tests going through
- Clear the sales channel cache before running the tests
- Some DAMP vs DRY readability changes https://stackoverflow.com/a/11837973/4354325
    - this removed some dead code also
@convenient
Copy link
Contributor

@barryvdh tests going green now because of this

/**
* Clear the GetStockBySalesChannelCache as it gets populated during fixture runtime and varies depending on the
* version of magento being tested.
*
* This way we can start our test with a clear cache after all the fixtures have run.
*
* @return void
* @throws \ReflectionException
*/
private function clearSalesChannelCache(): void
{
if (class_exists(GetStockBySalesChannelCache::class)) {
$getStockBySalesChannelCache = $this->objectManager->get(GetStockBySalesChannelCache::class);
$ref = new \ReflectionObject($getStockBySalesChannelCache);
try {
$refProperty = $ref->getProperty('channelCodes');
} catch (\ReflectionException $exception) {
$refProperty = $ref->getParentClass()->getProperty('channelCodes');
}
$refProperty->setAccessible(true);
$refProperty->setValue($getStockBySalesChannelCache, []);
}
$stockId = $this->objectManager->get(StockResolverInterface::class)
->execute(SalesChannelInterface::TYPE_WEBSITE, 'eu_website')
->getStockId();
$this->assertEquals(10, $stockId, 'The stock id for the eu_website should be 10');
}

Seems we were having odd / bad values cached on an object property during the fixture booting that needed cleared before running the tests.

@peekarboo would be good to get a few additional test cases in please

@convenient
Copy link
Contributor

Hey @barryvdh

I believe this is all sorted now, would you agree? Before I merge and tag just wanted to check.

Thanks,
Luke

@barryvdh
Copy link
Contributor

I think so. I tested this patch on my local shop and it seems to work correctly for the cases I encountered before. Thanks!

@convenient convenient merged commit 01e2ade into master May 30, 2023
@convenient convenient deleted the fix/SourceRepository branch May 30, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canceled item restored to wrong stock source
3 participants