From 0cff734b86866d29d0b8d1ad3c3b01ccf7742c2a Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Jun 2024 11:32:33 +1000 Subject: [PATCH 1/6] Refactor spec Linter said module was too big. I agreed, so made it smaller. Best viewed with whitespace ignored. --- .../orders_and_distributors_report_spec.rb | 174 +++++++++--------- 1 file changed, 83 insertions(+), 91 deletions(-) diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index 37fd4de00e..5fce5a30fb 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -2,108 +2,100 @@ require 'spec_helper' -module Reporting - module Reports - module OrdersAndDistributors - RSpec.describe Base do - describe 'orders and distributors report' do - it 'should return a header row describing the report' do - subject = Base.new nil +RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do + describe 'orders and distributors report' do + subject { described_class.new nil } - expect(subject.table_headers).to eq( - [ - 'Order date', 'Order Id', - 'Customer Name', 'Customer Email', 'Customer Phone', 'Customer City', - 'SKU', 'Item name', 'Variant', 'Quantity', 'Max Quantity', 'Cost', 'Shipping Cost', - 'Payment Method', - 'Distributor', 'Distributor address', 'Distributor city', 'Distributor postcode', - 'Shipping Method', 'Shipping instructions' - ] - ) - end + it 'should return a header row describing the report' do + expect(subject.table_headers).to eq( + [ + 'Order date', 'Order Id', + 'Customer Name', 'Customer Email', 'Customer Phone', 'Customer City', + 'SKU', 'Item name', 'Variant', 'Quantity', 'Max Quantity', 'Cost', 'Shipping Cost', + 'Payment Method', + 'Distributor', 'Distributor address', 'Distributor city', 'Distributor postcode', + 'Shipping Method', 'Shipping instructions' + ] + ) + end - context 'with completed order' do - let(:bill_address) { create(:address) } - let(:distributor) { create(:distributor_enterprise) } - let(:distributor1) { create(:distributor_enterprise) } - let(:product) { create(:product) } - let(:shipping_method) { create(:shipping_method) } - let(:shipping_instructions) { 'pick up on thursday please!' } - let(:order) { - create(:order, - state: 'complete', completed_at: Time.zone.now, - distributor:, bill_address:, - special_instructions: shipping_instructions) - } - let(:payment_method) { create(:payment_method, distributors: [distributor]) } - let(:payment) { create(:payment, payment_method:, order:) } - let(:line_item) { create(:line_item_with_shipment, product:, order:) } + context 'with completed order' do + let(:bill_address) { create(:address) } + let(:distributor) { create(:distributor_enterprise) } + let(:distributor1) { create(:distributor_enterprise) } + let(:product) { create(:product) } + let(:shipping_method) { create(:shipping_method) } + let(:shipping_instructions) { 'pick up on thursday please!' } + let(:order) { + create(:order, + state: 'complete', completed_at: Time.zone.now, + distributor:, bill_address:, + special_instructions: shipping_instructions) + } + let(:payment_method) { create(:payment_method, distributors: [distributor]) } + let(:payment) { create(:payment, payment_method:, order:) } + let(:line_item) { create(:line_item_with_shipment, product:, order:) } + subject { described_class.new create(:admin_user) } - before do - order.select_shipping_method(shipping_method.id) - order.payments << payment - order.line_items << line_item - end + before do + order.select_shipping_method(shipping_method.id) + order.payments << payment + order.line_items << line_item + end - it 'should denormalise order and distributor details for display as csv' do - subject = Base.new create(:admin_user), {} - allow(subject).to receive(:unformatted_render?).and_return(true) - table = subject.table_rows + it 'should denormalise order and distributor details for display as csv' do + allow(subject).to receive(:unformatted_render?).and_return(true) + table = subject.table_rows - expect(table.size).to eq 1 - expect(table[0]).to eq([ - order.reload.completed_at.strftime("%F %T"), - order.id, - bill_address.full_name, - order.email, - bill_address.phone, - bill_address.city, - line_item.product.sku, - line_item.product.name, - line_item.options_text, - line_item.quantity, - line_item.max_quantity, - line_item.price * line_item.quantity, - line_item.distribution_fee, - payment_method.name, - distributor.name, - distributor.address.address1, - distributor.address.city, - distributor.address.zipcode, - shipping_method.name, - shipping_instructions - ]) - end + expect(table.size).to eq 1 + expect(table[0]).to eq([ + order.reload.completed_at.strftime("%F %T"), + order.id, + bill_address.full_name, + order.email, + bill_address.phone, + bill_address.city, + line_item.product.sku, + line_item.product.name, + line_item.options_text, + line_item.quantity, + line_item.max_quantity, + line_item.price * line_item.quantity, + line_item.distribution_fee, + payment_method.name, + distributor.name, + distributor.address.address1, + distributor.address.city, + distributor.address.zipcode, + shipping_method.name, + shipping_instructions + ]) + end - it "prints one row per line item" do - create(:line_item_with_shipment, order:) + it "prints one row per line item" do + create(:line_item_with_shipment, order:) - subject = Base.new(create(:admin_user)) + table = subject.table_rows + expect(table.size).to eq 2 + end - table = subject.table_rows - expect(table.size).to eq 2 - end + context "filtering by distributor" do + it do + create(:line_item_with_shipment, order:) - context "filtering by distributor" do - it do - create(:line_item_with_shipment, order:) + report1 = described_class.new(create(:admin_user), {}) + table = report1.table_rows + expect(table.size).to eq 2 - report1 = Base.new(create(:admin_user), {}) - table = report1.table_rows - expect(table.size).to eq 2 + report2 = described_class.new(create(:admin_user), + { q: { distributor_id_in: [distributor.id] } }) + table2 = report2.table_rows + expect(table2.size).to eq 2 - report2 = Base.new(create(:admin_user), - { q: { distributor_id_in: [distributor.id] } }) - table2 = report2.table_rows - expect(table2.size).to eq 2 - - report3 = Base.new(create(:admin_user), - { q: { distributor_id_in: [distributor1.id] } }) - table3 = report3.table_rows - expect(table3.size).to eq 0 - end - end - end + report3 = described_class.new(create(:admin_user), + { q: { distributor_id_in: [distributor1.id] } }) + table3 = report3.table_rows + expect(table3.size).to eq 0 end end end From 5b8f590520a40afddeddabc31753237bf5c7eea8 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Jun 2024 11:23:32 +1000 Subject: [PATCH 2/6] Add spec for bug You wouldn't believe how long it took me to figure out all the bits and pieces. But now you don't have to! --- .../orders_and_distributors_report_spec.rb | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index 5fce5a30fb..ff23b9e63b 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -19,11 +19,13 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do ) end - context 'with completed order' do + context 'as a supplier, with completed order' do let(:bill_address) { create(:address) } let(:distributor) { create(:distributor_enterprise) } let(:distributor1) { create(:distributor_enterprise) } - let(:product) { create(:product) } + let(:supplier) { create(:supplier_enterprise) } + let(:user) { create(:admin_user) } + let(:product) { create(:product, supplier:) } let(:shipping_method) { create(:shipping_method) } let(:shipping_instructions) { 'pick up on thursday please!' } let(:order) { @@ -35,7 +37,7 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do let(:payment_method) { create(:payment_method, distributors: [distributor]) } let(:payment) { create(:payment, payment_method:, order:) } let(:line_item) { create(:line_item_with_shipment, product:, order:) } - subject { described_class.new create(:admin_user) } + subject { described_class.new user } before do order.select_shipping_method(shipping_method.id) @@ -98,6 +100,40 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do expect(table3.size).to eq 0 end end + + context "as a supplier, who has granted P-OC to the distributor" do + let(:user) { create(:enterprise_user, enterprises: [supplier]) } + let(:bill_address) { + create(:address, first_name: "FirstName", last_name: "LastName", phone: "123-456", + city: "City") + } + let(:row) { subject.table_rows.first } + + before do + create(:enterprise_relationship, parent: supplier, child: distributor, + permissions_list: [:add_to_order_cycle]) + end + + it "shows line items supplied by my producers, with contact details hidden" do + pending '#12559' + expect(row).not_to include("FirstName LastName") + expect(row).not_to include("123-456", "City", order.email) + expect(row[2..5]).to eq ["HIDDEN", "HIDDEN", "", ""] + end + + context "where the distributor allows suppliers to see customer names" do + before do + distributor.update_columns show_customer_names_to_suppliers: true + end + + it "shows line items supplied by my producers, with only contact names shown" do + expect(row).to include("FirstName LastName") + pending '#12559' + expect(row).not_to include("123-456", "City", order.email) + expect(row[2..5]).to eq [bill_address.full_name, "HIDDEN", "", ""] + end + end + end end end end From 141a883e4dc858aa95449c7d129cfe743169a52f Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Jun 2024 14:48:16 +1000 Subject: [PATCH 3/6] Refactor report So it turns out that all these features are built into the report framework. LineItems includes complete_not_canceled_visible_orders. It even takes care of masking non-editable orders. --- .../reports/orders_and_distributors/base.rb | 17 ++++++----------- .../orders_and_distributors_report_spec.rb | 2 -- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/lib/reporting/reports/orders_and_distributors/base.rb b/lib/reporting/reports/orders_and_distributors/base.rb index 2f54235d5e..a9cf0a2128 100644 --- a/lib/reporting/reports/orders_and_distributors/base.rb +++ b/lib/reporting/reports/orders_and_distributors/base.rb @@ -32,20 +32,11 @@ module Reporting # rubocop:enable Metrics/AbcSize def search - permissions.visible_orders.select("DISTINCT spree_orders.*"). - complete.not_state(:canceled). - ransack(ransack_params) + report_line_items.list end def query_result - orders = search.result - # Mask non editable order details - editable_orders_ids = permissions.editable_orders.select(&:id).map(&:id) - orders - .filter { |order| order.in?(editable_orders_ids) } - .each { |order| Orders::MaskDataService.new(order).call } - # Get Line Items - orders.map(&:line_items).flatten + search end private @@ -53,6 +44,10 @@ module Reporting def permissions @permissions ||= ::Permissions::Order.new(user, ransack_params) end + + def report_line_items + @report_line_items ||= Reporting::LineItems.new(permissions, params) + end end end end diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index ff23b9e63b..58374912f9 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -115,7 +115,6 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do end it "shows line items supplied by my producers, with contact details hidden" do - pending '#12559' expect(row).not_to include("FirstName LastName") expect(row).not_to include("123-456", "City", order.email) expect(row[2..5]).to eq ["HIDDEN", "HIDDEN", "", ""] @@ -128,7 +127,6 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do it "shows line items supplied by my producers, with only contact names shown" do expect(row).to include("FirstName LastName") - pending '#12559' expect(row).not_to include("123-456", "City", order.email) expect(row[2..5]).to eq [bill_address.full_name, "HIDDEN", "", ""] end From 54d068ee089cb1368430c37acf59acb143a024ea Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Jun 2024 16:46:11 +1000 Subject: [PATCH 4/6] Add spec for db queries --- .../orders_and_distributors_report_spec.rb | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index 58374912f9..76b86be318 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -132,6 +132,28 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do end end end + + it "minimises database queries" do + subject # build context first + + # surely we can do better than that for each row + expect { subject.table_rows }.to query_database [ + "Spree::Role Exists?", + "Spree::Role Exists?", + "SQL", + "Spree::LineItem Load", + "Spree::Order Load", + "Spree::Address Load", + "Spree::Payment Load", + "Spree::PaymentMethod Load", + "Spree::Calculator Load", + "Enterprise Load", + "Spree::Address Load", + "Spree::Shipment Load", + "Spree::ShippingRate Load", + "Spree::ShippingMethod Load", + ] + end end end end From d80e1efa7bc6735b9f11544f85fb8b25ccf07cdb Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 20 Jun 2024 15:37:30 +1000 Subject: [PATCH 5/6] Add includes for more efficient querying --- lib/reporting/reports/orders_and_distributors/base.rb | 7 ++++++- spec/lib/reports/orders_and_distributors_report_spec.rb | 6 ------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/reporting/reports/orders_and_distributors/base.rb b/lib/reporting/reports/orders_and_distributors/base.rb index a9cf0a2128..49d3d5bca2 100644 --- a/lib/reporting/reports/orders_and_distributors/base.rb +++ b/lib/reporting/reports/orders_and_distributors/base.rb @@ -32,7 +32,7 @@ module Reporting # rubocop:enable Metrics/AbcSize def search - report_line_items.list + report_line_items.list(line_item_includes) end def query_result @@ -41,6 +41,11 @@ module Reporting private + def line_item_includes + [{ variant: { product: :supplier }, + order: [:bill_address, :payments, { distributor: :address}] }] + end + def permissions @permissions ||= ::Permissions::Order.new(user, ransack_params) end diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index 76b86be318..0f3d046638 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -136,19 +136,13 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do it "minimises database queries" do subject # build context first - # surely we can do better than that for each row expect { subject.table_rows }.to query_database [ "Spree::Role Exists?", "Spree::Role Exists?", "SQL", "Spree::LineItem Load", - "Spree::Order Load", - "Spree::Address Load", - "Spree::Payment Load", "Spree::PaymentMethod Load", "Spree::Calculator Load", - "Enterprise Load", - "Spree::Address Load", "Spree::Shipment Load", "Spree::ShippingRate Load", "Spree::ShippingMethod Load", From 67cac1f4d6bd38dc5496ceb9eb0e74c0396d1118 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 24 Jun 2024 14:04:35 +1000 Subject: [PATCH 6/6] Fix `search` to return expected data As explained in Reporting::ReportTemplate::ReportsHelper, 'search' and `query_result` are not supposed to return the same things --- lib/reporting/reports/orders_and_distributors/base.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/reporting/reports/orders_and_distributors/base.rb b/lib/reporting/reports/orders_and_distributors/base.rb index 49d3d5bca2..8de02541fe 100644 --- a/lib/reporting/reports/orders_and_distributors/base.rb +++ b/lib/reporting/reports/orders_and_distributors/base.rb @@ -32,18 +32,18 @@ module Reporting # rubocop:enable Metrics/AbcSize def search - report_line_items.list(line_item_includes) + report_line_items.orders end def query_result - search + report_line_items.list(line_item_includes) end private def line_item_includes [{ variant: { product: :supplier }, - order: [:bill_address, :payments, { distributor: :address}] }] + order: [:bill_address, :payments, { distributor: :address }] }] end def permissions