Merge pull request #3223 from betagouv/code-cleanup

Code cleanup

.rubocop.yml

@@ -58,7 +58,7 @@ Layout/CommentIndentation:
   Enabled: true
 
 Layout/ConditionPosition:
-  Enabled: false
+  Enabled: true
 
 Layout/DefEndAlignment:
   Enabled: false
@@ -200,7 +200,7 @@ Layout/SpaceAfterColon:
   Enabled: true
 
 Layout/SpaceAfterComma:
-  Enabled: false
+  Enabled: true
 
 Layout/SpaceAfterMethodName:
   Enabled: true
@@ -224,7 +224,7 @@ Layout/SpaceAroundOperators:
   Enabled: true
 
 Layout/SpaceBeforeBlockBraces:
-  Enabled: false
+  Enabled: true
 
 Layout/SpaceBeforeComma:
   Enabled: true
@@ -252,7 +252,8 @@ Layout/SpaceInsideHashLiteralBraces:
   Enabled: true
 
 Layout/SpaceInsideArrayLiteralBrackets:
-  Enabled: false
+  Enabled: true
+  EnforcedStyle: no_space
 
 Layout/SpaceInsideParens:
   Enabled: true
@@ -339,40 +340,40 @@ Lint/EnsureReturn:
   Enabled: false
 
 Lint/FloatOutOfRange:
-  Enabled: false
+  Enabled: true
 
 Lint/FormatParameterMismatch:
-  Enabled: false
+  Enabled: true
 
 Lint/HandleExceptions:
   Enabled: false
 
 Lint/ImplicitStringConcatenation:
-  Enabled: false
+  Enabled: true
 
 Lint/IneffectiveAccessModifier:
   Enabled: false
 
 Lint/InheritException:
-  Enabled: false
+  Enabled: true
 
 Lint/InterpolationCheck:
   Enabled: true
 
 Lint/LiteralAsCondition:
-  Enabled: false
+  Enabled: true
 
 Lint/LiteralInInterpolation:
-  Enabled: false
+  Enabled: true
 
 Lint/Loop:
-  Enabled: false
+  Enabled: true
 
 Lint/MissingCopEnableDirective:
   Enabled: true
 
 Lint/MultipleCompare:
-  Enabled: false
+  Enabled: true
 
 Lint/NestedMethodDefinition:
   Enabled: false
@@ -381,7 +382,7 @@ Lint/NestedPercentLiteral:
   Enabled: true
 
 Lint/NextWithoutAccumulator:
-  Enabled: false
+  Enabled: true
 
 Lint/NonLocalExitFromIterator:
   Enabled: false
@@ -393,7 +394,9 @@ Lint/OrderedMagicComments:
   Enabled: true
 
 Lint/ParenthesesAsGroupedExpression:
-  Enabled: false
+  Enabled: true
+  Exclude:
+    - "spec/**/*"
 
 Lint/PercentStringArray:
   Enabled: false
@@ -417,16 +420,16 @@ Lint/RequireParentheses:
   Enabled: false
 
 Lint/RescueException:
-  Enabled: false
+  Enabled: true
 
 Lint/RescueType:
-  Enabled: false
+  Enabled: true
 
 Lint/ReturnInVoidContext:
   Enabled: false
 
 Lint/SafeNavigationChain:
-  Enabled: false
+  Enabled: true
 
 Lint/ScriptPermission:
   Enabled: false
@@ -435,25 +438,25 @@ Lint/ShadowedArgument:
   Enabled: true
 
 Lint/ShadowedException:
-  Enabled: false
+  Enabled: true
 
 Lint/ShadowingOuterLocalVariable:
   Enabled: false
 
 Lint/StringConversionInInterpolation:
-  Enabled: false
+  Enabled: true
 
 Lint/Syntax:
   Enabled: true
 
 Lint/UnderscorePrefixedVariableName:
-  Enabled: false
+  Enabled: true
 
 Lint/UnifiedInteger:
-  Enabled: false
+  Enabled: true
 
 Lint/UnneededCopDisableDirective:
-  Enabled: false
+  Enabled: true
 
 Lint/UnneededCopEnableDirective:
   Enabled: true
