Merge pull request #4721 from betagouv/rubocop-unscoped

Ajout d'une règle Rubocop pour interdire `unscoped`

.rubocop.yml

@@ -1,5 +1,6 @@
 require:
   - rubocop/rspec/focused
+  - ./lib/cops/unscoped.rb
 
 AllCops:
   Exclude:

app/controllers/manager/procedures_controller.rb

@@ -12,7 +12,7 @@ module Manager
         Procedure
       else
         # … but allow them to be searched and displayed.
-        Procedure.unscope(:where)
+        Procedure.with_hidden
       end
     end
 

app/models/dossier.rb

@@ -95,6 +95,8 @@ class Dossier < ApplicationRecord
   end
 
   default_scope { where(hidden_at: nil) }
+  scope :hidden,                               -> { unscope(where: :hidden_at).where.not(hidden_at: nil) }
+  scope :with_hidden,                          -> { unscope(where: :hidden_at) }
   scope :state_brouillon,                      -> { where(state: states.fetch(:brouillon)) }
   scope :state_not_brouillon,                  -> { where.not(state: states.fetch(:brouillon)) }
   scope :state_en_construction,                -> { where(state: states.fetch(:en_construction)) }

app/models/procedure.rb

@@ -45,6 +45,8 @@ class Procedure < ApplicationRecord
   accepts_nested_attributes_for :types_de_champ_private, reject_if: proc { |attributes| attributes['libelle'].blank? }, allow_destroy: true
 
   default_scope { where(hidden_at: nil) }
+  scope :hidden,                -> { unscope(where: :hidden_at).where.not(hidden_at: nil) }
+  scope :with_hidden,           -> { unscope(where: :hidden_at) }
   scope :brouillons,            -> { where(aasm_state: :brouillon) }
   scope :publiees,              -> { where(aasm_state: :publiee) }
   scope :closes,                -> { where(aasm_state: [:close, :depubliee]) }

lib/cops/unscoped.rb

@@ -0,0 +1,18 @@
+module RuboCop
+  module Cop
+    module DS
+      class Unscoped < Cop
+        MSG = "Avoid using `unscoped`. Instead unscope specific clauses by using `unscope(where: :attribute)`."
+
+        def_node_matcher :unscoped?, <<-END
+          (send _ :unscoped)
+        END
+
+        def on_send(node)
+          return unless unscoped?(node)
+          add_offense(node)
+        end
+      end
+    end
+  end
+end

lib/tasks/2017_09_19_set_confidentialite_to_old_avis.rake

@@ -1,5 +1,5 @@
 namespace :'2017_09_19_set_confidentialite_to_old_avis' do
   task set: :environment do
-    Avis.unscoped.update_all(confidentiel: true)
+    Avis.unscope(:joins).update_all(confidentiel: true)
   end
 end

lib/tasks/2017_09_22_set_dossier_updated_replied_to_initiated.rake

@@ -1,5 +1,5 @@
 namespace :'2017_09_22_set_dossier_updated_replied_to_initiated' do
   task set: :environment do
-    Dossier.unscoped.where(state: [:updated, :replied]).update_all(state: :initiated)
+    Dossier.with_hidden.where(state: [:updated, :replied]).update_all(state: :initiated)
   end
 end

lib/tasks/2017_10_18_regenerate_attestation.rake

@@ -12,8 +12,8 @@ namespace :'2017_10_18_regenerate_attestation' do
   end
 
   def regenerate_attestations(attestation)
-    Procedure.unscoped do
-      Dossier.unscoped do
+    Procedure.with_hidden do
+      Dossier.with_hidden do
         dossier = attestation.dossier
         procedure = dossier.procedure
 

lib/tasks/2017_12_04_translate_dossier_state_to_french.rake

@@ -1,37 +1,37 @@
 namespace :'2017_12_04_translate_dossier_state_to_french' do
   task brouillon: :environment do
-    Dossier.unscoped.where(state: 'draft').update_all(state: 'brouillon')
+    Dossier.with_hidden.where(state: 'draft').update_all(state: 'brouillon')
   end
 
   task en_construction: :environment do
-    Dossier.unscoped.where(state: 'initiated').update_all(state: 'en_construction')
+    Dossier.with_hidden.where(state: 'initiated').update_all(state: 'en_construction')
   end
 
   task en_instruction: :environment do
-    Dossier.unscoped.where(state: 'received').update_all(state: 'en_instruction')
+    Dossier.with_hidden.where(state: 'received').update_all(state: 'en_instruction')
   end
 
   task accepte: :environment do
-    Dossier.unscoped.where(state: 'closed').update_all(state: 'accepte')
+    Dossier.with_hidden.where(state: 'closed').update_all(state: 'accepte')
   end
 
   task refuse: :environment do
