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.

2 files changed, 85 insertions, 18 deletions

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