Add latest changes from gitlab-org/gitlab@master

10 files changed, 142 insertions, 36 deletions

diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb
index ddc95ff92d5..22bb1736f9f 100644
--- a/spec/services/bulk_create_integration_service_spec.rb
+++ b/spec/services/bulk_create_integration_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe BulkCreateIntegrationService do
-  include JiraServiceHelper
+  include JiraIntegrationHelpers
 
   before_all do
     stub_jira_integration_test
diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb
index 23e2a0ef0ff..e3e38aacaa2 100644
--- a/spec/services/bulk_update_integration_service_spec.rb
+++ b/spec/services/bulk_update_integration_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe BulkUpdateIntegrationService do
-  include JiraServiceHelper
+  include JiraIntegrationHelpers
 
   before_all do
     stub_jira_integration_test
diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb
index 57c130f76a4..befa9598964 100644
--- a/spec/services/git/branch_push_service_spec.rb
+++ b/spec/services/git/branch_push_service_spec.rb
@@ -410,7 +410,7 @@ RSpec.describe Git::BranchPushService, services: true do
     end
 
     context "for jira issue tracker" do
-      include JiraServiceHelper
+      include JiraIntegrationHelpers
 
       let(:jira_tracker) { project.create_jira_integration if project.jira_integration.nil? }
 
diff --git a/spec/services/integrations/propagate_service_spec.rb b/spec/services/integrations/propagate_service_spec.rb
index 7ae843f6aeb..c971c4a0ad0 100644
--- a/spec/services/integrations/propagate_service_spec.rb
+++ b/spec/services/integrations/propagate_service_spec.rb
@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Integrations::PropagateService do
   describe '.propagate' do
-    include JiraServiceHelper
+    include JiraIntegrationHelpers
 
     before do
       stub_jira_integration_test
diff --git a/spec/services/jira_import/start_import_service_spec.rb b/spec/services/jira_import/start_import_service_spec.rb
index e04e3314158..510f58f0e75 100644
--- a/spec/services/jira_import/start_import_service_spec.rb
+++ b/spec/services/jira_import/start_import_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe JiraImport::StartImportService do
-  include JiraServiceHelper
+  include JiraIntegrationHelpers
 
   let_it_be(:user) { create(:user) }
   let_it_be(:project, reload: true) { create(:project) }
diff --git a/spec/services/jira_import/users_importer_spec.rb b/spec/services/jira_import/users_importer_spec.rb
index af408847260..ace9e0d5779 100644
--- a/spec/services/jira_import/users_importer_spec.rb
+++ b/spec/services/jira_import/users_importer_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe JiraImport::UsersImporter do
-  include JiraServiceHelper
+  include JiraIntegrationHelpers
 
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group) }
diff --git a/spec/services/markdown_content_rewriter_service_spec.rb b/spec/services/markdown_content_rewriter_service_spec.rb
index 37c8a210ba5..91a117536ca 100644
--- a/spec/services/markdown_content_rewriter_service_spec.rb
+++ b/spec/services/markdown_content_rewriter_service_spec.rb
@@ -8,38 +8,63 @@ RSpec.describe MarkdownContentRewriterService do
   let_it_be(:target_parent) { create(:project, :public) }
 
   let(:content) { 'My content' }
+  let(:issue) { create(:issue, project: source_parent, description: content)}
 
   describe '#initialize' do
     it 'raises an error if source_parent is not a Project' do
       expect do
-        described_class.new(user, content, create(:group), target_parent)
+        described_class.new(user, issue, :description, create(:group), target_parent)
       end.to raise_error(ArgumentError, 'The rewriter classes require that `source_parent` is a `Project`')
     end
+
+    it 'raises an error if field does not have cached markdown' do
+      expect do
+        described_class.new(user, issue, :author, source_parent, target_parent)
+      end.to raise_error(ArgumentError, 'The `field` attribute does not contain cached markdown')
+    end
   end
 
   describe '#execute' do
-    subject { described_class.new(user, content, source_parent, target_parent).execute }
+    subject { described_class.new(user, issue, :description, source_parent, target_parent).execute }
 
-    it 'calls the rewriter classes successfully', :aggregate_failures do
-      [Gitlab::Gfm::ReferenceRewriter, Gitlab::Gfm::UploadsRewriter].each do |rewriter_class|
-        service = double
-
-        expect(service).to receive(:rewrite).with(target_parent)
-        expect(rewriter_class).to receive(:new).and_return(service)
+    context 'when content does not need a rewrite' do
+      it 'returns original content and cached html' do
+        expect(subject).to eq({
+          'description' => issue.description,
+          'description_html' => issue.description_html,
+          'skip_markdown_cache_validation' => true
+        })
       end
+    end
+
+    context 'when content needs a rewrite' do
+      it 'calls the rewriter classes successfully', :aggregate_failures do
+        described_class::REWRITERS.each do |rewriter_class|
+          service = double
 
-      subject
+          allow(service).to receive(:needs_rewrite?).and_return(true)
+
+          expect(service).to receive(:rewrite).with(target_parent)
+          expect(rewriter_class).to receive(:new).and_return(service)
+        end
+
+        subject
+      end
     end
 
     # Perform simple integration-style tests for each rewriter class.
     # to prove they run correctly.
-    context 'when content contains a reference' do
-      let_it_be(:issue) { create(:issue, project: source_parent) }
+    context 'when content has references' do
+      let_it_be(:issue_to_reference) { create(:issue, project: source_parent) }
 
-      let(:content) { "See ##{issue.iid}" }
+      let(:content) { "See ##{issue_to_reference.iid}" }
 
       it 'rewrites content' do
-        expect(subject).to eq("See #{source_parent.full_path}##{issue.iid}")
+        expect(subject).to eq({
+          'description' => "See #{source_parent.full_path}##{issue_to_reference.iid}",
+          'description_html' => nil,
+          'skip_markdown_cache_validation' => false
+        })
       end
     end
 
@@ -50,9 +75,37 @@ RSpec.describe MarkdownContentRewriterService do
       it 'rewrites content' do
         new_content = subject
 
-        expect(new_content).not_to eq(content)
-        expect(new_content.length).to eq(content.length)
+        expect(new_content[:description]).not_to eq(content)
+        expect(new_content[:description].length).to eq(content.length)
+        expect(new_content[1]).to eq(nil)
       end
     end
   end
+
+  describe '#safe_to_copy_markdown?' do
+    subject do
+      rewriter = described_class.new(user, issue, :description, source_parent, target_parent)
+      rewriter.safe_to_copy_markdown?
+    end
+
+    context 'when content has references' do
+      let(:milestone) { create(:milestone, project: source_parent) }
+      let(:content) { "Description that references #{milestone.to_reference}" }
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when content has uploaded file references' do
+      let(:image_uploader) { build(:file_uploader, project: source_parent) }
+      let(:content) { "Text and #{image_uploader.markdown_link}" }
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when content does not have references or uploads' do
+      let(:content) { "simples text with ```code```" }
+
+      it { is_expected.to eq(true) }
+    end
+  end
 end
diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb
index f7fbac612ee..d26bab7bb0a 100644
--- a/spec/services/members/approve_access_request_service_spec.rb
+++ b/spec/services/members/approve_access_request_service_spec.rb
@@ -9,36 +9,34 @@ RSpec.describe Members::ApproveAccessRequestService do
   let(:access_requester_user) { create(:user) }
   let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) }
   let(:opts) { {} }
