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

8 files changed, 36 insertions, 36 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 266c37fa3a1..86b61248534 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -39,6 +39,7 @@ module DiscussionOnDiff
 
   # Returns an array of at most 16 highlighted lines above a diff note
   def truncated_diff_lines(highlight: true, diff_limit: nil)
+    return [] unless on_text?
     return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote)
 
     diff_limit = [diff_limit, NUMBER_OF_TRUNCATED_DIFF_LINES].compact.min
diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml
index 1fbae2f64ed..83c7f548975 100644
--- a/app/views/notify/_note_email.html.haml
+++ b/app/views/notify/_note_email.html.haml
@@ -4,17 +4,13 @@
 - note_style = local_assigns.fetch(:note_style, "")
 
 - discussion = note.discussion if note.part_of_discussion?
-- diff_discussion = discussion&.diff_discussion?
-- on_image = discussion.on_image? if diff_discussion
 
 - if discussion
-  - phrase_end_char = on_image ? "." : ":"
-
   %p{ style: "color: #777777;" }
-    = succeed phrase_end_char do
+    = succeed ':' do
       = link_to note.author_name, user_url(note.author)
 
-      - if diff_discussion
+      - if discussion&.diff_discussion?
         - if discussion.new_discussion?
           started a new discussion
         - else
@@ -31,7 +27,7 @@
   %p.details
     #{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
     = stylesheet_link_tag 'mailers/highlighted_diff_email'
 
diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb
index 4bf252b6ce1..50209c46ed1 100644
--- a/app/views/notify/_note_email.text.erb
+++ b/app/views/notify/_note_email.text.erb
@@ -20,7 +20,7 @@
 
 
 <%  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| -%>
 <%=     "> #{line.text}\n" -%>
 <%    end -%>
diff --git a/changelogs/unreleased/dm-note-email-image-diff-discussion.yml b/changelogs/unreleased/dm-note-email-image-diff-discussion.yml
new file mode 100644
index 00000000000..6532052e132
--- /dev/null
+++ b/changelogs/unreleased/dm-note-email-image-diff-discussion.yml
@@ -0,0 +1,5 @@
+---
+title: Fix notification email for image diff notes
+merge_request:
+author:
+type: fixed
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 2d1f48bf249..c51f2f958f9 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -64,6 +64,21 @@ FactoryBot.define do
         resolved_at { Time.now }
         resolved_by { create(:user) }
       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
 
     factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index f6e5c9d33ac..f2d99872401 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -890,22 +890,14 @@ describe Notify do
       shared_examples 'an email for a note on a diff discussion' do  |model|
         let(:note) { create(model, author: note_author) }
 
-        context 'when note is on image' do
+        context 'when note is not on text' 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
 
           it 'does not include diffs with character-level highlighting' do
             is_expected.not_to have_body_text '<span class="p">}</span></span>'
           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
 
         it 'includes diffs with character-level highlighting' do
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
index 73eb7a1160d..4b16e6e3902 100644
--- a/spec/models/concerns/discussion_on_diff_spec.rb
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -50,11 +50,17 @@ describe DiscussionOnDiff do
     end
 
     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
-        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([])
       end
     end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 40ce8ab736a..fda00a693f0 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -337,24 +337,9 @@ describe DiffNote do
   end
 
   describe "image diff notes" do
-    let(:path) { "files/images/any_image.png" }
-
-    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
+    subject { build(:image_diff_note_on_merge_request, project: project, noteable: merge_request) }
 
     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 "does not validate diff line" do