@@ -465,10 +468,10 @@ Lint/UnneededSplatExpansion:
   Enabled: false
 
 Lint/UnreachableCode:
-  Enabled: false
+  Enabled: true
 
 Lint/UnusedBlockArgument:
-  Enabled: false
+  Enabled: true
 
 Lint/UnusedMethodArgument:
   Enabled: false
@@ -483,19 +486,21 @@ Lint/UselessAccessModifier:
   Enabled: false
 
 Lint/UselessAssignment:
-  Enabled: false
+  Enabled: true
+  Exclude:
+    - "spec/**/*"
 
 Lint/UselessComparison:
-  Enabled: false
+  Enabled: true
 
 Lint/UselessElseWithoutRescue:
-  Enabled: false
+  Enabled: true
 
 Lint/UselessSetterCall:
-  Enabled: false
+  Enabled: true
 
 Lint/Void:
-  Enabled: false
+  Enabled: true
 
 Metrics/AbcSize:
   Enabled: false

app/controllers/stats_controller.rb

@@ -211,7 +211,7 @@ class StatsController < ApplicationController
 
     association
       .where(date_attribute => min_date..max_date)
-      .group("DATE_TRUNC('month', #{date_attribute.to_s})")
+      .group("DATE_TRUNC('month', #{date_attribute})")
       .count
       .to_a
       .sort_by { |a| a[0] }
@@ -221,8 +221,8 @@ class StatsController < ApplicationController
   def cumulative_hash(association, date_attribute)
     sum = 0
     association
-      .where("#{date_attribute.to_s} < ?", max_date)
-      .group("DATE_TRUNC('month', #{date_attribute.to_s})")
+      .where("#{date_attribute} < ?", max_date)
+      .group("DATE_TRUNC('month', #{date_attribute})")
       .count
       .to_a
       .sort_by { |a| a[0] }
@@ -271,7 +271,7 @@ class StatsController < ApplicationController
       dossiers_grouped_by_procedure = value.group_by { |dossier| dossier[0] }
 
       # Compute the mean time for this procedure
-      procedure_processing_times = dossiers_grouped_by_procedure.map do |procedure_id, procedure_dossiers|
+      procedure_processing_times = dossiers_grouped_by_procedure.map do |_procedure_id, procedure_dossiers|
         procedure_dossiers_processing_time = procedure_dossiers.map do |dossier|
           (dossier[2] - dossier[1]).to_f / (3600 * 24)
         end

app/jobs/auto_receive_dossiers_for_procedure_job.rb

@@ -1,7 +1,7 @@
 class AutoReceiveDossiersForProcedureJob < ApplicationJob
   queue_as :cron
 
-  def perform(procedure_id, state, gestionnaire_id = nil)
+  def perform(procedure_id, state)
     procedure = Procedure.find(procedure_id)
     gestionnaire = procedure.gestionnaire_for_cron_job
 

app/models/champs/piece_justificative_champ.rb

@@ -31,11 +31,11 @@ class Champs::PieceJustificativeChamp < Champ
 
     if piece_justificative_file.attached? && piece_justificative_file.previous_changes.present?
       if piece_justificative_file.blob.byte_size > PIECE_JUSTIFICATIVE_FILE_MAX_SIZE
-        errors << "Le fichier #{piece_justificative_file.filename.to_s} est trop lourd, il doit faire au plus #{PIECE_JUSTIFICATIVE_FILE_MAX_SIZE.to_s(:human_size, precision: 2)}"
+        errors << "Le fichier #{piece_justificative_file.filename} est trop lourd, il doit faire au plus #{PIECE_JUSTIFICATIVE_FILE_MAX_SIZE.to_s(:human_size, precision: 2)}"
       end
 
       if !piece_justificative_file.blob.content_type.in?(PIECE_JUSTIFICATIVE_FILE_ACCEPTED_FORMATS)
-        errors << "Le fichier #{piece_justificative_file.filename.to_s} est dans un format que nous n'acceptons pas"
+        errors << "Le fichier #{piece_justificative_file.filename} est dans un format que nous n'acceptons pas"
       end
 
       # FIXME: add Clamav check

