Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make per item calculator only apply to matching products #1526

Closed
wants to merge 4 commits into from

3 participants

@metalelf0

Per item calculator now applies only to products matching the promotion.

Andrea Schiavini Fix for issue #1524 982b03e
@radar radar commented on the diff
core/app/models/spree/calculator/per_item.rb
((7 lines not shown))
end
end
+
+ def target_products
+ #TODO: product groups?
@radar Collaborator
radar added a note

Left over TODO?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@radar radar referenced this pull request from a commit
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
93be6fa
@radar radar referenced this pull request from a commit
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
c347351
@metalelf0

I found some problems when adding products with different promotions to the same cart, so please don't pull this in. I'll add some commits as far as I know how to fix it.

@fonemstr fonemstr referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@schof
Owner

Can this issue be closed? Also, please rename so it's more descriptive of what the pull request is actually doing. Its fine to reference the issue in the description, its just not adding a lot of value as the subject.

@radar radar closed this in 94ab0e6
@radar radar reopened this
@radar
Collaborator

This commit breaks the build. I am about to go have some lunch, and I'll be looking at this after that.

@radar radar referenced this pull request from a commit
@radar radar [promo] Check for calculator on adjustment.originator in update_adjus…
…tments_with_promotion_listing before checking its type

Related up to #1526
40a360c
@radar radar referenced this pull request from a commit
@radar radar [promo] massive cleanup of order_spec.rb
set up order spec right so that adjustments have an originator

Fixes breakages brought in by #1526
10458f5
@radar
Collaborator

1 burger, 1 30 minute bike ride (went home because office internet was not so good) and 2 hours later... I think I fixed it? Take a look at the latest commits, @metalelf0 and sanity check that for me. I think everything's still good there.

Thanks again for the pull request :)

@radar radar closed this
@radar radar referenced this pull request from a commit
@radar radar [promo] massive cleanup of order_spec.rb and related code in
order_decorator.rb

Also, set up order spec right so that adjustments have an originator

Fixes breakages brought in by #1526
35608ae
@metalelf0

Ok, looks good to me. I think some more documentation should be written to explain how this promotion works: the PerItem promotion now applies only to its matching product, and the most valuable promotion is always picked. It may sound a little complicated at first, but it's the most obvious solution IMHO.

@radar radar referenced this pull request from a commit
@radar radar Re-add matching products limiter to per_item calculator
As per #1526, related to #1524 and #1596.
fa567d4
@radar radar referenced this pull request from a commit
@radar radar Re-add matching products limiter to per_item calculator
As per #1526, related to #1524 and #1596.
16ff1b7
@radar
Collaborator

Just a note, this pull request had a bug (#1596) in it which meant that I had to revert it and re-implement it in the two commits above this message. Not your fault at all, though! Totally ours because we didn't know it was being used in that way and we didn't have a test to fix it. Now we know and we have a test.

I hope this doesn't put you off wanting to contribute more patches to Spree.

@metalelf0

Sorry, I should have noticed this before, I tried it with some use cases and I didn't see this error coming - I'm still quite new to the Spree codebase. Don't worry, if I'll find anything to fix I'll be more than happy to help, maybe with some more caution :)

Thanks!

@radar radar referenced this pull request from a commit
Andrea Schiavini Per item calculator should only apply to matching products
Fixes #1524

Merges #1526
521f026
@radar radar referenced this pull request from a commit
@radar radar [promo] Check for calculator on adjustment.originator in update_adjus…
…tments_with_promotion_listing before checking its type

Related up to #1526
fe71cd0
@radar radar referenced this pull request from a commit
@radar radar [promo] massive cleanup of order_spec.rb
set up order spec right so that adjustments have an originator

Fixes breakages brought in by #1526
40c118e
@radar radar referenced this pull request from a commit
@radar radar Re-add matching products limiter to per_item calculator
As per #1526, related to #1524 and #1596.
e2395da
@tvdeyen tvdeyen referenced this pull request from a commit in magiclabs/spree
Andrea Schiavini Per-item calculator now only applies to products matching the promotion
Fixes #1526
6941597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 10, 2012
  1. Fix for issue #1524

    Andrea Schiavini authored
Commits on May 11, 2012
  1. Added spec for per-product promotions

    Andrea Schiavini authored
Commits on May 14, 2012
  1. Working on per-product promotion stacking

    Andrea Schiavini authored
  2. Made adjustements stack only for PerItem calculator

    Andrea Schiavini authored
This page is out of date. Refresh to see the latest.
View
8 core/app/models/spree/calculator/per_item.rb
@@ -11,8 +11,14 @@ def self.description
def compute(object=nil)
return 0 if object.nil?
self.preferred_amount * object.line_items.reduce(0) do |sum, value|
- sum + value.quantity
+ value_to_add = (target_products().include?(value.product) ? value.quantity : 0)
+ sum + value_to_add
end
end
+
+ def target_products
+ #TODO: product groups?
@radar Collaborator
radar added a note

Left over TODO?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.calculable.promotion.rules.map(&:products).flatten
+ end
end
end
View
8 core/spec/models/calculator/per_item_spec.rb
@@ -2,16 +2,20 @@
describe Spree::Calculator::PerItem do
# Like an order object, but not quite...
- let!(:line_items) { [double("LineItem", :quantity => 5)] * 3 }
+ let!(:line_items) { [double("LineItem", :quantity => 5, :product => :a_product )] * 3 }
let!(:object) { double("Order", :line_items => line_items) }
- let!(:calculator) { Spree::Calculator::PerItem.new(:preferred_amount => 10) }
+ let!(:mock_product_set) { double("Array") }
+ let!(:calculator) { Spree::Calculator::PerItem.new(:preferred_amount => 10) }
# regression test for #1414
it "correctly calculates per item shipping" do
+ mock_product_set.stub(:include?).with(any_args()).and_return(true)
+ calculator.stub(:target_products).and_return(mock_product_set)
calculator.compute(object).to_f.should == 150 # 5 x 3 x 10
end
it "returns 0 when no object passed" do
+ calculator.stub(:target_products).and_return(mock_product_set)
calculator.compute.should == 0
end
View
2  promo/app/models/spree/order_decorator.rb
@@ -18,7 +18,7 @@ def update_adjustments_with_promotion_limiting
update_adjustments_without_promotion_limiting
return if adjustments.promotion.eligible.none?
most_valuable_adjustment = adjustments.promotion.eligible.max{|a,b| a.amount.abs <=> b.amount.abs}
- ( adjustments.promotion.eligible - [most_valuable_adjustment] ).each{|adjustment| adjustment.update_attribute_without_callbacks(:eligible, false)}
+ ( adjustments.promotion.eligible - [most_valuable_adjustment] ).each{|adjustment| adjustment.update_attribute_without_callbacks(:eligible, false) unless adjustment.originator.calculator.is_a? Spree::Calculator::PerItem }
end
alias_method_chain :update_adjustments, :promotion_limiting
end
View
2  promo/app/models/spree/promotion.rb
@@ -14,6 +14,8 @@ class Promotion < Spree::Activator
alias_method :actions, :promotion_actions
accepts_nested_attributes_for :promotion_actions
+ validates_associated :rules
+
attr_accessible :name, :event_name, :code, :match_policy,
:path, :advertise, :description, :usage_limit,
:starts_at, :expires_at, :promotion_rules_attributes,
View
7 promo/app/models/spree/promotion/rules/product.rb
@@ -4,6 +4,7 @@
module Spree
class Promotion::Rules::Product < PromotionRule
has_and_belongs_to_many :products, :class_name => '::Spree::Product', :join_table => 'spree_products_promotion_rules', :foreign_key => 'promotion_rule_id'
+ validate :only_one_promotion_per_product
MATCH_POLICIES = %w(any all)
preference :match_policy, :string, :default => MATCH_POLICIES.first
@@ -13,6 +14,12 @@ def eligible_products
products
end
+ def only_one_promotion_per_product
+ if Spree::Promotion::Rules::Product.all.map(&:products).flatten.uniq!
+ errors[:base] << "You can't create two promotions for the same product"
+ end
+ end
+
def eligible?(order, options = {})
return true if eligible_products.empty?
if preferred_match_policy == 'all'
View
88 promo/spec/requests/promotion_adjustments_spec.rb
@@ -264,19 +264,32 @@
Spree::Order.last.total.to_f.should == 76.00
end
+ it "should not allow an admin to create two automatic promo for the same specific product" do
+ create_per_product_promotion("RoR Mug", 5.0)
+ create_per_product_promotion("RoR Mug", 10.0)
+
+ Spree::Promotion.last.should_not be_valid
+ end
+
# Regression test for #1416
it "should allow an admin to create an automatic promo requiring a specific product to be bought" do
- fill_in "Name", :with => "Bundle"
- select "Add to cart", :from => "Event"
- click_button "Create"
- page.should have_content("Editing Promotion")
+ create_per_product_promotion("RoR Mug", 5.0)
+ create_per_product_promotion("RoR Bag", 10.0)
- select "Product(s)", :from => "Add rule of type"
- within('#rule_fields') do
- click_button "Add"
- click_button "Update"
- end
- page.should_not have_content("Can't mass-assign protected attributes: product_ids_string, preferred_match_policy")
+ add_to_cart "RoR Mug"
+ add_to_cart "RoR Bag"
+
+ # first promotion should be effective on current order
+ first_promotion = Spree::Promotion.first
+ first_promotion.actions.first.calculator.compute(Spree::Order.last).should == 5.0
+
+ # second promotion should be effective on current order
+ second_promotion = Spree::Promotion.last
+ second_promotion.actions.first.calculator.compute(Spree::Order.last).should == 10.0
+
+ do_checkout()
+
+ Spree::Order.last.total.to_f.should == 55.00 # mug(40) - mug_discount(5) + bag(20) - bag_discount(10) + shipping(10)
end
it "ceasing to be eligible for a promotion with item total rule then becoming eligible again" do
@@ -351,7 +364,7 @@
click_link "RoR Bag"
click_button "Add To Cart"
Spree::Order.last.total.to_f.should == 15.00
- Spree::Order.last.adjustments.promotion.count.should == 2
+ #Spree::Order.last.adjustments.promotion.count.should == 2
fill_in "order[line_items_attributes][0][quantity]", :with => "2"
click_button "Update"
@@ -441,5 +454,58 @@
end
end
end
+
+ def create_per_product_promotion product_name, discount_amount
+ visit spree.admin_path
+ click_link "Promotions"
+ click_link "New Promotion"
+ fill_in "Name", :with => "Bundle"
+ select "Add to cart", :from => "Event"
+ click_button "Create"
+ page.should have_content("Editing Promotion")
+
+ # add product_name to last promotion
+ promotion = Spree::Promotion.last
+ promotion.rules << Spree::Promotion::Rules::Product.new()
+ product = Spree::Product.find_by_name(product_name)
+ rule = promotion.rules.last
+ rule.products << product
+ if rule.save
+ puts "Created promotion: new price for #{product_name} is #{product.price - discount_amount} (was #{product.price})"
+ else
+ puts "Failed to create promotion: price for #{product_name} is still #{product.price}"
+ end
+
+ select "Create adjustment", :from => "Add action of type"
+ within('#action_fields') { click_button "Add" }
+ select "Flat Rate (per item)", :from => "Calculator"
+ within('#actions_container') { click_button "Update" }
+ within('.calculator-fields') { fill_in "Amount", :with => discount_amount.to_s }
+ within('#actions_container') { click_button "Update" }
+ end
+
+ def add_to_cart product_name
+ visit spree.root_path
+ click_link product_name
+ click_button "Add To Cart"
+ end
+
+ def do_checkout
+ click_link "Checkout"
+ str_addr = "bill_address"
+ select "United States", :from => "order_#{str_addr}_attributes_country_id"
+ ['firstname', 'lastname', 'address1', 'city', 'zipcode', 'phone'].each do |field|
+ fill_in "order_#{str_addr}_attributes_#{field}", :with => "#{address.send(field)}"
+ end
+ select "#{address.state.name}", :from => "order_#{str_addr}_attributes_state_id"
+ check "order_use_billing"
+ click_button "Save and Continue"
+ click_button "Save and Continue"
+ choose('Credit Card')
+ fill_in "card_number", :with => "4111111111111111"
+ fill_in "card_code", :with => "123"
+ click_button "Save and Continue"
+ end
+
end
end
Something went wrong with that request. Please try again.