Tiered pricing and quantity Increments do not work with decimal inventory MAGETWO-86164 (both front-end and back-end) #13359

vasilii-b · 2018-01-25T07:47:27Z

Description

All issues below are described in #12914

Fixed Issues

[1] Cannot close Advanced Inventory pop-up after setting Qty Uses Decimals to No and having Enable Qty Increments enabled

Steps to reproduce:

Create new product/ edit existing one
Open Advanced Inventory pop-up under Quantity
Set Qty Uses Decimals to Yes
Set Qty value to decimal value
Set Enable Qty Increments to Yes
Set Qty Increments a decimal value
Press DONE (may skip this step)
Open Advanced Inventory pop-up under Quantity (optional if 7 was skipped)
Set Set Qty Uses Decimals to No
Set Qty value to integer value
Set Enable Qty Increments to No
Press DONE or the X icon to close the button

Expected Result

Close the Advanced Inventory pop-up

Actual result

Pop-up not closed and no errors are visible

[2] Product Tiered Pricing Issue

Steps to reproduce

Create Simple Product
Set Inventory to 100
Set "Qty Uses Decimals" in Advanced Inventory to "Yes"
Set Base Price to 3.99
Open Advanced Pricing tab
Set a Tiered Price for Qty of .5 and price of 2.99
Save

[3] Quantity increment issue

Steps to reproduce

Open Advanced Inventory
Set "Enable Qty Increments" to "Yes"
Set "Qty Increments" to ".5"
Save

Expected result

During each of the above scenarios (2,3), the product should save and allow tiered pricing and qty increments to work with decimal-based inventory

Actual result

Product does not save, admin user is given error stating "Please enter a valid number"

[4] After saving a product with Qty Decimals enabled, Qty Increments and Tier Price Quantity are integer numbers if decimal value was given

Steps to reproduce

Create Simple Product
Set Inventory to 100
Set "Qty Uses Decimals" in Advanced Inventory to "Yes"
Set Base Price to 3.99
Open Advanced Pricing tab
Set a Tiered Price for Qty of 1.5 and price of 2.99
Save
Open Advanced Pricing pop-up

Expected result

Tier price Qty is 1.5

Actual result

Tier price Qty is 1

[5] integer vs decimal numbers affects the front-end in the same way, e.g. mini cart and checkout

Steps to reproduce

Use the same steps from [1] or [4]

Expected result

In cart popup, cart page, checkout cart summary product Qty appear with decimals (if applicable)

Actual result

In cart popup, cart page, checkout cart summary product Qty appear without decimals

rogyar · 2018-02-16T07:47:28Z

Please, fix the failed tests. Thanks

magento-cicd2 · 2018-03-24T11:39:10Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ vasilii-b
❌ Vasilii Burlacu

Vasilii Burlacu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

vasilii-b · 2018-03-24T21:12:09Z

@rogyar Could you please advice on the only remaining failed test ? It says to reduce __construct method parameters to under 10, while it has 10 now.
Thank you!

rogyar · 2018-03-25T08:55:48Z

As an option, you can add the stockItemRepository to the helper class (Magento\Bundle\Helper\Data) instead of injecting it directly to the dataProvider.

vasilii-b · 2018-06-30T08:37:01Z

Hi @rogyar , can you please advice on the latest failed test ? I see failed tests has nothing related to the work I've done in scope of this PR. Should I still fix them ?

Thank you!

PieterCappelle · 2018-07-02T13:42:23Z

I'm following this thread, having same issue with one of our clients.

app/code/Magento/Bundle/Helper/Data.php

app/code/Magento/CatalogInventory/Helper/ProductStockIsQtyDecimal.php

app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/General.php

ishakhsuvarov · 2018-07-05T13:47:06Z

I see failed tests has nothing related to the work I've done in scope of this PR. Should I still fix them ?

This fail relates to the work you have done indirectly. A new dependency added to the class triggered the failure. Usually high object coupling numbers indicates that class has too much responsibilities.

The class, where the test failed, indeed has too much different capabilities to it. You may try refactoring it and extracting some parts out of it.

sidolov · 2018-09-20T14:42:31Z

Hi @vasilii-b , will you continue progress with this issue?

rogyar · 2018-10-09T09:52:28Z

@vasilii-b. I'm closing this issue due to inactivity. Feel free to reopen it when you decide to proceed. Thank you

vasilii-b · 2018-10-14T07:44:42Z

Hi @rogyar @ishakhsuvarov ,
I've fixed all latest failed tests as well as added requested changes.
Can you please have a look ?

vasilii-b · 2018-10-21T08:32:15Z

Hi @ishakhsuvarov, could you please review this PR once again ?
Thank you!

sidolov · 2018-11-12T08:42:16Z

app/code/Magento/Bundle/Ui/DataProvider/Product/BundleDataProvider.php

     */
    public function __construct(
        $name,
        $primaryFieldName,
        $requestFieldName,
        CollectionFactory $collectionFactory,
        Data $dataHelper,
+        StockStateInterface $stockState,


Please, inject new dependencies according to our Backward Compatible Development Guide

sidolov · 2018-11-12T08:43:26Z

app/code/Magento/Bundle/Ui/DataProvider/Product/Form/Modifier/BundlePanel.php

@@ -69,7 +69,9 @@ public function __construct(
    }

    /**
-     * {@inheritdoc}


Please, revert this changes. Just remove curly braces for static test.

sidolov · 2018-11-12T08:43:42Z

app/code/Magento/Bundle/Ui/DataProvider/Product/Form/Modifier/BundlePrice.php

@@ -41,7 +41,8 @@ public function __construct(
    }

    /**
-     * {@inheritdoc}


Please, revert this changes. Just remove curly braces for static test.

sidolov · 2018-11-12T08:43:48Z

app/code/Magento/Bundle/Ui/DataProvider/Product/Form/Modifier/BundleWeight.php

@@ -30,7 +30,8 @@ public function __construct(ArrayManager $arrayManager)
    }

    /**
-     * {@inheritdoc}
+     * @param array $meta


Please, revert this changes. Just remove curly braces for static test.

sidolov · 2018-11-12T08:43:54Z

app/code/Magento/Bundle/Ui/DataProvider/Product/Form/Modifier/Composite.php

@@ -67,7 +67,8 @@ public function __construct(
    }

    /**
-     * {@inheritdoc}


Please, revert this changes. Just remove curly braces for static test.

sidolov · 2018-11-12T08:44:24Z

app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/AdvancedPricing.php

@@ -123,7 +130,8 @@ public function __construct(
        Data $directoryHelper,
        ArrayManager $arrayManager,
        $scopeName = '',
-        GroupSourceInterface $customerGroupSource = null
+        GroupSourceInterface $customerGroupSource = null,
+        StockStateInterface $stockState


Please, inject new dependencies according to our Backward Compatible Development Guide

sidolov · 2018-11-12T08:44:54Z

app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/General.php

     * @param AttributeRepositoryInterface|null $attributeRepository
     */
    public function __construct(
        LocatorInterface $locator,
        ArrayManager $arrayManager,
+        StockStateInterface $stockState,


Please, inject new dependencies according to our Backward Compatible Development Guide

sidolov · 2018-11-12T08:46:39Z

app/code/Magento/CatalogInventory/Api/StockStateInterface.php

+     *
+     * @return bool
+     */
+    public function isStockQtyDecimal($productId, $scopeId = null): bool;


We are not allowed to add new methods to the existing interfaces according to our Backward Compatible Development Guide. Consider creating a new interface with isStockQtyDecimal method

vasilii-b · 2018-11-19T07:13:49Z

Hi @sidolov, can you please see my latest changes according your requests ? Thank you!

sivaschenko

Thanks for collaboration. Please see the code review notes.

sivaschenko · 2018-11-19T10:16:09Z

app/code/Magento/Bundle/Helper/Data.php

@@ -5,6 +5,9 @@
 */
 namespace Magento\Bundle\Helper;

+use Magento\CatalogInventory\Api\StockItemRepositoryInterface;


Please remove the unused use references.

sivaschenko · 2018-11-19T10:17:08Z

app/code/Magento/Bundle/Ui/DataProvider/Product/BundleDataProvider.php

     * @param array $meta
     * @param array $data
+     * @param \Magento\Ui\DataProvider\AddFieldToCollectionInterface[] $addFieldStrategies
+     * @param \Magento\Ui\DataProvider\AddFilterToCollectionInterface[] $addFilterStrategies


Please remove the duplicate parameter declaration

Sorry, that is not a duplicated declaration. They are different.

Sorry, my bad, you are absolutely right!

sivaschenko · 2018-11-19T10:17:40Z

app/code/Magento/Bundle/Ui/DataProvider/Product/BundleDataProvider.php

     */
    public function __construct(
        $name,
        $primaryFieldName,
        $requestFieldName,
        CollectionFactory $collectionFactory,
        Data $dataHelper,
+        StockStateInterface $stockState = null,


Please move the new added dependency to the end of parameters list

sivaschenko · 2018-11-19T10:22:44Z

app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/General.php

    /**
     * @param LocatorInterface $locator
     * @param ArrayManager $arrayManager
+     * @param StockStateInterface $stockState


Docblock for parameters does not match the actual sequence of parameters

sivaschenko · 2018-11-19T10:26:25Z

app/code/Magento/CatalogInventory/Api/StockQtyDecimalInterface.php

+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+


Please enable the strict type mode with declare(strict_types=1); and specify strict types for parameters

sivaschenko · 2018-11-19T10:29:10Z

app/code/Magento/CatalogInventory/Model/StockState.php

+     *
+     * @return bool
+     */
+    public function isStockQtyDecimal($productId, $scopeId = null): bool


Please enable strict type mode with declare(strict_types=1);

vasilii-b · 2018-11-21T20:01:53Z

@sivaschenko can you please take a look on added changes ? Thank you!

sivaschenko

Thanks for the updates. The code is fine, however, there is an issue with commits. It looks like you made commits under two different accounts, CLA is signed only for one of them, can you please squash your changes under the correct github account

sivaschenko · 2018-11-22T08:15:37Z

app/code/Magento/Bundle/Ui/DataProvider/Product/BundleDataProvider.php

     * @param array $meta
     * @param array $data
+     * @param \Magento\Ui\DataProvider\AddFieldToCollectionInterface[] $addFieldStrategies
+     * @param \Magento\Ui\DataProvider\AddFilterToCollectionInterface[] $addFilterStrategies


Sorry, my bad, you are absolutely right!

vasilii-b · 2018-11-22T11:13:20Z

Ok @sivaschenko , I'll do it. Thank you!

… do not work with decimal inventory

vasilii-b · 2018-11-26T09:27:07Z

Hi @sivaschenko, all checks are passed now. Thank you!

magento-engcom-team · 2018-11-28T09:35:38Z

Hi @sivaschenko, thank you for the review.
ENGCOM-3577 has been created to process this Pull Request

sivaschenko

Hi @vasilii-b thanks for the contribution. Can you please cover the introduced functionality with MFTF/integration tests.

sivaschenko · 2018-12-14T16:28:51Z

app/code/Magento/CatalogInventory/Api/StockQtyDecimalInterface.php

+/**
+ *  StockQtyDecimalInterface
+ */
+interface StockQtyDecimalInterface


This interface looks more like SPI than API, it's better to place it in the Model namespace

sivaschenko · 2019-01-03T16:17:01Z

@vasilii-b closing this pull request due to inactivity. Feel free to reopen it if you'd like to continue progress on it.

vasilii-b · 2019-01-04T07:42:30Z

@sivaschenko perfect! I'll delete the branch then as I'm full of played ping-pong here: changes are accepted then after a while comes someone else and request changes (and so a couple of times). Moreover, I've spotted that it is a common approach to "kindly force" for integration tests. I'm doing contribution and as far as I understand it - that's what I like to help with.
Seriously, I'm done with it since this PR has more than 1 year from its creation and still playing ping-pong. It's either accepted either deleted at all.

P.S. moving that service from API to SPI folder could be done on Magento side.

vasilii-b assigned rogyar Jan 25, 2018

vasilii-b mentioned this pull request Jan 25, 2018

MAGETWO-86164 (Issue 12914) product Qty decimal related issues (both front-end and back-end) #13284

Closed

vasilii-b force-pushed the issue-12914-mage2.3 branch from f777c86 to 0671a8c Compare March 24, 2018 11:45

ishakhsuvarov requested changes Jul 5, 2018

View reviewed changes

rogyar added the Progress: needs update label Jul 17, 2018

vasilii-b changed the title ~~MAGETWO-86164 (Issue 12914) product Qty decimal related issues (both front-end and back-end)~~ [Forwardport] MAGETWO-86164 (Issue 12914) product Qty decimal related issues (both front-end and back-end) Aug 8, 2018

sidolov added the Release Line: 2.3 label Aug 8, 2018

ishakhsuvarov added this to the Release: 2.3.1 milestone Sep 18, 2018

rogyar closed this Oct 9, 2018

vasilii-b reopened this Oct 13, 2018

vasilii-b removed the Progress: needs update label Oct 20, 2018

magento deleted a comment from magento-engcom-team Oct 20, 2018

vasilii-b requested a review from sidolov November 5, 2018 07:27

vasilii-b assigned sidolov Nov 5, 2018

vasilii-b added Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Component: Catalog Component: Bundle labels Nov 9, 2018

vasilii-b added Component: CatalogInventory Area: Frontend Partner: Atwix Pull Request is created by partner Atwix labels Nov 9, 2018

sidolov requested changes Nov 12, 2018

View reviewed changes

vasilii-b changed the title ~~[Forwardport] MAGETWO-86164 (Issue 12914) product Qty decimal related issues (both front-end and back-end)~~ Tiered pricing and quantity Increments do not work with decimal inventory MAGETWO-86164 (both front-end and back-end) Nov 13, 2018

vasilii-b force-pushed the issue-12914-mage2.3 branch 2 times, most recently from 140eb17 to be53c05 Compare November 19, 2018 06:01

sivaschenko requested changes Nov 19, 2018

View reviewed changes

sivaschenko self-assigned this Nov 19, 2018

sivaschenko requested changes Nov 22, 2018

View reviewed changes

magento/magento2:magento#12914 Tiered pricing and quantity Increments…

c7e4db8

… do not work with decimal inventory

vasilii-b force-pushed the issue-12914-mage2.3 branch from d34408b to c7e4db8 Compare November 26, 2018 08:59

sivaschenko approved these changes Nov 28, 2018

View reviewed changes

magento-engcom-team added the partners-contribution Pull Request is created by Magento Partner label Nov 28, 2018

sivaschenko requested changes Dec 14, 2018

View reviewed changes

sivaschenko closed this Jan 3, 2019

vasilii-b mentioned this pull request Jan 28, 2019

Tiered pricing and quantity Increments do not work with decimal inventory #12914

Closed

Tiered pricing and quantity Increments do not work with decimal inventory MAGETWO-86164 (both front-end and back-end) #13359

Tiered pricing and quantity Increments do not work with decimal inventory MAGETWO-86164 (both front-end and back-end) #13359

Uh oh!

Conversation

vasilii-b commented Jan 25, 2018

Description

Fixed Issues

[1] Cannot close Advanced Inventory pop-up after setting Qty Uses Decimals to No and having Enable Qty Increments enabled

Steps to reproduce:

Expected Result

Actual result

[2] Product Tiered Pricing Issue

[3] Quantity increment issue

Steps to reproduce

Expected result

Actual result

[4] After saving a product with Qty Decimals enabled, Qty Increments and Tier Price Quantity are integer numbers if decimal value was given

Steps to reproduce

Expected result

Actual result

[5] integer vs decimal numbers affects the front-end in the same way, e.g. mini cart and checkout

Steps to reproduce

Expected result

Actual result

Uh oh!

rogyar commented Feb 16, 2018

Uh oh!

magento-cicd2 commented Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasilii-b commented Mar 24, 2018

Uh oh!

rogyar commented Mar 25, 2018

Uh oh!

vasilii-b commented Jun 30, 2018

Uh oh!

PieterCappelle commented Jul 2, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ishakhsuvarov commented Jul 5, 2018

Uh oh!

sidolov commented Sep 20, 2018

Uh oh!

rogyar commented Oct 9, 2018

Uh oh!

vasilii-b commented Oct 14, 2018

Uh oh!

vasilii-b commented Oct 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasilii-b commented Nov 19, 2018

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

magento-cicd2 commented Mar 24, 2018 •

edited

Loading