Add latest changes from gitlab-org/gitlab@master

10 files changed, 261 insertions, 95 deletions

diff --git a/spec/lib/gitlab/github_import/importer/attachments/base_importer_spec.rb b/spec/lib/gitlab/github_import/importer/attachments/base_importer_spec.rb
index 5e60be44621..bc1b32661b8 100644
--- a/spec/lib/gitlab/github_import/importer/attachments/base_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/attachments/base_importer_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Importer::Attachments::BaseImporter do
+RSpec.describe Gitlab::GithubImport::Importer::Attachments::BaseImporter, feature_category: :importers do
   subject(:importer) { importer_class.new(project, client) }
 
   let(:project) { instance_double(Project, id: 1) }
diff --git a/spec/lib/gitlab/github_import/importer/attachments/issues_importer_spec.rb b/spec/lib/gitlab/github_import/importer/attachments/issues_importer_spec.rb
index b44f1ec85f3..20152020897 100644
--- a/spec/lib/gitlab/github_import/importer/attachments/issues_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/attachments/issues_importer_spec.rb
@@ -10,39 +10,68 @@ RSpec.describe Gitlab::GithubImport::Importer::Attachments::IssuesImporter, feat
   let(:client) { instance_double(Gitlab::GithubImport::Client) }
 
   describe '#sequential_import', :clean_gitlab_redis_cache do
-    let_it_be(:issue_1) { create(:issue, project: project) }
-    let_it_be(:issue_2) { create(:issue, project: project) }
+    let_it_be(:issue) { create(:issue, project: project) }
 
-    let(:importer_stub) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') }
-    let(:importer_attrs) { [instance_of(Gitlab::GithubImport::Representation::NoteText), project, client] }
+    let_it_be(:issue_with_attachment) do
+      create(:issue,
+        project: project,
+        description: "![image](https://user-images.githubusercontent.com/1/uuid-1.png)"
+      )
+    end
+
+    it 'selects both issues, and selects only properties it needs' do
+      stubbed_collection = class_double(Issue, each_batch: [])
+
+      expect(project.issues).to receive(:id_not_in).with([]).and_return(stubbed_collection)
+      expect(stubbed_collection).to receive(:select).with(:id, :description, :iid).and_return(stubbed_collection)
 
-    it 'imports each project issue attachments' do
-      expect(project.issues).to receive(:id_not_in).with([]).and_return(project.issues)
-      expect(project.issues).to receive(:select).with(:id, :description, :iid).and_call_original
+      importer.sequential_import
+    end
 
-      expect_next_instances_of(
-        Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2, false, *importer_attrs
-      ) do |note_attachments_importer|
-        expect(note_attachments_importer).to receive(:execute)
+    it 'executes importer only for the issue with an attachment' do
+      expect_next_instance_of(
+        Gitlab::GithubImport::Importer::NoteAttachmentsImporter,
+        have_attributes(record_db_id: issue_with_attachment.id),
+        project,
+        client
+      ) do |importer|
+        expect(importer).to receive(:execute)
       end
 
       importer.sequential_import
     end
 
-    context 'when issue is already processed' do
-      it "doesn't import this issue attachments" do
-        importer.mark_as_imported(issue_1)
+    context 'when flag is disabled' do
+      before do
+        stub_feature_flags(github_importer_attachments: false)
+      end
 
-        expect(project.issues).to receive(:id_not_in).with([issue_1.id.to_s]).and_call_original
-        expect_next_instance_of(
-          Gitlab::GithubImport::Importer::NoteAttachmentsImporter, *importer_attrs
-        ) do |note_attachments_importer|
-          expect(note_attachments_importer).to receive(:execute)
+      it 'executes importer for both issues' do
+        expect_next_instances_of(Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2) do |importer|
+          expect(importer).to receive(:execute)
         end
 
         importer.sequential_import
       end
     end
+
+    context 'when issue has already been processed' do
+      before do
+        importer.mark_as_imported(issue_with_attachment)
+      end
+
+      it 'does not select issues that were processed' do
+        expect(project.issues).to receive(:id_not_in).with([issue_with_attachment.id.to_s]).and_call_original
+
+        importer.sequential_import
+      end
+
+      it 'does not execute importer for the issue with an attachment' do
+        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).not_to receive(:new)
+
+        importer.sequential_import
+      end
+    end
   end
 
   describe '#sidekiq_worker_class' do
