From 202e6ae96b46086b95ab46df066f11a7be06319c Mon Sep 17 00:00:00 2001
From: Jerry Seto <jseto@gitlab.com>
Date: Fri, 7 Jul 2023 13:34:48 +0000
Subject: [PATCH] Preserve behaviour of blob attributes when ref_type specified

- Unqualify commit id for methods rely on the unqualified ref
- Make test expectations more literal

Contributes to: https://gitlab.com/gitlab-org/gitlab/-/issues/20526
Changelog: fixed
---
 app/presenters/blob_presenter.rb       |  21 ++---
 spec/presenters/blob_presenter_spec.rb | 108 ++++++++++++++++++-------
 2 files changed, 88 insertions(+), 41 deletions(-)

diff --git a/app/presenters/blob_presenter.rb b/app/presenters/blob_presenter.rb
index cd473152b41c..bc12d210334b 100644
--- a/app/presenters/blob_presenter.rb
+++ b/app/presenters/blob_presenter.rb
@@ -86,7 +86,7 @@ def gitpod_blob_url
   end
 
   def find_file_path
-    url_helpers.project_find_file_path(project, blob.commit_id)
+    url_helpers.project_find_file_path(project, commit_id, ref_type: ref_type)
   end
 
   def blame_path
@@ -131,13 +131,13 @@ def fork_and_view_path
   end
 
   def can_modify_blob?
-    super(blob, project, blob.commit_id)
+    super(blob, project, commit_id)
   end
 
   def can_current_user_push_to_branch?
-    return false unless current_user && project.repository.branch_exists?(blob.commit_id)
+    return false unless current_user && project.repository.branch_exists?(commit_id)
 
-    user_access(project).can_push_to_branch?(blob.commit_id)
+    user_access(project).can_push_to_branch?(commit_id)
   end
 
   def archived?
@@ -145,7 +145,7 @@ def archived?
   end
 
   def ide_edit_path
-    super(project, blob.commit_id, blob.path)
+    super(project, commit_id, blob.path)
   end
 
   def external_storage_url
@@ -159,7 +159,7 @@ def code_navigation_path
   end
 
   def project_blob_path_root
-    project_blob_path(project, blob.commit_id)
+    project_blob_path(project, commit_id)
   end
 
   private
@@ -181,7 +181,7 @@ def url_helpers
   end
 
   def environment
-    environment_params = project.repository.branch_exists?(blob.commit_id) ? { ref: blob.commit_id } : { sha: blob.commit_id }
+    environment_params = project.repository.branch_exists?(commit_id) ? { ref: commit_id } : { sha: commit_id }
     environment_params[:find_latest] = true
     ::Environments::EnvironmentsByDeploymentsFinder.new(project, current_user, environment_params).execute.last
   end
@@ -190,12 +190,13 @@ def project
     blob.repository.project
   end
 
-  def ref_qualified_path
+  def commit_id
     # If `ref_type` is present the commit_id will include the ref qualifier e.g. `refs/heads/`.
     # We only accept/return unqualified refs so we need to remove the qualifier from the `commit_id`.
+    ExtractsRef.unqualify_ref(blob.commit_id, ref_type)
+  end
 
-    commit_id = ExtractsRef.unqualify_ref(blob.commit_id, ref_type)
-
+  def ref_qualified_path
     File.join(commit_id, blob.path)
   end
 
diff --git a/spec/presenters/blob_presenter_spec.rb b/spec/presenters/blob_presenter_spec.rb
index e776716bd2d7..150c7bd5f3ed 100644
--- a/spec/presenters/blob_presenter_spec.rb
+++ b/spec/presenters/blob_presenter_spec.rb
@@ -7,28 +7,52 @@
   let_it_be(:user) { project.first_owner }
 
   let(:repository) { project.repository }
-  let(:blob) { repository.blob_at('HEAD', 'files/ruby/regex.rb') }
+  let(:blob) { repository.blob_at(ref, path) }
+  let(:ref) { 'HEAD' }
+  let(:path) { 'files/ruby/regex.rb' }
 
   subject(:presenter) { described_class.new(blob, current_user: user) }
 
   describe '#web_url' do
-    it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}") }
+    it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{ref}/#{path}") }
   end
 
   describe '#web_path' do
-    it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}") }
+    it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{ref}/#{path}") }
   end
 
   describe '#edit_blob_path' do
