EmicoEcommerce / Magento2TweakwiseExport-archived

Magento 2 module for Tweakwise export
Other
6 stars 16 forks source link

Bundled products stock status #91

Closed maxserv-woostelbos closed 4 years ago

maxserv-woostelbos commented 4 years ago

Issue Brief

Hi,

I've tested the new method that checks if a bundled product 'is in stock' and should be added to the export. As far as i can see a bundled product is added to the xml when one of the simple products has $qty. Shouldn't a bundle product only be 'in Stock' if all of the simples are aswell?

If tested a bundle product and i think this would be sufficient for: 'Model/CombinedStock/Bundle.php'

use Magento\CatalogInventory\Model\StockRegistry;

$stockStatus = $this->stockRegistry->getStockStatus(
     $exportEntity->getId(),
     $storeId
);
$isInStock = $stockStatus['stock_status'];

Environment

Steps to reproduce

Run console command: 'bin/magento tweakwise:export'

Actual result

Bundled products that are not in stock are still added to the export file

Expected result

If one of the simple products of a bundled product is 'Out of stock', the bundled product is out of stock aswell.

edwinljacobs commented 4 years ago

Hi,

I assume you mean https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/e3d940ba6f6d35f305686e79f409d4adf459ce12/src/Model/CombinedStock/Bundle.php#L21

What we are trying to do here is select all option groups for a bundle and determine for each group if there is an option (i.e. product) in that group that is in stock. If for all option groups there exists at least one product that is in stock then that option group is marked in stock, you can see that at https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/e3d940ba6f6d35f305686e79f409d4adf459ce12/src/Model/CombinedStock/Bundle.php#L30

In the case of sample data (bin/magento sampledata:deploy) we can find an example (product id 51, name: "Sprite Yoga Companion Kit"). I dont know if you have a sample data installation but I use that for some of my test data.

This product has 8 children (in my case) and each of those children belong to one of 4 option groups

bundle-options

So for all groups we max is_in_stock to assign is_in_stock for that group and then we min over these group status to get the final stock status, so that if any of the groups do not have a product in stock then the bundle is not in stock. Here we also consider the stock status of the product. That is the goal.

You are problably thinking why not use something like the stock registry as you pointed out or some other interface implementation that will just give you the stock for the bundle? The reason for that is that the stock registry executes the following: 1)

/**
     * @param \Magento\CatalogInventory\Api\StockStatusCriteriaInterface $criteria
     * @return \Magento\CatalogInventory\Api\Data\StockStatusCollectionInterface
     */
    public function getList(\Magento\CatalogInventory\Api\StockStatusCriteriaInterface $criteria)
    {
        $queryBuilder = $this->queryBuilderFactory->create();
        $queryBuilder->setCriteria($criteria);
        $queryBuilder->setResource($this->resource);
        $query = $queryBuilder->create();
        $collection = $this->stockStatusCollectionFactory->create(['query' => $query]);
        return $collection;
    }

So a getList query Then the afterGetStockStatusPlugin triggers:

/**
     * @param StockRegistryInterface $subject
     * @param StockStatusInterface $stockStatus
     * @param int $productId
     * @param int $scopeId
     * @return StockStatusInterface
     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
     */
    public function afterGetStockStatus(
        StockRegistryInterface $subject,
        StockStatusInterface $stockStatus,
        $productId,
        $scopeId = null
    ): StockStatusInterface {
        $websiteCode = null === $scopeId
            ? $this->storeManager->getWebsite()->getCode()   
            : $this->storeManager->getWebsite($scopeId)->getCode();
        $stockId = $this->stockResolver->execute(SalesChannelInterface::TYPE_WEBSITE, $websiteCode)->getStockId();
        $sku = $this->getSkusByProductIds->execute([$productId])[$productId];

        $status = (int)$this->isProductSalable->execute($sku, $stockId);
        try {
            $qty = $this->getProductSalableQty->execute($sku, $stockId);
        } catch (InputException $e) {
            $qty = 0;
        }

        $stockStatus->setStockStatus($status);
        $stockStatus->setQty($qty);
        return $stockStatus;
    }