diff --git a/spec/lib/gitlab/github_import/importer/attachments/merge_requests_importer_spec.rb b/spec/lib/gitlab/github_import/importer/attachments/merge_requests_importer_spec.rb
index 381cb17bb52..5ed6dce8507 100644
--- a/spec/lib/gitlab/github_import/importer/attachments/merge_requests_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/attachments/merge_requests_importer_spec.rb
@@ -10,39 +10,69 @@ RSpec.describe Gitlab::GithubImport::Importer::Attachments::MergeRequestsImporte
   let(:client) { instance_double(Gitlab::GithubImport::Client) }
 
   describe '#sequential_import', :clean_gitlab_redis_cache do
-    let_it_be(:merge_request_1) { create(:merge_request, source_project: project, target_branch: 'feature1') }
-    let_it_be(:merge_request_2) { create(:merge_request, source_project: project, target_branch: 'feature2') }
+    let_it_be(:mr) { create(:merge_request, source_project: project, target_branch: 'feature1') }
+
+    let_it_be(:mr_with_attachment) do
+      create(:merge_request,
+        source_project: project,
+        target_branch: 'feature2',
+        description: "![image](https://user-images.githubusercontent.com/1/uuid-1.png)"
+      )
+    end
+
+    it 'selects both merge requests, and selects only properties it needs' do
+      stubbed_collection = class_double(MergeRequest, each_batch: [])
 
-    let(:importer_stub) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') }
-    let(:importer_attrs) { [instance_of(Gitlab::GithubImport::Representation::NoteText), project, client] }
+      expect(project.merge_requests).to receive(:id_not_in).with([]).and_return(stubbed_collection)
+      expect(stubbed_collection).to receive(:select).with(:id, :description, :iid).and_return(stubbed_collection)
 
-    it 'imports each project merge request attachments' do
-      expect(project.merge_requests).to receive(:id_not_in).with([]).and_return(project.merge_requests)
-      expect(project.merge_requests).to receive(:select).with(:id, :description, :iid).and_call_original
+      importer.sequential_import
+    end
 
-      expect_next_instances_of(
-        Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2, false, *importer_attrs
-      ) do |note_attachments_importer|
-        expect(note_attachments_importer).to receive(:execute)
+    it 'executes importer only for the merge request with an attachment' do
+      expect_next_instance_of(
+        Gitlab::GithubImport::Importer::NoteAttachmentsImporter,
+        have_attributes(record_db_id: mr_with_attachment.id),
+        project,
+        client
+      ) do |importer|
+        expect(importer).to receive(:execute)
       end
 
       importer.sequential_import
     end
 
-    context 'when merge request is already processed' do
-      it "doesn't import this merge request attachments" do
-        importer.mark_as_imported(merge_request_1)
+    context 'when flag is disabled' do
+      before do
+        stub_feature_flags(github_importer_attachments: false)
+      end
 
-        expect(project.merge_requests).to receive(:id_not_in).with([merge_request_1.id.to_s]).and_call_original
-        expect_next_instance_of(
-          Gitlab::GithubImport::Importer::NoteAttachmentsImporter, *importer_attrs
-        ) do |note_attachments_importer|
-          expect(note_attachments_importer).to receive(:execute)
+      it 'executes importer for both merge requests' do
+        expect_next_instances_of(Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2) do |importer|
+          expect(importer).to receive(:execute)
         end
 
         importer.sequential_import
       end
     end
+
+    context 'when merge request has already been processed' do
+      before do
+        importer.mark_as_imported(mr_with_attachment)
+      end
+
+      it 'does not select merge requests that were processed' do
+        expect(project.merge_requests).to receive(:id_not_in).with([mr_with_attachment.id.to_s]).and_call_original
+
+        importer.sequential_import
+      end
+
+      it 'does not execute importer for the merge request with an attachment' do
+        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).not_to receive(:new)
+
+        importer.sequential_import
+      end
+    end
   end
 
   describe '#sidekiq_worker_class' do
diff --git a/spec/lib/gitlab/github_import/importer/attachments/notes_importer_spec.rb b/spec/lib/gitlab/github_import/importer/attachments/notes_importer_spec.rb
index 5b3ad032702..da0ee1ed0dd 100644
--- a/spec/lib/gitlab/github_import/importer/attachments/notes_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/attachments/notes_importer_spec.rb
@@ -10,30 +10,75 @@ RSpec.describe Gitlab::GithubImport::Importer::Attachments::NotesImporter, featu
   let(:client) { instance_double(Gitlab::GithubImport::Client) }
 
   describe '#sequential_import', :clean_gitlab_redis_cache do