app/models/etablissement.rb

@@ -91,6 +91,6 @@ class Etablissement < ApplicationRecord
   def message_for_signature
     JSON.pretty_generate(as_json(include: {
       exercices: { only: [:ca, :date_fin_exercice, :date_fin_exercice_timestamp] }
-    }).delete_if { |k,v| v.blank? })
+    }).delete_if { |_k, v| v.blank? })
   end
 end

app/services/pieces_justificatives_service.rb

@@ -10,7 +10,7 @@ class PiecesJustificativesService
     errors = with_virus
       .map { |_, content| "#{content.original_filename} : virus détecté" }
 
-    errors += without_virus
+    errors + without_virus
       .map { |tpj, content| save_pj(content, dossier, tpj, user) }
       .compact()
   end

app/services/procedure_export_service.rb

@@ -212,7 +212,8 @@ class ProcedureExportService
         etablissement.champ.dossier_id,
         label_for_export(etablissement.champ.libelle).to_s
       ]
-      data += etablissement_data(etablissement)
+
+      data + etablissement_data(etablissement)
     end
   end
 

app/services/render_partial_service.rb

@@ -31,6 +31,6 @@ class RenderPartialService
   end
 
   def retrieve_name
-    "#{controller.to_s.parameterize.underscore}_#{method.to_s}"
+    "#{controller.to_s.parameterize.underscore}_#{method}"
   end
 end

config/initializers/active_job_log_subscriber.rb

@@ -32,12 +32,19 @@ class ActiveJobLogSubscriber < ::ActiveJob::Logging::LogSubscriber
 
   def process_event(event, type)
     data = extract_metadata(event)
-    data.merge! extract_exception(event)
-    data.merge! extract_scheduled_at(event) if type == 'enqueue_at'
-    data.merge! extract_duration(event) if type == 'perform'
+    data.merge!(extract_exception(event))
+
+    case type
+    when 'enqueue_at'
+      data.merge!(extract_scheduled_at(event))
+    when 'perform'
+      data.merge!(extract_duration(event))
+    end
 
     tags = ['job', type]
-    tags.push('exception') if data[:exception]
+    if data[:exception]
+      tags.push('exception')
+    end
     data[:tags] = tags
     data[:type] = 'tps'
     data[:source] = ENV['SOURCE']

config/initializers/after_party.rb

@@ -1,3 +1,3 @@
-AfterParty.setup do |config|
+AfterParty.setup do |_config|
   require "after_party/active_record.rb"
 end

config/initializers/env_vars.rb

@@ -5,7 +5,7 @@
 
 if ENV['RAILS_ENV'] != 'test' && File.basename($0) != 'rake'
   reference_env_file = File.join('config', 'env.example')
-  Dotenv::Environment.new(Rails.root.join(reference_env_file)).each do |key, value|
+  Dotenv::Environment.new(Rails.root.join(reference_env_file)).each do |key, _value|
     if !ENV.key?(key.to_s)
       raise "Configuration error: `#{key}` is not present in the process’ environment variables (declared in `#{reference_env_file}`)"
     end

config/initializers/lograge.rb

@@ -30,7 +30,7 @@ Rails.application.configure do
   end
 
   config.lograge.keep_original_rails_log = true
-  config.lograge.logger = ActiveSupport::Logger.new Rails.root.join('log', "logstash_#{Rails.env}.log")
+  config.lograge.logger = ActiveSupport::Logger.new(Rails.root.join('log', "logstash_#{Rails.env}.log"))
 
   if config.lograge.enabled
     ActiveJobLogSubscriber.attach_to(:active_job)

db/migrate/20170215102943_remove_duplicate_email_received.rb

@@ -2,7 +2,7 @@ class RemoveDuplicateEmailReceived < ActiveRecord::Migration[5.0]
   def change
     all_mails = MailReceived.all
     groupped = all_mails.group_by(&:procedure_id)
-    filtered = groupped.reject { |k, v| v.length < 2 }
+    filtered = groupped.reject { |_k, v| v.length < 2 }
     filtered.each_value do |duplicate_mails|
       duplicate_mails.pop
       duplicate_mails.each(&:destroy)

