From 38ed96e9b1a47dca5aa2590fa9b0ade908337435 Mon Sep 17 00:00:00 2001 From: hhoopes Date: Wed, 17 Aug 2016 16:27:01 -0600 Subject: Add diff hunks to notification emails on MR Added diff hunks to notification emails of messages on merge requests. This provides code context to the note. Uses existing template for formatting a diff for email (from repository push notifications). --- CHANGELOG.md | 1 + .../mailers/highlighted_diff_email.scss | 143 +++++++++++++++++++++ .../stylesheets/mailers/repository_push_email.scss | 143 --------------------- .../notify/note_merge_request_email.html.haml | 8 ++ app/views/notify/repository_push_email.html.haml | 2 +- spec/mailers/notify_spec.rb | 19 ++- 6 files changed, 170 insertions(+), 146 deletions(-) create mode 100644 app/assets/stylesheets/mailers/highlighted_diff_email.scss delete mode 100644 app/assets/stylesheets/mailers/repository_push_email.scss diff --git a/CHANGELOG.md b/CHANGELOG.md index 549336e4dff..56f749e94ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -797,6 +797,7 @@ entry. ## 8.11.0 (2016-08-22) - Use test coverage value from the latest successful pipeline in badge. !5862 + - Add git diff context to notifications of new notes on merge requests !5855 (hoopes) - Add test coverage report badge. !5708 - Remove the http_parser.rb dependency by removing the tinder gem. !5758 (tbalthazar) - Add Koding (online IDE) integration diff --git a/app/assets/stylesheets/mailers/highlighted_diff_email.scss b/app/assets/stylesheets/mailers/highlighted_diff_email.scss new file mode 100644 index 00000000000..8d1a6020ca4 --- /dev/null +++ b/app/assets/stylesheets/mailers/highlighted_diff_email.scss @@ -0,0 +1,143 @@ +@import "framework/variables"; + +// This file is largely copied from `highlight/white.scss`, but modified to +// avoid all descendant selectors (`table td`). This is because the CSS inlining +// we use performs dramatically worse on descendant selectors than the +// alternatives. +// +// +// DO NOT ADD ANY DESCENDANT SELECTORS TO THIS FILE. Instead, use (in order of +// preference): plain class selectors, type (element name) selectors, or +// explicit child selectors. + +.code { + background-color: #fff; + font-family: monospace; + font-size: $code_font_size; + -premailer-cellpadding: 0; + -premailer-cellspacing: 0; + -premailer-width: 100%; + + > tr { + line-height: $code_line_height; + } +} + +.diff-line-num { + padding: 0 5px; + text-align: right; + width: 35px; + background-color: $background-color; + color: $black-transparent; + border-right: 1px solid $table-border-gray; + + &.old { + background-color: $line-number-old; + border-right-color: $line-removed-dark; + } + + &.new { + background-color: $line-number-new; + border-right-color: $line-added-dark; + } +} + +.line_content { + padding-left: 0.5em; + padding-right: 0.5em; + + &.old { + background-color: $line-removed; + + > .line > span.idiff, + > .line > span > span.idiff { + background-color: $line-removed-dark; + } + } + + &.new { + background-color: $line-added; + + > .line > span.idiff, + > .line > span > span.idiff { + background-color: $line-added-dark; + } + } + + &.match { + color: $black-transparent; + background-color: $match-line; + } +} + +pre { + margin: 0; +} + +span.highlight_word { + background-color: #fafe3d !important; +} + +.hll { background-color: #f8f8f8; } +.c { color: #998; font-style: italic; } +.err { color: #a61717; background-color: #e3d2d2; } +.k { font-weight: bold; } +.o { font-weight: bold; } +.cm { color: #998; font-style: italic; } +.cp { color: #999; font-weight: bold; } +.c1 { color: #998; font-style: italic; } +.cs { color: #999; font-weight: bold; font-style: italic; } +.gd { color: #000; background-color: #fdd; } +.gd .x { color: #000; background-color: #faa; } +.ge { font-style: italic; } +.gr { color: #a00; } +.gh { color: #999; } +.gi { color: #000; background-color: #dfd; } +.gi .x { color: #000; background-color: #afa; } +.go { color: #888; } +.gp { color: #555; } +.gs { font-weight: bold; } +.gu { color: #800080; font-weight: bold; } +.gt { color: #a00; } +.kc { font-weight: bold; } +.kd { font-weight: bold; } +.kn { font-weight: bold; } +.kp { font-weight: bold; } +.kr { font-weight: bold; } +.kt { color: #458; font-weight: bold; } +.m { color: #099; } +.s { color: #d14; } +.n { color: #333; } +.na { color: teal; } +.nb { color: #0086b3; } +.nc { color: #458; font-weight: bold; } +.no { color: teal; } +.ni { color: purple; } +.ne { color: #900; font-weight: bold; } +.nf { color: #900; font-weight: bold; } +.nn { color: #555; } +.nt { color: navy; } +.nv { color: teal; } +.ow { font-weight: bold; } +.w { color: #bbb; } +.mf { color: #099; } +.mh { color: #099; } +.mi { color: #099; } +.mo { color: #099; } +.sb { color: #d14; } +.sc { color: #d14; } +.sd { color: #d14; } +.s2 { color: #d14; } +.se { color: #d14; } +.sh { color: #d14; } +.si { color: #d14; } +.sx { color: #d14; } +.sr { color: #009926; } +.s1 { color: #d14; } +.ss { color: #990073; } +.bp { color: #999; } +.vc { color: teal; } +.vg { color: teal; } +.vi { color: teal; } +.il { color: #099; } +.gc { color: #999; background-color: #eaf2f5; } diff --git a/app/assets/stylesheets/mailers/repository_push_email.scss b/app/assets/stylesheets/mailers/repository_push_email.scss deleted file mode 100644 index 8d1a6020ca4..00000000000 --- a/app/assets/stylesheets/mailers/repository_push_email.scss +++ /dev/null @@ -1,143 +0,0 @@ -@import "framework/variables"; - -// This file is largely copied from `highlight/white.scss`, but modified to -// avoid all descendant selectors (`table td`). This is because the CSS inlining -// we use performs dramatically worse on descendant selectors than the -// alternatives. -// -// -// DO NOT ADD ANY DESCENDANT SELECTORS TO THIS FILE. Instead, use (in order of -// preference): plain class selectors, type (element name) selectors, or -// explicit child selectors. - -.code { - background-color: #fff; - font-family: monospace; - font-size: $code_font_size; - -premailer-cellpadding: 0; - -premailer-cellspacing: 0; - -premailer-width: 100%; - - > tr { - line-height: $code_line_height; - } -} - -.diff-line-num { - padding: 0 5px; - text-align: right; - width: 35px; - background-color: $background-color; - color: $black-transparent; - border-right: 1px solid $table-border-gray; - - &.old { - background-color: $line-number-old; - border-right-color: $line-removed-dark; - } - - &.new { - background-color: $line-number-new; - border-right-color: $line-added-dark; - } -} - -.line_content { - padding-left: 0.5em; - padding-right: 0.5em; - - &.old { - background-color: $line-removed; - - > .line > span.idiff, - > .line > span > span.idiff { - background-color: $line-removed-dark; - } - } - - &.new { - background-color: $line-added; - - > .line > span.idiff, - > .line > span > span.idiff { - background-color: $line-added-dark; - } - } - - &.match { - color: $black-transparent; - background-color: $match-line; - } -} - -pre { - margin: 0; -} - -span.highlight_word { - background-color: #fafe3d !important; -} - -.hll { background-color: #f8f8f8; } -.c { color: #998; font-style: italic; } -.err { color: #a61717; background-color: #e3d2d2; } -.k { font-weight: bold; } -.o { font-weight: bold; } -.cm { color: #998; font-style: italic; } -.cp { color: #999; font-weight: bold; } -.c1 { color: #998; font-style: italic; } -.cs { color: #999; font-weight: bold; font-style: italic; } -.gd { color: #000; background-color: #fdd; } -.gd .x { color: #000; background-color: #faa; } -.ge { font-style: italic; } -.gr { color: #a00; } -.gh { color: #999; } -.gi { color: #000; background-color: #dfd; } -.gi .x { color: #000; background-color: #afa; } -.go { color: #888; } -.gp { color: #555; } -.gs { font-weight: bold; } -.gu { color: #800080; font-weight: bold; } -.gt { color: #a00; } -.kc { font-weight: bold; } -.kd { font-weight: bold; } -.kn { font-weight: bold; } -.kp { font-weight: bold; } -.kr { font-weight: bold; } -.kt { color: #458; font-weight: bold; } -.m { color: #099; } -.s { color: #d14; } -.n { color: #333; } -.na { color: teal; } -.nb { color: #0086b3; } -.nc { color: #458; font-weight: bold; } -.no { color: teal; } -.ni { color: purple; } -.ne { color: #900; font-weight: bold; } -.nf { color: #900; font-weight: bold; } -.nn { color: #555; } -.nt { color: navy; } -.nv { color: teal; } -.ow { font-weight: bold; } -.w { color: #bbb; } -.mf { color: #099; } -.mh { color: #099; } -.mi { color: #099; } -.mo { color: #099; } -.sb { color: #d14; } -.sc { color: #d14; } -.sd { color: #d14; } -.s2 { color: #d14; } -.se { color: #d14; } -.sh { color: #d14; } -.si { color: #d14; } -.sx { color: #d14; } -.sr { color: #009926; } -.s1 { color: #d14; } -.ss { color: #990073; } -.bp { color: #999; } -.vc { color: teal; } -.vg { color: teal; } -.vi { color: teal; } -.il { color: #099; } -.gc { color: #999; background-color: #eaf2f5; } diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index ea7e3d199fd..de3f32e6415 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,7 +1,15 @@ += content_for :head do + = stylesheet_link_tag 'mailers/highlighted_diff_email' + - if @note.diff_note? && @note.diff_file %p.details New comment on diff for = link_to @note.diff_file.file_path, @target_url \: + .diff-content.code.js-syntax-highlight + %table + - Discussion.new([@note]).truncated_diff_lines.each do |line| + = render "projects/diffs/line", line: line, diff_file: @note.diff_file, plain: true + = render 'note_message' 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/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 932a5dc4862..b40a6b1cbac 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -580,8 +580,10 @@ describe Notify do let(:note_author) { create(:user, name: 'author_name') } let(:note) { create(:note, project: project, author: note_author) } - before :each do - allow(Note).to receive(:find).with(note.id).and_return(note) + before do |example| + unless example.metadata[:skip_before] + allow(Note).to receive(:find).with(note.id).and_return(note) + end end shared_examples 'a note email' do @@ -663,6 +665,19 @@ describe Notify do end end + describe "on a merge request with diffs", :skip_before do + let(:merge_request) { create(:merge_request_with_diffs) } + let(:note_with_diff) {create(:diff_note_on_merge_request)} + + before(:each) { allow(note_with_diff).to receive(:noteable).and_return(merge_request) } + subject { Notify.note_merge_request_email(recipient.id, note_with_diff.id) } + + it 'includes diffs with character-level highlighting' do + expected_line_text = Discussion.new([note_with_diff]).truncated_diff_lines.first.text + is_expected.to have_body_text expected_line_text + end + end + describe 'on an issue' do let(:issue) { create(:issue, project: project) } let(:note_on_issue_path) { namespace_project_issue_path(project.namespace, project, issue, anchor: "note_#{note.id}") } -- cgit v1.2.1 From 24070bac45134c915c13d3e94723a44f59ab4e3a Mon Sep 17 00:00:00 2001 From: hhoopes Date: Mon, 22 Aug 2016 23:36:30 -0600 Subject: Add new template to handle both commit & mr notes Currently comments on commits and merge requests do not require merge request- or commit-specific information, but can use the same template. Rather than change the method which calls the template, I opted to keep the templates separate and create a new template to highlight their identicality, while preserving the option to distinguish them from each other in the future. Also removed some of the inconsistencies between text and html email versions. Still needed is a text-only version of git diffs and testing. --- app/mailers/emails/notes.rb | 2 ++ .../notify/_note_mr_or_commit_email.html.haml | 16 ++++++++++++++++ app/views/notify/note_commit_email.html.haml | 9 +++++++-- app/views/notify/note_commit_email.text.erb | 6 +++--- .../notify/note_merge_request_email.html.haml | 22 +++++++--------------- app/views/notify/note_merge_request_email.text.erb | 6 +++--- spec/mailers/notify_spec.rb | 19 ++----------------- 7 files changed, 40 insertions(+), 40 deletions(-) create mode 100644 app/views/notify/_note_mr_or_commit_email.html.haml 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/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..3e2046aa9cf --- /dev/null +++ b/app/views/notify/_note_mr_or_commit_email.html.haml @@ -0,0 +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' + \: + + %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_commit_email.html.haml b/app/views/notify/note_commit_email.html.haml index 1d961e4424c..f4293a3aa50 100644 --- a/app/views/notify/note_commit_email.html.haml +++ b/app/views/notify/note_commit_email.html.haml @@ -1,2 +1,7 @@ -= render 'note_message' - +%p.details + New comment for Commit + = @commit.short_id + on + = render partial: 'note_mr_or_commit_email', + locals: { note: @note, + discussion: @discussion} diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb index aaeaf5fdf73..a1ef9858021 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -2,8 +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}")) %> - -Author: <%= @note.author_name %> +<% if current_application_settings.email_author_in_body %> + <%= @note.author_name %> wrote: +<% end %> <%= @note.note %> - diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index de3f32e6415..75250b0383d 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,15 +1,7 @@ -= content_for :head do - = stylesheet_link_tag 'mailers/highlighted_diff_email' - -- if @note.diff_note? && @note.diff_file - %p.details - New comment on diff for - = link_to @note.diff_file.file_path, @target_url - \: - - .diff-content.code.js-syntax-highlight - %table - - Discussion.new([@note]).truncated_diff_lines.each do |line| - = render "projects/diffs/line", line: line, diff_file: @note.diff_file, plain: true - -= render 'note_message' +%p.details + New comment for Merge Request + = @merge_request.to_reference + on + = render partial: 'note_mr_or_commit_email', + locals: { note: @note, + discussion: @discussion} diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb index 8cdab63829e..42d29b34ebc 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -2,8 +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}")) %> - -<%= @note.author_name %> +<% if current_application_settings.email_author_in_body %> + <%= @note.author_name %> wrote: +<% end %> <%= @note.note %> - diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index b40a6b1cbac..932a5dc4862 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -580,10 +580,8 @@ describe Notify do let(:note_author) { create(:user, name: 'author_name') } let(:note) { create(:note, project: project, author: note_author) } - before do |example| - unless example.metadata[:skip_before] - allow(Note).to receive(:find).with(note.id).and_return(note) - end + before :each do + allow(Note).to receive(:find).with(note.id).and_return(note) end shared_examples 'a note email' do @@ -665,19 +663,6 @@ describe Notify do end end - describe "on a merge request with diffs", :skip_before do - let(:merge_request) { create(:merge_request_with_diffs) } - let(:note_with_diff) {create(:diff_note_on_merge_request)} - - before(:each) { allow(note_with_diff).to receive(:noteable).and_return(merge_request) } - subject { Notify.note_merge_request_email(recipient.id, note_with_diff.id) } - - it 'includes diffs with character-level highlighting' do - expected_line_text = Discussion.new([note_with_diff]).truncated_diff_lines.first.text - is_expected.to have_body_text expected_line_text - end - end - describe 'on an issue' do let(:issue) { create(:issue, project: project) } let(:note_on_issue_path) { namespace_project_issue_path(project.namespace, project, issue, anchor: "note_#{note.id}") } -- cgit v1.2.1 From f928dba99b0550cefa7534d7fd5bd1ea16609274 Mon Sep 17 00:00:00 2001 From: hhoopes Date: Thu, 25 Aug 2016 10:38:07 -0600 Subject: Change diff highlight/truncate for reusability Previously the `truncated_diff_lines` method for outputting a discussion diff took in already highlighted lines, which meant it wasn't reuseable for truncating ANY lines. In the way it was used, it also meant that for any email truncation, the whole diff was being highlighted before being truncated, meaning wasted time highlighting lines that wouldn't even be used (granted, they were being memoized, so perhaps this wasn't that great of an issue). I refactored truncation away from highlighting, in order to truncate formatted diffs for text templates in email, using `>`s to designate each line, but otherwise retaining the parsing already done to create `diff_lines`. Additionally, while notes on merge requests or commits had already been tested, there was no existing test for notes on a diff on an MR or commit. Added mailer tests for such, and a unit test for truncating diff lines. --- app/models/discussion.rb | 12 ++-- app/views/discussions/_diff_with_notes.html.haml | 2 +- .../notify/_note_mr_or_commit_email.html.haml | 9 +-- app/views/notify/_simple_diff.text.erb | 4 ++ app/views/notify/note_commit_email.html.haml | 6 +- app/views/notify/note_commit_email.text.erb | 2 + .../notify/note_merge_request_email.html.haml | 6 +- app/views/notify/note_merge_request_email.text.erb | 2 + lib/gitlab/diff/file.rb | 13 +++- spec/mailers/notify_spec.rb | 80 +++++++++++++++++++++- spec/models/discussion_spec.rb | 25 +++++++ 11 files changed, 139 insertions(+), 22 deletions(-) create mode 100644 app/views/notify/_simple_diff.text.erb 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 -- cgit v1.2.1 From a761c59a6bfc4d66649910d01e4c8412bb0b40ec Mon Sep 17 00:00:00 2001 From: hhoopes Date: Wed, 31 Aug 2016 10:18:26 -0600 Subject: Add keyword arguments to truncated_diff method * Added keyword arguments to truncated_diff_lines method to allow for using highlighting or not (html templates vs. text) * Tweaked templates for consistency and format appropriateness --- app/models/discussion.rb | 7 ++++--- app/views/discussions/_diff_with_notes.html.haml | 2 +- app/views/notify/_note_message.text.erb | 5 +++++ app/views/notify/_note_mr_or_commit_email.html.haml | 11 +++++++---- app/views/notify/_note_mr_or_commit_email.text.erb | 8 ++++++++ app/views/notify/_simple_diff.text.erb | 5 ++--- app/views/notify/note_commit_email.html.haml | 3 --- app/views/notify/note_commit_email.text.erb | 13 +++---------- app/views/notify/note_merge_request_email.html.haml | 3 --- app/views/notify/note_merge_request_email.text.erb | 13 +++---------- lib/gitlab/diff/file.rb | 11 ++--------- spec/mailers/notify_spec.rb | 8 +------- spec/models/discussion_spec.rb | 4 ++-- 13 files changed, 38 insertions(+), 55 deletions(-) create mode 100644 app/views/notify/_note_message.text.erb create mode 100644 app/views/notify/_note_mr_or_commit_email.text.erb diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 486bfd2c766..9bd37fe6d89 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -27,7 +27,7 @@ class Discussion delegate :blob, :highlighted_diff_lines, - :text_parsed_diff_lines, + :diff_lines, to: :diff_file, allow_nil: true @@ -164,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) + initial_lines = highlight ? highlighted_diff_lines : diff_lines prev_lines = [] - diff_file.diff_lines.each do |line| + initial_lines.each do |line| if line.meta? prev_lines.clear else diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 06493ba0105..5c667e4842b 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.highlighted_diff_lines(discussion.truncated_diff_lines), + collection: discussion.highlighted_diff_lines, as: :line, locals: { diff_file: diff_file, discussions: discussions, 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 index 7033842b557..15e92c42b14 100644 --- a/app/views/notify/_note_mr_or_commit_email.html.haml +++ b/app/views/notify/_note_mr_or_commit_email.html.haml @@ -1,17 +1,20 @@ = content_for :head do = stylesheet_link_tag 'mailers/highlighted_diff_email' +New comment + - if @note.diff_note? && @note.diff_file on = link_to @note.diff_file.file_path, @target_url, class: 'details' - \: +\: +- if @discussion %table = render partial: "projects/diffs/line", - collection: @discussion.highlighted_diff_lines(@discussion.truncated_diff_lines), + collection: @discussion.truncated_diff_lines, as: :line, locals: { diff_file: @note.diff_file, - plain: true, - email: true } + 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..3dd1b4d4c0e --- /dev/null +++ b/app/views/notify/_note_mr_or_commit_email.text.erb @@ -0,0 +1,8 @@ +<% if @note.diff_note? && @note.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 index a5a796bc168..58b0018c0ca 100644 --- a/app/views/notify/_simple_diff.text.erb +++ b/app/views/notify/_simple_diff.text.erb @@ -1,4 +1,3 @@ -<% lines = @discussion.truncated_diff_lines %> -<% @discussion.text_parsed_diff_lines(lines).each do |line| %> - <%= line %> +<% @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 17dcf36689f..0a650e3b2ca 100644 --- a/app/views/notify/note_commit_email.html.haml +++ b/app/views/notify/note_commit_email.html.haml @@ -1,5 +1,2 @@ %p.details - New comment for Commit - = @commit.short_id - = 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 715e58af61c..dc764b61659 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -1,11 +1,4 @@ -New comment for Commit <%= @commit.short_id %> +<% url = url_for(namespace_project_commit_url(@note.project.namespace, @note.project, id: @commit.id, anchor: "note_#{@note.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 %> - -<%= @note.note %> +New comment for Commit <%= @commit.short_id -%> +<%= render partial: 'note_mr_or_commit_email', locals: { url: url } %> diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index b7758f191dc..0a650e3b2ca 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,5 +1,2 @@ %p.details - New comment for Merge Request - = @merge_request.to_reference - = 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 d24e15af91f..e33d15daded 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -1,11 +1,4 @@ -New comment for Merge Request <%= @merge_request.to_reference %> +<% url = url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")) %> -<%= 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 %> - -<%= @note.note %> +New comment for Merge Request <%= @merge_request.to_reference -%> +<%= render partial: 'note_mr_or_commit_email', locals: { url: url }%> diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 9b60102947a..84784aaf2fd 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -76,15 +76,8 @@ module Gitlab @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a end - 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 + def highlighted_diff_lines + @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight 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 a80eb114c17..824b516f856 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -697,7 +697,7 @@ describe Notify do 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>/ + is_expected.to have_body_text /}<\/span><\/span>/ end it 'contains a link to the diff file' do @@ -743,9 +743,6 @@ describe Notify do 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 @@ -757,9 +754,6 @@ describe Notify do 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 diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index d4b1f480c56..187d0bc0150 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -595,7 +595,7 @@ describe Discussion, model: true 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(:initial_line_count) { subject.diff_lines.count } let(:truncated_line_count) { truncated_lines.count } it "returns fewer lines" do @@ -606,7 +606,7 @@ describe Discussion, model: true do end context "when some diff lines are meta" do - let(:initial_meta_lines?) { subject.diff_file.diff_lines.any?(&:meta?) } + let(:initial_meta_lines?) { subject.diff_lines.any?(&:meta?) } let(:truncated_meta_lines?) { truncated_lines.any?(&:meta?) } it "returns no meta lines" do -- cgit v1.2.1 From c0931722509bba7b26ce46777b6c71f976bc4b7b Mon Sep 17 00:00:00 2001 From: hhoopes Date: Fri, 2 Sep 2016 22:25:53 -0600 Subject: Clean up rubocop complaint --- spec/mailers/notify_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 824b516f856..76ea5f6be31 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -693,7 +693,7 @@ describe Notify do allow(Note).to receive(:find).with(note.id).and_return(note) end - shared_examples 'a note email on a diff' do | model | + 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 -- cgit v1.2.1 From 938de31167e262cb3eb952f6be1797b4c891d34c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 14 Nov 2016 13:25:25 +0000 Subject: Fix CHANGELOG --- CHANGELOG.md | 1 - .../hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/hoopes-gitlab-ce-21027-add-diff-hunks-to-notification-emails.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 56f749e94ac..549336e4dff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -797,7 +797,6 @@ entry. ## 8.11.0 (2016-08-22) - Use test coverage value from the latest successful pipeline in badge. !5862 - - Add git diff context to notifications of new notes on merge requests !5855 (hoopes) - Add test coverage report badge. !5708 - Remove the http_parser.rb dependency by removing the tinder gem. !5758 (tbalthazar) - Add Koding (online IDE) integration 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 -- cgit v1.2.1 From 854fbbfb07b84394cc952d0ae20b7f29957e6555 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 17 Nov 2016 22:22:39 +0000 Subject: Tidy up text emails --- app/models/discussion.rb | 7 +++++-- app/views/notify/_note_mr_or_commit_email.text.erb | 2 +- app/views/notify/_simple_diff.text.erb | 2 +- app/views/notify/note_commit_email.text.erb | 4 +--- app/views/notify/note_merge_request_email.text.erb | 4 +--- spec/mailers/notify_spec.rb | 8 ++++---- 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 9bd37fe6d89..75a85563235 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -165,18 +165,21 @@ class Discussion # Returns an array of at most 16 highlighted lines above a diff note def truncated_diff_lines(highlight: true) - initial_lines = highlight ? highlighted_diff_lines : diff_lines + lines = highlight ? highlighted_diff_lines : diff_lines prev_lines = [] - initial_lines.each do |line| + 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/notify/_note_mr_or_commit_email.text.erb b/app/views/notify/_note_mr_or_commit_email.text.erb index 3dd1b4d4c0e..b4fcdf6b1e9 100644 --- a/app/views/notify/_note_mr_or_commit_email.text.erb +++ b/app/views/notify/_note_mr_or_commit_email.text.erb @@ -1,4 +1,4 @@ -<% if @note.diff_note? && @note.diff_file -%> +<% if @discussion && @discussion.diff_file -%> on <%= @note.diff_file.file_path -%> <% end -%>: diff --git a/app/views/notify/_simple_diff.text.erb b/app/views/notify/_simple_diff.text.erb index 58b0018c0ca..c28d1cc34d3 100644 --- a/app/views/notify/_simple_diff.text.erb +++ b/app/views/notify/_simple_diff.text.erb @@ -1,3 +1,3 @@ <% @discussion.truncated_diff_lines(highlight: false).each do |line| %> - <%= "> " + line.text %> +> <%= line.text %> <% end %> diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb index dc764b61659..6aa085a172e 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -1,4 +1,2 @@ -<% url = url_for(namespace_project_commit_url(@note.project.namespace, @note.project, id: @commit.id, anchor: "note_#{@note.id}")) %> - New comment for Commit <%= @commit.short_id -%> -<%= render partial: 'note_mr_or_commit_email', locals: { url: url } %> +<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url } %> diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb index e33d15daded..2ce64c494cf 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -1,4 +1,2 @@ -<% url = url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")) %> - New comment for Merge Request <%= @merge_request.to_reference -%> -<%= render partial: 'note_mr_or_commit_email', locals: { url: url }%> +<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url }%> diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 76ea5f6be31..1c4ecf83141 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 @@ -726,7 +726,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 -- cgit v1.2.1 From 93f6fcc91e5f241aeef13e96a16bbb6f4027e2fe Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 22 Nov 2016 12:46:00 +0000 Subject: Don't remove + / - signs from diff emails In the browser, we remove the + and - signs from the front of a diff line because we add them in with CSS, so they aren't copied. We can't do that in an email, because the CSS isn't supported, so we should keep them in that case. --- app/helpers/diff_helper.rb | 4 +++- app/views/projects/diffs/_line.html.haml | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index f489f9aa0d6..ce16d971dc6 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -51,9 +51,11 @@ module DiffHelper html.html_safe end - def diff_line_content(line) + def diff_line_content(line, email: false) if line.blank? " ".html_safe + elsif email + line.html_safe else line.sub(/^[\-+ ]/, '').html_safe end diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index a3e4b5b777e..bf2519d4f1c 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= diff_line_content(line.text, email: true) - else = diff_line_content(line.text) -- cgit v1.2.1 From 87a2762b8b001aa8b8d715c964f5ce99d2585ead Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 23 Nov 2016 16:21:45 +0000 Subject: Don't use diff_line_content for emails --- app/helpers/diff_helper.rb | 4 +--- app/views/projects/diffs/_line.html.haml | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index ce16d971dc6..f489f9aa0d6 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -51,11 +51,9 @@ module DiffHelper html.html_safe end - def diff_line_content(line, email: false) + def diff_line_content(line) if line.blank? " ".html_safe - elsif email - line.html_safe else line.sub(/^[\-+ ]/, '').html_safe end diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index bf2519d4f1c..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, email: true) + %pre= line.text - else = diff_line_content(line.text) -- cgit v1.2.1 From b8917eb75e94cb13b02534c920ee926c9e97174e Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 23 Nov 2016 16:25:31 +0000 Subject: Fix spec style --- spec/mailers/notify_spec.rb | 1 + spec/models/discussion_spec.rb | 14 ++++---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 1c4ecf83141..39ba48f61cb 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -646,6 +646,7 @@ 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 } diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index 187d0bc0150..2a67c60b978 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -595,23 +595,17 @@ describe Discussion, model: true 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_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(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES - expect(truncated_line_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 - let(:initial_meta_lines?) { subject.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 + expect(subject.diff_lines).to include(be_meta) + expect(truncated_lines).not_to include(be_meta) end end end -- cgit v1.2.1 From 87665170eccfc423fc3f7fe2cadd208d808a6847 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 23 Nov 2016 16:25:37 +0000 Subject: Use assigned variables better --- app/views/discussions/_diff_with_notes.html.haml | 2 +- app/views/notify/_note_mr_or_commit_email.html.haml | 6 ++---- lib/gitlab/diff/file.rb | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 5c667e4842b..3a95a652810 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.highlighted_diff_lines, + collection: 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 15e92c42b14..edf8dfe7e9e 100644 --- a/app/views/notify/_note_mr_or_commit_email.html.haml +++ b/app/views/notify/_note_mr_or_commit_email.html.haml @@ -3,12 +3,10 @@ New comment -- if @note.diff_note? && @note.diff_file +- if @discussion && @discussion.diff_file on = link_to @note.diff_file.file_path, @target_url, class: 'details' -\: - -- if @discussion + \: %table = render partial: "projects/diffs/line", collection: @discussion.truncated_diff_lines, diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 84784aaf2fd..c6bf25b5874 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -69,7 +69,7 @@ module Gitlab diff_refs.try(:head_sha) end - attr_writer :highlighted_diff_lines, :text_parsed_diff_lines + attr_writer :highlighted_diff_lines # Array of Gitlab::Diff::Line objects def diff_lines -- cgit v1.2.1