Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Per item calculator also applies to non matching products #1524

Closed
metalelf0 opened this Issue · 9 comments

3 participants

@metalelf0

Here's an example, is this a desired behaviour?

Promotion:
Rules: any product in [ product_one ]
Actions: create adjustment per item, -5.0

product_one:
price: 10

product_two:
price: 20

Order:
1x product_one
2x product_two

Expected order total, excluding taxes and shipping: (10 - 5) + 20 * 2 = 45
Actual order total, excluding taxes and shipping: (10 - 5) + (20 - 5) * 2 = 35

@metalelf0

This has been tested on Spree 1.1.0 with ruby 1.9.3.

@metalelf0 metalelf0 referenced this issue from a commit in metalelf0/spree
Andrea Schiavini Fix for issue #1524 982b03e
@radar radar closed this issue from a commit
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
93be6fa
@radar radar closed this in 93be6fa
@fonemstr fonemstr referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@JDutil JDutil referenced this issue from a commit in JDutil/spree
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
c347351
@metalelf0

Ok, I understood what was wrong. The problem is that only the most valuable promotion is picked. So, if I have the following scenario:

  create_per_product_promotion("RoR Mug", 5.0)
  create_per_product_promotion("RoR Bag", 10.0)
  add_to_cart "RoR Mug"
  add_to_cart "RoR Bag"

Currently only the second promotion would be applied. I managed to modify the code so that both the promotions would count as valid (commenting lines 20 and 21 in promo/app/models/spree/order_decorator.rb), but:

  1. this obviously breaks a test about promotions not stacking
  2. something still would be needed to prevent promotions on the same product from stacking, e.g.

    create_per_product_promotion("RoR Mug", 5.0)
    create_per_product_promotion("RoR Mug", 10.0)
    add_to_cart "RoR Mug"

in this case the discount should not be 15. I think the best way to achieve this would be preventing the second promotion from being created, only allowing one fixed price promotion per product. What do you think?

@BDQ
Owner

I don't think preventing the creation of more than one promotion per product is the correct solution, as different promotions could have different criteria / rules.

@metalelf0

I know, but I would find quite strange to have two fixed price promotions on the same product with the same calculator type. Like, "Put product XYZ in the cart and get a 5 dollars discount" and "Put product XYZ in the cart and get a 10 dollars discount". In my commit #72f2abb, if you check, only the Rules::Product class is validating the uniqueness, so that it doesn't affect other rule types.

@BDQ
Owner

I could see:

  • "Put product XYZ in the cart and get a 5 dollars discount"
  • "Put product XYZ in the cart and get a 10 dollars discount if its your first order"
@metalelf0

So what about adding an "exclusive" (or, by contrary, "stackable") flag to discounts? Do you have any suggestion?

@BDQ
Owner

We used to have a feature like that, can't remember why it was removed from promotions. I'm open to re-introducing it.

@metalelf0

I've checked this again and I decided to implement a simple price-variation model on my own app, as I've seen the Spree Promotions model are more tied to the order than to the individual item, and changing this behavior would me a major change I can't afford at the moment. I'd suggest closing this issue and reverting the pull request, unless anybody else wants to step in.

@radar radar reopened this
@radar
Collaborator

This is now fixed with the two commits above.

@radar radar closed this
@tvdeyen tvdeyen referenced this issue from a commit in magiclabs/spree
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
521f026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.