Commit 7385e7cd authored by Tiago Botelho's avatar Tiago Botelho

Backports changes made to One notification per code review

The EE merge request can be found here:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8442
parent 61d91f64
...@@ -38,12 +38,13 @@ module DiscussionOnDiff ...@@ -38,12 +38,13 @@ module DiscussionOnDiff
end end
# 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) def truncated_diff_lines(highlight: true, diff_limit: nil)
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
lines = highlight ? highlighted_diff_lines : diff_lines 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 = [] prev_lines = []
......
- 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? - diff_discussion = discussion&.diff_discussion?
- on_image = discussion.on_image? if diff_discussion - on_image = discussion.on_image? if diff_discussion
- if discussion - if discussion
- phrase_end_char = on_image ? "." : ":" - phrase_end_char = on_image ? "." : ":"
%p.details %p{ style: "color: #777777;" }
= succeed phrase_end_char do = 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 diff_discussion
- if discussion.new_discussion? - if discussion.new_discussion?
...@@ -15,16 +20,16 @@ ...@@ -15,16 +20,16 @@
- else - else
commented on a discussion commented on a discussion
on #{link_to discussion.file_path, @target_url} on #{link_to discussion.file_path, target_url}
- else - else
- if discussion.new_discussion? - if discussion.new_discussion?
started a new discussion started a new discussion
- else - else
commented on a #{link_to 'discussion', @target_url} commented on a #{link_to 'discussion', target_url}
- elsif Gitlab::CurrentSettings.email_author_in_body - elsif Gitlab::CurrentSettings.email_author_in_body
%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 diff_discussion && !on_image
= content_for :head do = content_for :head do
...@@ -32,11 +37,11 @@ ...@@ -32,11 +37,11 @@
%table %table
= render partial: "projects/diffs/line", = render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines, collection: discussion.truncated_diff_lines(diff_limit: diff_limit),
as: :line, as: :line,
locals: { diff_file: discussion.diff_file, locals: { diff_file: discussion.diff_file,
plain: true, plain: true,
email: true } email: true }
%div %div{ style: note_style }
= markdown(@note.note, pipeline: :email, author: @note.author) = markdown(note.note, pipeline: :email, author: note.author)
<% 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? -%> <% if discussion && !discussion.individual_note? -%>
<%= @note.author_name -%> <%= note.author_name -%>
<% if discussion.new_discussion? -%> <% if discussion.new_discussion? -%>
<%= " started a new discussion" -%> <%= " started a new discussion" -%>
<% else -%> <% else -%>
...@@ -13,14 +16,14 @@ ...@@ -13,14 +16,14 @@
<% elsif Gitlab::CurrentSettings.email_author_in_body -%> <% elsif Gitlab::CurrentSettings.email_author_in_body -%>
<%= "#{@note.author_name} commented:" -%> <%= "#{note.author_name} commented:" -%>
<% end -%> <% end -%>
<% if discussion&.diff_discussion? -%> <% 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" -%> <%= "> #{line.text}\n" -%>
<% end -%> <% end -%>
<% end -%> <% end -%>
<%= @note.note -%> <%= note.note -%>
...@@ -8,11 +8,18 @@ class NewNoteWorker ...@@ -8,11 +8,18 @@ class NewNoteWorker
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def perform(note_id, _params = {}) def perform(note_id, _params = {})
if note = Note.find_by(id: note_id) 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 Notes::PostProcessService.new(note).execute
else else
Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job") Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
end end
end end
private
# EE-only method
def skip_notification?(note)
false
end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
end end
...@@ -12,6 +12,34 @@ describe DiscussionOnDiff do ...@@ -12,6 +12,34 @@ describe DiscussionOnDiff do
expect(truncated_lines.count).to be <= DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES expect(truncated_lines.count).to be <= DiffDiscussion::NUMBER_OF_TRUNCATED_DIFF_LINES
end 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 end
context "when some diff lines are meta" do context "when some diff lines are meta" 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