Merge branch 'fix-merge-request-that-removes-submodule' into 'master'

Fix 500 error when creating a merge request that removes a submodule

Fixes #3476

See merge request !1989

5 files changed, 50 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b00b8fb2774..bdf636ae1fa 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -6,6 +6,7 @@ v 8.2.3
   - Update documentation for "Guest" permissions
   - Properly convert Emoji-only comments into Award Emojis
   - Webhook payload has an added, modified and removed properties for each commit
+  - Fix 500 error when creating a merge request that removes a submodule
 
 v 8.2.2
   - Fix 404 in redirection after removing a project (Stan Hu)
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index c745b4e69bf..ba4353e5f09 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -2,8 +2,9 @@
   .diff-header{id: "file-path-#{hexdigest(diff_file.file_path)}"}
     - if diff_file.diff.submodule?
       %span
-        - submodule_item = project.repository.blob_at(@commit.id, diff_file.file_path)
-        = submodule_link(submodule_item, @commit.id, project.repository)
+        = icon('archive fw')
+        %strong
+          = submodule_link(blob, @commit.id, project.repository)
     - else
       %span
         - if diff_file.deleted_file
diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb
index bb3d87f3840..ab7c7a24065 100644
--- a/spec/controllers/commit_controller_spec.rb
+++ b/spec/controllers/commit_controller_spec.rb
@@ -95,6 +95,26 @@ describe Projects::CommitController do
         expect(response.body).to match(/^diff --git/)
       end
     end
+
+    context 'commit that removes a submodule' do
+      render_views
+
+      let(:fork_project) { create(:forked_project_with_submodules) }
+      let(:commit) { fork_project.commit('remove-submodule') }
+
+      before do
+        fork_project.team << [user, :master]
+      end
+
+      it 'renders it' do
+        get(:show,
+            namespace_id: fork_project.namespace.to_param,
+            project_id: fork_project.to_param,
+            id: commit.id)
+
+        expect(response).to be_success
+      end
+    end
   end
 
   describe "#branches" do
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 3e5e1fa87ae..6aaec224f6e 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -10,6 +10,30 @@ describe Projects::MergeRequestsController do
     project.team << [user, :master]
   end
 
+  describe '#new' do
+    context 'merge request that removes a submodule' do
+      render_views
+
+      let(:fork_project) { create(:forked_project_with_submodules) }
+
+      before do
+        fork_project.team << [user, :master]
+      end
+
+      it 'renders it' do
+        get :new,
+            namespace_id: fork_project.namespace.to_param,
+            project_id: fork_project.to_param,
+            merge_request: {
+              source_branch: 'remove-submodule',
+              target_branch: 'master'
+            }
+
+        expect(response).to be_success
+      end
+    end
+  end
+
   describe "#show" do
     shared_examples "export merge as" do |format|
       it "should generally work" do
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 787670e9297..78b9a0f42fa 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -21,7 +21,8 @@ module TestEnv
   # We currently only need a subset of the branches
   FORKED_BRANCH_SHA = {
     'add-submodule-version-bump' => '3f547c08',
-    'master' => '5937ac0'
+    'master' => '5937ac0',
+    'remove-submodule' => '2a33e0c0'
   }
 
   # Test environment