diff options
-rw-r--r-- | app/assets/stylesheets/mailers/highlighted_diff_email.scss (renamed from app/assets/stylesheets/mailers/repository_push_email.scss) | 0 | ||||
-rw-r--r-- | app/mailers/emails/notes.rb | 2 | ||||
-rw-r--r-- | app/models/discussion.rb | 12 | ||||
-rw-r--r-- | app/views/notify/_note_message.text.erb | 5 | ||||
-rw-r--r-- | app/views/notify/_note_mr_or_commit_email.html.haml | 18 | ||||
-rw-r--r-- | app/views/notify/_note_mr_or_commit_email.text.erb | 8 | ||||
-rw-r--r-- | app/views/notify/_simple_diff.text.erb | 3 | ||||
-rw-r--r-- | app/views/notify/note_commit_email.html.haml | 4 | ||||
-rw-r--r-- | app/views/notify/note_commit_email.text.erb | 11 | ||||
-rw-r--r-- | app/views/notify/note_merge_request_email.html.haml | 9 | ||||
-rw-r--r-- | app/views/notify/note_merge_request_email.text.erb | 11 | ||||
-rw-r--r-- | app/views/notify/repository_push_email.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/diffs/_line.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml | 4 | ||||
-rw-r--r-- | spec/mailers/notify_spec.rb | 79 | ||||
-rw-r--r-- | spec/models/discussion_spec.rb | 19 |
16 files changed, 154 insertions, 35 deletions
diff --git a/app/assets/stylesheets/mailers/repository_push_email.scss b/app/assets/stylesheets/mailers/highlighted_diff_email.scss index 8d1a6020ca4..8d1a6020ca4 100644 --- a/app/assets/stylesheets/mailers/repository_push_email.scss +++ b/app/assets/stylesheets/mailers/highlighted_diff_email.scss diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 96116e916dd..0d20c9092c4 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -4,6 +4,7 @@ module Emails setup_note_mail(note_id, recipient_id) @commit = @note.noteable + @discussion = @note.to_discussion if @note.diff_note? @target_url = namespace_project_commit_url(*note_target_url_options) mail_answer_thread(@commit, @@ -24,6 +25,7 @@ module Emails setup_note_mail(note_id, recipient_id) @merge_request = @note.noteable + @discussion = @note.to_discussion if @note.diff_note? @target_url = namespace_project_merge_request_url(*note_target_url_options) mail_answer_thread(@merge_request, note_thread_options(recipient_id)) end diff --git a/app/models/discussion.rb b/app/models/discussion.rb index de06c13481a..75a85563235 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, + :diff_lines, + + to: :diff_file, + allow_nil: true def self.for_notes(notes) notes.group_by(&:discussion_id).values.map { |notes| new(notes) } @@ -159,10 +164,11 @@ class Discussion end # Returns an array of at most 16 highlighted lines above a diff note - def truncated_diff_lines + def truncated_diff_lines(highlight: true) + lines = highlight ? highlighted_diff_lines : diff_lines prev_lines = [] - highlighted_diff_lines.each do |line| + lines.each do |line| if line.meta? prev_lines.clear else diff --git a/app/views/notify/_note_message.text.erb b/app/views/notify/_note_message.text.erb new file mode 100644 index 00000000000..f82cbc9a3fc --- /dev/null +++ b/app/views/notify/_note_message.text.erb @@ -0,0 +1,5 @@ +<% if current_application_settings.email_author_in_body %> + <%= @note.author_name %> wrote: +<% end -%> + +<%= @note.note %> diff --git a/app/views/notify/_note_mr_or_commit_email.html.haml b/app/views/notify/_note_mr_or_commit_email.html.haml new file mode 100644 index 00000000000..edf8dfe7e9e --- /dev/null +++ b/app/views/notify/_note_mr_or_commit_email.html.haml @@ -0,0 +1,18 @@ += content_for :head do + = stylesheet_link_tag 'mailers/highlighted_diff_email' + +New comment + +- if @discussion && @discussion.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, + as: :line, + locals: { diff_file: @note.diff_file, + plain: true, + email: true } + += render 'note_message' diff --git a/app/views/notify/_note_mr_or_commit_email.text.erb b/app/views/notify/_note_mr_or_commit_email.text.erb new file mode 100644 index 00000000000..b4fcdf6b1e9 --- /dev/null +++ b/app/views/notify/_note_mr_or_commit_email.text.erb @@ -0,0 +1,8 @@ +<% if @discussion && @discussion.diff_file -%> + on <%= @note.diff_file.file_path -%> +<% end -%>: + +<%= url %> + +<%= render 'simple_diff' if @discussion -%> +<%= render 'note_message' %> diff --git a/app/views/notify/_simple_diff.text.erb b/app/views/notify/_simple_diff.text.erb new file mode 100644 index 00000000000..c28d1cc34d3 --- /dev/null +++ b/app/views/notify/_simple_diff.text.erb @@ -0,0 +1,3 @@ +<% @discussion.truncated_diff_lines(highlight: false).each do |line| %> +> <%= line.text %> +<% end %> diff --git a/app/views/notify/note_commit_email.html.haml b/app/views/notify/note_commit_email.html.haml index 1d961e4424c..0a650e3b2ca 100644 --- a/app/views/notify/note_commit_email.html.haml +++ b/app/views/notify/note_commit_email.html.haml @@ -1,2 +1,2 @@ -= render 'note_message' - +%p.details + = 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 aaeaf5fdf73..6aa085a172e 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -1,9 +1,2 @@ -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}")) %> - - -Author: <%= @note.author_name %> - -<%= @note.note %> - +New comment for Commit <%= @commit.short_id -%> +<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url } %> diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index ea7e3d199fd..0a650e3b2ca 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,7 +1,2 @@ -- if @note.diff_note? && @note.diff_file - %p.details - New comment on diff for - = link_to @note.diff_file.file_path, @target_url - \: - -= render 'note_message' +%p.details + = 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 8cdab63829e..2ce64c494cf 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -1,9 +1,2 @@ -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}")) %> - - -<%= @note.author_name %> - -<%= @note.note %> - +New comment for Merge Request <%= @merge_request.to_reference -%> +<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url }%> diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index 307c5a11206..25883de257c 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -1,5 +1,5 @@ = content_for :head do - = stylesheet_link_tag 'mailers/repository_push_email' + = stylesheet_link_tag 'mailers/highlighted_diff_email' %h3 #{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name} diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index a3e4b5b777e..16c96b66714 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -25,7 +25,7 @@ %a{href: "##{line_code}", data: { linenumber: link_text }} %td.line_content.noteable_line{ class: type, data: (diff_view_line_data(line_code, diff_file.position(line), type) unless plain) }< - if email - %pre= diff_line_content(line.text) + %pre= line.text - else = diff_line_content(line.text) diff --git a/changelogs/unreleased/hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml b/changelogs/unreleased/hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml new file mode 100644 index 00000000000..73d8a52e001 --- /dev/null +++ b/changelogs/unreleased/hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml @@ -0,0 +1,4 @@ +--- +title: Add git diff context to notifications of new notes on merge requests +merge_request: +author: Heidi Hoopes diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 932a5dc4862..39ba48f61cb 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -50,7 +50,7 @@ describe Notify do context 'when enabled email_author_in_body' do before do - allow_any_instance_of(ApplicationSetting).to receive(:email_author_in_body).and_return(true) + stub_application_setting(email_author_in_body: true) end it 'contains a link to note author' do @@ -229,7 +229,7 @@ describe Notify do context 'when enabled email_author_in_body' do before do - allow_any_instance_of(ApplicationSetting).to receive(:email_author_in_body).and_return(true) + stub_application_setting(email_author_in_body: true) end it 'contains a link to note author' do @@ -607,7 +607,7 @@ describe Notify do context 'when enabled email_author_in_body' do before do - allow_any_instance_of(ApplicationSetting).to receive(:email_author_in_body).and_return(true) + stub_application_setting(email_author_in_body: true) end it 'contains a link to note author' do @@ -686,6 +686,79 @@ 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 /<span class=\"p\">}<\/span><\/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 + stub_application_setting(email_author_in_body: 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 '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 '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..2a67c60b978 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -590,4 +590,23 @@ 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 + it "returns fewer lines" do + expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + + expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + context "when some diff lines are meta" do + it "returns no meta lines" do + expect(subject.diff_lines).to include(be_meta) + expect(truncated_lines).not_to include(be_meta) + end + end + end end |