lib/tasks/2018_02_28_clean_gestionnaire_emails.rake

@@ -1,10 +1,10 @@
 namespace :'2018_02_28_clean_invalid_emails_accounts' do
   task clean: :environment do
-    Gestionnaire.pluck(:email, :id).select { |e, id| e.include?(" ") }.each do |email, id|
+    Gestionnaire.pluck(:email, :id).select { |e, _id| e.include?(" ") }.each do |_email, id|
       Gestionnaire.find_by(id: id, current_sign_in_at: nil)&.destroy # ensure account was never used
     end
 
-    User.pluck(:email, :id).select { |e, id| e.include?(" ") }.each do |email, id|
+    User.pluck(:email, :id).select { |e, _id| e.include?(" ") }.each do |_email, id|
       User.find_by(id: id, current_sign_in_at: nil)&.destroy # ensure account was never used
     end
   end

lib/tasks/2018_09_12_fix_templates.rake

@@ -2,7 +2,7 @@ namespace :'2018_09_12_fix_templates' do
   task run: :environment do
     dossiers_with_invalid_attestations = find_dossiers_with_sent_and_invalid_attestations
     fix_templates
-    fixed_attestations = delete_then_regenerate_attestations(dossiers_with_invalid_attestations)
+    delete_then_regenerate_attestations(dossiers_with_invalid_attestations)
     send_regenerated_attestations(dossiers_with_invalid_attestations)
   end
 
@@ -46,7 +46,7 @@ namespace :'2018_09_12_fix_templates' do
         .each do |instance|
 
         instance.update(body: instance.body.gsub("--libellé procédure--", "--libellé démarche--"))
-        rake_puts "Body mis-à-jour pour #{klass.to_s}##{instance.id}"
+        rake_puts "Body mis-à-jour pour #{klass}##{instance.id}"
       end
     end
   end

lib/tasks/admin.rake

@@ -1,5 +1,5 @@
 namespace :admin do
-  task :create_admin, [:email] => :environment do |t, args|
+  task :create_admin, [:email] => :environment do |_t, args|
     email = args[:email]
     puts "Creating Administration for #{email}"
     a = Administration.new(email: email, password: Devise.friendly_token[0, 20])
@@ -17,7 +17,7 @@ namespace :admin do
     end
   end
 
-  task :delete_admin, [:email] => :environment do |t, args|
+  task :delete_admin, [:email] => :environment do |_t, args|
     email = args[:email]
     puts "Deleting Administration for #{email}"
     a = Administration.find_by(email: email)

lib/tasks/deployment/20181120133842_remove_footer_from_email_templates.rake

@@ -47,7 +47,7 @@ namespace :after_party do
   def remove_footer(template)
     matches = template.body.match(FOOTER_REGEXP)
     if matches && FOOTER_EXCEPTIONS.none? { |exception| matches[0].include?(exception) }
-      rake_puts "#{template.model_name.to_s} \##{template.id}: removing footer"
+      rake_puts "#{template.model_name} \##{template.id}: removing footer"
       template.update(body: matches.pre_match)
     end
   end

lib/tasks/deployment/20181219122438_fix_email_templates_subjects.rake

@@ -17,7 +17,7 @@ namespace :after_party do
         .each do |instance|
 
         instance.update(subject: instance.subject.gsub("--libellé procédure--", "--libellé démarche--"))
-        rake_puts "Subject mis-à-jour pour #{klass.to_s}##{instance.id}"
+        rake_puts "Subject mis-à-jour pour #{klass}##{instance.id}"
       end
     end
 

spec/controllers/admin/procedures_controller_spec.rb

@@ -511,7 +511,7 @@ describe Admin::ProceduresController, type: :controller do
 
     subject { get :new_from_existing }
     let(:grouped_procedures) { subject; assigns(:grouped_procedures) }
-    let(:response_procedures) { grouped_procedures.map{ |o, procedures| procedures }.flatten }
+    let(:response_procedures) { grouped_procedures.map { |_o, procedures| procedures }.flatten }
 
     describe 'selecting' do
       let!(:large_draft_procedure)     { create(:procedure_with_dossiers, dossiers_count: 2) }
