Merge branch 'fj-205646-fix-project-moved-message' into 'master'

Fix project moved message after git operation

Closes #205646

See merge request gitlab-org/gitlab!27341

changelogs/unreleased/fj-205646-fix-project-moved-message.yml

+---
+title: Fix project moved message after git operation
+merge_request: 27341
+author:
+type: fixed

ee/lib/ee/gitlab/repo_path.rb

@@ -8,7 +8,7 @@ module EE
           return super unless License.feature_available?(:project_aliases)
 
           if project_alias = ProjectAlias.find_by_name(project_path)
-            [project_alias.project, false]
+            [project_alias.project, nil]
           else
             super
           end

ee/spec/lib/ee/gitlab/repo_path_spec.rb

@@ -25,14 +25,14 @@ describe Gitlab::RepoPath do
         let(:project_alias) { create(:project_alias, project: project) }
 
         it 'returns the project' do
-          expect(described_class.find_project(project_alias.name)).to eq([project, false])
+          expect(described_class.find_project(project_alias.name)).to eq([project, nil])
         end
       end
 
       context 'project_path does not match a project alias' do
         context 'project path matches project full path' do
           it 'returns the project' do
-            expect(described_class.find_project(project.full_path)).to eq([project, false])
+            expect(described_class.find_project(project.full_path)).to eq([project, nil])
           end
         end
 

lib/gitlab/checks/post_push_message.rb

@@ -3,8 +3,8 @@
 module Gitlab
   module Checks
     class PostPushMessage
-      def initialize(project, user, protocol)
-        @project = project
+      def initialize(repository, user, protocol)
+        @repository = repository
         @user = user
         @protocol = protocol
       end
@@ -34,14 +34,21 @@ module Gitlab
 
       protected
 
-      attr_reader :project, :user, :protocol
+      attr_reader :repository, :user, :protocol
+
+      delegate :project, to: :repository, allow_nil: true
+      delegate :container, to: :repository, allow_nil: false
 
       def self.message_key(user_id, project_id)
         raise NotImplementedError
       end
 
       def url_to_repo
-        protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo
+        protocol == 'ssh' ? message_subject.ssh_url_to_repo : message_subject.http_url_to_repo
+      end
+
+      def message_subject
+        repository.repo_type.wiki? ? project.wiki : container
       end
     end
   end

lib/gitlab/checks/project_moved.rb

@@ -5,10 +5,10 @@ module Gitlab
     class ProjectMoved < PostPushMessage
       REDIRECT_NAMESPACE = "redirect_namespace"
 
-      def initialize(project, user, protocol, redirected_path)
+      def initialize(repository, user, protocol, redirected_path)
         @redirected_path = redirected_path
 
-        super(project, user, protocol)
+        super(repository, user, protocol)
       end
 
       def message

lib/gitlab/git_access.rb

@@ -188,7 +188,7 @@ module Gitlab
     def add_project_moved_message!
       return if redirected_path.nil?
 
-      project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path)
+      project_moved = Checks::ProjectMoved.new(repository, user, protocol, redirected_path)
 
       project_moved.add_message
     end
@@ -250,7 +250,7 @@ module Gitlab
       @project = project
       user_access.project = @project
 
-      Checks::ProjectCreated.new(project, user, protocol).add_message
+      Checks::ProjectCreated.new(repository, user, protocol).add_message
     end
 
     def check_repository_existence!

lib/gitlab/git_access_snippet.rb

@@ -39,11 +39,11 @@ module Gitlab
 
     override :check_project!
     def check_project!(cmd, changes)
-      if snippet.is_a?(ProjectSnippet)
+      return unless snippet.is_a?(ProjectSnippet)
+
       check_namespace!
       check_project_accessibility!
-        # TODO add add_project_moved_message! to handle non-project repo https://gitlab.com/gitlab-org/gitlab/issues/205646
-      end
+      add_project_moved_message!
     end
 
     override :check_push_access!

lib/gitlab/repo_path.rb

@@ -19,8 +19,7 @@ module Gitlab
 
         # Removing the suffix (.wiki, .design, ...) from the project path
         full_path = repo_path.chomp(type.path_suffix)
-        container, project, was_redirected = find_container(type, full_path)
-        redirected_path = repo_path if was_redirected
+        container, project, redirected_path = find_container(type, full_path)
 
         return [container, project, type, redirected_path] if container
       end
@@ -33,22 +32,23 @@ module Gitlab
 
     def self.find_container(type, full_path)
       if type.snippet?
-        snippet, was_redirected = find_snippet(full_path)
+        snippet, redirected_path = find_snippet(full_path)
 
-        [snippet, snippet&.project, was_redirected]
+        [snippet, snippet&.project, redirected_path]
       else
-        project, was_redirected = find_project(full_path)
+        project, redirected_path = find_project(full_path)
 
-        [project, project, was_redirected]
+        [project, project, redirected_path]
       end
     end
 
     def self.find_project(project_path)
-      return [nil, false] if project_path.blank?
+      return [nil, nil] if project_path.blank?
 
       project = Project.find_by_full_path(project_path, follow_redirects: true)
+      redirected_path = redirected?(project, project_path) ? project_path : nil
 
-      [project, redirected?(project, project_path)]
+      [project, redirected_path]
     end
 
     def self.redirected?(project, project_path)
@@ -59,12 +59,12 @@ module Gitlab
     # - snippets/1
     # - h5bp/html5-boilerplate/snippets/53
     def self.find_snippet(snippet_path)
-      return [nil, false] if snippet_path.blank?
+      return [nil, nil] if snippet_path.blank?
 
       snippet_id, project_path = extract_snippet_info(snippet_path)
-      project, was_redirected = find_project(project_path)
+      project, redirected_path = find_project(project_path)
 
-      [Snippet.find_by_id_and_project(id: snippet_id, project: project), was_redirected]
+      [Snippet.find_by_id_and_project(id: snippet_id, project: project), redirected_path]
     end
 
     def self.extract_snippet_info(snippet_path)

spec/lib/gitlab/checks/project_created_spec.rb

@@ -3,24 +3,29 @@
 require 'spec_helper'
 
 describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:protocol) { 'http' }
+  let(:git_user) { user }
+  let(:repository) { project.repository }
+
+  subject { described_class.new(repository, git_user, 'http') }
 
   describe '.fetch_message' do
     context 'with a project created message queue' do
-      let(:project_created) { described_class.new(project, user, 'http') }
-
       before do
-        project_created.add_message
+        subject.add_message
       end
 
       it 'returns project created message' do
-        expect(described_class.fetch_message(user.id, project.id)).to eq(project_created.message)
+        expect(described_class.fetch_message(user.id, project.id)).to eq(subject.message)
       end
 
       it 'deletes the project created message from redis' do
         expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).not_to be_nil
+
         described_class.fetch_message(user.id, project.id)
+
         expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).to be_nil
       end
     end
@@ -34,15 +39,15 @@ describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state do
 
   describe '#add_message' do
     it 'queues a project created message' do
-      project_created = described_class.new(project, user, 'http')
-
-      expect(project_created.add_message).to eq('OK')
+      expect(subject.add_message).to eq('OK')
     end
 
-    it 'handles anonymous push' do
-      project_created = described_class.new(nil, user, 'http')
+    context 'when user is nil' do
+      let(:git_user) { nil }
 
-      expect(project_created.add_message).to be_nil
+      it 'handles anonymous push' do
+        expect(subject.add_message).to be_nil
+      end
     end
   end
 end

spec/lib/gitlab/checks/project_moved_spec.rb

@@ -3,24 +3,30 @@
 require 'spec_helper'
 
 describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:protocol) { 'http' }
+  let(:git_user) { user }
+  let(:redirect_path) { 'foo/bar' }
+
+  subject { described_class.new(repository, git_user, protocol, redirect_path) }
 
   describe '.fetch_message' do
     context 'with a redirect message queue' do
-      it 'returns the redirect message' do
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        project_moved.add_message
+      before do
+        subject.add_message
+      end
 
-        expect(described_class.fetch_message(user.id, project.id)).to eq(project_moved.message)
+      it 'returns the redirect message' do
+        expect(described_class.fetch_message(user.id, project.id)).to eq(subject.message)
       end
 
       it 'deletes the redirect message from redis' do
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        project_moved.add_message
-
         expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).not_to be_nil
+
         described_class.fetch_message(user.id, project.id)
+
         expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).to be_nil
       end
     end
@@ -34,29 +40,82 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
 
   describe '#add_message' do
     it 'queues a redirect message' do
-      project_moved = described_class.new(project, user, 'http', 'foo/bar')
-      expect(project_moved.add_message).to eq("OK")
+      expect(subject.add_message).to eq("OK")
     end
 
-    it 'handles anonymous clones' do
-      project_moved = described_class.new(project, nil, 'http', 'foo/bar')
+    context 'when user is nil' do
+      let(:git_user) { nil }
 
-      expect(project_moved.add_message).to eq(nil)
+      it 'handles anonymous clones' do
+        expect(subject.add_message).to be_nil
+      end
     end
   end
 
   describe '#message' do
-    it 'returns a redirect message' do
-      project_moved = described_class.new(project, user, 'http', 'foo/bar')
+    shared_examples 'errors per protocol' do
+      shared_examples 'returns redirect message' do
+        it do
           message = <<~MSG
-                Project 'foo/bar' was moved to '#{project.full_path}'.
+                    Project '#{redirect_path}' was moved to '#{project.full_path}'.
 
                     Please update your Git remote:
 
-                  git remote set-url origin #{project.http_url_to_repo}
+                      git remote set-url origin #{url_to_repo}
                     MSG
 
-      expect(project_moved.message).to eq(message)
+          expect(subject.message).to eq(message)
+        end
+      end
+
+      context 'when protocol is http' do
+        it_behaves_like 'returns redirect message' do
+          let(:url_to_repo) { http_url_to_repo }
+        end
+      end
+
+      context 'when protocol is ssh' do
+        let(:protocol) { 'ssh' }
+
+        it_behaves_like 'returns redirect message' do
+          let(:url_to_repo) { ssh_url_to_repo }
+        end
+      end
+    end
+
+    context 'with project' do
+      it_behaves_like 'errors per protocol' do
+        let(:http_url_to_repo) { project.http_url_to_repo }
+        let(:ssh_url_to_repo) { project.ssh_url_to_repo }
+      end
+    end
+
+    context 'with wiki' do
+      let(:repository) { project.wiki.repository }
+
+      it_behaves_like 'errors per protocol' do
+        let(:http_url_to_repo) { project.wiki.http_url_to_repo }
+        let(:ssh_url_to_repo) { project.wiki.ssh_url_to_repo }
+      end
+    end
+
+    context 'with project snippet' do
+      let_it_be(:snippet) { create(:project_snippet, :repository, project: project, author: user) }
+      let(:repository) { snippet.repository }
+
+      it_behaves_like 'errors per protocol' do
+        let(:http_url_to_repo) { snippet.http_url_to_repo }
+        let(:ssh_url_to_repo) { snippet.ssh_url_to_repo }
+      end
+    end
+
+    context 'with personal snippet' do
+      let_it_be(:snippet) { create(:personal_snippet, :repository, author: user) }
+      let(:repository) { snippet.repository }
+
+      it 'returns nil' do
+        expect(subject.add_message).to be_nil
+      end
     end
   end
 end

spec/lib/gitlab/repo_path_spec.rb

@@ -8,7 +8,8 @@ describe ::Gitlab::RepoPath do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:personal_snippet) { create(:personal_snippet) }
   let_it_be(:project_snippet) { create(:project_snippet, project: project) }
-  let_it_be(:redirect) { project.route.create_redirect('foo/bar/baz') }
+  let_it_be(:redirect_route) { 'foo/bar/baz' }
+  let_it_be(:redirect) { project.route.create_redirect(redirect_route) }
 
   describe '.parse' do
     context 'a repository storage path' do
@@ -43,22 +44,20 @@ describe ::Gitlab::RepoPath do
       end
 
       context 'of a redirected project' do
-        let(:redirect) { project.route.create_redirect('foo/bar') }
-
         it 'parses a relative repository path' do
-          expect(described_class.parse(redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, 'foo/bar'])
+          expect(described_class.parse(redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, redirect_route])
         end
 
         it 'parses a relative wiki path' do
-          expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, project, Gitlab::GlRepository::WIKI, 'foo/bar.wiki'])
+          expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, project, Gitlab::GlRepository::WIKI, redirect_route])
         end
 
         it 'parses a relative path starting with /' do
-          expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, 'foo/bar'])
+          expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, redirect_route])
         end
 
         it 'parses a redirected project snippet repository path' do
-          expect(described_class.parse(redirect.path + "/snippets/#{project_snippet.id}.git")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, "foo/bar/snippets/#{project_snippet.id}"])
+          expect(described_class.parse(redirect.path + "/snippets/#{project_snippet.id}.git")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, redirect_route])
         end
       end
     end
@@ -71,8 +70,8 @@ describe ::Gitlab::RepoPath do
   describe '.find_project' do
     context 'when finding a project by its canonical path' do
       context 'when the cases match' do
-        it 'returns the project and false' do
-          expect(described_class.find_project(project.full_path)).to eq([project, false])
+        it 'returns the project and nil' do
+          expect(described_class.find_project(project.full_path)).to eq([project, nil])
         end
       end
 
@@ -81,45 +80,45 @@ describe ::Gitlab::RepoPath do
         # easy and safe to redirect someone to the correctly-cased URL. For git
         # requests, we should accept wrongly-cased URLs because it is a pain to
         # block people's git operations and force them to update remote URLs.
-        it 'returns the project and false' do
-          expect(described_class.find_project(project.full_path.upcase)).to eq([project, false])
+        it 'returns the project and nil' do
+          expect(described_class.find_project(project.full_path.upcase)).to eq([project, nil])
         end
       end
     end
 
     context 'when finding a project via a redirect' do
-      it 'returns the project and true' do
-        expect(described_class.find_project(redirect.path)).to eq([project, true])
+      it 'returns the project and nil' do
+        expect(described_class.find_project(redirect.path)).to eq([project, redirect.path])
       end
     end
   end
 
   describe '.find_snippet' do
     it 'extracts path and id from personal snippet route' do
-      expect(described_class.find_snippet("snippets/#{personal_snippet.id}")).to eq([personal_snippet, false])
+      expect(described_class.find_snippet("snippets/#{personal_snippet.id}")).to eq([personal_snippet, nil])
     end
 
     it 'extracts path and id from project snippet route' do
-      expect(described_class.find_snippet("#{project.full_path}/snippets/#{project_snippet.id}")).to eq([project_snippet, false])
+      expect(described_class.find_snippet("#{project.full_path}/snippets/#{project_snippet.id}")).to eq([project_snippet, nil])
     end
 
     it 'returns nil for invalid snippet paths' do
       aggregate_failures do
-        expect(described_class.find_snippet("snippets/#{project_snippet.id}")).to eq([nil, false])
-        expect(described_class.find_snippet("#{project.full_path}/snippets/#{personal_snippet.id}")).to eq([nil, false])
-        expect(described_class.find_snippet('')).to eq([nil, false])
+        expect(described_class.find_snippet("snippets/#{project_snippet.id}")).to eq([nil, nil])
+        expect(described_class.find_snippet("#{project.full_path}/snippets/#{personal_snippet.id}")).to eq([nil, nil])
+        expect(described_class.find_snippet('')).to eq([nil, nil])
       end
     end
 
     it 'returns nil for snippets not associated with the project' do
       snippet = create(:project_snippet)
 
-      expect(described_class.find_snippet("#{project.full_path}/snippets/#{snippet.id}")).to eq([nil, false])
+      expect(described_class.find_snippet("#{project.full_path}/snippets/#{snippet.id}")).to eq([nil, nil])
     end
 
     context 'when finding a project snippet via a redirect' do
       it 'returns the project and true' do
-        expect(described_class.find_snippet("#{redirect.path}/snippets/#{project_snippet.id}")).to eq([project_snippet, true])
+        expect(described_class.find_snippet("#{redirect.path}/snippets/#{project_snippet.id}")).to eq([project_snippet, redirect.path])
       end
     end
   end

spec/services/post_receive_service_spec.rb

@@ -234,7 +234,7 @@ describe PostReceiveService do
 
   context 'with a redirected data' do
     it 'returns redirected message on the response' do
-      project_moved = Gitlab::Checks::ProjectMoved.new(project, user, 'http', 'foo/baz')
+      project_moved = Gitlab::Checks::ProjectMoved.new(project.repository, user, 'http', 'foo/baz')
       project_moved.add_message
 
       expect(subject).to include(build_basic_message(project_moved.message))
@@ -243,7 +243,7 @@ describe PostReceiveService do
 
   context 'with new project data' do
     it 'returns new project message on the response' do
-      project_created = Gitlab::Checks::ProjectCreated.new(project, user, 'http')
+      project_created = Gitlab::Checks::ProjectCreated.new(project.repository, user, 'http')
       project_created.add_message
 
       expect(subject).to include(build_basic_message(project_created.message))