diff --git a/app/assets/javascripts/admin/subscriptions/services/subscription_functions.js.coffee b/app/assets/javascripts/admin/subscriptions/services/subscription_functions.js.coffee index be274360d1..9eee828dd0 100644 --- a/app/assets/javascripts/admin/subscriptions/services/subscription_functions.js.coffee +++ b/app/assets/javascripts/admin/subscriptions/services/subscription_functions.js.coffee @@ -8,8 +8,11 @@ angular.module("admin.subscriptions").factory 'SubscriptionFunctions', ($injecto subtotal += item.price_estimate * item.quantity , 0 + estimatedFees: -> + @shipping_fee_estimate + @payment_fee_estimate + estimatedTotal: -> - @estimatedSubtotal() + @estimatedSubtotal() + @estimatedFees() customer: -> return unless @customer_id diff --git a/app/controllers/admin/subscription_line_items_controller.rb b/app/controllers/admin/subscription_line_items_controller.rb index 0726fbdf89..5011129c5f 100644 --- a/app/controllers/admin/subscription_line_items_controller.rb +++ b/app/controllers/admin/subscription_line_items_controller.rb @@ -11,10 +11,8 @@ module Admin def build @subscription_line_item.assign_attributes(params[:subscription_line_item]) - fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(@shop, @order_cycle) if @order_cycle - OpenFoodNetwork::ScopeVariantToHub.new(@shop).scope(@variant) - @subscription_line_item.variant = @variant # Ensures override price is used - render json: @subscription_line_item, serializer: Api::Admin::SubscriptionLineItemSerializer, fee_calculator: fee_calculator + @subscription_line_item.price_estimate = price_estimate + render json: @subscription_line_item, serializer: Api::Admin::SubscriptionLineItemSerializer end private @@ -44,5 +42,12 @@ module Admin error = "#{@shop.name} is not permitted to sell the selected product" render json: { errors: [error] }, status: :unprocessable_entity end + + def price_estimate + return unless @order_cycle + fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(@shop, @order_cycle) + OpenFoodNetwork::ScopeVariantToHub.new(@shop).scope(@variant) + @variant.price + fee_calculator.indexed_fees_for(@variant) + end end end diff --git a/app/controllers/admin/subscriptions_controller.rb b/app/controllers/admin/subscriptions_controller.rb index 8542076834..32a24883c1 100644 --- a/app/controllers/admin/subscriptions_controller.rb +++ b/app/controllers/admin/subscriptions_controller.rb @@ -31,18 +31,18 @@ module Admin end def create - form = SubscriptionForm.new(@subscription, params[:subscription], fee_calculator) + form = SubscriptionForm.new(@subscription, params[:subscription]) if form.save - render_as_json @subscription, fee_calculator: fee_calculator + render_as_json @subscription else render json: { errors: form.json_errors }, status: :unprocessable_entity end end def update - form = SubscriptionForm.new(@subscription, params[:subscription], fee_calculator) + form = SubscriptionForm.new(@subscription, params[:subscription]) if form.save - render_as_json @subscription, fee_calculator: fee_calculator, order_update_issues: form.order_update_issues + render_as_json @subscription, order_update_issues: form.order_update_issues else render json: { errors: form.json_errors }, status: :unprocessable_entity end @@ -52,7 +52,7 @@ module Admin @subscription.cancel(@open_orders_to_keep || []) respond_with(@subscription) do |format| - format.json { render_as_json @subscription, fee_calculator: fee_calculator } + format.json { render_as_json @subscription } end end @@ -62,12 +62,12 @@ module Admin end @subscription.update_attributes(paused_at: Time.zone.now) - render_as_json @subscription, fee_calculator: fee_calculator + render_as_json @subscription end def unpause @subscription.update_attributes(paused_at: nil) - render_as_json @subscription, fee_calculator: fee_calculator + render_as_json @subscription end private @@ -96,13 +96,6 @@ module Admin @payment_methods = Spree::PaymentMethod.for_distributor(@subscription.shop).for_subscriptions @shipping_methods = Spree::ShippingMethod.for_distributor(@subscription.shop) @order_cycles = OrderCycle.joins(:schedules).managed_by(spree_current_user) - @fee_calculator = fee_calculator - end - - def fee_calculator - shop, next_oc = @subscription.shop, @subscription.schedule.andand.current_or_next_order_cycle - return nil unless shop && next_oc - OpenFoodNetwork::EnterpriseFeeCalculator.new(shop, next_oc) end # Wrap :subscription_line_items_attributes in :subscription root diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 3b9a8345a8..c5a3d8bd55 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -57,6 +57,11 @@ class Subscription < ActiveRecord::Base "active" end + # Used to calculators to estimate fees + def line_items + subscription_line_items + end + private def pending? diff --git a/app/models/subscription_line_item.rb b/app/models/subscription_line_item.rb index 549cb05cd1..38f2694be0 100644 --- a/app/models/subscription_line_item.rb +++ b/app/models/subscription_line_item.rb @@ -10,5 +10,13 @@ class SubscriptionLineItem < ActiveRecord::Base (price_estimate || 0) * (quantity || 0) end + # Used to calculators to estimate fees + alias_method :amount, :total_estimate + + # Used to calculators to estimate fees + def price + price_estimate + end + default_scope order('id ASC') end diff --git a/app/serializers/api/admin/subscription_line_item_serializer.rb b/app/serializers/api/admin/subscription_line_item_serializer.rb index 999ec37d05..23f4c6bc52 100644 --- a/app/serializers/api/admin/subscription_line_item_serializer.rb +++ b/app/serializers/api/admin/subscription_line_item_serializer.rb @@ -8,13 +8,7 @@ module Api end def price_estimate - if object.price_estimate - object.price_estimate - elsif options[:fee_calculator] - (object.variant.price + options[:fee_calculator].indexed_fees_for(object.variant)).to_f - else - "?" - end + object.price_estimate.andand.to_f || "?" end end end diff --git a/app/serializers/api/admin/subscription_serializer.rb b/app/serializers/api/admin/subscription_serializer.rb index d0fbb0497c..cbba4cc483 100644 --- a/app/serializers/api/admin/subscription_serializer.rb +++ b/app/serializers/api/admin/subscription_serializer.rb @@ -3,6 +3,7 @@ module Api class SubscriptionSerializer < ActiveModel::Serializer attributes :id, :shop_id, :customer_id, :schedule_id, :payment_method_id, :shipping_method_id, :begins_at, :ends_at attributes :customer_email, :schedule_name, :edit_path, :canceled_at, :paused_at, :state, :credit_card_id + attributes :shipping_fee_estimate, :payment_fee_estimate has_many :subscription_line_items, serializer: Api::Admin::SubscriptionLineItemSerializer has_many :closed_proxy_orders, serializer: Api::Admin::ProxyOrderSerializer @@ -38,6 +39,14 @@ module Api return '' unless object.id edit_admin_subscription_path(object) end + + def shipping_fee_estimate + object.shipping_fee_estimate.to_f + end + + def payment_fee_estimate + object.payment_fee_estimate.to_f + end end end end diff --git a/app/services/subscription_estimator.rb b/app/services/subscription_estimator.rb new file mode 100644 index 0000000000..5abe388029 --- /dev/null +++ b/app/services/subscription_estimator.rb @@ -0,0 +1,58 @@ +# Responsible for estimating prices and fees for subscriptions +# Used by SubscriptionForm as part of the create/update process +# The values calculated here are intended to be persisted in the db + +class SubscriptionEstimator + def initialize(subscription) + @subscription = subscription + end + + def estimate! + assign_price_estimates + assign_fee_estimates + end + + private + + attr_accessor :subscription + + delegate :subscription_line_items, :shipping_method, :payment_method, :shop, to: :subscription + + def assign_price_estimates + subscription_line_items.each do |item| + item.price_estimate = + price_estimate_for(item.variant, item.price_estimate_was) + end + end + + def price_estimate_for(variant, fallback) + return fallback unless fee_calculator && variant + scoper.scope(variant) + fees = fee_calculator.indexed_fees_for(variant) + (variant.price + fees).to_d + end + + def fee_calculator + return @fee_calculator unless @fee_calculator.nil? + next_oc = subscription.schedule.andand.current_or_next_order_cycle + return nil unless shop && next_oc + @fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(shop, next_oc) + end + + def scoper + OpenFoodNetwork::ScopeVariantToHub.new(shop) + end + + def assign_fee_estimates + subscription.shipping_fee_estimate = shipping_fee_estimate + subscription.payment_fee_estimate = payment_fee_estimate + end + + def shipping_fee_estimate + shipping_method.calculator.compute(subscription) + end + + def payment_fee_estimate + payment_method.calculator.compute(subscription) + end +end diff --git a/app/services/subscription_form.rb b/app/services/subscription_form.rb index 26cf6190f3..6b20dd700c 100644 --- a/app/services/subscription_form.rb +++ b/app/services/subscription_form.rb @@ -1,24 +1,24 @@ 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 - def initialize(subscription, params = {}, fee_calculator = nil) + def initialize(subscription, params = {}) @subscription = subscription @params = params - @fee_calculator = fee_calculator + @estimator = SubscriptionEstimator.new(subscription) @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/app/views/admin/subscriptions/_data.html.haml b/app/views/admin/subscriptions/_data.html.haml index 046b732d14..0cf4e3db35 100644 --- a/app/views/admin/subscriptions/_data.html.haml +++ b/app/views/admin/subscriptions/_data.html.haml @@ -1,4 +1,4 @@ -= admin_inject_json_ams "admin.subscriptions", "subscription", @subscription, Api::Admin::SubscriptionSerializer, fee_calculator: @fee_calculator if @subscription += admin_inject_json_ams "admin.subscriptions", "subscription", @subscription, Api::Admin::SubscriptionSerializer if @subscription = admin_inject_json_ams_array "admin.subscriptions", "shops", @shops, Api::Admin::IdNameSerializer if @shops = admin_inject_json_ams_array "admin.subscriptions", "customers", @customers, Api::Admin::IdEmailSerializer if @customers = admin_inject_json_ams_array "admin.subscriptions", "schedules", @schedules, Api::Admin::IdNameSerializer if @schedules diff --git a/db/migrate/20180222231639_add_shipping_fee_estimate_and_payment_fee_estimate_to_subscription.rb b/db/migrate/20180222231639_add_shipping_fee_estimate_and_payment_fee_estimate_to_subscription.rb new file mode 100644 index 0000000000..2c4f29d891 --- /dev/null +++ b/db/migrate/20180222231639_add_shipping_fee_estimate_and_payment_fee_estimate_to_subscription.rb @@ -0,0 +1,6 @@ +class AddShippingFeeEstimateAndPaymentFeeEstimateToSubscription < ActiveRecord::Migration + def change + add_column :subscriptions, :shipping_fee_estimate, :decimal, :precision => 8, :scale => 2 + add_column :subscriptions, :payment_fee_estimate, :decimal, :precision => 8, :scale => 2 + end +end diff --git a/db/schema.rb b/db/schema.rb index 19204bbb1d..9e5675b63b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20180204235108) do +ActiveRecord::Schema.define(:version => 20180222231639) do create_table "account_invoices", :force => true do |t| t.integer "user_id", :null => false @@ -212,6 +212,7 @@ ActiveRecord::Schema.define(:version => 20180204235108) do t.string "description" t.text "long_description" t.boolean "is_primary_producer" + t.string "contact_name" t.string "phone" t.string "website" t.string "twitter" @@ -247,7 +248,6 @@ ActiveRecord::Schema.define(:version => 20180204235108) do t.text "invoice_text" t.boolean "display_invoice_logo", :default => false t.boolean "allow_order_changes", :default => false, :null => false - t.string "contact_name" t.boolean "enable_subscriptions", :default => false, :null => false end @@ -1104,20 +1104,22 @@ ActiveRecord::Schema.define(:version => 20180204235108) do add_index "subscription_line_items", ["variant_id"], :name => "index_subscription_line_items_on_variant_id" create_table "subscriptions", :force => true do |t| - t.integer "shop_id", :null => false - t.integer "customer_id", :null => false - t.integer "schedule_id", :null => false - t.integer "payment_method_id", :null => false - t.integer "shipping_method_id", :null => false + t.integer "shop_id", :null => false + t.integer "customer_id", :null => false + t.integer "schedule_id", :null => false + t.integer "payment_method_id", :null => false + t.integer "shipping_method_id", :null => false t.datetime "begins_at" t.datetime "ends_at" - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false - t.integer "bill_address_id", :null => false - t.integer "ship_address_id", :null => false + t.datetime "created_at", :null => false + t.datetime "updated_at", :null => false + t.integer "bill_address_id", :null => false + t.integer "ship_address_id", :null => false t.datetime "canceled_at" t.datetime "paused_at" t.integer "credit_card_id" + t.decimal "shipping_fee_estimate", :precision => 8, :scale => 2 + t.decimal "payment_fee_estimate", :precision => 8, :scale => 2 end add_index "subscriptions", ["bill_address_id"], :name => "index_subscriptions_on_bill_address_id" diff --git a/spec/features/admin/subscriptions_spec.rb b/spec/features/admin/subscriptions_spec.rb index cea558f523..9bc5377bb6 100644 --- a/spec/features/admin/subscriptions_spec.rb +++ b/spec/features/admin/subscriptions_spec.rb @@ -292,7 +292,7 @@ feature 'Subscriptions' do schedule: schedule, payment_method: payment_method, shipping_method: shipping_method, - subscription_line_items: [create(:subscription_line_item, variant: variant1, quantity: 2)], + subscription_line_items: [create(:subscription_line_item, variant: variant1, quantity: 2, price_estimate: 13.75)], with_proxy_orders: true) } diff --git a/spec/services/subscription_estimator_spec.rb b/spec/services/subscription_estimator_spec.rb new file mode 100644 index 0000000000..5230057888 --- /dev/null +++ b/spec/services/subscription_estimator_spec.rb @@ -0,0 +1,129 @@ +describe SubscriptionEstimator do + describe "estimating prices for subscription line items" 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(:estimator) { SubscriptionEstimator.new(subscription) } + + 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) + + # Simulating assignment of attrs from params + sli1.assign_attributes(price_estimate: 7.0) + sli2.assign_attributes(price_estimate: 8.0) + sli3.assign_attributes(price_estimate: 9.0) + end + + context "when a insufficient information exists to calculate price estimates" do + before do + # This might be because a shop has not been assigned yet, or no + # current or future order cycles exist for the schedule + allow(estimator).to receive(:fee_calculator) { nil } + end + + it "resets the price estimates for all items" do + estimator.estimate! + expect(sli1.price_estimate).to eq 4.0 + expect(sli2.price_estimate).to eq 5.0 + expect(sli3.price_estimate).to eq 6.0 + end + end + + context "when sufficient information to calculate price estimates exists" do + let(:fee_calculator) { instance_double(OpenFoodNetwork::EnterpriseFeeCalculator) } + + before do + allow(estimator).to receive(:fee_calculator) { fee_calculator } + 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 + + context "when no variant overrides apply" do + 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 + + context "when variant overrides apply" do + let!(:override1) { create(:variant_override, hub: subscription.shop, variant: sli1.variant, price: 1.2) } + let!(:override2) { create(:variant_override, hub: subscription.shop, variant: sli2.variant, price: 2.3) } + + it "recalculates price_estimates based on override prices and associated fees" do + estimator.estimate! + expect(sli1.price_estimate).to eq 2.2 + expect(sli2.price_estimate).to eq 2.3 + expect(sli3.price_estimate).to eq 6.0 + end + end + end + end + + describe "updating estimates for shipping and payment fees" do + let(:subscription) { create(:subscription, with_items: true, payment_method: payment_method, shipping_method: shipping_method) } + let!(:sli1) { subscription.subscription_line_items.first } + let!(:sli2) { subscription.subscription_line_items.second } + let!(:sli3) { subscription.subscription_line_items.third } + let(:estimator) { SubscriptionEstimator.new(subscription) } + + before do + allow(estimator).to receive(:assign_price_estimates) + sli1.update_attributes(price_estimate: 4.0) + sli2.update_attributes(price_estimate: 5.0) + sli3.update_attributes(price_estimate: 6.0) + end + + context "using flat rate calculators" do + let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 12.34)) } + let(:payment_method) { create(:payment_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 9.12)) } + + it "calculates fees based on the rates provided" do + estimator.estimate! + expect(subscription.shipping_fee_estimate.to_f).to eq 12.34 + expect(subscription.payment_fee_estimate.to_f).to eq 9.12 + end + end + + context "using flat percent item total calculators" do + let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10)) } + let(:payment_method) { create(:payment_method, calculator: Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 20)) } + + it "calculates fees based on the estimated item total and percentage provided" do + estimator.estimate! + expect(subscription.shipping_fee_estimate.to_f).to eq 1.5 + expect(subscription.payment_fee_estimate.to_f).to eq 3.0 + end + end + + context "using flat percent per item calculators" do + let(:shipping_method) { create(:shipping_method, calculator: Calculator::FlatPercentPerItem.new(preferred_flat_percent: 5)) } + let(:payment_method) { create(:payment_method, calculator: Calculator::FlatPercentPerItem.new(preferred_flat_percent: 10)) } + + it "calculates fees based on the estimated item prices and percentage provided" do + estimator.estimate! + expect(subscription.shipping_fee_estimate.to_f).to eq 0.75 + expect(subscription.payment_fee_estimate.to_f).to eq 1.5 + end + end + + context "using per item calculators" do + let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::PerItem.new(preferred_amount: 1.2)) } + let(:payment_method) { create(:payment_method, calculator: Spree::Calculator::PerItem.new(preferred_amount: 0.3)) } + + it "calculates fees based on the number of items and rate provided" do + estimator.estimate! + expect(subscription.shipping_fee_estimate.to_f).to eq 3.6 + expect(subscription.payment_fee_estimate.to_f).to eq 0.9 + end + end + end +end diff --git a/spec/services/subscription_form_spec.rb b/spec/services/subscription_form_spec.rb index 2e9a469f3c..2f4a19953e 100644 --- a/spec/services/subscription_form_spec.rb +++ b/spec/services/subscription_form_spec.rb @@ -35,9 +35,9 @@ 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} ] } } @@ -48,6 +48,10 @@ describe SubscriptionForm do 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 +92,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