-    it { expect(presenter.edit_blob_path).to eq("/#{project.full_path}/-/edit/#{blob.commit_id}/#{blob.path}") }
+    it { expect(presenter.edit_blob_path).to eq("/#{project.full_path}/-/edit/#{ref}/#{path}") }
   end
 
   describe '#raw_path' do
-    it { expect(presenter.raw_path).to eq("/#{project.full_path}/-/raw/#{blob.commit_id}/#{blob.path}") }
+    it { expect(presenter.raw_path).to eq("/#{project.full_path}/-/raw/#{ref}/#{path}") }
   end
 
   describe '#replace_path' do
-    it { expect(presenter.replace_path).to eq("/#{project.full_path}/-/update/#{blob.commit_id}/#{blob.path}") }
+    it { expect(presenter.replace_path).to eq("/#{project.full_path}/-/update/#{ref}/#{path}") }
+  end
+
+  shared_examples_for '#can_current_user_push_to_branch?' do
+    let(:branch_exists) { true }
+
+    before do
+      allow(project.repository).to receive(:branch_exists?).with(blob.commit_id).and_return(branch_exists)
+    end
+
+    it { expect(presenter.can_current_user_push_to_branch?).to eq(true) }
+
+    context 'current_user is nil' do
+      let(:user) { nil }
+
+      it { expect(presenter.can_current_user_push_to_branch?).to eq(false) }
+    end
+
+    context 'branch does not exist' do
+      let(:branch_exists) { false }
+
+      it { expect(presenter.can_current_user_push_to_branch?).to eq(false) }
+    end
   end
 
   context 'when blob has ref_type' do
@@ -37,46 +61,67 @@
     end
 
     describe '#web_url' do
-      it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}?ref_type=heads") }
+      it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{ref}/#{path}?ref_type=heads") }
     end
 
     describe '#web_path' do
-      it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}?ref_type=heads") }
+      it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{ref}/#{path}?ref_type=heads") }
     end
 
     describe '#edit_blob_path' do
-      it { expect(presenter.edit_blob_path).to eq("/#{project.full_path}/-/edit/#{blob.commit_id}/#{blob.path}?ref_type=heads") }
+      it { expect(presenter.edit_blob_path).to eq("/#{project.full_path}/-/edit/#{ref}/#{path}?ref_type=heads") }
     end
 
     describe '#raw_path' do
-      it { expect(presenter.raw_path).to eq("/#{project.full_path}/-/raw/#{blob.commit_id}/#{blob.path}?ref_type=heads") }
+      it { expect(presenter.raw_path).to eq("/#{project.full_path}/-/raw/#{ref}/#{path}?ref_type=heads") }
     end
 
     describe '#replace_path' do
-      it { expect(presenter.replace_path).to eq("/#{project.full_path}/-/update/#{blob.commit_id}/#{blob.path}?ref_type=heads") }
+      it { expect(presenter.replace_path).to eq("/#{project.full_path}/-/update/#{ref}/#{path}?ref_type=heads") }
     end
+
+    it_behaves_like '#can_current_user_push_to_branch?'
   end
 
-  describe '#can_current_user_push_to_branch' do
-    let(:branch_exists) { true }
+  describe '#can_modify_blob?' do
+    context 'when blob is store externally' do
+      before do
+        allow(blob).to receive(:stored_externally?).and_return(true)
+      end
 
-    before do
-      allow(project.repository).to receive(:branch_exists?).with(blob.commit_id).and_return(branch_exists)
+      it { expect(presenter.can_modify_blob?).to be_falsey }
     end
 
-    it { expect(presenter.can_current_user_push_to_branch?).to eq(true) }
+    context 'when the user cannot edit the tree' do
+      before do
+        allow(presenter).to receive(:can_edit_tree?).with(project, ref).and_return(false)
+      end
 
-    context 'current_user is nil' do
-      let(:user) { nil }
+      it { expect(presenter.can_modify_blob?).to be_falsey }
+    end
 
-      it { expect(presenter.can_current_user_push_to_branch?).to eq(false) }
+    context 'when ref is a branch' do
+      let(:ref) { 'feature' }
+
+      it { expect(presenter.can_modify_blob?).to be_truthy }
     end
+  end
 
