From 412bb24e423b0a9c1f973021c2e3bcd5810fd0d3 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Feb 2021 15:45:07 +0100 Subject: [PATCH 1/6] Ensure report rows are always in the same order I saw the row were returned not respecting any ordering when refreshing the page locally. It made it hard to debug whether or not the customer balance was right. It's less than ideal to use `allow_any_instance_of` but with this legacy and very coupled code, it's the best we can do. --- lib/open_food_network/order_cycle_management_report.rb | 2 +- .../order_cycle_management_report_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index e1f35eed6a..b66e67332f 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -48,7 +48,7 @@ module OpenFoodNetwork end def orders - filter search.result + filter(search.result.order(:id)) end def table_items diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index c11c751df9..9824dcb432 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -26,6 +26,14 @@ module OpenFoodNetwork expect(subject.orders).to eq([o2]) end + it 'orders them by id' do + result = instance_double(ActiveRecord::Relation) + allow_any_instance_of(Ransack::Search).to receive(:result).and_return(result) + expect(result).to receive(:order).with(:id) { Spree::Order.none } + + subject.orders + end + context "default date range" do it "fetches orders completed in the past month" do o1 = create(:order, completed_at: 1.month.ago - 1.day) From 106dcbae01e704019b8f36862d14c02b2255cdfc Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Feb 2021 17:53:48 +0100 Subject: [PATCH 2/6] Add test coverage to the two distinct output rows --- .../order_cycle_management_report.rb | 2 + .../order_cycle_management_report_spec.rb | 66 ++++++++++++++++++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index b66e67332f..2024aac6f0 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -3,7 +3,9 @@ require 'open_food_network/user_balance_calculator' module OpenFoodNetwork class OrderCycleManagementReport DEFAULT_DATE_INTERVAL = { from: -1.month, to: 1.day }.freeze + attr_reader :params + def initialize(user, params = {}, render_table = false) @params = sanitize_params(params) @user = user diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index 9824dcb432..f0cdd89803 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -6,12 +6,14 @@ require 'open_food_network/order_cycle_management_report' module OpenFoodNetwork describe OrderCycleManagementReport do context "as a site admin" do + subject { OrderCycleManagementReport.new(user, params, true) } + let(:params) { {} } + let(:user) do user = create(:user) user.spree_roles << Spree::Role.find_or_create_by!(name: "admin") user end - subject { OrderCycleManagementReport.new user, {}, true } describe "fetching orders" do it "fetches completed orders" do @@ -130,6 +132,68 @@ module OpenFoodNetwork expect(subject.filter(orders)).to eq([order1]) end end + + describe '#table_items' do + subject { OrderCycleManagementReport.new(user, params, true) } + + let(:distributor) { create(:distributor_enterprise) } + before { distributor.enterprise_roles.create!(user: user) } + + context 'when the report type is payment_methods' do + let(:params) { { report_type: 'payment_methods' } } + + let!(:order) { create(:order, distributor: distributor, completed_at: 1.day.ago) } + + it 'returns rows with payment information' do + expect(subject.table_items).to eq([[ + order.billing_address.firstname, + order.billing_address.lastname, + order.distributor.name, + '', + order.email, + order.billing_address.phone, + nil, + nil, + nil, + -0.0 + ]]) + end + end + + context 'when the report type is not payment_methods' do + let(:params) { {} } + + let(:shipping_method) { create(:shipping_method) } + let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } + + let!(:order) do + create(:order, distributor: distributor, completed_at: 1.day.ago, shipments: [shipment]) + end + + before do + order.ship_address = order.address_from_distributor + order.save! + end + + it 'returns rows with delivery information' do + expect(subject.table_items).to eq([[ + order.ship_address.firstname, + order.ship_address.lastname, + order.distributor.name, + '', + "#{order.ship_address.address1} #{order.ship_address.address2} #{order.ship_address.city}", + order.ship_address.zipcode, + order.ship_address.phone, + shipping_method.name, + nil, + nil, + -0.0, + false, + order.special_instructions + ]]) + end + end + end end end end From a66d1b7299371f6b2b0fc9650f55ce77770812a2 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Feb 2021 18:37:39 +0100 Subject: [PATCH 3/6] Compute order balance chaining with query object --- .../order_cycle_management_report.rb | 18 ++++++++++--- .../order_cycle_management_report_spec.rb | 26 +++++++++++++++---- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index 2024aac6f0..ab273497c2 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -46,11 +46,21 @@ module OpenFoodNetwork end def search - Spree::Order.complete.where("spree_orders.state != ?", :canceled).distributed_by_user(@user).managed_by(@user).search(params[:q]) + Spree::Order. + complete. + where("spree_orders.state != ?", :canceled). + distributed_by_user(@user). + managed_by(@user). + search(params[:q]) end def orders - filter(search.result.order(:id)) + search_result = search.result.order(:id) + orders_with_balance = OutstandingBalance.new(search_result). + query. + select('spree_orders.*') + + filter(orders_with_balance) end def table_items @@ -80,7 +90,7 @@ module OpenFoodNetwork order.shipping_method.andand.name, order.payments.first.andand.payment_method.andand.name, order.payments.first.andand.amount, - OpenFoodNetwork::UserBalanceCalculator.new(order.email, order.distributor).balance] + order.balance_value] end def delivery_row(order) @@ -95,7 +105,7 @@ module OpenFoodNetwork order.shipping_method.andand.name, order.payments.first.andand.payment_method.andand.name, order.payments.first.andand.amount, - OpenFoodNetwork::UserBalanceCalculator.new(order.email, order.distributor).balance, + order.balance_value, has_temperature_controlled_items?(order), order.special_instructions] end diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index f0cdd89803..2158ad2f02 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -142,7 +142,15 @@ module OpenFoodNetwork context 'when the report type is payment_methods' do let(:params) { { report_type: 'payment_methods' } } - let!(:order) { create(:order, distributor: distributor, completed_at: 1.day.ago) } + let!(:order) do + create( + :order, + distributor: distributor, + completed_at: 1.day.ago, + state: 'complete', + total: 10.0 + ) + end it 'returns rows with payment information' do expect(subject.table_items).to eq([[ @@ -155,7 +163,7 @@ module OpenFoodNetwork nil, nil, nil, - -0.0 + -10.0 ]]) end end @@ -167,10 +175,18 @@ module OpenFoodNetwork let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } let!(:order) do - create(:order, distributor: distributor, completed_at: 1.day.ago, shipments: [shipment]) + create( + :order, + distributor: distributor, + completed_at: 1.day.ago, + shipments: [shipment] + ) end before do + line_item = create(:line_item, order: order, price: 10.0, quantity: 1) + + order.state = 'complete' order.ship_address = order.address_from_distributor order.save! end @@ -180,14 +196,14 @@ module OpenFoodNetwork order.ship_address.firstname, order.ship_address.lastname, order.distributor.name, - '', + nil, "#{order.ship_address.address1} #{order.ship_address.address2} #{order.ship_address.city}", order.ship_address.zipcode, order.ship_address.phone, shipping_method.name, nil, nil, - -0.0, + -10.0, false, order.special_instructions ]]) From e4319b06e6db2aa128b37b5b77381758484b9cf5 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Feb 2021 19:32:48 +0100 Subject: [PATCH 4/6] Take into account other finalized order states This makes it consistent with other places where we show order balances. Here though, we purposefully skip canceled ones. --- .../order_cycle_management_report.rb | 4 +-- .../order_cycle_management_report_spec.rb | 26 ++++++++++++++----- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index ab273497c2..8cc5eed49b 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -47,8 +47,8 @@ module OpenFoodNetwork def search Spree::Order. - complete. - where("spree_orders.state != ?", :canceled). + finalized. + where.not(spree_orders: { state: :canceled }). distributed_by_user(@user). managed_by(@user). search(params[:q]) diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index 2158ad2f02..4dc7a5c521 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -18,13 +18,25 @@ module OpenFoodNetwork describe "fetching orders" do it "fetches completed orders" do o1 = create(:order) - o2 = create(:order, completed_at: 1.day.ago) + o2 = create(:order, completed_at: 1.day.ago, state: 'complete') expect(subject.orders).to eq([o2]) end + it 'fetches resumed orders' do + order = create(:order, state: 'resumed', completed_at: 1.day.ago) + expect(subject.orders).to eq([order]) + end + + it 'calls OutstandingBalance query object' do + outstanding_balance = instance_double(OutstandingBalance, query: Spree::Order.none) + expect(OutstandingBalance).to receive(:new).and_return(outstanding_balance) + + subject.orders + end + it "does not show cancelled orders" do - o1 = create(:order, state: "canceled", completed_at: 1.day.ago) - o2 = create(:order, completed_at: 1.day.ago) + o1 = create(:order, state: 'canceled', completed_at: 1.day.ago) + o2 = create(:order, state: 'complete', completed_at: 1.day.ago) expect(subject.orders).to eq([o2]) end @@ -38,8 +50,8 @@ module OpenFoodNetwork context "default date range" do it "fetches orders completed in the past month" do - o1 = create(:order, completed_at: 1.month.ago - 1.day) - o2 = create(:order, completed_at: 1.month.ago + 1.day) + o1 = create(:order, state: 'complete', completed_at: 1.month.ago - 1.day) + o2 = create(:order, state: 'complete', completed_at: 1.month.ago + 1.day) expect(subject.orders).to eq([o2]) end end @@ -62,8 +74,8 @@ module OpenFoodNetwork d2 = create(:distributor_enterprise) d2.enterprise_roles.create!(user: create(:user)) - o1 = create(:order, distributor: d1, completed_at: 1.day.ago) - o2 = create(:order, distributor: d2, completed_at: 1.day.ago) + o1 = create(:order, distributor: d1, state: 'complete', completed_at: 1.day.ago) + o2 = create(:order, distributor: d2, state: 'complete', completed_at: 1.day.ago) expect(subject).to receive(:filter).with([o1]).and_return([o1]) expect(subject.orders).to eq([o1]) From cbfea1ba9719db83c44d0a2b285fa8610fe14bec Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Feb 2021 19:39:25 +0100 Subject: [PATCH 5/6] Hide new report's balance under toggle --- .../order_cycle_management_report.rb | 47 ++++++++++++++----- .../order_cycle_management_report_spec.rb | 33 ++++++++----- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index 8cc5eed49b..52048b9ee1 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -46,21 +46,34 @@ module OpenFoodNetwork end def search - Spree::Order. - finalized. - where.not(spree_orders: { state: :canceled }). - distributed_by_user(@user). - managed_by(@user). - search(params[:q]) + if FeatureToggle.enabled?(:customer_balance, @user) + Spree::Order. + finalized. + where.not(spree_orders: { state: :canceled }). + distributed_by_user(@user). + managed_by(@user). + search(params[:q]) + else + Spree::Order. + complete. + where("spree_orders.state != ?", :canceled). + distributed_by_user(@user). + managed_by(@user). + search(params[:q]) + end end def orders - search_result = search.result.order(:id) - orders_with_balance = OutstandingBalance.new(search_result). - query. - select('spree_orders.*') + if FeatureToggle.enabled?(:customer_balance, @user) + search_result = search.result.order(:id) + orders_with_balance = OutstandingBalance.new(search_result). + query. + select('spree_orders.*') - filter(orders_with_balance) + filter(orders_with_balance) + else + filter search.result + end end def table_items @@ -79,6 +92,14 @@ module OpenFoodNetwork private + def balance(order) + if FeatureToggle.enabled?(:customer_balance, @user) + order.balance_value + else + UserBalanceCalculator.new(order.email, order.distributor).balance + end + end + def payment_method_row(order) ba = order.billing_address [ba.andand.firstname, @@ -90,7 +111,7 @@ module OpenFoodNetwork order.shipping_method.andand.name, order.payments.first.andand.payment_method.andand.name, order.payments.first.andand.amount, - order.balance_value] + balance(order)] end def delivery_row(order) @@ -105,7 +126,7 @@ module OpenFoodNetwork order.shipping_method.andand.name, order.payments.first.andand.payment_method.andand.name, order.payments.first.andand.amount, - order.balance_value, + balance(order), has_temperature_controlled_items?(order), order.special_instructions] end diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index 4dc7a5c521..46c17ac640 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -27,11 +27,28 @@ module OpenFoodNetwork expect(subject.orders).to eq([order]) end - it 'calls OutstandingBalance query object' do - outstanding_balance = instance_double(OutstandingBalance, query: Spree::Order.none) - expect(OutstandingBalance).to receive(:new).and_return(outstanding_balance) + context 'when the customer_balance feature is enabled' do + let(:customers_with_balance) { instance_double(CustomersWithBalance) } - subject.orders + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, anything) { true } + end + + it 'calls OutstandingBalance query object' do + outstanding_balance = instance_double(OutstandingBalance, query: Spree::Order.none) + expect(OutstandingBalance).to receive(:new).and_return(outstanding_balance) + + subject.orders + end + + it 'orders them by id' do + result = instance_double(ActiveRecord::Relation) + allow_any_instance_of(Ransack::Search).to receive(:result).and_return(result) + expect(result).to receive(:order).with(:id) { Spree::Order.none } + + subject.orders + end end it "does not show cancelled orders" do @@ -40,14 +57,6 @@ module OpenFoodNetwork expect(subject.orders).to eq([o2]) end - it 'orders them by id' do - result = instance_double(ActiveRecord::Relation) - allow_any_instance_of(Ransack::Search).to receive(:result).and_return(result) - expect(result).to receive(:order).with(:id) { Spree::Order.none } - - subject.orders - end - context "default date range" do it "fetches orders completed in the past month" do o1 = create(:order, state: 'complete', completed_at: 1.month.ago - 1.day) From 36ce39a217124f2ea3536641c82dc072f4b5ab45 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 16 Feb 2021 13:25:21 +0100 Subject: [PATCH 6/6] Sort OC report orders by completed_at This is then consistent with the ordering we use to list orders in /admin, which is more useful. As a result, the test is also more robust. --- lib/open_food_network/order_cycle_management_report.rb | 2 +- .../order_cycle_management_report_spec.rb | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index 52048b9ee1..1858469c6e 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -65,7 +65,7 @@ module OpenFoodNetwork def orders if FeatureToggle.enabled?(:customer_balance, @user) - search_result = search.result.order(:id) + search_result = search.result.order(:completed_at) orders_with_balance = OutstandingBalance.new(search_result). query. select('spree_orders.*') diff --git a/spec/lib/open_food_network/order_cycle_management_report_spec.rb b/spec/lib/open_food_network/order_cycle_management_report_spec.rb index 46c17ac640..7e31c66e3c 100644 --- a/spec/lib/open_food_network/order_cycle_management_report_spec.rb +++ b/spec/lib/open_food_network/order_cycle_management_report_spec.rb @@ -43,11 +43,10 @@ module OpenFoodNetwork end it 'orders them by id' do - result = instance_double(ActiveRecord::Relation) - allow_any_instance_of(Ransack::Search).to receive(:result).and_return(result) - expect(result).to receive(:order).with(:id) { Spree::Order.none } + order1 = create(:order, completed_at: 1.day.ago, state: 'complete') + order2 = create(:order, completed_at: 2.days.ago, state: 'complete') - subject.orders + expect(subject.orders.pluck(:id)).to eq([order2.id, order1.id]) end end