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

Refactor calculators to be more opinionated what to compute #4041

Closed
wants to merge 15 commits into
base: master
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+136 −68
Diff settings

Always

Just for now

@@ -2,11 +2,19 @@ module Spree
class Calculator < ActiveRecord::Base
belongs_to :calculable, polymorphic: true
# This method must be overriden in concrete calculator.
# This method calls a compute_<computable> method. must be overriden in concrete calculator.
#
# It should return amount computed based on #calculable and/or optional parameter
def compute(something = nil)
raise NotImplementedError, 'please use concrete calculator'
# It should return amount computed based on #calculable and the computable parameter
def compute(computable)
# Spree::LineItem -> :compute_line_item
computable_name = computable.class.name.demodulize.underscore
method = "compute_#{computable_name}".to_sym
the_caller = caller[0].split(':')[0]

This comment has been minimized.

@radar

radar Dec 2, 2013

Member

Interesting use of caller here.

@radar

radar Dec 2, 2013

Member

Interesting use of caller here.

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 5, 2013

Member

Yes, also totally wrong! Was thinking the caller was the subclass, but was basically the spec or controller. Have other solution for this in the works though.

@peterberkenbosch

peterberkenbosch Dec 5, 2013

Member

Yes, also totally wrong! Was thinking the caller was the subclass, but was basically the spec or controller. Have other solution for this in the works though.

begin
self.send(method, computable)
rescue NoMethodError
raise NotImplementedError, "Please implement '#{method}(#{computable_name})' in your calculator: #{the_caller}"
end
end
# overwrite to provide description for your calculators
@@ -6,50 +6,38 @@ def self.description
Spree.t(:default_tax)
end
def compute(computable)
case computable
when Spree::Shipment
compute_shipment(computable)
when Spree::LineItem
compute_line_item(computable)
when Spree::Order # for legacy reasons
compute_order(computable)
# Default tax calculator still needs to support orders for legacy reasons
# Orders created before Spree 2.1 had tax adjustments applied to the order, as a whole.
# Orders created with Spree 2.2 and after, have them applied to the line items individually.
def compute_order(order)
matched_line_items = order.line_items.select do |line_item|
line_item.tax_category == rate.tax_category
end
end
private
line_items_total = matched_line_items.sum(&:total)
round_to_two_places(line_items_total * rate.amount)
end
def rate
self.calculable
end
def compute_shipment(shipment)
round_to_two_places(shipment.discounted_cost * rate.amount)
end
# Default tax calculator still needs to support orders for legacy reasons
# Orders created before Spree 2.1 had tax adjustments applied to the order, as a whole.
# Orders created with Spree 2.2 and after, have them applied to the line items individually.
def compute_order(order)
matched_line_items = order.line_items.select do |line_item|
line_item.tax_category == rate.tax_category
def compute_line_item(line_item)
if line_item.tax_category == rate.tax_category
if rate.included_in_price
deduced_total_by_rate(line_item, rate)
else
round_to_two_places(line_item.discounted_amount * rate.amount)
end
line_items_total = matched_line_items.sum(&:total)
round_to_two_places(line_items_total * rate.amount)
else
0
end
end
def compute_shipment(shipment)
round_to_two_places(shipment.discounted_cost * rate.amount)
end
def compute_line_item(line_item)
if line_item.tax_category == rate.tax_category
if rate.included_in_price
deduced_total_by_rate(line_item, rate)
else
round_to_two_places(line_item.discounted_amount * rate.amount)
end
else
0
end
def rate
self.calculable
end
def round_to_two_places(amount)
@@ -1,13 +1,9 @@
module Spree
class ShippingCalculator < Calculator
def compute(package_or_shipment)
package = if package_or_shipment.respond_to?(:to_package)
package_or_shipment.to_package
else
package_or_shipment
end
compute_package package
def compute_shipment(shipment)
compute(shipment.to_package)

This comment has been minimized.

@radar

radar Dec 3, 2013

Member

Interesting that this calculator went from taking either a package or a shipment to now just taking a shipment that's turned into a package. Why is that? What was passing in a package before?

@radar

radar Dec 3, 2013

Member

Interesting that this calculator went from taking either a package or a shipment to now just taking a shipment that's turned into a package. Why is that? What was passing in a package before?

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 3, 2013

Member

It supports both calls, for a shipment or a package. The previous code just got the package from the shipment and computed that. (see https://github.com/spree/spree/blob/master/core/app/models/spree/shipping_calculator.rb)

@peterberkenbosch

peterberkenbosch Dec 3, 2013

Member

It supports both calls, for a shipment or a package. The previous code just got the package from the shipment and computed that. (see https://github.com/spree/spree/blob/master/core/app/models/spree/shipping_calculator.rb)

This comment has been minimized.

@mrpollo

mrpollo Dec 5, 2013

Contributor

@radar I recall this allowing for an order to be passed as well at some point.

@peterberkenbosch I believe the main use for calculators on shipments are for compute_package specially on spree_active_shipping, I advice care if you use the different implementations for the same thing which is calling compute because of the different overrides we might find already implemented, I know that this happened before when we went from compute to compute_package on spree_active_shipping, what's stopping us from doing compute_package(shipment.to_package) in here? that might help with this, unless I'm totally missing the point which might be the case :trollface:

@mrpollo

mrpollo Dec 5, 2013

Contributor

@radar I recall this allowing for an order to be passed as well at some point.

@peterberkenbosch I believe the main use for calculators on shipments are for compute_package specially on spree_active_shipping, I advice care if you use the different implementations for the same thing which is calling compute because of the different overrides we might find already implemented, I know that this happened before when we went from compute to compute_package on spree_active_shipping, what's stopping us from doing compute_package(shipment.to_package) in here? that might help with this, unless I'm totally missing the point which might be the case :trollface:

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 6, 2013

Member

good point @mrpollo the current implementation is aiming on keeping the original specs pass. But I see what you meen, the compute_shipment can only work if the shipment actually responds to to_package which is not very clear.

@peterberkenbosch

peterberkenbosch Dec 6, 2013

Member

good point @mrpollo the current implementation is aiming on keeping the original specs pass. But I see what you meen, the compute_shipment can only work if the shipment actually responds to to_package which is not very clear.

end
def compute_package(package)
@@ -10,10 +10,10 @@ module Calculator::Shipping
let(:line_item2) { build(:line_item, variant: variant2) }
let(:package) do
double(
Stock::Package,
order: mock_model(Order),
contents: [
Stock::Package.new(
build(:stock_location),
mock_model(Order),
[
Stock::Package::ContentItem.new(line_item1, variant1, 2),
Stock::Package::ContentItem.new(line_item2, variant2, 1)
]
@@ -5,15 +5,22 @@ module Calculator::Shipping
describe FlatRate do
let(:variant1) { build(:variant) }
let(:variant2) { build(:variant) }
let(:package) { double(Stock::Package,
order: mock_model(Order),
contents: [Stock::Package::ContentItem.new(variant1, 2),
Stock::Package::ContentItem.new(variant2, 1)]) }
let(:package) do
Stock::Package.new(
build(:stock_location),
mock_model(Order),
[
Stock::Package::ContentItem.new(variant1, 2),
Stock::Package::ContentItem.new(variant2, 1)
]
)
end
subject { Calculator::Shipping::FlatRate.new(:preferred_amount => 4.00) }
it 'always returns the same rate' do
subject.compute(package)
expect(subject.compute(package)).to eql 4.00
end
end
end
@@ -10,10 +10,10 @@ module Calculator::Shipping
let(:line_item2) { build(:line_item, variant: variant2) }
let(:package) do
double(
Stock::Package,
order: mock_model(Order),
contents: [
Stock::Package.new(
build(:stock_location),
mock_model(Order),
[
Stock::Package::ContentItem.new(line_item1, variant1, 4),
Stock::Package::ContentItem.new(line_item2, variant2, 6)
]
@@ -10,10 +10,10 @@ module Calculator::Shipping
let(:line_item2) { build(:line_item, variant: variant2) }
let(:package) do
double(
Stock::Package,
order: mock_model(Order),
contents: [
Stock::Package.new(
build(:stock_location),
mock_model(Order),
[
Stock::Package::ContentItem.new(line_item1, variant1, 5),
Stock::Package::ContentItem.new(line_item2, variant2, 3)
]
@@ -0,0 +1,69 @@
require 'spec_helper'
describe Spree::Calculator do
let(:order) { create(:order) }
let!(:line_item) { create(:line_item, :order => order) }
let(:shipment) { create(:shipment, :order => order, :stock_location => create(:stock_location_with_items)) }
context "with computable" do
context "and compute methods stubbed out" do
context "with a Spree::LineItem" do
it "calls compute_line_item" do
subject.should_receive(:compute_line_item).with(line_item)
subject.compute(line_item)
end
end
context "with a Spree::Order" do
it "calls compute_order" do
subject.should_receive(:compute_order).with(order)
subject.compute(order)
end
end
context "with a Spree::Shipment" do
it "calls compute_shipment" do
subject.should_receive(:compute_shipment).with(shipment)
subject.compute(shipment)
end
end
context "with a arbitray object" do
it "calls the correct compute" do
s = "Calculator can all"
subject.should_receive(:compute_string).with(s)
subject.compute(s)
end
end
end
context "with no stubbing" do
context "with a Spree::LineItem" do
it "raises NotImplementedError" do
expect{subject.compute(line_item)}.to raise_error NotImplementedError, /Please implement \'compute_line_item\(line_item\)\' in your calculator/
end
end
context "with a Spree::Order" do
it "raises NotImplementedError" do
expect{subject.compute(order)}.to raise_error NotImplementedError, /Please implement \'compute_order\(order\)\' in your calculator/
end
end
context "with a Spree::Shipment" do
it "raises NotImplementedError" do
expect{subject.compute(shipment)}.to raise_error NotImplementedError, /Please implement \'compute_shipment\(shipment\)\' in your calculator/
end
end
context "with a arbitray object" do
it "raises NotImplementedError" do
s = "Calculator can all"
expect{subject.compute(s)}.to raise_error NotImplementedError, /Please implement \'compute_string\(string\)\' in your calculator/
end
end
end
end
end
@@ -9,10 +9,10 @@ module Spree
let(:line_item2) { build(:line_item, variant: variant2) }
let(:package) do
double(
Stock::Package,
order: mock_model(Order),
contents: [
Stock::Package.new(
build(:stock_location),
mock_model(Order),
[
Stock::Package::ContentItem.new(line_item1, variant1, 2),
Stock::Package::ContentItem.new(line_item2, variant2, 1)
]
ProTip! Use n and p to navigate between commits in a pull request.