-
-  shared_examples 'a service raising ActiveRecord::RecordNotFound' do
-    it 'raises ActiveRecord::RecordNotFound' do
-      expect { described_class.new(current_user).execute(access_requester, **opts) }.to raise_error(ActiveRecord::RecordNotFound)
-    end
-  end
+  let(:params) { {} }
+  let(:custom_access_level) { Gitlab::Access::MAINTAINER }
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(current_user).execute(access_requester, **opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(current_user, params).execute(access_requester, **opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service approving an access request' do
     it 'succeeds' do
-      expect { described_class.new(current_user).execute(access_requester, **opts) }.to change { source.requesters.count }.by(-1)
+      expect { described_class.new(current_user, params).execute(access_requester, **opts) }.to change { source.requesters.count }.by(-1)
     end
 
     it 'returns a <Source>Member' do
-      member = described_class.new(current_user).execute(access_requester, **opts)
+      member = described_class.new(current_user, params).execute(access_requester, **opts)
 
       expect(member).to be_a "#{source.class}Member".constantize
       expect(member.requested_at).to be_nil
     end
 
     context 'with a custom access level' do
+      let(:params) { { access_level: custom_access_level } }
+
       it 'returns a ProjectMember with the custom access level' do
-        member = described_class.new(current_user, access_level: Gitlab::Access::MAINTAINER).execute(access_requester, **opts)
+        member = described_class.new(current_user, params).execute(access_requester, **opts)
 
-        expect(member.access_level).to eq(Gitlab::Access::MAINTAINER)
+        expect(member.access_level).to eq(custom_access_level)
       end
     end
   end
@@ -111,5 +109,38 @@ RSpec.describe Members::ApproveAccessRequestService do
         let(:source) { group }
       end
     end
+
+    context 'in a project' do
+      let_it_be(:group_project) { create(:project, :public, group: create(:group, :public)) }
+
+      let(:source) { group_project }
+      let(:custom_access_level) { Gitlab::Access::OWNER }
+      let(:params) { { access_level: custom_access_level } }
+
+      before do
+        group_project.request_access(access_requester_user)
+      end
+
+      context 'maintainers' do
+        before do
+          group_project.add_maintainer(current_user)
+        end
+
+        context 'cannot approve the access request of a requester to give them OWNER permissions' do
+          it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
+        end
+      end
+
+      context 'owners' do
+        before do
+          # so that `current_user` is considered an `OWNER` in the project via inheritance.
+          group_project.group.add_owner(current_user)
+        end
+
+        context 'can approve the access request of a requester to give them OWNER permissions' do
+          it_behaves_like 'a service approving an access request'
+        end
+      end
+    end
   end
 end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 3e2cccfb350..a2d73d8c9b1 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -161,7 +161,7 @@ RSpec.describe MergeRequests::MergeService do
       end
 
       context 'with Jira integration' do
-        include JiraServiceHelper
+        include JiraIntegrationHelpers
 
         let(:jira_tracker) { project.create_jira_integration }
         let(:jira_issue)   { ExternalIssue.new('JIRA-123', project) }
diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb
index dd11fa15ea8..fd8802e6640 100644
--- a/spec/services/notes/copy_service_spec.rb
+++ b/spec/services/notes/copy_service_spec.rb
@@ -16,9 +16,8 @@ RSpec.describe Notes::CopyService do
     let_it_be(:group) { create(:group) }
     let_it_be(:from_project) { create(:project, :public, group: group) }
     let_it_be(:to_project) { create(:project, :public, group: group) }
-
-    let(:from_noteable) { create(:issue, project: from_project) }
-    let(:to_noteable) { create(:issue, project: to_project) }
+    let_it_be(:from_noteable) { create(:issue, project: from_project) }
+    let_it_be(:to_noteable) { create(:issue, project: to_project) }
 
     subject(:execute_service) { described_class.new(user, from_noteable, to_noteable).execute }
 
@@ -85,6 +84,15 @@ RSpec.describe Notes::CopyService do
             expect(execute_service).to be_success
           end
         end
+
+        it 'copies rendered markdown from note_html' do
+          expect(Banzai::Renderer).not_to receive(:cacheless_render_field)
+
+          execute_service
+
+          new_note = to_noteable.notes.first
+          expect(new_note.note_html).to eq(notes.first.note_html)
+        end
       end
 
       context 'notes with mentions' do
@@ -119,6 +127,13 @@ RSpec.describe Notes::CopyService do
             expect(new_note.author).to eq(note.author)
           end
         end
+
+        it 'does not copy rendered markdown from note_html' do
+          execute_service
+
+          new_note = to_noteable.notes.first
+          expect(new_note.note_html).not_to eq(note.note_html)
+        end
       end
 
       context 'notes with upload' do
@@ -137,6 +152,13 @@ RSpec.describe Notes::CopyService do
             expect(note.note_html).not_to eq(new_note.note_html)
           end
         end
+
+        it 'does not copy rendered markdown from note_html' do
+          execute_service
+
+          new_note = to_noteable.notes.first
+          expect(new_note.note_html).not_to eq(note.note_html)
+        end
       end
 
       context 'discussion notes' do