From 5a3e6fdff96f50cb293a8c9fe64ccbf59619936f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= <fjlopez@gitlab.com>
Date: Thu, 13 Dec 2018 18:49:05 +0100
Subject: Fixing image lfs bug and also displaying text lfs

This commit, introduced in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23812,
fixes a problem creating a displaying image diff notes when the image
is stored in LFS. The main problem was that `Gitlab::Diff::File` was
returning an invalid valid in `text?` for this kind of files.

It also fixes a rendering problem with other LFS files, like text
ones. They LFS pointer shouldn't be shown when LFS is enabled
for the project, but they were.
---
 .../user_creates_image_diff_notes_spec.rb          | 33 ++++++++--
 spec/features/merge_request/user_sees_diff_spec.rb | 70 +++++++++++++++++-----
 .../projects/commits/user_browses_commits_spec.rb  |  2 +-
 .../fixtures/api/schemas/entities/diff_viewer.json | 16 ++++-
 spec/helpers/diff_helper_spec.rb                   | 37 ------------
 spec/lib/gitlab/blob_helper_spec.rb                |  4 +-
 spec/lib/gitlab/diff/file_spec.rb                  |  8 +--
 spec/lib/gitlab/diff/lines_unfolder_spec.rb        |  2 +-
 spec/lib/gitlab/git/blob_spec.rb                   |  8 +--
 .../gitlab/gitaly_client/blobs_stitcher_spec.rb    |  4 +-
 spec/models/blob_spec.rb                           | 22 +++----
 spec/models/diff_viewer/base_spec.rb               | 23 ++++++-
 spec/models/diff_viewer/server_side_spec.rb        | 20 +++++++
 spec/support/helpers/fake_blob_helpers.rb          |  2 +-
 spec/support/helpers/test_env.rb                   |  3 +-
 15 files changed, 169 insertions(+), 85 deletions(-)

(limited to 'spec')

diff --git a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
index d790bdc82ce..d19408ee87f 100644
--- a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
+++ b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
@@ -90,9 +90,6 @@ describe 'Merge request > User creates image diff notes', :js do
 
   %w(inline parallel).each do |view|
     context "#{view} view" do
-      let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
-      let(:path)          { "files/images/ee_repo_logo.png" }
-
       let(:position) do
         Gitlab::Diff::Position.new(
           old_path: path,
@@ -108,9 +105,11 @@ describe 'Merge request > User creates image diff notes', :js do
 
       let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
 
-      describe 'creating a new diff note' do
+      shared_examples 'creates image diff note' do
         before do
           visit diffs_project_merge_request_path(project, merge_request, view: view)
+          wait_for_requests
+
           create_image_diff_note
         end
 
@@ -132,6 +131,32 @@ describe 'Merge request > User creates image diff notes', :js do
           expect(page).to have_content('image diff test comment')
         end
       end
+
+      context 'when images are not stored in LFS' do
+        let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
+        let(:path)          { 'files/images/ee_repo_logo.png' }
+
+        it_behaves_like 'creates image diff note'
+      end
+
+      context 'when images are stored in LFS' do
+        let(:merge_request) { create(:merge_request, source_project: project, target_project: project, source_branch: 'png-lfs', target_branch: 'master', author: user) }
+        let(:path)          { 'files/images/logo-black.png' }
+
+        before do
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+          project.update_attribute(:lfs_enabled, true)
+        end
+
+        it 'shows lfs badges' do
+          visit diffs_project_merge_request_path(project, merge_request, view: view)
+          wait_for_requests
+
+          expect(page.all('.diff-file span.label-lfs', visible: :all)).not_to be_empty
+        end
+
+        it_behaves_like 'creates image diff note'
+      end
     end
   end
 
diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb
index 0df9e4bbc1a..04b07525919 100644
--- a/spec/features/merge_request/user_sees_diff_spec.rb
+++ b/spec/features/merge_request/user_sees_diff_spec.rb
@@ -87,20 +87,6 @@ describe 'Merge request > User sees diff', :js do
       let(:current_user) { project.owner }
       let(:branch_name) {"test_branch"}
 
-      def create_file(branch_name, file_name, content)
-        Files::CreateService.new(
-          project,
-          current_user,
-          start_branch: branch_name,
-          branch_name: branch_name,
-          commit_message: "Create file",
-          file_path: file_name,
-          file_content: content
-        ).execute
-
-        project.commit(branch_name)
-      end
-
       it 'escapes any HTML special characters in the diff chunk header' do
         file_content =
           <<~CONTENT
@@ -136,5 +122,61 @@ describe 'Merge request > User sees diff', :js do
         expect(page).to have_css(".line[lang='rust'] .k")
       end
     end
+
+    context 'when file is stored in LFS' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+      let(:current_user) { project.owner }
+
+      context 'when LFS is enabled on the project' do
+        before do
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+          project.update_attribute(:lfs_enabled, true)
+
+          create_file('master', file_name, project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data)
+
+          visit diffs_project_merge_request_path(project, merge_request)
+        end
+
+        context 'when file is an image', :js do
+          let(:file_name) { 'files/lfs/image.png' }
+
+          it 'shows an error message' do
+            expect(page).not_to have_content('could not be displayed because it is stored in LFS')
+          end
+        end
+
+        context 'when file is not an image' do
+          let(:file_name) { 'files/lfs/ruby.rb' }
+
+          it 'shows an error message' do
+            expect(page).to have_content('This source diff could not be displayed because it is stored in LFS')
+          end
+        end
+      end
+
+      context 'when LFS is not enabled' do
+        before do
+          visit diffs_project_merge_request_path(project, merge_request)
+        end
+
+        it 'displays the diff' do
+          expect(page).to have_content('size 1575078')
+        end
+      end
+    end
+
+    def create_file(branch_name, file_name, content)
+      Files::CreateService.new(
+        project,
+        current_user,
+        start_branch: branch_name,
+        branch_name: branch_name,
+        commit_message: "Create file",
+        file_path: file_name,
+        file_content: content
+      ).execute
+
+      project.commit(branch_name)
+    end
   end
 end
diff --git a/spec/features/projects/commits/user_browses_commits_spec.rb b/spec/features/projects/commits/user_browses_commits_spec.rb
index 2159adf49fc..574a8aefd63 100644
--- a/spec/features/projects/commits/user_browses_commits_spec.rb
+++ b/spec/features/projects/commits/user_browses_commits_spec.rb
@@ -93,7 +93,7 @@ describe 'User browses commits' do
 
     it 'shows a blank label' do
       allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(nil)
-      allow_any_instance_of(Gitlab::Diff::File).to receive(:raw_binary?).and_return(true)
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:binary?).and_return(true)
 
       visit(project_commit_path(project, commit))
 
diff --git a/spec/fixtures/api/schemas/entities/diff_viewer.json b/spec/fixtures/api/schemas/entities/diff_viewer.json
index 19780f49a88..81325cd86c6 100644
--- a/spec/fixtures/api/schemas/entities/diff_viewer.json
+++ b/spec/fixtures/api/schemas/entities/diff_viewer.json
@@ -1,8 +1,20 @@
 {
   "type": "object",
-  "required": ["name"],
+  "required": [
+    "name"
+  ],
   "properties": {
-    "name": { "type": ["string"] }
+    "name": {
+      "type": [
+        "string"
+      ]
+    },
+    "error": {
+      "type": [
+        "string",
+        "null"
+      ]
+    }
   },
   "additionalProperties": false
 }
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 53c010fa0db..5396243f44d 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -256,43 +256,6 @@ describe DiffHelper do
     end
   end
 
-  context 'viewer related' do
-    let(:viewer) { diff_file.simple_viewer }
-
-    before do
-      assign(:project, project)
-    end
-
-    describe '#diff_render_error_reason' do
-      context 'for error :too_large' do
-        before do
-          expect(viewer).to receive(:render_error).and_return(:too_large)
-        end
-
-        it 'returns an error message' do
-          expect(helper.diff_render_error_reason(viewer)).to eq('it is too large')
-        end
-      end
-
-      context 'for error :server_side_but_stored_externally' do
-        before do
-          expect(viewer).to receive(:render_error).and_return(:server_side_but_stored_externally)
-          expect(diff_file).to receive(:external_storage).and_return(:lfs)
-        end
-
-        it 'returns an error message' do
-          expect(helper.diff_render_error_reason(viewer)).to eq('it is stored in LFS')
-        end
-      end
-    end
-
-    describe '#diff_render_error_options' do
-      it 'includes a "view the blob" link' do
-        expect(helper.diff_render_error_options(viewer)).to include(/view the blob/)
-      end
-    end
-  end
-
   context '#diff_file_path_text' do
     it 'returns full path by default' do
       expect(diff_file_path_text(diff_file)).to eq(diff_file.new_path)
diff --git a/spec/lib/gitlab/blob_helper_spec.rb b/spec/lib/gitlab/blob_helper_spec.rb
index 0b56f8687c3..e057385b35f 100644
--- a/spec/lib/gitlab/blob_helper_spec.rb
+++ b/spec/lib/gitlab/blob_helper_spec.rb
@@ -53,11 +53,11 @@ describe Gitlab::BlobHelper do
 
   describe '#text?' do
     it 'returns true' do
