Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393

luckyduck · 2018-01-26T17:02:56Z

Description

The price for custom options should be allowed to be negative. The contained changes do just that. In addition, a cosmetic problem is solved. Once options are allowed to be negative, available options (in a drop-down) add a plus sign as prefix to the price, resulting in '+-1,50 €" for a negative option price (-1,50 in this example). This is fixed as well.

Fixed Issues (if relevant)

Unable to set negative custom option fixed price in admin view. #7333: Unable to set negative custom option fixed price in admin view

Manual testing scenarios

Add a custom option to any product.
Enter a negative price (for example -3) for a custom option, see Unable to set negative custom option fixed price in admin view. #7333 for details.
Save the product
Visit the product url
Take a look at the Drop-Down (select element). You'll find the option text like "+-3 €" (or dollar)

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…-60573

magento-cicd2 · 2018-01-26T17:02:59Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

luckyduck · 2018-01-26T17:05:31Z

Just signed the CLA (and added the mail address to my github account as well)

oliverklee · 2018-01-28T21:06:44Z

app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php

@@ -84,7 +84,7 @@ public function testIsValidTitle($title, $type, $priceType, $price, $product, $m
        $valueMock->expects($this->once())->method('getTitle')->will($this->returnValue($title));
        $valueMock->expects($this->any())->method('getType')->will($this->returnValue($type));
        $valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue($priceType));
-        $valueMock->expects($this->once())->method('getPrice')->will($this->returnValue($price));
+        $valueMock->expects($this->never())->method('getPrice')->will($this->returnValue($price));


If the method should never be called, the will is not necessary (and will probably confuse the reader).

oliverklee · 2018-01-28T21:07:00Z

app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php

    {
        $methods = ['getTitle', 'getType', 'getPriceType', 'getPrice', '__wakeup', 'getProduct'];
        $valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods);
        $valueMock->expects($this->once())->method('getTitle')->will($this->returnValue($title));
        $valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue($type));
        $valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue($priceType));
-        $valueMock->expects($this->once())->method('getPrice')->will($this->returnValue($price));
+        $valueMock->expects($this->never())->method('getPrice')->will($this->returnValue($price));


The same here.

oliverklee · 2018-01-28T21:07:34Z

app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php

-        $this->assertFalse($this->validator->isValid($valueMock));
-        $this->assertEquals($messages, $this->validator->getMessages());
+        $this->assertTrue($this->validator->isValid($valueMock));
+        $this->assertNotEquals($messages, $this->validator->getMessages());


I propose making the check more specific, i.e., to check for the expected messages instead.

Thank you so much for your feedback! Will make the proposed changes and improve the tests.

…nvalid price types fail. Also makes sure price is not relevant for validation (assert that getPrice is not called)

luckyduck · 2018-01-29T11:45:59Z

Updated tests, to reflect the new situation:

price is not relevant, make sure getPrice is not called during validation
make sure valid price types are recognized as valid and invalid types cause error messages

dmanners · 2018-05-24T10:17:22Z

Hi @luckyduck could you take a look at the merge conflict for me please, then I can process this PR.

luckyduck · 2018-05-29T08:23:48Z

Oh, of course. Can you give me a hint, how best to do this?

dmanners · 2018-05-29T08:28:15Z

I would recommend updating your main 2.2-develop branch from our repository and then updating your branch from this.

First you need to add a new remote.

git remote add upstream [email protected]:magento/magento2.git
git fetch -ap upstream

Then when you are checked out on your feature branch you can rebase on the upstream/2.2-develop branch rather than the origin/2.2-develop branch.

git rebase -i upstream/2.2-develop

It will then ask you which commits you would like and it will also ask you to fix any conflicts that you come across.

dmanners · 2018-06-27T10:04:27Z

Hi @luckyduck thank you for this PR. I am currently doing a couple of things.

Checking with the core team about the history of not allowing negative prices to make sure we do not miss something,
Working out the differences between this PR and [Bugfix] #7333 Unable to set negative custom option fixed price in admin view. #15267 and Add ability to set custom product option with negative price #14342 to see as to which one is the preferred fix.

I will update you via this PR once I have some more information. Thank you for your patience.

dmanners · 2018-07-11T12:31:06Z

Hi @luckyduck thank you for your pull request. I am closing this pull request as it has been covered by #15267.

Cosmetic fix for magento#7333 in GitHub or internal ticket id MAGETWO…

4d9c75e

…-60573

luckyduck added 2 commits January 26, 2018 18:52

Don't prevent negative custom option prices.

0570a7c

fixed unit tests

df6e275

luckyduck changed the title ~~Cosmetic fix for #7333 in GitHub or internal ticket id MAGETWO-60573~~ Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) Jan 27, 2018

oliverklee reviewed Jan 28, 2018

View reviewed changes

luckyduck added 2 commits January 29, 2018 11:59

If the method should never be called, the will is not necessary

a846858

Change tests: make sure valid price types for options are valid and i…

a553cd0

…nvalid price types fail. Also makes sure price is not relevant for validation (assert that getPrice is not called)

luckyduck added 2 commits January 29, 2018 13:41

Remove unused parameter from test

b11965f

cosmectic fix: remove empty line

b51d8c0

ihor-sviziev mentioned this pull request Mar 24, 2018

Add ability to set custom product option with negative price #14342

Closed

This was referenced May 12, 2018

Fix #7333 Unable to set negative custom option fixed price in admin view. #15179

Closed

[Bugfix] #7333 Unable to set negative custom option fixed price in admin view. #15267

Merged

dmanners self-assigned this May 24, 2018

dmanners closed this Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393

Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393

Uh oh!

luckyduck commented Jan 26, 2018 •

edited

Loading

Uh oh!

magento-cicd2 commented Jan 26, 2018 •

edited

Loading

Uh oh!

luckyduck commented Jan 26, 2018 •

edited

Loading

Uh oh!

oliverklee Jan 28, 2018

Uh oh!

oliverklee Jan 28, 2018

Uh oh!

oliverklee Jan 28, 2018

Uh oh!

luckyduck Jan 29, 2018

Uh oh!

luckyduck commented Jan 29, 2018

Uh oh!

dmanners commented May 24, 2018

Uh oh!

luckyduck commented May 29, 2018

Uh oh!

dmanners commented May 29, 2018

Uh oh!

dmanners commented Jun 27, 2018

Uh oh!

dmanners commented Jul 11, 2018

Uh oh!

Uh oh!

Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393

Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393

Uh oh!

Conversation

luckyduck commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

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

Uh oh!

Uh oh!

luckyduck commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverklee Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

oliverklee Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

oliverklee Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

luckyduck Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

luckyduck commented Jan 29, 2018

Uh oh!

dmanners commented May 24, 2018

Uh oh!

luckyduck commented May 29, 2018

Uh oh!

dmanners commented May 29, 2018

Uh oh!

dmanners commented Jun 27, 2018

Uh oh!

dmanners commented Jul 11, 2018

Uh oh!

Uh oh!

luckyduck commented Jan 26, 2018 •

edited

Loading

magento-cicd2 commented Jan 26, 2018 •

edited

Loading

luckyduck commented Jan 26, 2018 •

edited

Loading