From 43b1eca0267f63c0dbcedb55a69a76d2520afae6 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 18 Oct 2023 15:49:53 +1100 Subject: [PATCH 1/3] Remove unneeded report spec Testing for the absence of behaviour is useful when changing code to assert that the change is successful but in the long term, it doesn't add any value. If you don't have any reason to believe that the report may delete parameters then we don't need to test for that. --- .../enterprise_fee_summary_report_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb b/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb index aaeb80717c..2ddcf23d6c 100644 --- a/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb +++ b/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb @@ -100,12 +100,6 @@ describe Reporting::Reports::EnterpriseFeeSummary::FeeSummary do let!(:second_customer_order) { prepare_order(customer:) } let!(:other_customer_order) { prepare_order(customer: another_customer) } - it "doesn't delete params" do - params = ActionController::Parameters.new("completed_at_gt" => "2023-02-08+00:00") - described_class.new(current_user, params) - expect(params["completed_at_gt"]).to eq "2023-02-08+00:00" - end - it "groups and sorts entries correctly" do totals = subject.query_result From 4fc66f6585c4ce39ad2bf3c9af697041ecfe0e55 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 20 Oct 2023 15:35:17 +1100 Subject: [PATCH 2/3] Spec fee report failing on removed variant Well, creating a unit test level spec in the first place. Edge cases like this don't need to be tested in slow system specs. --- ...e_fees_with_tax_report_by_producer_spec.rb | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb diff --git a/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb b/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb new file mode 100644 index 0000000000..72e903d68d --- /dev/null +++ b/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe Reporting::Reports::EnterpriseFeeSummary::EnterpriseFeesWithTaxReportByProducer do + let(:current_user) { create(:admin_user) } + + let(:enterprise) { + create(:distributor_enterprise_with_tax, is_primary_producer: true, + shipping_methods: [shipping_method]) + } + let(:shipping_method) { create(:shipping_method) } + let(:order_cycle) { + create(:simple_order_cycle, coordinator: enterprise).tap do |order_cycle| + incoming = order_cycle.exchanges.create!(incoming: true, sender: enterprise, + receiver: enterprise) + outgoing = order_cycle.exchanges.create!(incoming: false, sender: enterprise, + receiver: enterprise) + + supplier_fee = create(:enterprise_fee, :per_item, enterprise:, amount: 15, + name: 'Transport', + fee_type: 'transport',) + incoming.exchange_fees.create!(enterprise_fee: supplier_fee) + + incoming.exchange_variants.create(variant:) + outgoing.exchange_variants.create(variant:) + end + } + let(:variant) { create(:product, supplier: enterprise).variants.first } + let(:order) { + create( + :order, :with_line_item, + variant:, distributor: enterprise, order_cycle:, + shipping_method:, ship_address: create(:address) + ).tap do |order| + order.recreate_all_fees! + OrderWorkflow.new(order).complete! + end + } + + it "renders an empty report" do + report = described_class.new(current_user) + expect(report.query_result).to eq({}) + end + + it "lists orders" do + order + report = described_class.new(current_user) + expect(report.query_result.values).to eq [[order]] + end + + it "handles products removed from the order cycle" do + pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/11530" + + order + order_cycle.exchanges.incoming.first.exchange_variants.first.destroy! + + report = described_class.new(current_user) + + expect(report.query_result.values).to eq [[order]] + end +end From b83651a7ae870abb99ca07c77316b78834d209bd Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 20 Oct 2023 15:45:32 +1100 Subject: [PATCH 3/3] Avoid error when generating fee report Instead some data is missing from the report. A slightly better outcome but still wrong. This is tracked in another issue though. --- .../enterprise_fees_with_tax_report_by_producer.rb | 2 +- .../enterprise_fees_with_tax_report_by_producer_spec.rb | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer.rb b/lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer.rb index d7aa97d9b2..2760112f1b 100644 --- a/lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer.rb +++ b/lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer.rb @@ -138,7 +138,7 @@ module Reporting filtered_line_items(order) .filter do |line_item| item[:enterprise_fee_id].in?( - enterprise_fees_per_variant[line_item.variant] + enterprise_fees_per_variant.fetch(line_item.variant, []) ) end .map do |line_item| diff --git a/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb b/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb index 72e903d68d..4c25519041 100644 --- a/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb +++ b/spec/lib/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer_spec.rb @@ -50,13 +50,12 @@ describe Reporting::Reports::EnterpriseFeeSummary::EnterpriseFeesWithTaxReportBy end it "handles products removed from the order cycle" do - pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/11530" - order order_cycle.exchanges.incoming.first.exchange_variants.first.destroy! report = described_class.new(current_user) + pending "https://github.com/openfoodfoundation/openfoodnetwork/issues/11529" expect(report.query_result.values).to eq [[order]] end end