summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/discussion.rb12
-rw-r--r--app/views/discussions/_diff_with_notes.html.haml2
-rw-r--r--app/views/notify/_note_mr_or_commit_email.html.haml9
-rw-r--r--app/views/notify/_simple_diff.text.erb4
-rw-r--r--app/views/notify/note_commit_email.html.haml6
-rw-r--r--app/views/notify/note_commit_email.text.erb2
-rw-r--r--app/views/notify/note_merge_request_email.html.haml6
-rw-r--r--app/views/notify/note_merge_request_email.text.erb2
-rw-r--r--lib/gitlab/diff/file.rb13
-rw-r--r--spec/mailers/notify_spec.rb80
-rw-r--r--spec/models/discussion_spec.rb25
11 files changed, 139 insertions, 22 deletions
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[<Hash>] 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 /\<span class='idiff left right'>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
View Lorry Controller Status