-    context 'branch does not exist' do
-      let(:branch_exists) { false }
+  describe '#can_current_user_push_to_branch?' do
+    context 'when ref is a branch' do
+      let(:ref) { 'feature' }
 
-      it { expect(presenter.can_current_user_push_to_branch?).to eq(false) }
+      it 'delegates to UserAccess' do
+        allow_next_instance_of(Gitlab::UserAccess) do |instance|
+          expect(instance).to receive(:can_push_to_branch?).with(ref).and_call_original
+        end
+        expect(presenter.can_current_user_push_to_branch?).to be_truthy
+      end
     end
+
+    it_behaves_like '#can_current_user_push_to_branch?'
+
+    it { expect(presenter.can_current_user_push_to_branch?).to be_falsey }
   end
 
   describe '#archived?' do
@@ -95,9 +140,10 @@
         )
       end
 
-      let(:blob) { repository.blob_at('main', '.gitlab-ci.yml') }
+      let(:ref) { 'main' }
+      let(:path) { '.gitlab-ci.yml' }
 
-      it { expect(presenter.pipeline_editor_path).to eq("/#{project.full_path}/-/ci/editor?branch_name=#{blob.commit_id}") }
+      it { expect(presenter.pipeline_editor_path).to eq("/#{project.full_path}/-/ci/editor?branch_name=#{ref}") }
     end
   end
 
@@ -114,7 +160,7 @@
 
     context 'Gitpod enabled for application and user' do
       describe '#gitpod_blob_url' do
-        it { expect(presenter.gitpod_blob_url).to eq("#{gitpod_url}##{"http://localhost/#{project.full_path}/-/tree/#{blob.commit_id}/#{blob.path}"}") }
+        it { expect(presenter.gitpod_blob_url).to eq("#{gitpod_url}##{"http://localhost/#{project.full_path}/-/tree/#{ref}/#{path}"}") }
       end
     end
 
@@ -157,7 +203,7 @@
     let!(:deployment) { create(:deployment, :success, environment: environment, project: project, sha: blob.commit_id) }
 
     before do
-      allow(project).to receive(:public_path_for_source_path).with(blob.path, blob.commit_id).and_return(blob.path)
+      allow(project).to receive(:public_path_for_source_path).with(path, blob.commit_id).and_return(path)
     end
 
     describe '#environment_formatted_external_url' do
@@ -165,7 +211,7 @@
     end
 
     describe '#environment_external_url_for_route_map' do
-      it { expect(presenter.environment_external_url_for_route_map).to eq("#{external_url}/#{blob.path}") }
+      it { expect(presenter.environment_external_url_for_route_map).to eq("#{external_url}/#{path}") }
     end
 
     describe 'chooses the latest deployed environment for #environment_formatted_external_url and #environment_external_url_for_route_map' do
@@ -174,7 +220,7 @@
       let!(:another_deployment) { create(:deployment, :success, environment: another_environment, project: project, sha: blob.commit_id) }
 
       it { expect(presenter.environment_formatted_external_url).to eq("another.environment") }
-      it { expect(presenter.environment_external_url_for_route_map).to eq("#{another_external_url}/#{blob.path}") }
+      it { expect(presenter.environment_external_url_for_route_map).to eq("#{another_external_url}/#{path}") }
     end
   end
 
@@ -219,7 +265,7 @@
   end
 
   describe '#code_navigation_path' do
-    let(:code_navigation_path) { Gitlab::CodeNavigationPath.new(project, blob.commit_id).full_json_path_for(blob.path) }
+    let(:code_navigation_path) { Gitlab::CodeNavigationPath.new(project, blob.commit_id).full_json_path_for(path) }
 
     it { expect(presenter.code_navigation_path).to eq(code_navigation_path) }
   end
@@ -232,11 +278,11 @@
     let(:blob) { Gitlab::Graphql::Representation::TreeEntry.new(super(), repository) }
 
     describe '#web_url' do
-      it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}") }
+      it { expect(presenter.web_url).to eq("http://localhost/#{project.full_path}/-/blob/#{ref}/#{path}") }
     end
 
     describe '#web_path' do
-      it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{blob.commit_id}/#{blob.path}") }
+      it { expect(presenter.web_path).to eq("/#{project.full_path}/-/blob/#{ref}/#{path}") }
     end
   end
 
-- 
GitLab