This code triggers about a million database queries (this is obviously exaggerated), this is not an issue per se for a small catalog and a limited amount of stores. However we must facilitate for 10+ stores and 50000+ products per store. The code you presented is 75 times slower (on my machine) then the current situation and for a large catalog this is simply not an option. This is exactly the reason why the export module is written this way. I would love to just use magento's interfaces for this but I cant as it is way to slow. This does mean we have to know a lot of the internals and this is error prone, trust me I am well aware of that.

I could take a look into the code to see if there are errors with it but there is a reason why we don't use the normal magento interfaces and their implementations.

If your catalog is not too big by all means make a preference for the class in your own module and simply ask the stock registry for stock status and be done with it. I could add method "getStoreId" to the export entity class so that you can ask the correct store id from the exportEntity if that helps. But the general approach to data retrieval will not change. That being said, if there is something wrong with the bundle implementation as is then it should be fixed, if there are any, feel free to point them out :) If you have cases where the code breaks then I could take a look at a stripped version of your database if this is an option, or describe to me exactly what I need to do to reproduce your situation.

I hope this explains a bit about why the module is written the way it is. I have a quality of life update planned for the module, a lot of the properties are private, this will change soon so that whoever implements the module can override, plugin or do whatever to change the behaviour without having to deal with all kinds of private access modifiers.

With kind regards

maxserv-woostelbos commented 4 years ago

He Edwin,

Thanks for the clear explanation why you have chosen to set this up the way you did. I'll check again with our bundle why it is in stock (as i suspect it should not be). If i find something that could be usefull for you guys i'll let you know.

Aside from that i'll check the speed difference with usage of the stockRegistry. May we could go for a preference.

edwinljacobs commented 4 years ago

If i can be of any help let me know, the challenge in the current implementation is that it should work for magento 2.2.X (so without the multi source inventory stuff) and 2.3.X (with multistore inventory) there are adapters written for this but it feels very error prone. Both these adapters will take whatever the stock implementation is and spit out the simplified Tweakwise\Model\StockItem so that the CombinedStockProvider can determine the stock status for composed products against abstracted stock items. These adapters can be found at Emico\TweakwiseExport\Model\Write\Products\CollectionDecorator\StockData\SourceItemMapProvider Emico\TweakwiseExport\Model\Write\Products\CollectionDecorator\StockData\StockItemMapProvider

The code in these two classes is meant to determine stock items for all products, then after that the CombinedStockItemProvider should aggregate stock data of child products into one StockItem and set that on the parent product.

Perhaps this is of help, again if there is something amiss in the code let me know :)

With kind regards

maxserv-woostelbos commented 4 years ago

There is one thing i'm missing here, maybe you can clarify that?

I'm testing your theory with a bundle i know is out of stock:

Lets say the bundle has 3 simple products. When all the simples are in stock '$exportEntity->getExportChildren()' loops through all the 3 simples.

When i set one of the simples to 'out of stock' that item is not returned in the '$exportEntity->getExportChildren()' anymore. So all of the items in '$optionGroups' always have a 'is_in_stock=> 1'. Resulting in the bundle to be set as 'in stock' aswell.

What am i missing :-)?

edwinljacobs commented 4 years ago

Hmmm ill check that

edwinljacobs commented 4 years ago

You have a point, in my test the following export-out-of-stock-children

