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

[Promotion] Contains product promotion fix #6451

Merged

Conversation

lchrusciel
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Related tickets
License MIT

I have added few tests and fix current implementation of contains product promotion.

@@ -540,6 +542,16 @@ public function theCouponWasUsed(PromotionCouponInterface $coupon)
}

/**
* @Given /^([^"]+) gives ("(?:€|£|\$)[^"]+") off if order contains [a|an] ("[^"]*" product)$/
Copy link
Contributor

Choose a reason for hiding this comment

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

[a|an] matches a, | or n, you probably meant (?:a|an)

@@ -540,6 +542,16 @@ public function theCouponWasUsed(PromotionCouponInterface $coupon)
}

/**
* @Given /^([^"]+) gives ("(?:€|£|\$)[^"]+") off if order contains [a|an] ("[^"]*" product)$/
Copy link
Contributor

Choose a reason for hiding this comment

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

("[^"]*" product) -> ("[^"]+" product)

@@ -62,4 +62,11 @@ public function createContainsTaxon($taxonCode, $count);
* @return PromotionRuleInterface
*/
public function createNthOrder($nth);

/**
* @param int $productCode
Copy link
Contributor

Choose a reason for hiding this comment

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

int -> string

->add('variant', 'sylius_product_variant_from_identifier', [
'label' => 'sylius.form.promotion_action.add_product_configuration.variant',
'class' => $this->variantRepository->getClassName(),
->add('product', 'sylius_product_from_identifier', [
Copy link
Contributor

Choose a reason for hiding this comment

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

product -> product_code?

if ($configuration['variant'] != $item->getVariant()->getId()) {
continue;
}
if ($configuration['product'] === $item->getVariant()->getProduct()->getCode()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you typehint on Core's OrderItemInterface, you can use $item->getProduct() instead of $item->getVariant()->getProduct().

}

function it_returns_true_if_variant_is_right_and_exclude_is_not_set(
function it_returns_true_if_product_is_right(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have two products here and the wanted one as the second so we will be sure that we are checking every item.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lchrusciel lchrusciel force-pushed the contains-product-promotion branch 2 times, most recently from 0ea249a to 1b171eb Compare October 18, 2016 15:10
@@ -11,12 +11,14 @@

namespace Sylius\Behat\Context\Setup;

use Behat\Behat\Tester\Exception\PendingException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed

@@ -11,13 +11,15 @@

namespace Sylius\Behat\Context\Ui\Admin;

use Behat\Behat\Tester\Exception\PendingException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

if ($configuration['variant'] != $item->getVariant()->getId()) {
continue;
}
if ($configuration['product_code'] === $item->getProduct()->getCode()) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant blank line

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -34,14 +37,13 @@ public function isEligible(PromotionSubjectInterface $subject, array $configurat

Copy link
Contributor

Choose a reason for hiding this comment

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

Not visible, but above this line we should use Assert::instanceOf instead of if

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, I guess. There is a custom exception thrown. Similar to all others rule checkers

When I add product "PHP T-Shirt" to the cart
Then my cart total should be "$80.00"
And my discount should be "-$20.00"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing one more case, if I buy 2 units of "PHP T-Shirt" should I receive -$20 discount or -$40 discount?


@ui
Scenario: Receiving no discount on order while buying product different then discounted
Given the promotion gives "$20.00" off if order contains a "PHP T-Shirt" product
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be moved to the background.

Background:
Given the store operates on a single channel in "United States"
And the store has a product "PHP T-Shirt" priced at "$100.00"
And the store has a product "PHP Mug" priced at "$20.00"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be placed in the second scenario only

@lchrusciel lchrusciel force-pushed the contains-product-promotion branch 2 times, most recently from 7a36310 to 6d3cb29 Compare October 19, 2016 07:48
@michalmarcinkowski michalmarcinkowski merged commit 7e7c4d3 into Sylius:master Oct 19, 2016
@michalmarcinkowski
Copy link
Contributor

Thanks Łukasz!

@lchrusciel lchrusciel deleted the contains-product-promotion branch October 19, 2016 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants