Merge branch 'dm-submodule-helper-routing' into 'master'

Ensure SubmoduleHelper works outside view context

Closes #64833

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

(cherry picked from commit 8b284a51)

26ac53b3 Ensure SubmoduleHelper works outside view context

app/helpers/submodule_helper.rb

@@ -34,8 +34,8 @@ module SubmoduleHelper
       project.sub!(/\.git\z/, '')
       if self_url?(url, namespace, project)
-        [namespace_project_path(namespace, project),
-         namespace_project_tree_path(namespace, project, submodule_item_id)]
+        [url_helpers.namespace_project_path(namespace, project),
+         url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id)]
       elsif relative_self_url?(url)
         relative_self_links(url, submodule_item_id, repository.project)
       elsif github_dot_com_url?(url)
@@ -99,8 +99,8 @@ module SubmoduleHelper
     begin
       [
-        namespace_project_path(target_namespace_path, submodule_base),
-        namespace_project_tree_path(target_namespace_path, submodule_base, commit)
+        url_helpers.namespace_project_path(target_namespace_path, submodule_base),
+        url_helpers.namespace_project_tree_path(target_namespace_path, submodule_base, commit)
       ]
     rescue ActionController::UrlGenerationError
       [nil, nil]
@@ -118,4 +118,8 @@ module SubmoduleHelper
   rescue URI::InvalidURIError
     nil
   end
+
+  def url_helpers
+    Gitlab::Routing.url_helpers
+  end
 end

changelogs/unreleased/dm-submodule-helper-routing.yml

+---
+title: Fix bug that caused diffs not to show on MRs with changes to submodules
+merge_request:
+author:
+type: fixed

spec/helpers/submodule_helper_spec.rb

@@ -3,15 +3,11 @@ require 'spec_helper'
 describe SubmoduleHelper do
   include RepoHelpers
-  describe 'submodule links' do
-    let(:submodule_item) { double(id: 'hash', path: 'rack') }
-    let(:config) { Gitlab.config.gitlab }
-    let(:repo) { double }
-
-    before do
-      self.instance_variable_set(:@repository, repo)
-    end
+  let(:submodule_item) { double(id: 'hash', path: 'rack') }
+  let(:config) { Gitlab.config.gitlab }
+  let(:repo) { double }
+  shared_examples 'submodule_links' do
     context 'submodule on self' do
       before do
         allow(Gitlab.config.gitlab).to receive(:protocol).and_return('http') # set this just to be sure
@@ -21,28 +17,28 @@ describe SubmoduleHelper do
         allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(22) # set this just to be sure
         allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
         stub_url([config.user, '@', config.host, ':gitlab-org/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
       end
       it 'detects ssh on non-standard port' do
         allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(2222)
         allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
         stub_url(['ssh://', config.user, '@', config.host, ':2222/gitlab-org/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
       end
       it 'detects http on standard port' do
         allow(Gitlab.config.gitlab).to receive(:port).and_return(80)
         allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
         stub_url(['http://', config.host, '/gitlab-org/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
       end
       it 'detects http on non-standard port' do
         allow(Gitlab.config.gitlab).to receive(:port).and_return(3000)
         allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
         stub_url(['http://', config.host, ':3000/gitlab-org/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
       end
       it 'works with relative_url_root' do
@@ -50,7 +46,7 @@ describe SubmoduleHelper do
         allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root')
         allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
         stub_url(['http://', config.host, '/gitlab/root/gitlab-org/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-ce'), namespace_project_tree_path('gitlab-org', 'gitlab-ce', 'hash')])
       end
       it 'works with subgroups' do
@@ -58,34 +54,34 @@ describe SubmoduleHelper do
         allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root')
         allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
         stub_url(['http://', config.host, '/gitlab/root/gitlab-org/sub/gitlab-ce.git'].join(''))
-        expect(submodule_links(submodule_item)).to eq([namespace_project_path('gitlab-org/sub', 'gitlab-ce'), namespace_project_tree_path('gitlab-org/sub', 'gitlab-ce', 'hash')])
+        expect(subject).to eq([namespace_project_path('gitlab-org/sub', 'gitlab-ce'), namespace_project_tree_path('gitlab-org/sub', 'gitlab-ce', 'hash')])
       end
     end
     context 'submodule on github.com' do
       it 'detects ssh' do
         stub_url('git@github.com:gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'detects http' do
         stub_url('http://github.com/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'detects https' do
         stub_url('https://github.com/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'handles urls with no .git on the end' do
         stub_url('http://github.com/gitlab-org/gitlab-ce')
-        expect(submodule_links(submodule_item)).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://github.com/gitlab-org/gitlab-ce', 'https://github.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'returns original with non-standard url' do
         stub_url('http://github.com/another/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil])
+        expect(subject).to eq([repo.submodule_url_for, nil])
       end
     end
@@ -97,39 +93,39 @@ describe SubmoduleHelper do
         allow(repo).to receive(:project).and_return(project)
         stub_url('./')
-        expect(submodule_links(submodule_item)).to eq(["/master-project/#{project.path}", "/master-project/#{project.path}/tree/hash"])
+        expect(subject).to eq(["/master-project/#{project.path}", "/master-project/#{project.path}/tree/hash"])
       end
     end
     context 'submodule on gitlab.com' do
       it 'detects ssh' do
         stub_url('git@gitlab.com:gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'detects http' do
         stub_url('http://gitlab.com/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'detects https' do
         stub_url('https://gitlab.com/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'handles urls with no .git on the end' do
         stub_url('http://gitlab.com/gitlab-org/gitlab-ce')
-        expect(submodule_links(submodule_item)).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'handles urls with trailing whitespace' do
         stub_url('http://gitlab.com/gitlab-org/gitlab-ce.git  ')
-        expect(submodule_links(submodule_item)).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
+        expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-ce', 'https://gitlab.com/gitlab-org/gitlab-ce/tree/hash'])
       end
       it 'returns original with non-standard url' do
         stub_url('http://gitlab.com/another/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil])
+        expect(subject).to eq([repo.submodule_url_for, nil])
       end
     end
@@ -137,27 +133,25 @@ describe SubmoduleHelper do
       it 'sanitizes unsupported protocols' do
         stub_url('javascript:alert("XSS");')
-        expect(helper.submodule_links(submodule_item)).to eq([nil, nil])
+        expect(subject).to eq([nil, nil])
       end
       it 'sanitizes unsupported protocols disguised as a repository URL' do
         stub_url('javascript:alert("XSS");foo/bar.git')
-        expect(helper.submodule_links(submodule_item)).to eq([nil, nil])
+        expect(subject).to eq([nil, nil])
       end
       it 'sanitizes invalid URL with extended ASCII' do
         stub_url('é')
-        expect(helper.submodule_links(submodule_item)).to eq([nil, nil])
+        expect(subject).to eq([nil, nil])
       end
       it 'returns original' do
         stub_url('http://mygitserver.com/gitlab-org/gitlab-ce')
-        expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil])
-        stub_url('http://mygitserver.com/gitlab-org/gitlab-ce.git')
-        expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil])
+        expect(subject).to eq([repo.submodule_url_for, nil])
       end
     end
@@ -168,8 +162,7 @@ describe SubmoduleHelper do
       def expect_relative_link_to_resolve_to(relative_path, expected_path)
         allow(repo).to receive(:submodule_url_for).and_return(relative_path)
-
-        result = submodule_links(submodule_item)
+        result = subject
         expect(result).to eq([expected_path, "#{expected_path}/tree/#{submodule_item.id}"])
       end
@@ -190,7 +183,7 @@ describe SubmoduleHelper do
         it 'returns nil' do
           allow(repo).to receive(:submodule_url_for).and_return('../../test.git')
-          result = submodule_links(submodule_item)
+          result = subject
           expect(result).to eq([nil, nil])
         end
@@ -200,7 +193,7 @@ describe SubmoduleHelper do
         it 'returns nil because it is not possible to have repo nested under another repo' do
           allow(repo).to receive(:submodule_url_for).and_return('./test.git')
-          result = submodule_links(submodule_item)
+          result = subject
           expect(result).to eq([nil, nil])
         end
@@ -238,6 +231,22 @@ describe SubmoduleHelper do
     end
   end
+  context 'as view helpers in view context' do
+    subject { helper.submodule_links(submodule_item) }
+
+    before do
+      self.instance_variable_set(:@repository, repo)
+    end
+
+    it_behaves_like 'submodule_links'
+  end
+
+  context 'as stand-alone module' do
+    subject { described_class.submodule_links(submodule_item, nil, repo) }
+
+    it_behaves_like 'submodule_links'
+  end
+
   def stub_url(url)
     allow(repo).to receive(:submodule_url_for).and_return(url)
   end