Merge branch '10-3-stable-patch-3' into '10-3-stable'

Prepare 10.3.3 release

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

10 files changed, 108 insertions, 22 deletions

diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index d67b16584a4..bd6af622bfb 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -23,8 +23,13 @@ class DiffDiscussion < Discussion
   def merge_request_version_params
     return unless for_merge_request?
 
+    version_params = get_params
+
+    return version_params unless on_merge_request_commit? && commit_id
+
+    version_params ||= {}
     version_params.tap do |params|
-      params[:commit_id] = commit_id if on_merge_request_commit?
+      params[:commit_id] = commit_id
     end
   end
 
@@ -37,7 +42,7 @@ class DiffDiscussion < Discussion
 
   private
 
-  def version_params
+  def get_params
     return {} if active?
 
     noteable.version_params_for(position.diff_refs)
diff --git a/app/views/shared/_recaptcha_form.html.haml b/app/views/shared/_recaptcha_form.html.haml
index 0e816870f15..93a4301f366 100644
--- a/app/views/shared/_recaptcha_form.html.haml
+++ b/app/views/shared/_recaptcha_form.html.haml
@@ -1,9 +1,10 @@
 - resource_name = spammable.class.model_name.singular
 - humanized_resource_name = spammable.class.model_name.human.downcase
 - script = local_assigns.fetch(:script, true)
+- method = params[:action] == 'create' ? :post : :put
 - has_submit = local_assigns.fetch(:has_submit, true)
 
-= form_for resource_name, method: :post, html: { class: 'recaptcha-form js-recaptcha-form' } do |f|
+= form_for resource_name, method: method, html: { class: 'recaptcha-form js-recaptcha-form' } do |f|
   .recaptcha
     - params[resource_name].each do |field, value|
       = hidden_field(resource_name, field, value: value)
diff --git a/changelogs/unreleased/41492-mr-comment-fix.yml b/changelogs/unreleased/41492-mr-comment-fix.yml
new file mode 100644
index 00000000000..45ddc16aad0
--- /dev/null
+++ b/changelogs/unreleased/41492-mr-comment-fix.yml
@@ -0,0 +1,5 @@
+---
+title: Fix links to old commits in merge request comments
+merge_request:
+author:
+type: fixed
diff --git a/changelogs/unreleased/sh-fix-spam-update-404.yml b/changelogs/unreleased/sh-fix-spam-update-404.yml
new file mode 100644
index 00000000000..13daec35ecf
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-spam-update-404.yml
@@ -0,0 +1,5 @@
+---
+title: Fix 404 errors after a user edits an issue description and solves the reCAPTCHA
+merge_request:
+author:
+type: fixed
diff --git a/changelogs/unreleased/sh-handle-orphaned-deploy-keys.yml b/changelogs/unreleased/sh-handle-orphaned-deploy-keys.yml
new file mode 100644
index 00000000000..7d3b622534e
--- /dev/null
+++ b/changelogs/unreleased/sh-handle-orphaned-deploy-keys.yml
@@ -0,0 +1,5 @@
+---
+title: Gracefully handle orphaned write deploy keys in /internal/post_receive
+merge_request:
+author:
+type: fixed
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index ccaaeca10d4..79b302aae70 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -190,9 +190,12 @@ module API
 
         project = Gitlab::GlRepository.parse(params[:gl_repository]).first
         user = identify(params[:identifier])
-        redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)
-        if redirect_message
-          output[:redirected_message] = redirect_message
+
+        # A user is not guaranteed to be returned; an orphaned write deploy
+        # key could be used
+        if user
+          redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)
+          output[:redirected_message] = redirect_message if redirect_message
         end
 
         output
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index fbdd646ea13..da67683fd15 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -104,7 +104,7 @@ module Gitlab
       end
 
       def exists?
-        Gitlab::GitalyClient.migrate(:repository_exists, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |enabled|
+        Gitlab::GitalyClient.migrate(:repository_exists) do |enabled|
           if enabled
             gitaly_repository_client.exists?
           else
@@ -166,7 +166,7 @@ module Gitlab
       end
 
       def local_branches(sort_by: nil)
-        gitaly_migrate(:local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
+        gitaly_migrate(:local_branches) do |is_enabled|
           if is_enabled
             gitaly_ref_client.local_branches(sort_by: sort_by)
           else
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
index fa02434b0fd..50b19000799 100644
--- a/spec/models/diff_discussion_spec.rb
+++ b/spec/models/diff_discussion_spec.rb
@@ -47,8 +47,20 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
-      it 'returns the diff ID for the version to show' do
-        expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id)
+      context 'when commit_id is not present' do
+        it 'returns the diff ID for the version to show' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id)
+        end
+      end
+
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'includes the commit_id in the result' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id, commit_id: 'commit_123')
+        end
       end
     end
 
@@ -70,8 +82,20 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
-      it 'returns the diff ID and start sha of the versions to compare' do
-        expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
+      context 'when commit_id is not present' do
+        it 'returns the diff ID and start sha of the versions to compare' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
+        end
+      end
+
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'includes the commit_id in the result' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha, commit_id: 'commit_123')
+        end
       end
     end
 
@@ -83,8 +107,20 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
-      it 'returns nil' do
-        expect(subject.merge_request_version_params).to be_nil
+      context 'when commit_id is not present' do
+        it 'returns empty hash' do
+          expect(subject.merge_request_version_params).to eq(nil)
+        end
+      end
+
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'returns the commit_id' do
+          expect(subject.merge_request_version_params).to eq(commit_id: 'commit_123')
+        end
       end
     end
   end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 3c31980b273..c352285b1bf 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -784,6 +784,16 @@ describe API::Internal do
         expect(json_response["redirected_message"]).to eq(project_moved.redirect_message)
       end
     end
+
+    context 'with an orphaned write deploy key' do
+      it 'does not try to notify that project moved' do
+        allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil)
+
+        post api("/internal/post_receive"), valid_params
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
   end
 
   describe 'POST /internal/pre_receive' do
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 47412110b4b..17f4136c2b0 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe SystemNoteService do
   include Gitlab::Routing
+  include RepoHelpers
 
   set(:group)    { create(:group) }
   set(:project)  { create(:project, :repository, group: group) }
@@ -1070,17 +1071,32 @@ describe SystemNoteService do
       let(:action)            { 'outdated' }
     end
 
-    it 'creates a new note in the discussion' do
-      # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
-      expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+    context 'when the change_position is valid for the discussion' do
+      it 'creates a new note in the discussion' do
+        # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+        expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'links to the diff in the system note' do
+        expect(subject.note).to include('version 1')
+
+        diff_id = merge_request.merge_request_diff.id
+        line_code = change_position.line_code(project.repository)
+        expect(subject.note).to include(diffs_project_merge_request_url(project, merge_request, diff_id: diff_id, anchor: line_code))
+      end
     end
 
-    it 'links to the diff in the system note' do
-      expect(subject.note).to include('version 1')
+    context 'when the change_position is invalid for the discussion' do
+      let(:change_position) { project.commit(sample_commit.id) }
 
-      diff_id = merge_request.merge_request_diff.id
-      line_code = change_position.line_code(project.repository)
-      expect(subject.note).to include(diffs_project_merge_request_url(project, merge_request, diff_id: diff_id, anchor: line_code))
+      it 'creates a new note in the discussion' do
+        # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+        expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'does not create a link' do
+        expect(subject.note).to eq('changed this line in version 1 of the diff')
+      end
     end
   end