Skip to content

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

Closed
mszydlo wants to merge 1 commit intomagento:2.2-developfrom
mszydlo:patch-issue-7333
Closed

Add ability to set custom product option with negative price#14342
mszydlo wants to merge 1 commit intomagento:2.2-developfrom
mszydlo:patch-issue-7333

Conversation

@mszydlo
Copy link
Copy Markdown
Contributor

@mszydlo mszydlo commented Mar 24, 2018

Description

Enabled ability to set negative prices for custom product options.

Fixed Issues

  1. 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

  1. Create product.
  2. Add custom option and set negative value.
  3. Save product. For fixed price absolute value of option's price cannot exceed product's special/regular price and if it does - 'Invalid option value' message is shown. For percentage the value of 100 cannot be exceeded.
  4. If negative value is in proper range product saves correctly and option is displayed in storefront with proper + or - sign

Modify option value validator to accept negative prices that don't
exceed product's special/regular price
@magento-cicd2
Copy link
Copy Markdown
Contributor

magento-cicd2 commented Mar 24, 2018

CLA assistant check
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.

@mszydlo mszydlo added the Event: distributed-cd Distributed Contribution Day label Mar 24, 2018
@ihor-sviziev
Copy link
Copy Markdown
Contributor

Related PR: #13393

Copy link
Copy Markdown
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

I reviewed your changes, looks good, but in some parts it's not clear. Also unit tests are failing currently. Could you adjust them to be consistent with your changes + cover new cases?

optionHandlers: {},
optionTemplate: '<%= data.label %>' +
'<% if (data.finalPrice.value) { %>' +
'<% if (data.finalPrice.value > 0) { %>' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't get, why do we need this change?

$title = $value['title'] ?? null;

if ($type === ProductPriceOptionsInterface::VALUE_PERCENT) {
$price += 100.0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you describe why do we need to add +100 there?

}

$customOption = $this->customOptionFactory->create(['data' => $customOptionData]);
$customOption->setProduct($product);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this change?

{
return $this->isInRange($option->getPriceType(), $this->priceTypes) && !$this->isNegative($option->getPrice());
$productPrice = 0.0;
if ($option->getPriceType() === ProductPriceOptionsInterface::VALUE_PERCENT) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you cover these changes with tests?

@dmanners
Copy link
Copy Markdown
Contributor

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

  1. Checking with the core team about the history of not allowing negative prices to make sure we do not miss something,
  2. Working out the differences between this PR and [Bugfix] #7333 Unable to set negative custom option fixed price in admin view. #15267 and Allow negative price for custom option: fixes #7333 in GitHub (internal MAGETWO-60573) #13393 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
Copy link
Copy Markdown
Contributor

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

@dmanners dmanners closed this Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Event: distributed-cd Distributed Contribution Day Release Line: 2.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants