From b85e38010f7358699526b2650b7ef0bdae88a86b Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 23 Feb 2018 12:36:22 +1100 Subject: [PATCH] Delegate responsibility to estimating prices for subscriptions to dedicated service object --- app/services/subscription_estimator.rb | 28 +++++++++ app/services/subscription_form.rb | 31 +--------- spec/services/subscription_estimator_spec.rb | 45 ++++++++++++++ spec/services/subscription_form_spec.rb | 65 +++----------------- 4 files changed, 85 insertions(+), 84 deletions(-) create mode 100644 app/services/subscription_estimator.rb create mode 100644 spec/services/subscription_estimator_spec.rb diff --git a/app/services/subscription_estimator.rb b/app/services/subscription_estimator.rb new file mode 100644 index 0000000000..7af3ffb109 --- /dev/null +++ b/app/services/subscription_estimator.rb @@ -0,0 +1,28 @@ +class SubscriptionEstimator + def initialize(subscription, fee_calculator) + @subscription = subscription + @fee_calculator = fee_calculator + end + + def estimate! + assign_price_estimates + end + + private + + attr_accessor :subscription, :fee_calculator + + delegate :subscription_line_items, to: :subscription + + def assign_price_estimates + subscription_line_items.each do |item| + item.price_estimate = price_estimate_for(item.variant) + end + end + + def price_estimate_for(variant) + return 0.0 unless fee_calculator && variant + fees = fee_calculator.indexed_fees_for(variant) + (variant.price + fees).to_d + end +end diff --git a/app/services/subscription_form.rb b/app/services/subscription_form.rb index 26cf6190f3..707c664a5d 100644 --- a/app/services/subscription_form.rb +++ b/app/services/subscription_form.rb @@ -1,7 +1,7 @@ require 'open_food_network/proxy_order_syncer' class SubscriptionForm - attr_accessor :subscription, :params, :fee_calculator, :order_update_issues, :validator, :order_syncer + attr_accessor :subscription, :params, :order_update_issues, :validator, :order_syncer, :estimator delegate :json_errors, :valid?, to: :validator delegate :order_update_issues, to: :order_syncer @@ -9,16 +9,16 @@ class SubscriptionForm def initialize(subscription, params = {}, fee_calculator = nil) @subscription = subscription @params = params - @fee_calculator = fee_calculator + @estimator = SubscriptionEstimator.new(subscription, fee_calculator) @validator = SubscriptionValidator.new(subscription) @order_syncer = OrderSyncer.new(subscription) end def save - validate_price_estimates subscription.assign_attributes(params) return false unless valid? subscription.transaction do + estimator.estimate! proxy_order_syncer.sync! order_syncer.sync! subscription.save! @@ -30,29 +30,4 @@ class SubscriptionForm def proxy_order_syncer OpenFoodNetwork::ProxyOrderSyncer.new(subscription) end - - def validate_price_estimates - return unless params[:subscription_line_items_attributes] - return clear_price_estimates unless fee_calculator - calculate_prices_from_variant_ids - end - - def clear_price_estimates - params[:subscription_line_items_attributes].each do |item_attrs| - item_attrs.delete(:price_estimate) - end - end - - def calculate_prices_from_variant_ids - params[:subscription_line_items_attributes].each do |item_attrs| - variant = Spree::Variant.find_by_id(item_attrs[:variant_id]) - next item_attrs.delete(:price_estimate) unless variant - item_attrs[:price_estimate] = price_estimate_for(variant) - end - end - - def price_estimate_for(variant) - fees = fee_calculator.indexed_fees_for(variant) - (variant.price + fees).to_d - end end diff --git a/spec/services/subscription_estimator_spec.rb b/spec/services/subscription_estimator_spec.rb new file mode 100644 index 0000000000..555ea28f39 --- /dev/null +++ b/spec/services/subscription_estimator_spec.rb @@ -0,0 +1,45 @@ +describe SubscriptionEstimator do + describe "#estimate!" do + let!(:subscription) { create(:subscription, with_items: true) } + let!(:sli1) { subscription.subscription_line_items.first } + let!(:sli2) { subscription.subscription_line_items.second } + let!(:sli3) { subscription.subscription_line_items.third } + let(:fee_calculator) { nil } + let(:estimator) { SubscriptionEstimator.new(subscription, fee_calculator) } + + before do + sli1.update_attributes(price_estimate: 4.0) + sli2.update_attributes(price_estimate: 5.0) + sli3.update_attributes(price_estimate: 6.0) + sli1.variant.update_attributes(price: 1.0) + sli2.variant.update_attributes(price: 2.0) + sli3.variant.update_attributes(price: 3.0) + end + + context "when a fee calculator is not present" do + it "removes price estimates from all items" do + estimator.estimate! + subscription.subscription_line_items.each do |item| + expect(item.price_estimate).to eq 0 + end + end + end + + context "when a fee calculator is present" do + let(:fee_calculator) { instance_double(OpenFoodNetwork::EnterpriseFeeCalculator) } + + before do + allow(fee_calculator).to receive(:indexed_fees_for).with(sli1.variant) { 1.0 } + allow(fee_calculator).to receive(:indexed_fees_for).with(sli2.variant) { 0.0 } + allow(fee_calculator).to receive(:indexed_fees_for).with(sli3.variant) { 3.0 } + end + + it "recalculates price_estimates based on variant prices and associated fees" do + estimator.estimate! + expect(sli1.price_estimate).to eq 2.0 + expect(sli2.price_estimate).to eq 2.0 + expect(sli3.price_estimate).to eq 6.0 + end + end + end +end diff --git a/spec/services/subscription_form_spec.rb b/spec/services/subscription_form_spec.rb index 2e9a469f3c..1e2d44bb08 100644 --- a/spec/services/subscription_form_spec.rb +++ b/spec/services/subscription_form_spec.rb @@ -21,6 +21,7 @@ describe SubscriptionForm do let!(:payment_method) { create(:payment_method, distributors: [shop]) } let!(:shipping_method) { create(:shipping_method, distributors: [shop]) } let!(:address) { create(:address) } + let!(:fee_calculator) { OpenFoodNetwork::EnterpriseFeeCalculator.new(shop, order_cycle2) } let(:subscription) { Subscription.new } let!(:params) { @@ -35,19 +36,23 @@ describe SubscriptionForm do begins_at: 4.days.ago, ends_at: 14.days.from_now, subscription_line_items_attributes: [ - {variant_id: variant1.id, quantity: 1}, - {variant_id: variant2.id, quantity: 2}, - {variant_id: variant3.id, quantity: 3} + {variant_id: variant1.id, quantity: 1, price_estimate: 7.0}, + {variant_id: variant2.id, quantity: 2, price_estimate: 8.0}, + {variant_id: variant3.id, quantity: 3, price_estimate: 9.0} ] } } - let(:form) { SubscriptionForm.new(subscription, params) } + let(:form) { SubscriptionForm.new(subscription, params, fee_calculator) } it "creates orders for each order cycle in the schedule" do Spree::Config.set allow_backorders: false expect(form.save).to be true expect(subscription.proxy_orders.count).to be 2 + expect(subscription.subscription_line_items.count).to be 3 + expect(subscription.subscription_line_items[0].price_estimate).to eq 13.75 + expect(subscription.subscription_line_items[1].price_estimate).to eq 7.75 + expect(subscription.subscription_line_items[2].price_estimate).to eq 4.25 # This order cycle has already closed, so no order is initialized proxy_order1 = subscription.proxy_orders.find_by_order_cycle_id(order_cycle1.id) @@ -88,56 +93,4 @@ describe SubscriptionForm do expect(proxy_order4).to be nil end end - - describe "validating price_estimates on subscription line items" do - let(:params) { { } } - let(:form) { SubscriptionForm.new(nil, params) } - - context "when line_item params are present" do - before { allow(form).to receive(:price_estimate_for) } - - it "does nothing" do - form.send(:validate_price_estimates) - expect(form.params[:subscription_line_items_attributes]).to be nil - end - end - - context "when line_item params are present" do - before do - params[:subscription_line_items_attributes] = [{ id: 1, price_estimate: 2.50 }, { id: 2, price_estimate: 3.50 }] - end - - context "when no fee calculator is present" do - before { allow(form).to receive(:price_estimate_for) } - - it "clears price estimates on all subscription line item attributes" do - form.send(:validate_price_estimates) - attrs = form.params[:subscription_line_items_attributes] - expect(attrs.first.keys).to_not include :price_estimate - expect(attrs.last.keys).to_not include :price_estimate - expect(form).to_not have_received(:price_estimate_for) - end - end - - context "when a fee calculator is present" do - let(:variant) { create(:variant) } - let(:fee_calculator) { double(:fee_calculator) } - - before do - allow(form).to receive(:fee_calculator) { fee_calculator } - allow(form).to receive(:price_estimate_for) { 5.30 } - params[:subscription_line_items_attributes].first[:variant_id] = variant.id - end - - it "clears price estimates on subscription line item attributes without variant ids" do - form.send(:validate_price_estimates) - attrs = form.params[:subscription_line_items_attributes] - expect(attrs.first.keys).to include :price_estimate - expect(attrs.last.keys).to_not include :price_estimate - expect(attrs.first[:price_estimate]).to eq 5.30 - expect(form).to have_received(:price_estimate_for).with(variant) - end - end - end - end end