raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-02-27 01:43:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2096 from oeoeaio/subs-estimates

Browse Source 
Subs:  Improve estimates of item prices and fees
This commit is contained in:
Rob H
2018-03-02 13:22:27 +11:00
committed by GitHub
parent 14caac0212 6f554814ce
commit 1d8f332cc6
15 changed files with 262 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
app/assets/javascripts/admin/subscriptions/services/subscription_functions.js.coffee
View File

@@ -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

13
app/controllers/admin/subscription_line_items_controller.rb
View File

@@ -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

21
app/controllers/admin/subscriptions_controller.rb
View File

@@ -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

5
app/models/subscription.rb
View File

@@ -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?

8
app/models/subscription_line_item.rb
View File

@@ -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

8
app/serializers/api/admin/subscription_line_item_serializer.rb
View File

@@ -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

9
app/serializers/api/admin/subscription_serializer.rb
View File

@@ -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

58
app/services/subscription_estimator.rb Normal file
View File

@@ -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

33
app/services/subscription_form.rb
View File

@@ -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

2
app/views/admin/subscriptions/_data.html.haml
View File

@@ -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

6
db/migrate/20180222231639_add_shipping_fee_estimate_and_payment_fee_estimate_to_subscription.rb Normal file
View File

@@ -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

24
db/schema.rb
View File

@@ -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"

2
spec/features/admin/subscriptions_spec.rb
View File

@@ -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)
}

129
spec/services/subscription_estimator_spec.rb Normal file
View File

@@ -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

62
spec/services/subscription_form_spec.rb
View File

@@ -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