Merge branch 'fix-n-plus-one-when-getting-notification-settings-for-recipients' into 'master'

Use Goldiloader for handling N+1 queries

See merge request gitlab-org/gitlab-ce!18217

2 files changed, 89 insertions, 8 deletions

diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb
index 7ea25059756..91d4d5d3de9 100644
--- a/spec/requests/api/pipeline_schedules_spec.rb
+++ b/spec/requests/api/pipeline_schedules_spec.rb
@@ -17,6 +17,17 @@ describe API::PipelineSchedules do
         pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
       end
 
+      def create_pipeline_schedules(count)
+        create_list(:ci_pipeline_schedule, count, project: project)
+          .each do |pipeline_schedule|
+          create(:user).tap do |user|
+            project.add_developer(user)
+            pipeline_schedule.update_attributes(owner: user)
+          end
+          pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
+        end
+      end
+
       it 'returns list of pipeline_schedules' do
         get api("/projects/#{project.id}/pipeline_schedules", developer)
 
@@ -26,18 +37,14 @@ describe API::PipelineSchedules do
       end
 
       it 'avoids N + 1 queries' do
+        # We need at least two users to trigger a preload for that relation.
+        create_pipeline_schedules(1)
+
         control_count = ActiveRecord::QueryRecorder.new do
           get api("/projects/#{project.id}/pipeline_schedules", developer)
         end.count
 
-        create_list(:ci_pipeline_schedule, 10, project: project)
-          .each do |pipeline_schedule|
-          create(:user).tap do |user|
-            project.add_developer(user)
-            pipeline_schedule.update_attributes(owner: user)
-          end
-          pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
-        end
+        create_pipeline_schedules(10)
 
         expect do
           get api("/projects/#{project.id}/pipeline_schedules", developer)
diff --git a/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb b/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb
new file mode 100644
index 00000000000..6d769c8e6fd
--- /dev/null
+++ b/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb
@@ -0,0 +1,74 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/gitlab/has_many_through_scope'
+
+describe RuboCop::Cop::Gitlab::HasManyThroughScope do # rubocop:disable RSpec/FilePath
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in a model file' do
+    before do
+      allow(cop).to receive(:in_model?).and_return(true)
+    end
+
+    context 'when the model does not use has_many :through' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<-RUBY)
+          class User < ActiveRecord::Base
+            has_many :tags, source: 'UserTag'
+          end
+        RUBY
+      end
+    end
+
+    context 'when the model uses has_many :through' do
+      context 'when the association has no scope defined' do
+        it 'registers an offense on the association' do
+          expect_offense(<<-RUBY)
+            class User < ActiveRecord::Base
+              has_many :tags, through: :user_tags
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+            end
+           RUBY
+        end
+      end
+
+      context 'when the association has a scope defined' do
+        context 'when the scope does not disable auto-loading' do
+          it 'registers an offense on the scope' do
+            expect_offense(<<-RUBY)
+              class User < ActiveRecord::Base
+                has_many :tags, -> { where(active: true) }, through: :user_tags
+                                ^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+              end
+             RUBY
+          end
+        end
+
+        context 'when the scope has auto_include(false)' do
+          it 'does not register an offense' do
+            expect_no_offenses(<<-RUBY)
+              class User < ActiveRecord::Base
+                has_many :tags, -> { where(active: true).auto_include(false).reorder(nil) }, through: :user_tags
+              end
+            RUBY
+          end
+        end
+      end
+    end
+  end
+
+  context 'outside of a migration spec file' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<-RUBY)
+        class User < ActiveRecord::Base
+          has_many :tags, through: :user_tags
+        end
+      RUBY
+    end
+  end
+end