-    let_it_be(:note_1) { create(:note, project: project) }
-    let_it_be(:note_2) { create(:note, project: project) }
-    let_it_be(:system_note) { create(:note, :system, project: project) }
+    let_it_be(:note) { create(:note, project: project) }
 
-    let(:importer_stub) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') }
-    let(:importer_attrs) { [instance_of(Gitlab::GithubImport::Representation::NoteText), project, client] }
+    let_it_be(:note_with_attachment) do
+      create(:note,
+        project: project,
+        note: "![image](https://user-images.githubusercontent.com/1/uuid-1.png)"
+      )
+    end
+
+    let_it_be(:system_note_with_attachment) do
+      create(:note,
+        :system,
+        project: project,
+        note: "![image](https://user-images.githubusercontent.com/1/uuid-1.png)"
+      )
+    end
 
-    it 'imports each project user note' do
-      expect(project.notes).to receive(:id_not_in).with([]).and_call_original
-      expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).to receive(:new)
-        .with(*importer_attrs).twice.and_return(importer_stub)
-      expect(importer_stub).to receive(:execute).twice
+    it 'selects only user notes, and selects only properties it needs' do
+      stubbed_collection = class_double(Note, each_batch: [])
+
+      expect(project.notes).to receive(:id_not_in).with([]).and_return(stubbed_collection)
+      expect(stubbed_collection).to receive(:user).and_return(stubbed_collection)
+      expect(stubbed_collection)
+        .to receive(:select).with(:id, :note, :system, :noteable_type)
+        .and_return(stubbed_collection)
 
       importer.sequential_import
     end
 
-    context 'when note is already processed' do
-      it "doesn't import this note" do
-        importer.mark_as_imported(note_1)
+    it 'executes importer only for the note with an attachment' do
+      expect_next_instance_of(
+        Gitlab::GithubImport::Importer::NoteAttachmentsImporter,
+        have_attributes(record_db_id: note_with_attachment.id),
+        project,
+        client
+      ) do |importer|
+        expect(importer).to receive(:execute)
+      end
+
+      importer.sequential_import
+    end
+
+    context 'when flag is disabled' do
+      before do
+        stub_feature_flags(github_importer_attachments: false)
+      end
+
+      it 'executes importer for both user notes' do
+        expect_next_instances_of(Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2) do |importer|
+          expect(importer).to receive(:execute)
+        end
+
+        importer.sequential_import
+      end
+    end
+
+    context 'when note has already been processed' do
+      before do
+        importer.mark_as_imported(note_with_attachment)
+      end
+
+      it 'does not select notes that were processed' do
+        expect(project.notes).to receive(:id_not_in).with([note_with_attachment.id.to_s]).and_call_original
+
+        importer.sequential_import
+      end
 
-        expect(project.notes).to receive(:id_not_in).with([note_1.id.to_s]).and_call_original
-        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).to receive(:new)
-          .with(*importer_attrs).once.and_return(importer_stub)
-        expect(importer_stub).to receive(:execute).once
+      it 'does not execute importer for the note with an attachment' do
+        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).not_to receive(:new)
 
         importer.sequential_import
       end
diff --git a/spec/lib/gitlab/github_import/importer/attachments/releases_importer_spec.rb b/spec/lib/gitlab/github_import/importer/attachments/releases_importer_spec.rb
index c1c19c40afb..cf51760d966 100644
--- a/spec/lib/gitlab/github_import/importer/attachments/releases_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/attachments/releases_importer_spec.rb
@@ -10,31 +10,64 @@ RSpec.describe Gitlab::GithubImport::Importer::Attachments::ReleasesImporter, fe
   let(:client) { instance_double(Gitlab::GithubImport::Client) }
 
   describe '#sequential_import', :clean_gitlab_redis_cache do
-    let_it_be(:release_1) { create(:release, project: project) }
-    let_it_be(:release_2) { create(:release, project: project) }
+    let_it_be(:release) { create(:release, project: project) }
 
-    let(:importer_stub) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') }
-    let(:importer_attrs) { [instance_of(Gitlab::GithubImport::Representation::NoteText), project, client] }
+    let_it_be(:release_with_attachment) do
+      create(:release,
+        project: project,
+        description: "![image](https://user-images.githubusercontent.com/1/uuid-1.png)"
+      )
+    end
 
