Merge branch 'ce-4326-one-notification-per-code-review' into 'master'

Backports changes made to One notification per code review

See merge request gitlab-org/gitlab-ce!23656

5 files changed, 61 insertions, 17 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index c180d7b7c9a..266c37fa3a1 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -38,12 +38,13 @@ module DiscussionOnDiff
   end
 
   # Returns an array of at most 16 highlighted lines above a diff note
-  def truncated_diff_lines(highlight: true)
+  def truncated_diff_lines(highlight: true, diff_limit: nil)
     return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote)
 
+    diff_limit = [diff_limit, NUMBER_OF_TRUNCATED_DIFF_LINES].compact.min
     lines = highlight ? highlighted_diff_lines : diff_lines
 
-    initial_line_index = [diff_line.index - NUMBER_OF_TRUNCATED_DIFF_LINES + 1, 0].max
+    initial_line_index = [diff_line.index - diff_limit + 1, 0].max
 
     prev_lines = []
 
diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml
index 94bd6f96dbc..1fbae2f64ed 100644
--- a/app/views/notify/_note_email.html.haml
+++ b/app/views/notify/_note_email.html.haml
@@ -1,13 +1,18 @@
-- discussion = @note.discussion if @note.part_of_discussion?
+- note = local_assigns.fetch(:note, @note)
+- diff_limit = local_assigns.fetch(:diff_limit, nil)
+- target_url = local_assigns.fetch(:target_url, @target_url)
+- 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.details
+  %p{ style: "color: #777777;" }
     = succeed phrase_end_char do
-      = link_to @note.author_name, user_url(@note.author)
+      = link_to note.author_name, user_url(note.author)
 
       - if diff_discussion
         - if discussion.new_discussion?
@@ -15,16 +20,16 @@
         - else
           commented on a discussion
 
-        on #{link_to discussion.file_path, @target_url}
+        on #{link_to discussion.file_path, target_url}
       - else
         - if discussion.new_discussion?
           started a new discussion
         - else
-          commented on a #{link_to 'discussion', @target_url}
+          commented on a #{link_to 'discussion', target_url}
 
 - elsif Gitlab::CurrentSettings.email_author_in_body
   %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
   = content_for :head do
@@ -32,11 +37,11 @@
 
   %table
     = render partial: "projects/diffs/line",
-      collection: discussion.truncated_diff_lines,
+      collection: discussion.truncated_diff_lines(diff_limit: diff_limit),
       as: :line,
       locals: { diff_file: discussion.diff_file,
         plain: true,
         email: true }
 
-%div
-  = markdown(@note.note, pipeline: :email, author: @note.author)
+%div{ style: note_style }
+  = markdown(note.note, pipeline: :email, author: note.author)
diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb
index c319cb55e87..4bf252b6ce1 100644
--- a/app/views/notify/_note_email.text.erb
+++ b/app/views/notify/_note_email.text.erb
@@ -1,6 +1,9 @@
-<%  discussion = @note.discussion if @note.part_of_discussion? -%>
+<% note = local_assigns.fetch(:note, @note) -%>
+<% diff_limit = local_assigns.fetch(:diff_limit, nil) -%>
+
+<%  discussion = note.discussion if note.part_of_discussion? -%>
 <%  if discussion && !discussion.individual_note? -%>
-<%=   @note.author_name -%>
+<%=   note.author_name -%>
 <%    if discussion.new_discussion? -%>
 <%=     " started a new discussion" -%>
 <%    else -%>
@@ -13,14 +16,14 @@
 
 
 <%  elsif Gitlab::CurrentSettings.email_author_in_body -%>
-<%=     "#{@note.author_name} commented:" -%>
+<%=     "#{note.author_name} commented:" -%>
 
 
 <%  end -%>
 <%  if discussion&.diff_discussion? -%>
-<%    discussion.truncated_diff_lines(highlight: false).each do |line| -%>
+<%    discussion.truncated_diff_lines(highlight: false, diff_limit: diff_limit).each do |line| -%>
 <%=     "> #{line.text}\n" -%>
 <%    end -%>
 
 <%  end -%>
-<%= @note.note -%>
+<%= note.note -%>
diff --git a/app/workers/new_note_worker.rb b/app/workers/new_note_worker.rb
index 42f5b945a75..98f9f45e608 100644
--- a/app/workers/new_note_worker.rb
+++ b/app/workers/new_note_worker.rb
@@ -8,11 +8,18 @@ class NewNoteWorker
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(note_id, _params = {})
     if note = Note.find_by(id: note_id)
-      NotificationService.new.new_note(note)
+      NotificationService.new.new_note(note) unless skip_notification?(note)
       Notes::PostProcessService.new(note).execute
     else
       Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
     end
   end
+
+  private
+
+  # EE-only method
+  def skip_notification?(note)
+    false
+  end
   # rubocop: enable CodeReuse/ActiveRecord
 end
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
index 8cd129dc851..73eb7a1160d 100644
--- a/spec/models/concerns/discussion_on_diff_spec.rb
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -12,6 +12,34 @@ describe DiscussionOnDiff do
 
         expect(truncated_lines.count).to be <= DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES
       end
+
+      context 'with truncated diff lines diff limit set' do
+        let(:truncated_lines) do
+          subject.truncated_diff_lines(
+            diff_limit: diff_limit
+          )
+        end
+
+        context 'when diff limit is higher than default' do
+          let(:diff_limit) { DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES + 1 }
+
+          it 'returns fewer lines than the default' do
+            expect(subject.diff_lines.count).to be > diff_limit
+
+            expect(truncated_lines.count).to be <= DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES
+          end
+        end
+
+        context 'when diff_limit is lower than default' do
+          let(:diff_limit) { 3 }
+
+          it 'returns fewer lines than the default' do
+            expect(subject.diff_lines.count).to be > DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES
+
+            expect(truncated_lines.count).to be <= diff_limit
+          end
+        end
+      end
     end
 
     context "when some diff lines are meta" do