was set to yes. This results in all the children being available on the parent item at all times :( Sadly this was missed :( Just to check could you change the call $exportEntity->getExportChildren() to $exportEntity->getExportChildrenIncludeOutOfStock()

In vendor/emico/tweakwise-export/src/Model/CombinedStock/Bundle.php ? I think this might fix the issue. This issue highlights the need for testcases ...

WIth kind regards

maxserv-woostelbos commented 4 years ago

Ok that fixed the problem with the simples that are out of stock. I am your testcase now ;-). You are going to add this in another version?

One more thing. I could be that all of the simple products of a bundle are in stock but 1 of the simple products is disabled. This makes: '$product->isSaleable()' false. At the moment the bundled product is still added to the export xml.

Can you take a look at that aswell?

edwinljacobs commented 4 years ago

Yes I will add this to a release asap as it is quite severe that products end up in the feed when they should not belong there (depending on settings).

I will take a look at the disabled product.

edwinljacobs commented 4 years ago

The disabled product suffers from the same kind of issue. Method getExportChildren and getExportChildrenIncludeOutOfStock both filter disabled products so that the stock calculation is not aware that there are disabled children.

However the stock calculation is not the place to fix that particular issue. In my opinion methods shouldExport https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/dde60f316ba662533cccb34c9f14bf2f353e4ad4/src/Model/Write/Products/ExportEntity.php#L256

and https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/dde60f316ba662533cccb34c9f14bf2f353e4ad4/src/Model/Write/Products/ExportEntityChild.php#L76

need an overhaul. Also, how children are managed on the parent export item is less then ideal. Ill get on it

maxserv-woostelbos commented 4 years ago

Ok thanks, good to hear the fixes are on the way. Can you let me know when the release is done with these fixes?

edwinljacobs commented 4 years ago

Ill update as soon as I have something.

With kind regards

edwinljacobs commented 4 years ago

Sadly I was not able to complete this today,

Ill continue next week

edwinljacobs commented 4 years ago

I will release the fix for the out of stock products today. The issue with disabled child products has more impact and is less likely to happen. I am still working on that and it will likely be a major version update.

edwinljacobs commented 4 years ago

Hi,

the stock issue should be fixed in https://github.com/EmicoEcommerce/Magento2TweakwiseExport/releases/tag/v1.6.3

maxserv-woostelbos commented 4 years ago

Hi,

I'll update and test the changed next week for the stock issue.

Thanks for the changes!

maxserv-woostelbos commented 4 years ago

BTW: Any idea when we can expect an update for the disabled childs?

maxserv-woostelbos commented 4 years ago

Hi Edwin,

Do you have an update on the "disabled childs" customisations for me?

edwinljacobs commented 4 years ago

Hi,

I was on vacation hence the late response, I am still working on that issue. As mentioned above this will be a major version update as I am moving around quite some code.

As is there is only one Export entity class This will be split into several classes i.e. simple, configurable, grouped etcetc. These classes will hold different logic to determine which child entities should be exported. But since we are splitting theses classes it would also make sense to move the stock qty and status to those classes (in my opinion) and that is quite a change. The subdivision of these classes is already there but not properly tested and also the stock logic is not yet there. You can take a look at: https://github.com/EmicoEcommerce/Magento2TweakwiseExport/tree/issue/91-composite-products/src/Model/Write/Products

Here the subdivision is done however not everything is implemented

In the meantime, is there a way for you work around this issue?

With kind regards

maxserv-woostelbos commented 4 years ago

Hi,

Thanks for the update. I have an idea on a workaround to not export bundles with disabled simples. The problem with this is that we have to wait for the export to be done and published before the bundled product is not visibile anymore. That leaves a gap for a while and probably some caching issues aswell.

Is there temporary fix to hide bundled products (that still are enabled but have simples that are disabled) in the listing and search without having to depend on the export?

edwinljacobs commented 4 years ago

Hi,

I think the issue might be solved on branch: https://github.com/EmicoEcommerce/Magento2TweakwiseExport/tree/issue/91-composite-products It is quite the change though and not everything has been tested thoroughly.

The whole stock combined thing has been moved to seperate export entities, Child product status is also checked when determining if the product should be exported.

The most relevant code can be found in : https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/Model/Write/Products/ExportEntity.php https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/Model/Write/Products/ExportEntityConfigurable.php https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/Model/Write/Products/ExportEntityBundle.php https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/Model/Write/Products/ExportEntityGrouped.php https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/Model/Write/Products/ExportEntityFactory.php https://github.com/EmicoEcommerce/Magento2TweakwiseExport/blob/issue/91-composite-products/src/etc/di.xml

Is it an option for you to take a look / test that branch? It is also available on branch "release"

This will be a 2.0.0 release so if you have any custom code in you application that depends on this module you might need to change some things.

With kind regards

edwinljacobs commented 4 years ago

The changes have been released in version https://github.com/EmicoEcommerce/Magento2TweakwiseExport/releases/tag/v2.0.0

With kind regards

edwinljacobs commented 4 years ago

Closed because of release v2.0.0