diff --git a/app/models/discussion.rb b/app/models/discussion.rb index de06c13481a..486bfd2c766 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -25,7 +25,12 @@ class Discussion to: :last_resolved_note, allow_nil: true - delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true + delegate :blob, + :highlighted_diff_lines, + :text_parsed_diff_lines, + + to: :diff_file, + allow_nil: true def self.for_notes(notes) notes.group_by(&:discussion_id).values.map { |notes| new(notes) } @@ -162,18 +167,15 @@ class Discussion def truncated_diff_lines prev_lines = [] - highlighted_diff_lines.each do |line| + diff_file.diff_lines.each do |line| if line.meta? prev_lines.clear else prev_lines << line - break if for_line?(line) - prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES end end - prev_lines end diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 3a95a652810..06493ba0105 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -9,7 +9,7 @@ %table - discussions = { discussion.original_line_code => discussion } = render partial: "projects/diffs/line", - collection: discussion.truncated_diff_lines, + collection: discussion.highlighted_diff_lines(discussion.truncated_diff_lines), as: :line, locals: { diff_file: diff_file, discussions: discussions, diff --git a/app/views/notify/_note_mr_or_commit_email.html.haml b/app/views/notify/_note_mr_or_commit_email.html.haml index 3e2046aa9cf..7033842b557 100644 --- a/app/views/notify/_note_mr_or_commit_email.html.haml +++ b/app/views/notify/_note_mr_or_commit_email.html.haml @@ -1,15 +1,16 @@ = content_for :head do = stylesheet_link_tag 'mailers/highlighted_diff_email' -- if note.diff_note? && note.diff_file - = link_to note.diff_file.file_path, @target_url, class: 'details' +- if @note.diff_note? && @note.diff_file + on + = link_to @note.diff_file.file_path, @target_url, class: 'details' \: %table = render partial: "projects/diffs/line", - collection: discussion.truncated_diff_lines, + collection: @discussion.highlighted_diff_lines(@discussion.truncated_diff_lines), as: :line, - locals: { diff_file: note.diff_file, + locals: { diff_file: @note.diff_file, plain: true, email: true } diff --git a/app/views/notify/_simple_diff.text.erb b/app/views/notify/_simple_diff.text.erb new file mode 100644 index 00000000000..a5a796bc168 --- /dev/null +++ b/app/views/notify/_simple_diff.text.erb @@ -0,0 +1,4 @@ +<% lines = @discussion.truncated_diff_lines %> +<% @discussion.text_parsed_diff_lines(lines).each do |line| %> + <%= line %> +<% end %> diff --git a/app/views/notify/note_commit_email.html.haml b/app/views/notify/note_commit_email.html.haml index f4293a3aa50..17dcf36689f 100644 --- a/app/views/notify/note_commit_email.html.haml +++ b/app/views/notify/note_commit_email.html.haml @@ -1,7 +1,5 @@ %p.details New comment for Commit = @commit.short_id - on - = render partial: 'note_mr_or_commit_email', - locals: { note: @note, - discussion: @discussion} + + = render 'note_mr_or_commit_email' diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb index a1ef9858021..715e58af61c 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -2,6 +2,8 @@ New comment for Commit <%= @commit.short_id %> <%= url_for(namespace_project_commit_url(@note.project.namespace, @note.project, id: @commit.id, anchor: "note_#{@note.id}")) %> +<%= render 'simple_diff' if @discussion %> + <% if current_application_settings.email_author_in_body %> <%= @note.author_name %> wrote: <% end %> diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index 75250b0383d..b7758f191dc 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,7 +1,5 @@ %p.details New comment for Merge Request = @merge_request.to_reference - on - = render partial: 'note_mr_or_commit_email', - locals: { note: @note, - discussion: @discussion} + + = render 'note_mr_or_commit_email' diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb index 42d29b34ebc..d24e15af91f 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -2,6 +2,8 @@ New comment for Merge Request <%= @merge_request.to_reference %> <%= url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")) %> +<%= render 'simple_diff' if @discussion %> + <% if current_application_settings.email_author_in_body %> <%= @note.author_name %> wrote: <% end %> diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index c6bf25b5874..9b60102947a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -69,15 +69,22 @@ module Gitlab diff_refs.try(:head_sha) end - attr_writer :highlighted_diff_lines + attr_writer :highlighted_diff_lines, :text_parsed_diff_lines # Array of Gitlab::Diff::Line objects def diff_lines @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a end - def highlighted_diff_lines - @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight + def highlighted_diff_lines(lines = self) + @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(lines, repository: self.repository).highlight + end + + def text_parsed_diff_lines(lines) + @text_parsed_diff_lines ||= + lines.map do | line | + "> " + line.text + end end # Array[] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 932a5dc4862..a80eb114c17 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -646,7 +646,6 @@ describe Notify do before(:each) { allow(note).to receive(:noteable).and_return(merge_request) } subject { Notify.note_merge_request_email(recipient.id, note.id) } - it_behaves_like 'a note email' it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } @@ -686,6 +685,85 @@ describe Notify do end end end + + context 'items that are noteable, emails for a note on a diff' do + let(:note_author) { create(:user, name: 'author_name') } + + before :each do + allow(Note).to receive(:find).with(note.id).and_return(note) + end + + shared_examples 'a note email on a diff' do | model | + let(:note) { create(model, project: project, author: note_author) } + + it "includes diffs with character-level highlighting" do + is_expected.to have_body_text /\vars = {<\/span>/ + end + + it 'contains a link to the diff file' do + is_expected.to have_body_text /#{note.diff_file.file_path}/ + end + + it_behaves_like 'it should have Gmail Actions links' + + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + expect(sender.display_name).to eq(note_author.name) + expect(sender.address).to eq(gitlab_sender) + end + + it 'is sent to the given recipient' do + is_expected.to deliver_to recipient.notification_email + end + + it 'contains the message from the note' do + is_expected.to have_body_text /#{note.note}/ + end + + it 'does not contain note author' do + is_expected.not_to have_body_text /wrote\:/ + end + + context 'when enabled email_author_in_body' do + before do + allow_any_instance_of(ApplicationSetting).to receive(:email_author_in_body).and_return(true) + end + + it 'contains a link to note author' do + is_expected.to have_body_text note.author_name + is_expected.to have_body_text /wrote\:/ + end + end + end + + describe 'on a commit' do + let(:commit) { project.commit } + let(:note) { create(:diff_note_on_commit) } + + subject { Notify.note_commit_email(recipient.id, note.id) } + + it_behaves_like 'a note email on a diff', :diff_note_on_commit + # it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + # let(:model) { commit } + # end + it_behaves_like 'it should show Gmail Actions View Commit link' + it_behaves_like 'a user cannot unsubscribe through footer link' + end + + describe 'on a merge request' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:note) { create(:diff_note_on_merge_request) } + + subject { Notify.note_merge_request_email(recipient.id, note.id) } + + it_behaves_like 'a note email on a diff', :diff_note_on_merge_request + # it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + # let(:model) { merge_request } + # end + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' + end + end end context 'for a group' do diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index 0142706d140..d4b1f480c56 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -590,4 +590,29 @@ describe Discussion, model: true do end end end + + describe "#truncated_diff_lines" do + let(:truncated_lines) { subject.truncated_diff_lines } + + context "when diff is greater than allowed number of truncated diff lines " do + let(:initial_line_count) { subject.diff_file.diff_lines.count } + let(:truncated_line_count) { truncated_lines.count } + + it "returns fewer lines" do + expect(initial_line_count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + + expect(truncated_line_count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + context "when some diff lines are meta" do + let(:initial_meta_lines?) { subject.diff_file.diff_lines.any?(&:meta?) } + let(:truncated_meta_lines?) { truncated_lines.any?(&:meta?) } + + it "returns no meta lines" do + expect(initial_meta_lines?).to be true + expect(truncated_meta_lines?).to be false + end + end + end end