-    it 'imports each project release' do
-      expect(project.releases).to receive(:id_not_in).with([]).and_return(project.releases)
-      expect(project.releases).to receive(:select).with(:id, :description, :tag).and_call_original
+    it 'selects both releases, and selects only properties it needs' do
+      stubbed_collection = class_double(Release, each_batch: [])
 
-      expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).to receive(:new)
-        .with(*importer_attrs).twice.and_return(importer_stub)
-      expect(importer_stub).to receive(:execute).twice
+      expect(project.releases).to receive(:id_not_in).with([]).and_return(stubbed_collection)
+      expect(stubbed_collection).to receive(:select).with(:id, :description, :tag).and_return(stubbed_collection)
 
       importer.sequential_import
     end
 
-    context 'when note is already processed' do
-      it "doesn't import this release" do
-        importer.mark_as_imported(release_1)
+    it 'executes importer only for the release with an attachment' do
+      expect_next_instance_of(
+        Gitlab::GithubImport::Importer::NoteAttachmentsImporter,
+        have_attributes(record_db_id: release_with_attachment.id),
+        project,
+        client
+      ) do |importer|
+        expect(importer).to receive(:execute)
+      end
+
+      importer.sequential_import
+    end
+
+    context 'when flag is disabled' do
+      before do
+        stub_feature_flags(github_importer_attachments: false)
+      end
+
+      it 'executes importer for both releases' do
+        expect_next_instances_of(Gitlab::GithubImport::Importer::NoteAttachmentsImporter, 2) do |importer|
+          expect(importer).to receive(:execute)
+        end
+
+        importer.sequential_import
+      end
+    end
+
+    context 'when release has already been processed' do
+      before do
+        importer.mark_as_imported(release_with_attachment)
+      end
+
+      it 'does not select releases that were processed' do
+        expect(project.releases).to receive(:id_not_in).with([release_with_attachment.id.to_s]).and_call_original
+
+        importer.sequential_import
+      end
 
-        expect(project.releases).to receive(:id_not_in).with([release_1.id.to_s]).and_call_original
-        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).to receive(:new)
-          .with(*importer_attrs).once.and_return(importer_stub)
-        expect(importer_stub).to receive(:execute).once
+      it 'does not execute importer for the release with an attachment' do
+        expect(Gitlab::GithubImport::Importer::NoteAttachmentsImporter).not_to receive(:new)
 
         importer.sequential_import
       end
diff --git a/spec/lib/gitlab/github_import/markdown_text_spec.rb b/spec/lib/gitlab/github_import/markdown_text_spec.rb
index 3f771970588..ff3821dedec 100644
--- a/spec/lib/gitlab/github_import/markdown_text_spec.rb
+++ b/spec/lib/gitlab/github_import/markdown_text_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::MarkdownText do
+RSpec.describe Gitlab::GithubImport::MarkdownText, feature_category: :importers do
   describe '.format' do
     it 'formats the text' do
       author = double(:author, login: 'Alice')
@@ -103,6 +103,10 @@ RSpec.describe Gitlab::GithubImport::MarkdownText do
         "https://github.com/nickname/public-test-repo/files/3/git-cheat-sheet.#{doc_extension}"
       )
     end
+
+    it 'returns an empty array when passed nil' do
+      expect(described_class.fetch_attachments(nil)).to be_empty
+    end
   end
 
   describe '#to_s' do
diff --git a/spec/lib/gitlab/github_import/representation/note_text_spec.rb b/spec/lib/gitlab/github_import/representation/note_text_spec.rb
index 7aa458a1c33..b1ca1512855 100644
--- a/spec/lib/gitlab/github_import/representation/note_text_spec.rb
+++ b/spec/lib/gitlab/github_import/representation/note_text_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Representation::NoteText do
+RSpec.describe Gitlab::GithubImport::Representation::NoteText, feature_category: :importers do
   shared_examples 'a Note text data' do |match_record_type|
     it 'returns an instance of NoteText' do
       expect(representation).to be_an_instance_of(described_class)
@@ -153,4 +153,36 @@ RSpec.describe Gitlab::GithubImport::Representation::NoteText do
       end
     end
   end
+
+  describe '#has_attachments?' do
+    subject { described_class.new({ text: text }).has_attachments? }
+
+    context 'when text has attachments' do
+      let(:text) { 'See ![image](https://user-images.githubusercontent.com/1/uuid-1.png) for details' }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when text does not have attachments' do
+      let(:text) { 'Some text here'  }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#attachments' do
+    subject { described_class.new({ text: text }).attachments }
+
+    context 'when text has attachments' do
+      let(:text) { 'See ![image](https://user-images.githubusercontent.com/1/uuid-1.png) for details' }
+
+      it { is_expected.to contain_exactly(instance_of(Gitlab::GithubImport::Markdown::Attachment)) }
+    end
+
+    context 'when text does not have attachments' do
+      let(:text) { 'Some text here'  }
+
+      it { is_expected.to be_empty }
+    end
+  end
 end