-      expect(blob.text?).to be_truthy
+      expect(blob.text_in_repo?).to be_truthy
     end
 
     it 'returns false' do
-      expect(large_blob.text?).to be_falsey
+      expect(large_blob.text_in_repo?).to be_falsey
     end
   end
 
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index b15d22c634a..862590268ca 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -310,7 +310,7 @@ describe Gitlab::Diff::File do
     context 'when the content changed' do
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns a No Preview viewer' do
@@ -345,7 +345,7 @@ describe Gitlab::Diff::File do
 
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns an Added viewer' do
@@ -380,7 +380,7 @@ describe Gitlab::Diff::File do
 
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns a Deleted viewer' do
@@ -436,7 +436,7 @@ describe Gitlab::Diff::File do
         allow(diff_file).to receive(:deleted_file?).and_return(false)
         allow(diff_file).to receive(:renamed_file?).and_return(false)
         allow(diff_file).to receive(:mode_changed?).and_return(false)
-        allow(diff_file).to receive(:raw_text?).and_return(false)
+        allow(diff_file).to receive(:text?).and_return(false)
       end
 
       it 'returns a No Preview viewer' do
diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
index 8e00c8e0e30..f22c2c90334 100644
--- a/spec/lib/gitlab/diff/lines_unfolder_spec.rb
+++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
@@ -185,7 +185,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   let(:project) { create(:project) }
 
