Merge pull request #11148 from cillian/fix-duplicate-rows-in-customer-addresses-report

Don't return duplicate rows in the customer addresses report

lib/reporting/report_rows_builder.rb

@@ -38,10 +38,16 @@ module Reporting
     private
 
     def computed_data
-      @computed_data ||= report.query_result.map { |item|
+      return @computed_data if defined? @computed_data
+
+      @computed_data = report.query_result.map { |item|
         row = @builder.build_row(item)
         OpenStruct.new(item: item, full_row: row, row: @builder.slice_and_format_row(row))
       }
+
+      @computed_data.uniq! { |data| data.row.to_h.values } if @report.skip_duplicate_rows?
+
+      @computed_data
     end
 
     def extract_rows(data, result)

lib/reporting/report_template.rb

@@ -99,6 +99,10 @@ module Reporting
       {}
     end
 
+    def skip_duplicate_rows?
+      false
+    end
+
     private
 
     def renderer

lib/reporting/reports/customers/addresses.rb

@@ -30,6 +30,10 @@ module Reporting
             shipping_method: proc { |order| order.shipping_method&.name },
           }
         end
+
+        def skip_duplicate_rows?
+          true
+        end
       end
     end
   end

spec/lib/reports/customers_report_spec.rb

@@ -140,7 +140,7 @@ module Reporting
               end
 
               context "orders with different shipping methods" do
-                let!(:sm2) { create(:shipping_method, distributors: [d]) }
+                let!(:sm2) { create(:shipping_method, distributors: [d], name: "Bike") }
                 let!(:o2) {
                   create(:order_with_totals_and_distribution, :completed, distributor: d, bill_address: a, shipping_method: sm2)
                 }
@@ -148,22 +148,65 @@ module Reporting
                   o2.select_shipping_method(sm2.id)
                 end
 
-                it "returns one row per customer per shipping method" do
-                  expect(subject.query_result.size).to eq(2)
-                  expect(subject.table_rows.size).to eq(2)
-                  expect(subject.table_rows).to eq([[
-                                                     a.firstname, a.lastname,
-                                                     [a.address1, a.address2, a.city].join(" "),
-                                                     o1.email, a.phone, d.name,
-                                                     [d.address.address1, d.address.address2, d.address.city].join(" "),
-                                                     o1.shipping_method.name
-                                                   ], [
-                                                     a.firstname, a.lastname,
-                                                     [a.address1, a.address2, a.city].join(" "),
-                                                     o2.email, a.phone, d.name,
-                                                     [d.address.address1, d.address.address2, d.address.city].join(" "),
-                                                     sm2.name
-                                                   ]])
+                context "when the shipping method column is being included" do
+                  let(:fields_to_show) do
+                    [:first_name, :last_name, :billing_address, :email, :phone, :hub, :hub_address,
+                     :shipping_method]
+                  end
+                  subject { Addresses.new(user, { fields_to_show: fields_to_show }) }
+
+                  it "returns one row per customer per shipping method" do
+                    expect(subject.query_result.size).to eq(2)
+                    expect(subject.table_rows.size).to eq(2)
+                    expect(subject.table_rows).to eq(
+                      [
+                        [
+                          a.firstname,
+                          a.lastname,
+                          [a.address1, a.address2, a.city].join(" "),
+                          o1.email,
+                          a.phone,
+                          d.name,
+                          [d.address.address1, d.address.address2, d.address.city].join(" "),
+                          o1.shipping_method.name
+                        ],
+                        [
+                          a.firstname,
+                          a.lastname,
+                          [a.address1, a.address2, a.city].join(" "),
+                          o2.email,
+                          a.phone,
+                          d.name,
+                          [d.address.address1, d.address.address2, d.address.city].join(" "),
+                          sm2.name
+                        ]
+                      ]
+                    )
+                  end
+                end
+
+                context "when the shipping method column is not included in the report" do
+                  let(:fields_to_show) do
+                    [:first_name, :last_name, :billing_address, :email, :phone, :hub, :hub_address]
+                  end
+                  subject { Addresses.new(user, { fields_to_show: fields_to_show }) }
+
+                  it "returns a single row for the customer, otherwise it would return two identical
+                      rows" do
+                    expect(subject.query_result.size).to eq(2)
+                    expect(subject.table_rows.size).to eq(1)
+                    expect(subject.table_rows).to eq(
+                      [[
+                        a.firstname,
+                        a.lastname,
+                        [a.address1, a.address2, a.city].join(" "),
+                        o1.email,
+                        a.phone,
+                        d.name,
+                        [d.address.address1, d.address.address2, d.address.city].join(" ")
+                      ]]
+                    )
+                  end
                 end
               end
             end

spec/lib/reports/report_spec.rb

@@ -131,6 +131,40 @@ module Reporting
         ]
         check_report
       end
+
+      context "when report contains duplicate rows" do
+        before do
+          @columns = {
+            customer: proc { |item| item.customer },
+            address: proc { |item| item.address }
+          }
+          @query_result = [
+            OpenStruct.new(customer: "John", address: "1 Main Street"),
+            OpenStruct.new(customer: "John", address: "1 Main Street")
+          ]
+        end
+
+        context "and the report type allows duplicate rows i.e. the default behaviour" do
+          it "outputs duplicate rows" do
+            @expected_table_rows = [
+              ["John", "1 Main Street"],
+              ["John", "1 Main Street"]
+            ]
+            check_report
+          end
+        end
+
+        context "and the report type does not allow duplicate rows" do
+          before { allow(subject).to receive(:skip_duplicate_rows?).and_return(true) }
+
+          it "outputs only unique rows" do
+            @expected_table_rows = [
+              ["John", "1 Main Street"]
+            ]
+            check_report
+          end
+        end
+      end
     end
 
     describe ".rules" do