From 09247b21cd2897c41d5a585c3ca0f3e938218314 Mon Sep 17 00:00:00 2001 From: Sebastian Castro Date: Tue, 29 Mar 2022 17:48:54 +0200 Subject: [PATCH] Reports Refactor 1 Use code closer to the new packing report controller Handle nil @report_subtypes --- .rubocop_todo.yml | 10 ++-- .../spree/admin/reports_controller.rb | 58 ++++++++++--------- .../reports/_rendering_options.html.haml | 2 +- .../spree/admin/reports/_table.html.haml | 35 ++++++----- app/views/spree/admin/reports/show.html.haml | 17 ++++++ lib/reporting/report_renderer.rb | 4 +- .../admin/reports/packing_report_spec.rb | 10 ++-- spec/system/admin/reports_spec.rb | 20 +++---- 8 files changed, 87 insertions(+), 69 deletions(-) create mode 100644 app/views/spree/admin/reports/show.html.haml diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index ac90405d33..ac1cfe8ee6 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -560,7 +560,7 @@ Metrics/CyclomaticComplexity: - 'lib/open_food_network/customers_report.rb' - 'lib/open_food_network/enterprise_issue_validator.rb' - 'lib/open_food_network/group_buy_report.rb' - - 'lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb' + - 'lib/open_food_network/orders_and_fulfillment_report/customer_totals_report.rb' - 'lib/open_food_network/payments_report.rb' - 'lib/open_food_network/xero_invoices_report.rb' - 'lib/spree/core/controller_helpers/order.rb' @@ -716,7 +716,7 @@ Naming/VariableNumber: - 'app/controllers/spree/orders_controller.rb' - 'app/models/content_configuration.rb' - 'app/models/preference_sections/main_links_section.rb' - - 'lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb' + - 'lib/open_food_network/orders_and_fulfillment_report/customer_totals_report.rb' - 'lib/spree/core/controller_helpers/common.rb' - 'spec/controllers/spree/admin/search_controller_spec.rb' - 'spec/factories/stock_location_factory.rb' @@ -1092,7 +1092,7 @@ Style/MissingRespondToMissing: # Offense count: 1 Style/MixinUsage: Exclude: - - 'lib/open_food_network/orders_and_fulfillments_report.rb' + - 'lib/open_food_network/orders_and_fulfillment_report.rb' # Offense count: 2 # Cop supports --auto-correct. @@ -1137,7 +1137,7 @@ Style/OptionalBooleanParameter: - 'lib/open_food_network/customers_report.rb' - 'lib/open_food_network/order_and_distributor_report.rb' - 'lib/open_food_network/order_cycle_management_report.rb' - - 'lib/open_food_network/orders_and_fulfillments_report.rb' + - 'lib/open_food_network/orders_and_fulfillment_report.rb' - 'lib/open_food_network/payments_report.rb' - 'lib/open_food_network/products_and_inventory_report_base.rb' - 'lib/open_food_network/users_and_enterprises_report.rb' @@ -1236,7 +1236,7 @@ Style/StringConcatenation: - 'app/services/embedded_page_service.rb' - 'app/services/products_renderer.rb' - 'engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb' - - 'lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb' + - 'lib/open_food_network/orders_and_fulfillment_report/customer_totals_report.rb' - 'lib/spree/api/controller_setup.rb' - 'lib/spree/core/environment_extension.rb' - 'spec/lib/open_food_network/order_grouper_spec.rb' diff --git a/app/controllers/spree/admin/reports_controller.rb b/app/controllers/spree/admin/reports_controller.rb index de57236dbf..56e8a8e28d 100644 --- a/app/controllers/spree/admin/reports_controller.rb +++ b/app/controllers/spree/admin/reports_controller.rb @@ -14,12 +14,13 @@ require 'open_food_network/order_cycle_management_report' require 'open_food_network/sales_tax_report' require 'open_food_network/xero_invoices_report' require 'open_food_network/payments_report' -require 'open_food_network/orders_and_fulfillments_report' +require 'open_food_network/orders_and_fulfillment_report' module Spree module Admin class ReportsController < Spree::Admin::BaseController include Spree::ReportsHelper + include ReportsActions helper ::ReportsHelper ORDER_MANAGEMENT_ENGINE_REPORTS = [ @@ -32,7 +33,6 @@ module Spree before_action :cache_search_state # Fetches user's distributors, suppliers and order_cycles before_action :load_basic_data, only: [:customers, :products_and_inventory, :order_cycle_management] - before_action :load_associated_data, only: [:orders_and_fulfillment] respond_to :html @@ -101,21 +101,21 @@ module Spree end def orders_and_fulfillment - raw_params[:q] ||= orders_and_fulfillment_default_filters + now = Time.zone.now + raw_params[:q] ||= { + completed_at_gt: (now - 1.month).beginning_of_day, + completed_at_lt: (now + 1.day).beginning_of_day + } - @report_types = report_types[:orders_and_fulfillment] - @report_type = params[:report_type] + form_options = Reporting::FrontendData.new(spree_current_user) - @include_blank = I18n.t(:all) + @distributors = form_options.distributors + @suppliers = form_options.suppliers + @order_cycles = form_options.order_cycles - # -- Build Report with Order Grouper - @report = OpenFoodNetwork::OrdersAndFulfillmentsReport.new spree_current_user, - raw_params, - render_content? - @table = order_grouper_table - csv_file_name = "#{params[:report_type]}_#{timestamp}.csv" + @report_message = I18n.t("spree.admin.reports.customer_names_message.customer_names_tip") - render_report(@report.header, @table, params[:csv], csv_file_name) + render_report2 end def products_and_inventory @@ -190,19 +190,27 @@ module Spree @searching end + def render_report2 + @report_subtypes = report_types[action_name.to_sym] + @report_subtype = params[:report_subtype] + klass = "OpenFoodNetwork::#{action_name.camelize}Report".constantize + @report = klass.new spree_current_user, raw_params, render_content? + + if report_format.present? + data = Reporting::ReportRenderer.new(@report).public_send("to_#{report_format}") + send_data data, filename: report_filename + else + @header = @report.table_headers + @table = @report.table_rows + + render "show" + end + end + def render_report(header, table, create_csv, csv_file_name) send_data csv_report(header, table), filename: csv_file_name if create_csv @header = header @table = table - # Rendering HTML is the default. - end - - def load_associated_data - form_options = Reporting::FrontendData.new(spree_current_user) - - @distributors = form_options.distributors - @suppliers = form_options.suppliers - @order_cycles = form_options.order_cycles end def csv_report(header, table) @@ -299,12 +307,6 @@ module Spree def timestamp Time.zone.now.strftime("%Y%m%d") end - - def orders_and_fulfillment_default_filters - now = Time.zone.now - { completed_at_gt: (now - 1.month).beginning_of_day, - completed_at_lt: (now + 1.day).beginning_of_day } - end end end end diff --git a/app/views/admin/reports/_rendering_options.html.haml b/app/views/admin/reports/_rendering_options.html.haml index fa7b387bc0..b6b20fd0cf 100644 --- a/app/views/admin/reports/_rendering_options.html.haml +++ b/app/views/admin/reports/_rendering_options.html.haml @@ -1,4 +1,4 @@ -- if @report_subtypes.any? +- if @report_subtypes.present? && @report_subtypes.count > 1 .row .alpha.two.columns= label_tag nil, t(:report_type) .omega.fourteen.columns diff --git a/app/views/spree/admin/reports/_table.html.haml b/app/views/spree/admin/reports/_table.html.haml index e9347669b3..81d7dd524f 100644 --- a/app/views/spree/admin/reports/_table.html.haml +++ b/app/views/spree/admin/reports/_table.html.haml @@ -1,20 +1,19 @@ - column_partials ||= {} -- if render_content? - %table.report__table{id: id} - %thead +%table.report__table + %thead + %tr + - @header.each do |heading| + %th= heading + %tbody + - @table.each do |row| %tr - - @header.each do |heading| - %th= heading - %tbody - - @table.each do |row| - %tr - - row.each_with_index do |cell_value, column_index| - %td - - partial = column_partials[column_index] - - if partial - = render partial, value: cell_value - - else - = cell_value - - if @table.empty? - %tr - %td{colspan: @header.count}= t(:none) + - row.each_with_index do |cell_value, column_index| + %td + - partial = column_partials[column_index] + - if partial + = render partial, value: cell_value + - else + = cell_value + - if @table.empty? + %tr + %td{colspan: @header.count}= t(:none) diff --git a/app/views/spree/admin/reports/show.html.haml b/app/views/spree/admin/reports/show.html.haml new file mode 100644 index 0000000000..4e7a640944 --- /dev/null +++ b/app/views/spree/admin/reports/show.html.haml @@ -0,0 +1,17 @@ += form_for @report.search, :url => url_for(only_path: false) do |f| + %fieldset.no-border-bottom.print-hidden + %legend{ align: 'center'}= t(:report_filters) + = render partial: "spree/admin/reports/filters/#{action_name}", locals: { f: f } + + %fieldset.print-hidden + %legend{ align: 'center'}= t(:report_render_options) + = render partial: "admin/reports/rendering_options" + + .actions.filter-actions + = button t(:go), "report__submit-btn" + +- if @report_message.present? + %p.report__message.print-hidden= @report_message + +- if render_content? + = render "table" diff --git a/lib/reporting/report_renderer.rb b/lib/reporting/report_renderer.rb index 16b78d91de..a4cfcc1d6d 100644 --- a/lib/reporting/report_renderer.rb +++ b/lib/reporting/report_renderer.rb @@ -9,11 +9,11 @@ module Reporting end def table_headers - @report.report_data.columns + @report.respond_to?(:report_data) ? @report.report_data.columns : @report.table_headers end def table_rows - @report.report_data.rows + @report.respond_to?(:report_data) ? @report.report_data.rows : @report.table_rows end def as_json diff --git a/spec/system/admin/reports/packing_report_spec.rb b/spec/system/admin/reports/packing_report_spec.rb index dcd5660641..421144b223 100644 --- a/spec/system/admin/reports/packing_report_spec.rb +++ b/spec/system/admin/reports/packing_report_spec.rb @@ -48,20 +48,20 @@ describe "Packing Reports", js: true do fill_in 'q_completed_at_lt', with: '2013-04-25 16:00:00' click_button 'Search' - rows = find("table#listing_orders").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table).to eq([ ["Hub", "Code", "First Name", "Last Name", "Supplier", "Product", "Variant", "Quantity", "TempControlled?"].map(&:upcase) ]) - expect(page).to have_selector 'table#listing_orders tbody tr', count: 5 # Totals row per order + expect(page).to have_selector 'table.report__table tbody tr', count: 5 # Totals row per order end it "sorts alphabetically" do click_link "Pack By Customer" click_button 'Search' - rows = find("table#listing_orders").all("tr") + rows = find("table.report__table").all("tr") table = rows.map { |r| r.all("th,td").map { |c| c.text.strip }[3] } expect(table).to eq([ "LAST NAME", @@ -81,13 +81,13 @@ describe "Packing Reports", js: true do fill_in 'q_completed_at_lt', with: '2013-04-25 16:00:00' click_button 'Search' - rows = find("table#listing_orders").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table).to eq([ ["Hub", "Supplier", "Code", "First Name", "Last Name", "Product", "Variant", "Quantity", "TempControlled?"].map(&:upcase) ]) - expect(all('table#listing_orders tbody tr').count).to eq(4) # Totals row per supplier + expect(all('table.report__table tbody tr').count).to eq(4) # Totals row per supplier end end end diff --git a/spec/system/admin/reports_spec.rb b/spec/system/admin/reports_spec.rb index 7e0b7e00d6..3c8b6f3122 100644 --- a/spec/system/admin/reports_spec.rb +++ b/spec/system/admin/reports_spec.rb @@ -38,10 +38,10 @@ describe ' it "customers report" do click_link "Mailing List" - expect(page).to have_select('report_type', selected: 'Mailing List') + expect(page).to have_select('report_subtype', selected: 'Mailing List') click_button "Go" - rows = find("table#listing_customers").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table.sort).to eq([ ["Email", "First Name", "Last Name", "Suburb"].map(&:upcase) @@ -50,10 +50,10 @@ describe ' it "customers report" do click_link "Addresses" - expect(page).to have_select('report_type', selected: 'Addresses') + expect(page).to have_select('report_subtype', selected: 'Addresses') click_button "Go" - rows = find("table#listing_customers").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table.sort).to eq([ ["First Name", "Last Name", "Billing Address", "Email", "Phone", "Hub", "Hub Address", @@ -70,7 +70,7 @@ describe ' it "payment method report" do click_link "Payment Methods Report" click_button "Search" - rows = find("table#listing_ocm_orders").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table.sort).to eq([ ["First Name", "Last Name", "Hub", "Hub Code", "Email", "Phone", "Shipping Method", @@ -81,7 +81,7 @@ describe ' it "delivery report" do click_link "Delivery Report" click_button "Search" - rows = find("table#listing_ocm_orders").all("thead tr") + rows = find("table.report__table").all("thead tr") table = rows.map { |r| r.all("th").map { |c| c.text.strip } } expect(table.sort).to eq([ ["First Name", "Last Name", "Hub", "Hub Code", "Delivery Address", "Delivery Postcode", @@ -242,7 +242,7 @@ describe ' pick_datetime "#q_completed_at_gt", datetime_start pick_datetime "#q_completed_at_lt", datetime_end - select 'Order Cycle Customer Totals', from: 'report_type' + select 'Order Cycle Customer Totals', from: 'report_subtype' click_button 'Search' # Then I should see the rows for the first order but not the second expect(all('table#listing_orders tbody tr').count).to eq(4) # Two rows per order @@ -349,7 +349,7 @@ describe ' it "shows users and enterprises report" do click_button "Search" - rows = find("table#users_and_enterprises").all("tr") + rows = find("table.report__table").all("tr") table = rows.map { |r| r.all("th,td").map { |c| c.text.strip }[0..2] } expect(table.sort).to eq([ @@ -370,7 +370,7 @@ describe ' click_button "Search" - rows = find("table#users_and_enterprises").all("tr") + rows = find("table.report__table").all("tr") table = rows.map { |r| r.all("th,td").map { |c| c.text.strip }[0..2] } expect(table.sort).to eq([ @@ -555,7 +555,7 @@ describe ' private def xero_invoice_table - find("table#listing_invoices") + find("table.report__table") end def xero_invoice_header