From 7fe913713adc8148ceac5740a8012c6f8be26896 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 8 Feb 2023 17:18:08 +1100 Subject: [PATCH 1/3] Use same parameters for report job as for original The Enterprise Fee Summary report modified the `params` object. So when we pass the same `params` a second time to the report class it doesn't find the same values. In some cases that would lead to a server error. The next commit implements a better solution though. --- lib/reporting/reports/enterprise_fee_summary/base.rb | 3 ++- .../enterprise_fee_summary_report_spec.rb | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/reporting/reports/enterprise_fee_summary/base.rb b/lib/reporting/reports/enterprise_fee_summary/base.rb index 837c84df7d..ccbef3d323 100644 --- a/lib/reporting/reports/enterprise_fee_summary/base.rb +++ b/lib/reporting/reports/enterprise_fee_summary/base.rb @@ -8,7 +8,8 @@ module Reporting def initialize(user, params = {}, render: false) super(user, params, render: render) - p = params[:q] + # Clone hash before modifying it: + p = params[:q].clone if p.present? p['start_at'] = p.delete('completed_at_gt') p['end_at'] = p.delete('completed_at_lt') 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 6ea086c7dd..f9ce422d39 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 @@ -99,6 +99,12 @@ describe Reporting::Reports::EnterpriseFeeSummary::Base do let!(:second_customer_order) { prepare_order(customer: 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 180434c5bdd87b5398302fc792157b182df767bc Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 8 Feb 2023 17:30:32 +1100 Subject: [PATCH 2/3] Avoid renaming params within report I changed the used parameter names within the report so that we don't have to rename the given parameter names when the report runs. This avoids modifying the `params` object and therefore other problems. --- .../reports/enterprise_fee_summary/base.rb | 8 +------- .../enterprise_fee_summary/parameters.rb | 4 ++-- .../reports/parameters/base.rb | 4 ++-- .../reports/enterprise_fee_summary/scope.rb | 6 +++--- .../enterprise_fee_summary_report_spec.rb | 8 ++++---- .../enterprise_fee_summary/parameters_spec.rb | 20 +++++++++---------- 6 files changed, 22 insertions(+), 28 deletions(-) diff --git a/lib/reporting/reports/enterprise_fee_summary/base.rb b/lib/reporting/reports/enterprise_fee_summary/base.rb index ccbef3d323..c3a8a042f0 100644 --- a/lib/reporting/reports/enterprise_fee_summary/base.rb +++ b/lib/reporting/reports/enterprise_fee_summary/base.rb @@ -8,13 +8,7 @@ module Reporting def initialize(user, params = {}, render: false) super(user, params, render: render) - # Clone hash before modifying it: - p = params[:q].clone - if p.present? - p['start_at'] = p.delete('completed_at_gt') - p['end_at'] = p.delete('completed_at_lt') - end - @parameters = Parameters.new(p || {}) + @parameters = Parameters.new(params.fetch(:q, {})) @parameters.validate! @permissions = Permissions.new(user) @parameters.authorize!(@permissions) diff --git a/lib/reporting/reports/enterprise_fee_summary/parameters.rb b/lib/reporting/reports/enterprise_fee_summary/parameters.rb index 59c97aeaa2..5163625b7b 100644 --- a/lib/reporting/reports/enterprise_fee_summary/parameters.rb +++ b/lib/reporting/reports/enterprise_fee_summary/parameters.rb @@ -6,12 +6,12 @@ module Reporting class Parameters < Reporting::Reports::EnterpriseFeeSummary::Reports::Parameters::Base include ActiveModel::Validations - attr_accessor :start_at, :end_at, :distributor_ids, :producer_ids, :order_cycle_ids, + attr_accessor :completed_at_gt, :completed_at_lt, :distributor_ids, :producer_ids, :order_cycle_ids, :enterprise_fee_ids, :shipping_method_ids, :payment_method_ids before_validation :cleanup_arrays - validates :start_at, :end_at, date_time_string: true + validates :completed_at_gt, :completed_at_lt, date_time_string: true validates :distributor_ids, :producer_ids, integer_array: true validates :order_cycle_ids, integer_array: true validates :enterprise_fee_ids, integer_array: true diff --git a/lib/reporting/reports/enterprise_fee_summary/reports/parameters/base.rb b/lib/reporting/reports/enterprise_fee_summary/reports/parameters/base.rb index 0906658d88..c365c7d246 100644 --- a/lib/reporting/reports/enterprise_fee_summary/reports/parameters/base.rb +++ b/lib/reporting/reports/enterprise_fee_summary/reports/parameters/base.rb @@ -28,10 +28,10 @@ module Reporting protected def require_valid_datetime_range - return if start_at.blank? || end_at.blank? + return if completed_at_gt.blank? || completed_at_lt.blank? error_message = self.class.date_end_before_start_error_message - errors.add(:end_at, error_message) unless start_at < end_at + errors.add(:completed_at_lt, error_message) unless completed_at_gt < completed_at_lt end end end diff --git a/lib/reporting/reports/enterprise_fee_summary/scope.rb b/lib/reporting/reports/enterprise_fee_summary/scope.rb index 177ba90713..0b5cfa8944 100644 --- a/lib/reporting/reports/enterprise_fee_summary/scope.rb +++ b/lib/reporting/reports/enterprise_fee_summary/scope.rb @@ -315,9 +315,9 @@ module Reporting end def filter_by_date(params) - filter_scope("spree_orders.completed_at >= ?", params.start_at) \ - if params.start_at.present? - filter_scope("spree_orders.completed_at <= ?", params.end_at) if params.end_at.present? + filter_scope("spree_orders.completed_at >= ?", params.completed_at_gt) \ + if params.completed_at_gt.present? + filter_scope("spree_orders.completed_at <= ?", params.completed_at_lt) if params.completed_at_lt.present? end def filter_by_distribution(params) 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 f9ce422d39..f36faea77e 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 @@ -468,8 +468,8 @@ describe Reporting::Reports::EnterpriseFeeSummary::Base do end end - context "on or after start_at" do - let(:parameters_attributes) { { start_at: timestamp } } + context "on or after completed_at_gt" do + let(:parameters_attributes) { { completed_at_gt: timestamp } } it "filters entries" do totals = subject.query_result @@ -480,8 +480,8 @@ describe Reporting::Reports::EnterpriseFeeSummary::Base do end end - context "on or before end_at" do - let(:parameters_attributes) { { end_at: timestamp } } + context "on or before completed_at_lt" do + let(:parameters_attributes) { { completed_at_lt: timestamp } } it "filters entries" do totals = subject.query_result diff --git a/spec/lib/reports/enterprise_fee_summary/parameters_spec.rb b/spec/lib/reports/enterprise_fee_summary/parameters_spec.rb index 9a3967edf5..d04d140283 100644 --- a/spec/lib/reports/enterprise_fee_summary/parameters_spec.rb +++ b/spec/lib/reports/enterprise_fee_summary/parameters_spec.rb @@ -16,8 +16,8 @@ module Reporting end context "for type of parameters" do - it { is_expected.to validate_date_time_format_of(:start_at) } - it { is_expected.to validate_date_time_format_of(:end_at) } + it { is_expected.to validate_date_time_format_of(:completed_at_gt) } + it { is_expected.to validate_date_time_format_of(:completed_at_lt) } it { is_expected.to validate_integer_array(:distributor_ids) } it { is_expected.to validate_integer_array(:producer_ids) } it { is_expected.to validate_integer_array(:order_cycle_ids) } @@ -43,21 +43,21 @@ module Reporting expect(subject.payment_method_ids).to eq(["1"]) end - describe "requiring start_at to be before end_at" do + describe "requiring completed_at_gt to be before completed_at_lt" do let(:now) { Time.zone.now.utc } - it "adds error when start_at is after end_at" do - allow(subject).to receive(:start_at) { now.to_s } - allow(subject).to receive(:end_at) { (now - 1.hour).to_s } + it "adds error when completed_at_gt is after completed_at_lt" do + allow(subject).to receive(:completed_at_gt) { now.to_s } + allow(subject).to receive(:completed_at_lt) { (now - 1.hour).to_s } expect(subject).not_to be_valid error_message = described_class.date_end_before_start_error_message - expect(subject.errors[:end_at]).to eq([error_message]) + expect(subject.errors[:completed_at_lt]).to eq([error_message]) end - it "does not add error when start_at is before end_at" do - allow(subject).to receive(:start_at) { now.to_s } - allow(subject).to receive(:end_at) { (now + 1.hour).to_s } + it "does not add error when completed_at_gt is before completed_at_lt" do + allow(subject).to receive(:completed_at_gt) { now.to_s } + allow(subject).to receive(:completed_at_lt) { (now + 1.hour).to_s } expect(subject).to be_valid end From 4aa6898ecb13606b66c661a2d37a2d77494f4501 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Feb 2023 11:58:50 +1100 Subject: [PATCH 3/3] Reduce line length for easier reading --- lib/reporting/reports/enterprise_fee_summary/parameters.rb | 5 +++-- lib/reporting/reports/enterprise_fee_summary/scope.rb | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/reporting/reports/enterprise_fee_summary/parameters.rb b/lib/reporting/reports/enterprise_fee_summary/parameters.rb index 5163625b7b..de74521af0 100644 --- a/lib/reporting/reports/enterprise_fee_summary/parameters.rb +++ b/lib/reporting/reports/enterprise_fee_summary/parameters.rb @@ -6,8 +6,9 @@ module Reporting class Parameters < Reporting::Reports::EnterpriseFeeSummary::Reports::Parameters::Base include ActiveModel::Validations - attr_accessor :completed_at_gt, :completed_at_lt, :distributor_ids, :producer_ids, :order_cycle_ids, - :enterprise_fee_ids, :shipping_method_ids, :payment_method_ids + attr_accessor :completed_at_gt, :completed_at_lt, :distributor_ids, + :producer_ids, :order_cycle_ids, :enterprise_fee_ids, + :shipping_method_ids, :payment_method_ids before_validation :cleanup_arrays diff --git a/lib/reporting/reports/enterprise_fee_summary/scope.rb b/lib/reporting/reports/enterprise_fee_summary/scope.rb index 0b5cfa8944..651ecbb035 100644 --- a/lib/reporting/reports/enterprise_fee_summary/scope.rb +++ b/lib/reporting/reports/enterprise_fee_summary/scope.rb @@ -317,7 +317,8 @@ module Reporting def filter_by_date(params) filter_scope("spree_orders.completed_at >= ?", params.completed_at_gt) \ if params.completed_at_gt.present? - filter_scope("spree_orders.completed_at <= ?", params.completed_at_lt) if params.completed_at_lt.present? + filter_scope("spree_orders.completed_at <= ?", params.completed_at_lt) \ + if params.completed_at_lt.present? end def filter_by_distribution(params)