-    Dossier.unscoped.where(state: 'refused').update_all(state: 'refuse')
+    Dossier.with_hidden.where(state: 'refused').update_all(state: 'refuse')
   end
 
   task sans_suite: :environment do
-    Dossier.unscoped.where(state: 'without_continuation').update_all(state: 'sans_suite')
+    Dossier.with_hidden.where(state: 'without_continuation').update_all(state: 'sans_suite')
   end
 
   task all: [:brouillon, :en_construction, :en_instruction, :accepte, :refuse, :sans_suite] do
   end
 
   task revert: :environment do
-    Dossier.unscoped.where(state: 'brouillon').update_all(state: 'draft')
-    Dossier.unscoped.where(state: 'en_construction').update_all(state: 'initiated')
-    Dossier.unscoped.where(state: 'en_instruction').update_all(state: 'received')
-    Dossier.unscoped.where(state: 'accepte').update_all(state: 'closed')
-    Dossier.unscoped.where(state: 'refuse').update_all(state: 'refused')
-    Dossier.unscoped.where(state: 'sans_suite').update_all(state: 'without_continuation')
+    Dossier.with_hidden.where(state: 'brouillon').update_all(state: 'draft')
+    Dossier.with_hidden.where(state: 'en_construction').update_all(state: 'initiated')
+    Dossier.with_hidden.where(state: 'en_instruction').update_all(state: 'received')
+    Dossier.with_hidden.where(state: 'accepte').update_all(state: 'closed')
+    Dossier.with_hidden.where(state: 'refuse').update_all(state: 'refused')
+    Dossier.with_hidden.where(state: 'sans_suite').update_all(state: 'without_continuation')
   end
 end

lib/tasks/2018_04_04_fetch_etablissement_with_no_entreprise.rake

@@ -4,7 +4,7 @@ namespace :'2018_04_04_fetch_etablissement_with_no_entreprise' do
   task fetch: :environment do
     dossiers = Entreprise.joins('LEFT JOIN etablissements et ON entreprises.id = et.entreprise_id')
       .where('et.id IS NULL')
-      .map(&:dossier_id).map { |id| Dossier.unscoped.find_by(id: id) }.compact
+      .map(&:dossier_id).map { |id| Dossier.with_hidden.find_by(id: id) }.compact
 
     dossiers.each do |dossier|
       siret = dossier.entreprise.siret_siege_social

lib/tasks/2018_05_15_add_aasm_state_to_procedure.rake

@@ -3,6 +3,6 @@ namespace :'2018_05_15_add_aasm_state_to_procedure' do
     Procedure.archivees.update_all(aasm_state: :archivee)
     Procedure.publiees.update_all(aasm_state: :publiee)
     Procedure.brouillons.update_all(aasm_state: :brouillon)
-    Procedure.unscoped.where.not(hidden_at: nil).update_all(aasm_state: :hidden)
+    Procedure.hidden.update_all(aasm_state: :hidden)
   end
 end

lib/tasks/2018_06_13_unhide_dossiers.rake

@@ -1,6 +1,6 @@
 namespace :'2018_06_13_unhide_dossiers' do
   task run: :environment do
-    Dossier.unscoped.where.not(hidden_at: nil).state_instruction_commencee.each do |d|
+    Dossier.hidden.state_instruction_commencee.each do |d|
       if !d.procedure.nil? # ensure the procedure was not deleted by administrateur for testing
         d.update(hidden_at: nil)
         DeletedDossier.find_by(dossier_id: d.id)&.destroy

lib/tasks/2019_06_06_fix_timestamps_of_migrated_dossiers.rake

@@ -4,7 +4,7 @@ namespace :fix_timestamps_of_migrated_dossiers do
   desc 'Fix the timestamps of dossiers affected by the faulty PJ migration'
   task run: :environment do
     affected_time_range = Time.utc(2019, 6, 4, 8, 0)..Time.utc(2019, 6, 4, 18, 0)
-    dossiers = Dossier.unscoped.includes(:groupe_instructeur).where(groupe_instructeurs: { procedure_id: 0..1000 }).where(updated_at: affected_time_range)
+    dossiers = Dossier.with_hidden.includes(:groupe_instructeur).where(groupe_instructeurs: { procedure_id: 0..1000 }).where(updated_at: affected_time_range)
 
     progress = ProgressReport.new(dossiers.count)
 

lib/tasks/deployment/20190704133852_create_dummy_paths_for_archived_and_hidden_procedures.rake

@@ -3,11 +3,11 @@ namespace :after_party do
   task create_dummy_paths_for_archived_and_hidden_procedures: :environment do
     rake_puts "Running deploy task 'create_dummy_paths_for_archived_procedures'"
 
-    Procedure.unscoped.archivees.where(path: nil).each do |p|
+    Procedure.with_hidden.archivees.where(path: nil).each do |p|
       p.update_column(:path, SecureRandom.uuid)
     end
 
-    Procedure.unscoped.hidden.where(path: nil).each do |p|
+    Procedure.hidden.where(path: nil).each do |p|
       p.update_column(:path, SecureRandom.uuid)
     end
 

lib/tasks/deployment/20190819142551_create_default_groupe_instructeur.rake

@@ -2,7 +2,7 @@ namespace :after_party do
   desc 'Deployment task: create_default_groupe_instructeur'
   task create_default_groupe_instructeur: :environment do
     Procedure
-      .unscoped
+      .with_hidden
       .left_outer_joins(:groupe_instructeurs)
       .where('groupe_instructeurs.id is null')
       .find_each do |procedure|

spec/models/dossier_spec.rb

@@ -5,6 +5,42 @@ describe Dossier do
 
   let(:user) { create(:user) }
 
+  describe 'scopes' do
+    describe '.default_scope' do
+      let!(:dossier) { create(:dossier) }
+      let!(:hidden_dossier) { create(:dossier, :hidden) }
+
+      subject { Dossier.all }
+
+      it { is_expected.to match_array([dossier]) }
+    end
+
+    describe '.hidden' do
+      let!(:dossier) { create(:dossier) }
+      let!(:hidden_dossier) { create(:dossier, :hidden) }
+
+      subject { Dossier.all.hidden }
+
+      it { is_expected.to match_array([hidden_dossier]) }
+    end
+
+    describe '.with_hidden' do
+      let!(:dossier) { create(:dossier) }
+      let!(:hidden_dossier) { create(:dossier, :hidden) }
+
+      subject { Dossier.all.with_hidden }
+
+      it { is_expected.to match_array([dossier, hidden_dossier]) }
+    end
+
+    describe '.without_followers' do
+      let!(:dossier_with_follower) { create(:dossier, :followed, :with_entreprise, user: user) }
+      let!(:dossier_without_follower) { create(:dossier, :with_entreprise, user: user) }
+
+      it { expect(Dossier.without_followers.to_a).to eq([dossier_without_follower]) }
+    end
+  end
+
   describe 'validations' do
     let(:procedure) { create(:procedure, :for_individual) }
     subject(:dossier) { create(:dossier, procedure: procedure) }
@@ -12,13 +48,6 @@ describe Dossier do
     it { is_expected.to validate_presence_of(:individual) }
   end
 
-  describe "without_followers scope" do
-    let!(:dossier) { create(:dossier, :followed, :with_entreprise, user: user) }
-    let!(:dossier2) { create(:dossier, :with_entreprise, user: user) }
-
-    it { expect(Dossier.without_followers.to_a).to eq([dossier2]) }
-  end
-
   describe 'with_champs' do
     let(:procedure) { create(:procedure) }
     let(:dossier) { Dossier.create(user: create(:user), groupe_instructeur: procedure.defaut_groupe_instructeur) }
@@ -522,23 +551,6 @@ describe Dossier do
     end
   end
 
-  describe ".default_scope" do
-    let!(:dossier) { create(:dossier, hidden_at: hidden_at) }
-
-    context "when dossier is not hidden" do
-      let(:hidden_at) { nil }
-
-      it { expect(Dossier.count).to eq(1) }
-      it { expect(Dossier.all).to include(dossier) }
-    end
-
-    context "when dossier is hidden" do
-      let(:hidden_at) { 1.day.ago }
-
-      it { expect(Dossier.count).to eq(0) }
-    end
-  end
-
   describe 'updated_at' do
     let!(:dossier) { create(:dossier) }
     let(:modif_date) { Time.zone.parse('01/01/2100') }

spec/models/procedure_spec.rb

@@ -151,6 +151,26 @@ describe Procedure do
     end
   end
 
+  describe 'scopes' do
+    let!(:procedure) { create(:procedure) }
+    let!(:hidden_procedure) { create(:procedure, :hidden) }
+
+    describe 'default_scope' do
+      subject { Procedure.all }
+      it { is_expected.to match_array([procedure]) }
+    end
+
+    describe '.hidden' do
+      subject { Procedure.all.hidden }
+      it { is_expected.to match_array([hidden_procedure]) }
+    end
+
+    describe '.with_hidden' do
+      subject { Procedure.all.with_hidden }
+      it { is_expected.to match_array([procedure, hidden_procedure]) }
+    end
+  end
+
   describe 'validation' do
     context 'libelle' do
       it { is_expected.not_to allow_value(nil).for(:libelle) }