Commit d7796005 authored by Nick Thomas's avatar Nick Thomas

Fix viewing some legacy diff notes in discussions

LegacyDiffNotes created by the GitHub importer lack details about the
SHAs they referenced on the GitHub side. This exposes a bug in our
diff viewer selection logic, where we inappropriately pick the image
viewer whenever both old blob and new blob are nil.

In practice, the image diff viewer relies on both old and new blob to
do its work, so this just leads to us being unable to render the diff.

Adding this condition allows us to fall back to the basic text diff,
which is usually sufficient.
parent 3ac9a973
......@@ -10,5 +10,13 @@ module DiffViewer
self.binary = true
self.switcher_icon = 'doc-image'
self.switcher_title = _('image diff')
def self.can_render?(diff_file, verify_binary: true)
# When both blobs are missing, we often still have a textual diff that can
# be displayed
return false if diff_file.old_blob.nil? && diff_file.new_blob.nil?
super
end
end
end
---
title: Fix viewing GitHub-imported diff notes in discussions
merge_request: 45920
author:
type: fixed
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe DiffViewer::Image do
describe '.can_render?' do
let(:diff_file) { double(Gitlab::Diff::File) }
let(:blob) { double(Gitlab::Git::Blob, binary_in_repo?: true, extension: 'png') }
subject { described_class.can_render?(diff_file, verify_binary: false) }
it 'returns false if both old and new blob are absent' do
allow(diff_file).to receive(:old_blob) { nil }
allow(diff_file).to receive(:new_blob) { nil }
is_expected.to be_falsy
end
it 'returns true if the old blob is present' do
allow(diff_file).to receive(:old_blob) { blob }
allow(diff_file).to receive(:new_blob) { nil }
is_expected.to be_truthy
end
it 'returns true if the new blob is present' do
allow(diff_file).to receive(:old_blob) { nil }
allow(diff_file).to receive(:new_blob) { blob }
is_expected.to be_truthy
end
it 'returns true if both old and new blobs are present' do
allow(diff_file).to receive(:old_blob) { blob }
allow(diff_file).to receive(:new_blob) { blob }
is_expected.to be_truthy
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment