diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index e0b44ec1d7..4a2f4242e9 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -17,13 +17,22 @@ module Admin respond_to do |format| format.html format.json do - render_as_json @collection, - tag_rule_mapping: tag_rule_mapping, - customer_tags: customer_tags_by_id + render json: @collection, + each_serializer: index_each_serializer, + tag_rule_mapping: tag_rule_mapping, + customer_tags: customer_tags_by_id end end end + def index_each_serializer + if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user) + ::Api::Admin::CustomerWithBalanceSerializer + else + ::Api::Admin::CustomerWithCalculatedBalanceSerializer + end + end + def show render_as_json @customer, ams_prefix: params[:ams_prefix] end @@ -63,10 +72,20 @@ module Admin private def collection - return Customer.where("1=0") unless json_request? && params[:enterprise_id].present? + if json_request? && params[:enterprise_id].present? + customers_relation. + includes(:bill_address, :ship_address, user: :credit_cards) + else + Customer.where('1=0') + end + end - Customer.of(managed_enterprise_id). - includes(:bill_address, :ship_address, user: :credit_cards) + def customers_relation + if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user) + CustomersWithBalance.new(managed_enterprise_id).query + else + Customer.of(managed_enterprise_id) + end end def managed_enterprise_id diff --git a/app/serializers/api/admin/customer_serializer.rb b/app/serializers/api/admin/customer_serializer.rb index cafc2a6a2f..99d8bad28c 100644 --- a/app/serializers/api/admin/customer_serializer.rb +++ b/app/serializers/api/admin/customer_serializer.rb @@ -4,7 +4,7 @@ module Api module Admin class CustomerSerializer < ActiveModel::Serializer attributes :id, :email, :enterprise_id, :user_id, :code, :tags, :tag_list, :name, - :allow_charges, :default_card_present?, :balance, :balance_status + :allow_charges, :default_card_present? has_one :ship_address, serializer: Api::AddressSerializer has_one :bill_address, serializer: Api::AddressSerializer @@ -17,20 +17,6 @@ module Api customer_tag_list.join(",") end - def balance - Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s - end - - def balance_status - if balance_value.positive? - "credit_owed" - elsif balance_value.negative? - "balance_due" - else - "" - end - end - def tags customer_tag_list.map do |tag| tag_rule_map = options[:tag_rule_mapping].andand[tag] @@ -51,11 +37,6 @@ module Api options[:customer_tags].andand[object.id] || [] end - - def balance_value - @balance_value ||= - OpenFoodNetwork::UserBalanceCalculator.new(object.email, object.enterprise).balance - end end end end diff --git a/app/serializers/api/admin/customer_with_balance_serializer.rb b/app/serializers/api/admin/customer_with_balance_serializer.rb new file mode 100644 index 0000000000..dfbb088991 --- /dev/null +++ b/app/serializers/api/admin/customer_with_balance_serializer.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Api + module Admin + class CustomerWithBalanceSerializer < CustomerSerializer + attributes :balance, :balance_status + + delegate :balance_value, to: :object + + def balance + Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s + end + + def balance_status + if balance_value.positive? + "credit_owed" + elsif balance_value.negative? + "balance_due" + else + "" + end + end + end + end +end diff --git a/app/serializers/api/admin/customer_with_calculated_balance_serializer.rb b/app/serializers/api/admin/customer_with_calculated_balance_serializer.rb new file mode 100644 index 0000000000..6db568ed35 --- /dev/null +++ b/app/serializers/api/admin/customer_with_calculated_balance_serializer.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Api + module Admin + class CustomerWithCalculatedBalanceSerializer < CustomerSerializer + attributes :balance, :balance_status + + def balance + Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s + end + + def balance_status + if balance_value.positive? + "credit_owed" + elsif balance_value.negative? + "balance_due" + else + "" + end + end + + def balance_value + @balance_value ||= + OpenFoodNetwork::UserBalanceCalculator.new(object.email, object.enterprise).balance + end + end + end +end diff --git a/app/services/customers_with_balance.rb b/app/services/customers_with_balance.rb new file mode 100644 index 0000000000..56a3c2d171 --- /dev/null +++ b/app/services/customers_with_balance.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +class CustomersWithBalance + def initialize(enterprise) + @enterprise = enterprise + end + + def query + Customer.of(enterprise). + joins(left_join_complete_orders). + group("customers.id"). + select("customers.*"). + select(outstanding_balance) + end + + private + + attr_reader :enterprise + + # Arel doesn't support CASE statements until v7.1.0 so we'll have to wait with SQL literals + # a little longer. See https://github.com/rails/arel/pull/400 for details. + def outstanding_balance + <<-SQL.strip_heredoc + SUM( + CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total + WHEN state IS NOT NULL THEN payment_total - total + ELSE 0 END + ) AS balance_value + SQL + end + + # The resulting orders are in states that belong after the checkout. Only these can be considered + # for a customer's balance. + def left_join_complete_orders + <<-SQL.strip_heredoc + LEFT JOIN spree_orders ON spree_orders.customer_id = customers.id + AND #{complete_orders.to_sql} + SQL + end + + def complete_orders + states_group = prior_to_completion_states.map { |state| Arel::Nodes.build_quoted(state) } + Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states_group) + end + + def non_fulfilled_states_group + states_group = non_fulfilled_states.map { |state| Arel::Nodes.build_quoted(state) } + Arel::Nodes::Grouping.new(states_group) + end + + # All the states an order can be in before completing the checkout + def prior_to_completion_states + %w(cart address delivery payment) + end + + # All the states of a complete order but that shouldn't count towards the balance. Those that the + # customer won't enjoy. + def non_fulfilled_states + %w(canceled returned) + end +end diff --git a/lib/open_food_network/feature_toggle.rb b/lib/open_food_network/feature_toggle.rb index d40a89b0db..5ee51a82f9 100644 --- a/lib/open_food_network/feature_toggle.rb +++ b/lib/open_food_network/feature_toggle.rb @@ -31,17 +31,19 @@ module OpenFoodNetwork end def self.enable(feature_name, user_emails) + return unless user_emails.present? + Thread.current[:features] ||= {} Thread.current[:features][feature_name] = Feature.new(user_emails) end def initialize - @features = Thread.current[:features] + @features = Thread.current[:features] || {} end def enabled?(feature_name, user) if user.present? - feature = features[feature_name] + feature = features.fetch(feature_name, NullFeature.new) feature.enabled?(user) else true?(env_variable_value(feature_name)) @@ -78,4 +80,10 @@ module OpenFoodNetwork attr_reader :users end + + class NullFeature + def enabled?(_user) + false + end + end end diff --git a/spec/controllers/admin/customers_controller_spec.rb b/spec/controllers/admin/customers_controller_spec.rb index 1a4595ee19..b15b5efdd8 100644 --- a/spec/controllers/admin/customers_controller_spec.rb +++ b/spec/controllers/admin/customers_controller_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'spec_helper' +require 'open_food_network/user_balance_calculator' module Admin describe CustomersController, type: :controller do @@ -41,6 +42,124 @@ module Admin expect(ActiveModel::ArraySerializer).to receive(:new) spree_get :index, params end + + context 'when the customer_balance feature is enabled' do + let(:customers_with_balance) { instance_double(CustomersWithBalance) } + + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, enterprise.owner) { true } + end + + it 'calls CustomersWithBalance' do + allow(CustomersWithBalance) + .to receive(:new).with(enterprise) { customers_with_balance } + + expect(customers_with_balance).to receive(:query) { Customer.none } + + spree_get :index, params + end + + it 'serializes using CustomerWithBalanceSerializer' do + expect(Api::Admin::CustomerWithBalanceSerializer).to receive(:new) + + spree_get :index, params + end + end + + context 'when the customer_balance feature is not enabled' do + let(:calculator) do + instance_double(OpenFoodNetwork::UserBalanceCalculator, balance: 0) + end + + it 'calls Customer.of' do + expect(Customer).to receive(:of).twice.with(enterprise) { Customer.none } + + spree_get :index, params + end + + it 'serializes calling the UserBalanceCalculator' do + expect(OpenFoodNetwork::UserBalanceCalculator) + .to receive(:new).with(customer.email, customer.enterprise) { calculator } + + spree_get :index, params + end + end + + context 'when the customer has no orders' do + it 'includes the customer balance in the response' do + spree_get :index, params + expect(json_response.first["balance"]).to eq("$0.00") + end + end + + context 'when the customer has complete orders' do + let(:order) { create(:order, customer: customer, state: 'complete') } + let!(:line_item) { create(:line_item, order: order, price: 10.0) } + + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, enterprise.owner) { true } + end + + it 'includes the customer balance in the response' do + spree_get :index, params + expect(json_response.first["balance"]).to eq("$-10.00") + end + end + + context 'when the customer has canceled orders' do + let(:order) { create(:order, customer: customer) } + let!(:line_item) { create(:line_item, order: order, price: 10.0) } + let!(:payment) { create(:payment, order: order, amount: order.total) } + + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, enterprise.owner) { true } + + allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true) + order.process_payments! + + order.update_attribute(:state, 'canceled') + end + + it 'includes the customer balance in the response' do + spree_get :index, params + expect(json_response.first["balance"]).to eq("$10.00") + end + end + + context 'when the customer has cart orders' do + let(:order) { create(:order, customer: customer, state: 'cart') } + let!(:line_item) { create(:line_item, order: order, price: 10.0) } + + it 'includes the customer balance in the response' do + spree_get :index, params + expect(json_response.first["balance"]).to eq("$0.00") + end + end + + context 'when the customer has an order with a void payment' do + let(:order) { create(:order, customer: customer, state: 'complete') } + let!(:line_item) { create(:line_item, order: order, price: 10.0) } + let!(:payment) { create(:payment, order: order, amount: order.total) } + + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, enterprise.owner) { true } + + allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true) + order.process_payments! + + payment.void_transaction! + end + + it 'includes the customer balance in the response' do + expect(order.payment_total).to eq(0) + spree_get :index, params + expect(json_response.first["balance"]).to eq('$-10.00') + end + end end context "and enterprise_id is not given in params" do diff --git a/spec/factories/order_factory.rb b/spec/factories/order_factory.rb index 04885c0965..aec931b752 100644 --- a/spec/factories/order_factory.rb +++ b/spec/factories/order_factory.rb @@ -9,7 +9,7 @@ FactoryBot.define do user bill_address completed_at { nil } - email { user.email } + email { user&.email || customer.email } factory :order_with_totals do after(:create) do |order| diff --git a/spec/features/admin/customers_spec.rb b/spec/features/admin/customers_spec.rb index fe428fed00..47b2b11df2 100644 --- a/spec/features/admin/customers_spec.rb +++ b/spec/features/admin/customers_spec.rb @@ -97,9 +97,36 @@ feature 'Customers' do describe "for a shop with multiple customers" do before do - mock_balance(customer1.email, managed_distributor1, 88) - mock_balance(customer2.email, managed_distributor1, -99) - mock_balance(customer4.email, managed_distributor1, 0) + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, user) { true } + + create( + :order, + total: 0, + payment_total: 88, + distributor: managed_distributor1, + user: nil, + state: 'complete', + customer: customer1 + ) + create( + :order, + total: 99, + payment_total: 0, + distributor: managed_distributor1, + user: nil, + state: 'complete', + customer: customer2 + ) + create( + :order, + total: 0, + payment_total: 0, + distributor: managed_distributor1, + user: nil, + state: 'complete', + customer: customer4 + ) customer4.update enterprise: managed_distributor1 end @@ -121,13 +148,6 @@ feature 'Customers' do expect(page).to have_content "$0.00" end end - - def mock_balance(email, enterprise, balance) - user_balance_calculator = instance_double(OpenFoodNetwork::UserBalanceCalculator) - expect(OpenFoodNetwork::UserBalanceCalculator).to receive(:new). - with(email, enterprise).and_return(user_balance_calculator) - expect(user_balance_calculator).to receive(:balance).and_return(balance) - end end it "allows updating of attributes" do diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 2361ff3a21..2a88499e6e 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -201,16 +201,27 @@ describe Spree::Order do let(:payment) { build(:payment) } before { allow(order).to receive_messages pending_payments: [payment], total: 10 } - it "should process the payments" do - expect(payment).to receive(:process!) - expect(order.process_payments!).to be_truthy - end - it "should return false if no pending_payments available" do allow(order).to receive_messages pending_payments: [] expect(order.process_payments!).to be_falsy end + context "when the processing is sucessful" do + it "should process the payments" do + expect(payment).to receive(:process!) + expect(order.process_payments!).to be_truthy + end + + it "stores the payment total on the order" do + allow(payment).to receive(:process!) + allow(payment).to receive(:completed?).and_return(true) + + order.process_payments! + + expect(order.payment_total).to eq(payment.amount) + end + end + context "when a payment raises a GatewayError" do before { expect(payment).to receive(:process!).and_raise(Spree::Core::GatewayError) } diff --git a/spec/models/spree/payment_spec.rb b/spec/models/spree/payment_spec.rb index 17c084e2a0..226855025f 100644 --- a/spec/models/spree/payment_spec.rb +++ b/spec/models/spree/payment_spec.rb @@ -584,6 +584,15 @@ describe Spree::Payment do ) end end + + context 'when the payment was completed but now void' do + let(:payment) { create(:payment, amount: 100, order: order, state: 'completed') } + + it 'updates order payment total' do + payment.void + expect(order.payment_total).to eq 0 + end + end end context "#build_source" do diff --git a/spec/serializers/api/admin/customer_with_balance_serializer_spec.rb b/spec/serializers/api/admin/customer_with_balance_serializer_spec.rb new file mode 100644 index 0000000000..5c0c3f1df5 --- /dev/null +++ b/spec/serializers/api/admin/customer_with_balance_serializer_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::Admin::CustomerWithBalanceSerializer do + let(:serialized_customer) { described_class.new(customer) } + + describe '#balance' do + let(:customer) { double(Customer, balance_value: 1.2) } + let(:money) { instance_double(Spree::Money, to_s: "$1.20") } + + before do + allow(Spree::Money).to receive(:new).with(1.2, currency: "AUD") { money } + end + + it 'returns the balance_value as a money amount' do + expect(serialized_customer.balance).to eq("$1.20") + end + end + + describe '#balance_status' do + context 'when the balance_value is positive' do + let(:customer) { double(Customer, balance_value: 1) } + + it 'returns credit_owed' do + expect(serialized_customer.balance_status).to eq("credit_owed") + end + end + + context 'when the balance_value is negative' do + let(:customer) { double(Customer, balance_value: -1) } + + it 'returns credit_owed' do + expect(serialized_customer.balance_status).to eq("balance_due") + end + end + + context 'when the balance_value is zero' do + let(:customer) { double(Customer, balance_value: 0) } + + it 'returns credit_owed' do + expect(serialized_customer.balance_status).to eq("") + end + end + end +end diff --git a/spec/services/customers_with_balance_spec.rb b/spec/services/customers_with_balance_spec.rb new file mode 100644 index 0000000000..fefd1d9090 --- /dev/null +++ b/spec/services/customers_with_balance_spec.rb @@ -0,0 +1,185 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CustomersWithBalance do + subject(:customers_with_balance) { described_class.new(customer.enterprise.id) } + + describe '#query' do + let(:customer) { create(:customer) } + let(:total) { 200.00 } + let(:order_total) { 100.00 } + + context 'when orders are in cart state' do + before do + create(:order, customer: customer, total: order_total, payment_total: 0, state: 'cart') + create(:order, customer: customer, total: order_total, payment_total: 0, state: 'cart') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(0) + end + end + + context 'when orders are in address state' do + before do + create(:order, customer: customer, total: order_total, payment_total: 0, state: 'address') + create(:order, customer: customer, total: order_total, payment_total: 50, state: 'address') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(0) + end + end + + context 'when orders are in delivery state' do + before do + create(:order, customer: customer, total: order_total, payment_total: 0, state: 'delivery') + create(:order, customer: customer, total: order_total, payment_total: 50, state: 'delivery') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(0) + end + end + + context 'when orders are in payment state' do + before do + create(:order, customer: customer, total: order_total, payment_total: 0, state: 'payment') + create(:order, customer: customer, total: order_total, payment_total: 50, state: 'payment') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(0) + end + end + + context 'when no orders where paid' do + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(-total) + end + end + + context 'when an order was paid' do + let(:payment_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'checkout') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - total) + end + end + + context 'when an order is canceled' do + let(:payment_total) { 100.00 } + let(:non_canceled_orders_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + create( + :order, + customer: customer, + total: order_total, + payment_total: order_total, + state: 'canceled' + ) + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - non_canceled_orders_total) + end + end + + context 'when an order is resumed' do + let(:payment_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'resumed') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - total) + end + end + + context 'when an order is in payment' do + let(:payment_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'payment') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - total) + end + end + + context 'when an order is awaiting_return' do + let(:payment_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'awaiting_return') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - total) + end + end + + context 'when an order is returned' do + let(:payment_total) { order_total } + let(:non_returned_orders_total) { order_total } + + before do + order = create(:order, customer: customer, total: order_total, payment_total: 0) + order.update_attribute(:state, 'checkout') + order = create(:order, customer: customer, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'returned') + end + + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(payment_total - non_returned_orders_total) + end + end + + context 'when there are no orders' do + it 'returns the customer balance' do + customer = customers_with_balance.query.first + expect(customer.balance_value).to eq(0) + end + end + end +end