-  let(:old_blob) { Gitlab::Git::Blob.new(data: raw_old_blob) }
+  let(:old_blob) { Blob.decorate(Gitlab::Git::Blob.new(data: raw_old_blob, size: 10)) }
 
   let(:diff) do
     Gitlab::Git::Diff.new(diff: raw_diff,
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index 80dd3dcc58e..1bcec04d28f 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -59,7 +59,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       it { expect(blob.data[0..10]).to eq("*.rbc\n*.sas") }
       it { expect(blob.size).to eq(241) }
       it { expect(blob.mode).to eq("100644") }
-      it { expect(blob).not_to be_binary }
+      it { expect(blob).not_to be_binary_in_repo }
     end
 
     context 'file in root with leading slash' do
@@ -92,7 +92,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       end
 
       it 'does not mark the blob as binary' do
-        expect(blob).not_to be_binary
+        expect(blob).not_to be_binary_in_repo
       end
     end
 
@@ -123,7 +123,7 @@ describe Gitlab::Git::Blob, :seed_helper do
           .with(hash_including(binary: true))
           .and_call_original
 
-        expect(blob).to be_binary
+        expect(blob).to be_binary_in_repo
       end
     end
   end
@@ -196,7 +196,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       it { expect(blob.id).to eq('409f37c4f05865e4fb208c771485f211a22c4c2d') }
       it { expect(blob.data).to eq('') }
       it 'does not mark the blob as binary' do
-        expect(blob).not_to be_binary
+        expect(blob).not_to be_binary_in_repo
       end
     end
 
diff --git a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
index 9db710e759e..742b2872c40 100644
--- a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
@@ -21,7 +21,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
       expect(blobs[0].size).to eq(1642)
       expect(blobs[0].commit_id).to eq('f00ba7')
       expect(blobs[0].data).to eq("first-line\nsecond-line")
-      expect(blobs[0].binary?).to be false
+      expect(blobs[0].binary_in_repo?).to be false
 
       expect(blobs[1].id).to eq('abcdef2')
       expect(blobs[1].mode).to eq('100644')
@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
       expect(blobs[1].size).to eq(2461)
       expect(blobs[1].commit_id).to eq('f00ba8')
       expect(blobs[1].data).to eq("GIF87a\x90\x01".b)
-      expect(blobs[1].binary?).to be true
+      expect(blobs[1].binary_in_repo?).to be true
     end
   end
 end
diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb
index e8c03b587e2..05cf242e84d 100644
--- a/spec/models/blob_spec.rb
+++ b/spec/models/blob_spec.rb
@@ -122,14 +122,14 @@ describe Blob do
     end
   end
 
-  describe '#raw_binary?' do
+  describe '#binary?' do
     context 'if the blob is stored externally' do
       context 'if the extension has a rich viewer' do
         context 'if the viewer is binary' do
           it 'returns true' do
             blob = fake_blob(path: 'file.pdf', lfs: true)
 
-            expect(blob.raw_binary?).to be_truthy
+            expect(blob.binary?).to be_truthy
           end
         end
 
@@ -137,7 +137,7 @@ describe Blob do
           it 'return false' do
             blob = fake_blob(path: 'file.md', lfs: true)
 
-            expect(blob.raw_binary?).to be_falsey
+            expect(blob.binary?).to be_falsey
           end
         end
       end
@@ -148,7 +148,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.txt', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -156,7 +156,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.ics', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
         end
@@ -166,7 +166,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.rb', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -174,7 +174,7 @@ describe Blob do
             it 'returns true' do
               blob = fake_blob(path: 'file.exe', lfs: true)
 
-              expect(blob.raw_binary?).to be_truthy
+              expect(blob.binary?).to be_truthy
             end
           end
         end
@@ -184,7 +184,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.ini', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -192,7 +192,7 @@ describe Blob do
             it 'returns true' do
               blob = fake_blob(path: 'file.wtf', lfs: true)
 
-              expect(blob.raw_binary?).to be_truthy
+              expect(blob.binary?).to be_truthy
             end
           end
         end
@@ -204,7 +204,7 @@ describe Blob do
         it 'returns true' do
           blob = fake_blob(path: 'file.pdf', binary: true)
 
-          expect(blob.raw_binary?).to be_truthy
+          expect(blob.binary?).to be_truthy
         end
       end
 
@@ -212,7 +212,7 @@ describe Blob do
         it 'return false' do
           blob = fake_blob(path: 'file.md')
 
-          expect(blob.raw_binary?).to be_falsey
+          expect(blob.binary?).to be_falsey
         end
       end
     end
diff --git a/spec/models/diff_viewer/base_spec.rb b/spec/models/diff_viewer/base_spec.rb
index c90b32c5d77..f4efe5a7b3a 100644
--- a/spec/models/diff_viewer/base_spec.rb
+++ b/spec/models/diff_viewer/base_spec.rb
@@ -58,7 +58,7 @@ describe DiffViewer::Base do
 
       context 'when the binaryness does not match' do
         before do
-          allow_any_instance_of(Blob).to receive(:binary?).and_return(true)
+          allow_any_instance_of(Blob).to receive(:binary_in_repo?).and_return(true)
         end
 
         it 'returns false' do
@@ -141,4 +141,25 @@ describe DiffViewer::Base do
       end
     end
   end
+
+  describe '#render_error_message' do
+    it 'returns nothing when no render_error' do
+      expect(viewer.render_error).to be_nil
+      expect(viewer.render_error_message).to be_nil
+    end
+
+    context 'when render_error error' do
+      before do
+        allow(viewer).to receive(:render_error).and_return(:too_large)
+      end
+
+      it 'returns an error message' do
+        expect(viewer.render_error_message).to include('it is too large')
+      end
+
+      it 'includes a "view the blob" link' do
+        expect(viewer.render_error_message).to include('view the blob')
+      end
+    end
+  end
 end
diff --git a/spec/models/diff_viewer/server_side_spec.rb b/spec/models/diff_viewer/server_side_spec.rb
index 98a8f6d4cc9..86b14b6ebf3 100644
--- a/spec/models/diff_viewer/server_side_spec.rb
+++ b/spec/models/diff_viewer/server_side_spec.rb
@@ -32,4 +32,24 @@ describe DiffViewer::ServerSide do
       end
     end
   end
+
+  describe '#render_error_reason' do
+    context 'when the diff file is stored externally' do
+      before do
+        allow(diff_file).to receive(:stored_externally?).and_return(true)
+      end
+
+      it 'returns error message if stored in LFS' do
+        allow(diff_file).to receive(:external_storage).and_return(:lfs)
+
+        expect(subject.render_error_message).to include('it is stored in LFS')
+      end
+
+      it 'returns error message if stored externally' do
+        allow(diff_file).to receive(:external_storage).and_return(:foo)
+
+        expect(subject.render_error_message).to include('it is stored externally')
+      end
+    end
+  end
 end
diff --git a/spec/support/helpers/fake_blob_helpers.rb b/spec/support/helpers/fake_blob_helpers.rb
index bc9686ed9cf..801ca8b7412 100644
--- a/spec/support/helpers/fake_blob_helpers.rb
+++ b/spec/support/helpers/fake_blob_helpers.rb
@@ -23,7 +23,7 @@ module FakeBlobHelpers
       0
     end
 
-    def binary?
+    def binary_in_repo?
       @binary
     end
 
diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb
index d52c40ff4f1..d352a7cdf1a 100644
--- a/spec/support/helpers/test_env.rb
+++ b/spec/support/helpers/test_env.rb
@@ -62,7 +62,8 @@ module TestEnv
     'between-create-delete-modify-move'  => '3f5f443',
     'after-create-delete-modify-move'    => 'ba3faa7',
     'with-codeowners'                    => '219560e',
-    'submodule_inside_folder'            => 'b491b92'
+    'submodule_inside_folder'            => 'b491b92',
+    'png-lfs'                            => 'fe42f41'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily
-- 
cgit v1.2.1