diff --git a/spec/lib/gitlab/pagination/keyset/iterator_spec.rb b/spec/lib/gitlab/pagination/keyset/iterator_spec.rb
index afaad48d363..326f3c6d344 100644
--- a/spec/lib/gitlab/pagination/keyset/iterator_spec.rb
+++ b/spec/lib/gitlab/pagination/keyset/iterator_spec.rb
@@ -46,12 +46,6 @@ RSpec.describe Gitlab::Pagination::Keyset::Iterator do
         end
       end
 
-      it 'raises error when ordering configuration cannot be automatically determined' do
-        expect do
-          described_class.new(scope: MergeRequestDiffCommit.order(:merge_request_diff_id, :relative_order))
-        end.to raise_error /The order on the scope does not support keyset pagination/
-      end
-
       it 'accepts a custom batch size' do
         count = 0
 
diff --git a/spec/lib/gitlab/pagination/keyset/paginator_spec.rb b/spec/lib/gitlab/pagination/keyset/paginator_spec.rb
index 230ac01af31..16c5b3ab748 100644
--- a/spec/lib/gitlab/pagination/keyset/paginator_spec.rb
+++ b/spec/lib/gitlab/pagination/keyset/paginator_spec.rb
@@ -3,11 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Pagination::Keyset::Paginator do
-  let_it_be(:project_1) { create(:project, created_at: 10.weeks.ago) }
-  let_it_be(:project_2) { create(:project, created_at: 2.weeks.ago) }
-  let_it_be(:project_3) { create(:project, created_at: 3.weeks.ago) }
-  let_it_be(:project_4) { create(:project, created_at: 5.weeks.ago) }
-  let_it_be(:project_5) { create(:project, created_at: 2.weeks.ago) }
+  let_it_be(:project_1) { create(:project, :public, name: 'Project A', created_at: 10.weeks.ago) }
+  let_it_be(:project_2) { create(:project, :public, name: 'Project E', created_at: 2.weeks.ago) }
+  let_it_be(:project_3) { create(:project, :private, name: 'Project C', created_at: 3.weeks.ago) }
+  let_it_be(:project_4) { create(:project, :private, name: 'Project B', created_at: 5.weeks.ago) }
+  let_it_be(:project_5) { create(:project, :private, name: 'Project B', created_at: 2.weeks.ago) }
 
   describe 'pagination' do
     let(:per_page) { 10 }
@@ -98,6 +98,13 @@ RSpec.describe Gitlab::Pagination::Keyset::Paginator do
       end
     end
 
+    context 'when the relation is ordered by more than 2 columns' do
+      let(:scope) { Project.order(visibility_level: :asc, name: :asc, id: :asc) }
+      let(:expected_order) { [project_4, project_5, project_3, project_1, project_2] }
+
+      it { expect(paginator.records).to eq(expected_order) }
+    end
+
     describe 'default keyset direction parameter' do
       let(:cursor_converter_class) { Gitlab::Pagination::Keyset::Paginator::Base64CursorConverter }
       let(:per_page) { 2 }
@@ -110,14 +117,6 @@ RSpec.describe Gitlab::Pagination::Keyset::Paginator do
     end
   end
 
-  context 'when unsupported order is given' do
-    it 'raises error' do
-      scope = Project.order(path: :asc, name: :asc, id: :desc) # Cannot build 3 column order automatically
-
-      expect { scope.keyset_paginate }.to raise_error(/does not support keyset pagination/)
-    end
-  end
-
   context 'when use_union_optimization option is true and ordering by two columns' do
     let(:scope) { Project.order(name: :asc, id: :desc) }
 
diff --git a/spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb b/spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb
index e85b0354ff6..fd38fff2b81 100644
--- a/spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb
+++ b/spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb
@@ -146,7 +146,7 @@ RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
     context 'when more than 2 columns are given for the order' do
       let(:scope) { Project.order(created_at: :asc, updated_at: :desc, id: :asc) }
 
-      it { is_expected.to eq(false) }
+      it { is_expected.to eq(true) }
     end
   end
 end