Commit 0a0c9282 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'dm-note-email-image-diff-discussion' into 'master'

Fix notification email for image diff notes

See merge request gitlab-org/gitlab-ce!23958
parents cc5e58a7 cce0e0b2
...@@ -39,6 +39,7 @@ module DiscussionOnDiff ...@@ -39,6 +39,7 @@ module DiscussionOnDiff
# Returns an array of at most 16 highlighted lines above a diff note # Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true, diff_limit: nil) def truncated_diff_lines(highlight: true, diff_limit: nil)
return [] unless on_text?
return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote) return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote)
diff_limit = [diff_limit, NUMBER_OF_TRUNCATED_DIFF_LINES].compact.min diff_limit = [diff_limit, NUMBER_OF_TRUNCATED_DIFF_LINES].compact.min
......
...@@ -4,17 +4,13 @@ ...@@ -4,17 +4,13 @@
- note_style = local_assigns.fetch(:note_style, "") - note_style = local_assigns.fetch(:note_style, "")
- discussion = note.discussion if note.part_of_discussion? - discussion = note.discussion if note.part_of_discussion?
- diff_discussion = discussion&.diff_discussion?
- on_image = discussion.on_image? if diff_discussion
- if discussion - if discussion
- phrase_end_char = on_image ? "." : ":"
%p{ style: "color: #777777;" } %p{ style: "color: #777777;" }
= succeed phrase_end_char do = succeed ':' do
= link_to note.author_name, user_url(note.author) = link_to note.author_name, user_url(note.author)
- if diff_discussion - if discussion&.diff_discussion?
- if discussion.new_discussion? - if discussion.new_discussion?
started a new discussion started a new discussion
- else - else
...@@ -31,7 +27,7 @@ ...@@ -31,7 +27,7 @@
%p.details %p.details
#{link_to note.author_name, user_url(note.author)} commented: #{link_to note.author_name, user_url(note.author)} commented:
- if diff_discussion && !on_image - if discussion&.diff_discussion? && discussion.on_text?
= content_for :head do = content_for :head do
= stylesheet_link_tag 'mailers/highlighted_diff_email' = stylesheet_link_tag 'mailers/highlighted_diff_email'
......
...@@ -20,7 +20,7 @@ ...@@ -20,7 +20,7 @@
<% end -%> <% end -%>
<% if discussion&.diff_discussion? -%> <% if discussion&.diff_discussion? && discussion.on_text? -%>
<% discussion.truncated_diff_lines(highlight: false, diff_limit: diff_limit).each do |line| -%> <% discussion.truncated_diff_lines(highlight: false, diff_limit: diff_limit).each do |line| -%>
<%= "> #{line.text}\n" -%> <%= "> #{line.text}\n" -%>
<% end -%> <% end -%>
......
---
title: Fix notification email for image diff notes
merge_request:
author:
type: fixed
...@@ -64,6 +64,21 @@ FactoryBot.define do ...@@ -64,6 +64,21 @@ FactoryBot.define do
resolved_at { Time.now } resolved_at { Time.now }
resolved_by { create(:user) } resolved_by { create(:user) }
end end
factory :image_diff_note_on_merge_request do
position do
Gitlab::Diff::Position.new(
old_path: "files/images/any_image.png",
new_path: "files/images/any_image.png",
width: 10,
height: 10,
x: 1,
y: 1,
diff_refs: diff_refs,
position_type: "image"
)
end
end
end end
factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
......
...@@ -890,22 +890,14 @@ describe Notify do ...@@ -890,22 +890,14 @@ describe Notify do
shared_examples 'an email for a note on a diff discussion' do |model| shared_examples 'an email for a note on a diff discussion' do |model|
let(:note) { create(model, author: note_author) } let(:note) { create(model, author: note_author) }
context 'when note is on image' do context 'when note is not on text' do
before do before do
allow_any_instance_of(DiffDiscussion).to receive(:on_image?).and_return(true) allow_any_instance_of(DiffDiscussion).to receive(:on_text?).and_return(false)
end end
it 'does not include diffs with character-level highlighting' do it 'does not include diffs with character-level highlighting' do
is_expected.not_to have_body_text '<span class="p">}</span></span>' is_expected.not_to have_body_text '<span class="p">}</span></span>'
end end
it 'ends the intro with a dot' do
is_expected.to have_body_text "#{note.diff_file.file_path}</a>."
end
end
it 'ends the intro with a colon' do
is_expected.to have_body_text "#{note.diff_file.file_path}</a>:"
end end
it 'includes diffs with character-level highlighting' do it 'includes diffs with character-level highlighting' do
......
...@@ -50,11 +50,17 @@ describe DiscussionOnDiff do ...@@ -50,11 +50,17 @@ describe DiscussionOnDiff do
end end
context "when the diff line does not exist on a legacy diff note" do context "when the diff line does not exist on a legacy diff note" do
subject { create(:legacy_diff_note_on_merge_request).to_discussion }
it "returns an empty array" do it "returns an empty array" do
legacy_note = LegacyDiffNote.new expect(truncated_lines).to eq([])
end
end
allow(subject).to receive(:first_note).and_return(legacy_note) context 'when the discussion is on an image' do
subject { create(:image_diff_note_on_merge_request).to_discussion }
it 'returns an empty array' do
expect(truncated_lines).to eq([]) expect(truncated_lines).to eq([])
end end
end end
......
...@@ -337,24 +337,9 @@ describe DiffNote do ...@@ -337,24 +337,9 @@ describe DiffNote do
end end
describe "image diff notes" do describe "image diff notes" do
let(:path) { "files/images/any_image.png" } subject { build(:image_diff_note_on_merge_request, project: project, noteable: merge_request) }
let!(:position) do
Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
width: 10,
height: 10,
x: 1,
y: 1,
diff_refs: merge_request.diff_refs,
position_type: "image"
)
end
describe "validations" do describe "validations" do
subject { build(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
it { is_expected.not_to validate_presence_of(:line_code) } it { is_expected.not_to validate_presence_of(:line_code) }
it "does not validate diff line" do it "does not validate diff line" do
......
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