Automatic merge of gitlab-org/gitlab-ce master

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

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'
 

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 -%>

changelogs/unreleased/dm-note-email-image-diff-discussion.yml

+---
+title: Fix notification email for image diff notes
+merge_request:
+author:
+type: fixed

config/initializers/8_metrics.rb → config/initializers/zz_metrics.rb

+# This file was prefixed with zz_ because we want to load it the last!
+# See: https://gitlab.com/gitlab-org/gitlab-ce/issues/55611
+
 # Autoload all classes that we want to instrument, and instrument the methods we
 # need. This takes the Gitlab::Metrics::Instrumentation module as an argument so
 # that we can stub it for testing, as it is only called when metrics are

doc/development/instrumentation.md

@@ -35,7 +35,7 @@ Using this method is in general preferred over directly calling the various
 instrumentation methods.
 
 Method instrumentation should be added in the initializer
-`config/initializers/8_metrics.rb`.
+`config/initializers/zz_metrics.rb`.
 
 ### Examples
 

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

spec/initializers/8_metrics_spec.rb → spec/initializers/zz_metrics_spec.rb

@@ -16,7 +16,7 @@ describe 'instrument_classes' do
   end
 
   it 'can autoload and instrument all files' do
-    require_relative '../../config/initializers/8_metrics'
+    require_relative '../../config/initializers/zz_metrics'
     expect { instrument_classes(config) }.not_to raise_error
   end
 end

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

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

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