From cdd09e8f975ead6c0c3d36434b32904301b299cc Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 21 Aug 2020 15:08:58 +0100 Subject: [PATCH 1/3] Fix the bulk coop report date and distributor filters. Before the date and distributor filters would have no effect. This is because the BulkCoopReport is still generated using an older style method, and isn't generated using the newer method like in the EnterpriseFeeSummaryReport. This older style report expects to receive a :q parameter but it actually received the newer style :report parameter so the filters were not being applied. This keeps the newer style report params but converts them, after they are authorised as safe, into the older style in the controller. --- .../reports/bulk_coop_controller.rb | 14 +++++- .../bulk_coop/bulk_coop_report_spec.rb | 46 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb index 5a308fbdab..7bd86e83a2 100644 --- a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb +++ b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb @@ -13,7 +13,8 @@ module OrderManagement @report_parameters.authorize!(@permissions) - @report = report_klass::ReportService.new(@permissions, params[:report], spree_current_user) + @report = report_klass::ReportService.new(@permissions, legacy_format_report_params, + spree_current_user) renderer.render(self) rescue ::Reports::Authorizer::ParameterNotAllowedError => e flash[:error] = e.message @@ -39,6 +40,17 @@ module OrderManagement OrderManagement::Reports::BulkCoop end + def legacy_format_report_params + { + q: { + completed_at_gt: params[:report][:start_at], + completed_at_lt: params[:report][:end_at], + distributor_id_in: params[:report][:distributor_ids], + report_type: params[:report][:report_type] + } + } + end + def load_report_parameters @report_parameters = report_klass::Parameters.new(params[:report] || {}) end diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index 8f45355ee6..3c609cf952 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -28,6 +28,52 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do end end + context "filtering by date" do + it do + user = create(:admin_user) + o2 = create(:order, completed_at: 3.days.ago, order_cycle: oc1, distributor: d1) + li2 = build(:line_item_with_shipment) + o2.line_items << li2 + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true + expect(report.table_items).to eq([li1, li2]) + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( + user, { q: { completed_at_gt: 2.days.ago } }, true + ) + expect(report.table_items).to eq([li1]) + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( + user, { q: { completed_at_lt: 2.days.ago } }, true + ) + expect(report.table_items).to eq([li2]) + end + end + + context "filtering by distributor" do + it do + user = create(:admin_user) + d2 = create(:distributor_enterprise) + o2 = create(:order, distributor: d2, order_cycle: oc1, + completed_at: Time.zone.now) + li2 = build(:line_item_with_shipment) + o2.line_items << li2 + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true + expect(report.table_items).to eq([li1, li2]) + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( + user, { q: { distributor_id_in: [d1.id] } }, true + ) + expect(report.table_items).to eq([li1]) + + report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( + user, { q: { distributor_id_in: [d2.id] } }, true + ) + expect(report.table_items).to eq([li2]) + end + end + context "as a manager of a supplier" do let!(:user) { create(:user) } subject { OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true } From a4419796494170bfd1cdbd31ee705e24440679d3 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 28 Aug 2020 11:09:34 +0100 Subject: [PATCH 2/3] Pass in :report_type parameter correctly to bulk coop report so different report types are generated Also add tests for each of the different report types. I didn't make these JavaScript tests because not sure that is necessary and they would be slower. --- .../reports/bulk_coop_controller.rb | 4 +- .../reports/bulk_coop_spec.rb | 67 +++++++++++++++++-- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb index 7bd86e83a2..c1810a92b6 100644 --- a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb +++ b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb @@ -46,8 +46,8 @@ module OrderManagement completed_at_gt: params[:report][:start_at], completed_at_lt: params[:report][:end_at], distributor_id_in: params[:report][:distributor_ids], - report_type: params[:report][:report_type] - } + }, + report_type: params[:report][:report_type] } end diff --git a/engines/order_management/spec/features/order_management/reports/bulk_coop_spec.rb b/engines/order_management/spec/features/order_management/reports/bulk_coop_spec.rb index 2cbc464c75..62181aebaf 100644 --- a/engines/order_management/spec/features/order_management/reports/bulk_coop_spec.rb +++ b/engines/order_management/spec/features/order_management/reports/bulk_coop_spec.rb @@ -6,11 +6,70 @@ feature "bulk coop" do include AuthenticationHelper include WebHelper - scenario "bulk co-op report" do - login_as_admin_and_visit spree.admin_reports_path - click_link 'Bulk Co-Op' + scenario "generating Bulk Coop Supplier Report" do + login_as_admin_and_visit new_order_management_reports_bulk_coop_path + select "Bulk Coop Supplier Report", from: "report_report_type" click_button 'Generate Report' - expect(page).to have_content 'Supplier' + expect(page).to have_table_row [ + "Supplier", + "Product", + "Bulk Unit Size", + "Variant", + "Variant Value", + "Variant Unit", + "Weight", + "Sum Total", + "Units Required", + "Unallocated", + "Max Quantity Excess" + ] + end + + scenario "generating Bulk Co-op Allocation report" do + login_as_admin_and_visit new_order_management_reports_bulk_coop_path + select "Bulk Coop Allocation", from: "report_report_type" + click_button 'Generate Report' + + expect(page).to have_table_row [ + "Customer", + "Product", + "Bulk Unit Size", + "Variant", + "Variant Value", + "Variant Unit", + "Weight", + "Sum Total", + "Total available", + "Unallocated", + "Max Quantity Excess" + ] + end + + scenario "generating Bulk Co-op Packing Sheets report" do + login_as_admin_and_visit new_order_management_reports_bulk_coop_path + select "Bulk Coop Packing Sheets", from: "report_report_type" + click_button 'Generate Report' + + expect(page).to have_table_row [ + "Customer", + "Product", + "Variant", + "Sum Total" + ] + end + + scenario "generating Bulk Co-op Customer Payments report" do + login_as_admin_and_visit new_order_management_reports_bulk_coop_path + select "Bulk Coop Customer Payments", from: "report_report_type" + click_button 'Generate Report' + + expect(page).to have_table_row [ + "Customer", + "Date of Order", + "Total Cost", + "Amount Owing", + "Amount Paid" + ] end end From 9efee1b0be1bc67cd99471777206dcc3e1c2ffb9 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 28 Aug 2020 12:20:47 +0100 Subject: [PATCH 3/3] In bulk coop report service spec just check array has the same elements but don't check the order. The #table_items methos seems to return line items in different order sometimes making this test a bit flaky. The test passed on Semaphore previously and is passing in development. I don't think the order matters so using :match_array instead of :eq. --- .../reports/bulk_coop/bulk_coop_report_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index 3c609cf952..5a580481de 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -36,7 +36,7 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do o2.line_items << li2 report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true - expect(report.table_items).to eq([li1, li2]) + expect(report.table_items).to match_array [li1, li2] report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( user, { q: { completed_at_gt: 2.days.ago } }, true @@ -60,7 +60,7 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do o2.line_items << li2 report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true - expect(report.table_items).to eq([li1, li2]) + expect(report.table_items).to match_array [li1, li2] report = OrderManagement::Reports::BulkCoop::BulkCoopReport.new( user, { q: { distributor_id_in: [d1.id] } }, true