@@ -542,8 +542,8 @@ describe Admin::ProceduresController, type: :controller do
 
       it 'groups procedures with services as well as procedures with organisations' do
         expect(grouped_procedures.length).to eq 2
-        expect(grouped_procedures.find{ |o, p| o == 'DDT des Vosges' }.last).to contain_exactly(procedure_with_service_1)
-        expect(grouped_procedures.find{ |o, p| o == 'DDT du Loiret'  }.last).to contain_exactly(procedure_with_service_2, procedure_without_service)
+        expect(grouped_procedures.find { |o, _p| o == 'DDT des Vosges' }.last).to contain_exactly(procedure_with_service_1)
+        expect(grouped_procedures.find { |o, _p| o == 'DDT du Loiret'  }.last).to contain_exactly(procedure_with_service_2, procedure_without_service)
       end
     end
   end

spec/factories/champ.rb

@@ -23,7 +23,7 @@ FactoryBot.define do
     end
 
     trait :with_piece_justificative_file do
-      after(:create) do |champ, evaluator|
+      after(:create) do |champ, _evaluator|
         champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
       end
     end
@@ -147,7 +147,7 @@ FactoryBot.define do
   factory :champ_piece_justificative, class: 'Champs::PieceJustificativeChamp' do
     type_de_champ { create(:type_de_champ_piece_justificative) }
 
-    after(:create) do |champ, evaluator|
+    after(:create) do |champ, _evaluator|
       champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
     end
   end
@@ -161,7 +161,7 @@ FactoryBot.define do
     association :etablissement, factory: [:etablissement]
     value { '44011762001530' }
 
-    after(:build) do |champ, evaluator|
+    after(:build) do |champ, _evaluator|
       champ.etablissement.signature = champ.etablissement.sign
     end
   end

spec/factories/procedure.rb

@@ -16,8 +16,8 @@ FactoryBot.define do
         dossiers_count { 1 }
       end
 
-      after(:build) do |procedure, _evaluator|
-        procedure.dossiers << create_list(:dossier, _evaluator.dossiers_count, procedure: procedure)
+      after(:build) do |procedure, evaluator|
+        procedure.dossiers << create_list(:dossier, evaluator.dossiers_count, procedure: procedure)
       end
     end
 

spec/features/new_gestionnaire/gestionnaire_spec.rb

@@ -105,7 +105,7 @@ feature 'The gestionnaire part' do
     avis = dossier.avis.first
     test_mail(expert_email, sign_up_gestionnaire_avis_path(avis, expert_email))
 
-    avis_sign_up(avis, expert_email, 'a good password')
+    avis_sign_up(avis, expert_email)
 
     expect(page).to have_current_path(gestionnaire_avis_index_path)
     expect(page).to have_text('avis à donner 1')
@@ -225,7 +225,7 @@ feature 'The gestionnaire part' do
     texts.each { |text| expect(page).to have_text(text) }
   end
 
-  def avis_sign_up(avis, email, password)
+  def avis_sign_up(avis, email)
     visit sign_up_gestionnaire_avis_path(avis, email)
     fill_in 'gestionnaire_password', with: 'a good password'
     click_on 'Créer un compte'

spec/services/render_partial_service_spec.rb

@@ -8,12 +8,12 @@ describe RenderPartialService do
   describe 'navbar' do
     subject { service.navbar }
 
-    it { is_expected.to eq "layouts/navbars/navbar_#{controller.to_s.parameterize}_#{method.to_s}" }
+    it { is_expected.to eq "layouts/navbars/navbar_#{controller.to_s.parameterize}_#{method}" }
   end
 
   describe 'left_panel' do
     subject { service.left_panel }
 
-    it { is_expected.to eq "layouts/left_panels/left_panel_#{controller.to_s.parameterize}_#{method.to_s}" }
+    it { is_expected.to eq "layouts/left_panels/left_panel_#{controller.to_s.parameterize}_#{method}" }
   end
 end