From 828b2f6f44b3e14497a8498f3c7257ff23adecc2 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 24 May 2023 04:54:13 +0100
Subject: [PATCH] Replace too long list of arguments with keywords

Rubocop was complaining about too many arguments. But
`ApplicationJob#perform` needs all arguments handled in one call. While
we could allow the `perform` method generally to have more arguments,
there could be other methods called `perform` which should still be
scrutinised. Instead, it seems acceptable to me to have more arguments
as long as they are clearly named as keyword arguments. Rails uses this
a lot to document all options including their default values, for
example in Active Storage. It's better then bundling several arguments
in an undocumented hash just to reduce the number of given arguments.

And once we upgraded to Ruby 3.1, we can clean the method calls up as
well. `call(user: user)` becomes `call(user:)` without repetition.
---
 .rubocop_styleguide.yml                     |  3 +++
 app/controllers/admin/reports_controller.rb |  3 ++-
 app/jobs/report_job.rb                      |  2 +-
 spec/jobs/report_job_spec.rb                | 23 +++++++++++++++------
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml
index a5878eef94..202aeb940a 100644
--- a/.rubocop_styleguide.yml
+++ b/.rubocop_styleguide.yml
@@ -44,6 +44,9 @@ Metrics/BlockLength:
     "xdescribe",
   ]
 
+Metrics/ParameterLists:
+  CountKeywordArgs: false
+
 Rails/ApplicationRecord:
   Exclude:
     # Migrations should not contain application code:
diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 120c7a689d..4171d48e14 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -65,7 +65,8 @@ module Admin
       blob = ReportBlob.create_for_upload_later!(report_filename)
 
       ReportJob.perform_later(
-        report_class, spree_current_user, params, format, blob, ScopedChannel.for_id(params[:uuid])
+        report_class: report_class, user: spree_current_user, params: params,
+        format: format, blob: blob, channel: ScopedChannel.for_id(params[:uuid]),
       )
 
       render cable_ready: cable_car.
diff --git a/app/jobs/report_job.rb b/app/jobs/report_job.rb
index 15c40d79ba..1f7e4d3d7a 100644
--- a/app/jobs/report_job.rb
+++ b/app/jobs/report_job.rb
@@ -9,7 +9,7 @@ class ReportJob < ApplicationJob
 
   NOTIFICATION_TIME = 5.seconds
 
-  def perform(report_class, user, params, format, blob, channel = nil)
+  def perform(report_class:, user:, params:, format:, blob:, channel: nil)
     start_time = Time.zone.now
 
     report = report_class.new(user, params, render: true)
diff --git a/spec/jobs/report_job_spec.rb b/spec/jobs/report_job_spec.rb
index b66d4a33f2..54cb7c53f7 100644
--- a/spec/jobs/report_job_spec.rb
+++ b/spec/jobs/report_job_spec.rb
@@ -3,7 +3,10 @@
 require 'spec_helper'
 
 describe ReportJob do
-  let(:report_args) { [report_class, user, params, format, blob] }
+  let(:report_args) {
+    { report_class: report_class, user: user, params: params, format: format,
+      blob: blob }
+  }
   let(:report_class) { Reporting::Reports::UsersAndEnterprises::Base }
   let(:user) { enterprise.owner }
   let(:enterprise) { create(:enterprise) }
@@ -13,13 +16,13 @@ describe ReportJob do
 
   it "generates a report" do
     job = perform_enqueued_jobs(only: ReportJob) do
-      ReportJob.perform_later(*report_args)
+      ReportJob.perform_later(**report_args)
     end
     expect_csv_report
   end
 
   it "enqueues a job for async processing" do
-    job = ReportJob.perform_later(*report_args)
+    job = ReportJob.perform_later(**report_args)
     expect(blob.content_stored?).to eq false
 
     perform_enqueued_jobs(only: ReportJob)
@@ -29,12 +32,16 @@ describe ReportJob do
   end
 
   it "triggers an email when the report is done" do
+    # Setup test data which also triggers emails:
+    report_args
+
     # Send emails for quick jobs as well:
     stub_const("ReportJob::NOTIFICATION_TIME", 0)
 
-    ReportJob.perform_later(*report_args)
-
     expect {
+      # We need to create this job within the block because of a bug in
+      # rspec-rails: https://github.com/rspec/rspec-rails/issues/2668
+      ReportJob.perform_later(**report_args)
       perform_enqueued_jobs(only: ReportJob)
     }.to enqueue_mail(ReportMailer, :report_ready).with(
       params: {
@@ -46,9 +53,13 @@ describe ReportJob do
   end
 
   it "triggers no email when the report is done quickly" do
-    ReportJob.perform_later(*report_args)
+    # Setup test data which also triggers emails:
+    report_args
 
     expect {
+      # We need to create this job within the block because of a bug in
+      # rspec-rails: https://github.com/rspec/rspec-rails/issues/2668
+      ReportJob.perform_later(**report_args)
       perform_enqueued_jobs(only: ReportJob)
     }.to_not enqueue_mail
   end