Commit 18ef054b authored by Douwe Maan's avatar Douwe Maan

Merge branch '17464-backport-email-syntax-highlighting' into 'master'

Syntax-highlight diffs in push emails

![image](/uploads/8ecbabc65382214b8de63aae24f66cea/image.png)

Based on:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/151



See merge request !4147
parents c50e7b12 a9977f2b
@import "framework/variables";
table.code {
width: 100%;
font-family: monospace;
border: none;
border-collapse: separate;
margin: 0;
padding: 0;
-premailer-cellpadding: 0;
-premailer-cellspacing: 0;
-premailer-width: 100%;
td {
line-height: $code_line_height;
font-family: monospace;
font-size: $code_font_size;
}
td.diff-line-num {
margin: 0;
padding: 0;
border: none;
background: $background-color;
color: rgba(0, 0, 0, 0.3);
padding: 0 5px;
border-right: 1px solid $border-color;
text-align: right;
min-width: 35px;
max-width: 50px;
width: 35px;
}
td.line_content {
display: block;
margin: 0;
padding: 0 0.5em;
border: none;
white-space: pre;
}
}
@import "highlight/white";
...@@ -32,12 +32,6 @@ module EmailsHelper ...@@ -32,12 +32,6 @@ module EmailsHelper
nil nil
end end
def color_email_diff(diffcontent)
formatter = Rouge::Formatters::HTML.new(css_class: 'highlight', inline_theme: 'github')
lexer = Rouge::Lexers::Diff
raw formatter.format(lexer.lex(diffcontent))
end
def password_reset_token_valid_time def password_reset_token_valid_time
valid_hours = Devise.reset_password_within / 60 / 60 valid_hours = Devise.reset_password_within / 60 / 60
if valid_hours >= 24 if valid_hours >= 24
......
...@@ -65,7 +65,8 @@ module Emails ...@@ -65,7 +65,8 @@ module Emails
# used in notify layout # used in notify layout
@target_url = @message.target_url @target_url = @message.target_url
@project = Project.find project_id @project = Project.find(project_id)
@diff_notes_disabled = true
add_project_headers add_project_headers
headers['X-GitLab-Author'] = @message.author_username headers['X-GitLab-Author'] = @message.author_username
......
...@@ -10,6 +10,8 @@ class Notify < BaseMailer ...@@ -10,6 +10,8 @@ class Notify < BaseMailer
include Emails::Builds include Emails::Builds
add_template_helper MergeRequestsHelper add_template_helper MergeRequestsHelper
add_template_helper DiffHelper
add_template_helper BlobHelper
add_template_helper EmailsHelper add_template_helper EmailsHelper
def test_email(recipient_email, subject, body) def test_email(recipient_email, subject, body)
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
%title %title
GitLab GitLab
= stylesheet_link_tag 'notify' = stylesheet_link_tag 'notify'
= yield :head
%body %body
%div.content %div.content
= yield = yield
......
= content_for :head do
= stylesheet_link_tag 'mailers/repository_push_email'
%h3 %h3
#{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name} #{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name}
at #{link_to(@message.project_name_with_namespace, namespace_project_url(@message.project_namespace, @message.project))} at #{link_to(@message.project_name_with_namespace, namespace_project_url(@message.project_namespace, @message.project))}
...@@ -43,26 +46,38 @@ ...@@ -43,26 +46,38 @@
= diff.new_path = diff.new_path
- unless @message.disable_diffs? - unless @message.disable_diffs?
- diff_files = @message.diffs
- if @message.compare_timeout
%h5 The diff was not included because it is too large.
- else
%h4 Changes: %h4 Changes:
- @message.diffs.each_with_index do |diff, i| - diff_files.each_with_index do |diff_file, i|
%li{id: "diff-#{i}"} %li{id: "diff-#{i}"}
%a{href: @message.target_url + "#diff-#{i}"} %a{href: @message.target_url + "#diff-#{i}"}<
- if diff.deleted_file - if diff_file.deleted_file
%strong %strong<
= diff.old_path = diff_file.old_path
deleted deleted
- elsif diff.renamed_file - elsif diff_file.renamed_file
%strong %strong<
= diff.old_path = diff_file.old_path
&rarr; &rarr;
%strong %strong<
= diff.new_path = diff_file.new_path
- else
%strong<
= diff_file.new_path
- if diff_file.too_large?
The diff for this file was not included because it is too large.
- else - else
%strong
= diff.new_path
%hr %hr
= color_email_diff(diff.diff) - diff_commit = diff_file.deleted_file ? @message.diff_refs.first : @message.diff_refs.last
- blob = @message.project.repository.blob_for_diff(diff_commit, diff_file)
- if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
%table.code.white
- diff_file.highlighted_diff_lines.each do |line|
= render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: nil, plain: true}
- else
No preview for this file type
%br %br
- if @message.compare_timeout
%h5 Huge diff. To prevent performance issues changes are hidden
...@@ -25,24 +25,28 @@ ...@@ -25,24 +25,28 @@
- else - else
\- #{diff.new_path} \- #{diff.new_path}
- unless @message.disable_diffs? - unless @message.disable_diffs?
- if @message.compare_timeout
\
\
The diff was not included because it is too large.
- else
\ \
\ \
Changes: Changes:
- @message.diffs.each do |diff| - @message.diffs.each do |diff_file|
\ \
\===================================== \=====================================
- if diff.deleted_file - if diff_file.deleted_file
#{diff.old_path} deleted #{diff_file.old_path} deleted
- elsif diff.renamed_file - elsif diff_file.renamed_file
#{diff.old_path}#{diff.new_path} #{diff_file.old_path}#{diff_file.new_path}
- else - else
= diff.new_path = diff_file.new_path
\===================================== \=====================================
!= diff.diff - if diff_file.too_large?
- if @message.compare_timeout The diff for this file was not included because it is too large.
\ - else
\ != diff_file.diff.diff
Huge diff. To prevent performance issues it was hidden
- if @message.target_url - if @message.target_url
\ \
\ \
......
...@@ -41,7 +41,7 @@ ...@@ -41,7 +41,7 @@
.diff-content.diff-wrap-lines .diff-content.diff-wrap-lines
- # Skip all non non-supported blobs - # Skip all non non-supported blobs
- return unless blob.respond_to?('text?') - return unless blob.respond_to?(:text?)
- if diff_file.too_large? - if diff_file.too_large?
.nothing-here-block This diff could not be displayed because it is too large. .nothing-here-block This diff could not be displayed because it is too large.
- elsif blob_text_viewable?(blob) && !project.repository.diffable?(blob) - elsif blob_text_viewable?(blob) && !project.repository.diffable?(blob)
......
...@@ -27,15 +27,18 @@ class EmailsOnPushWorker ...@@ -27,15 +27,18 @@ class EmailsOnPushWorker
:push :push
end end
diff_refs = nil
compare = nil compare = nil
reverse_compare = false reverse_compare = false
if action == :push if action == :push
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha) compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
diff_refs = [project.merge_base_commit(before_sha, after_sha), project.commit(after_sha)]
return false if compare.same return false if compare.same
if compare.commits.empty? if compare.commits.empty?
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha) compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
diff_refs = [project.merge_base_commit(after_sha, before_sha), project.commit(before_sha)]
reverse_compare = true reverse_compare = true
...@@ -53,6 +56,7 @@ class EmailsOnPushWorker ...@@ -53,6 +56,7 @@ class EmailsOnPushWorker
action: action, action: action,
compare: compare, compare: compare,
reverse_compare: reverse_compare, reverse_compare: reverse_compare,
diff_refs: diff_refs,
send_from_committer_email: send_from_committer_email, send_from_committer_email: send_from_committer_email,
disable_diffs: disable_diffs disable_diffs: disable_diffs
) )
......
...@@ -78,6 +78,7 @@ module Gitlab ...@@ -78,6 +78,7 @@ module Gitlab
config.assets.precompile << "*.png" config.assets.precompile << "*.png"
config.assets.precompile << "print.css" config.assets.precompile << "print.css"
config.assets.precompile << "notify.css" config.assets.precompile << "notify.css"
config.assets.precompile << "mailers/repository_push_email.css"
# Version of your assets, change this if you want to expire all your assets # Version of your assets, change this if you want to expire all your assets
config.assets.version = '1.0' config.assets.version = '1.0'
......
...@@ -5,6 +5,7 @@ module Gitlab ...@@ -5,6 +5,7 @@ module Gitlab
attr_reader :author_id, :ref, :action attr_reader :author_id, :ref, :action
include Gitlab::Routing.url_helpers include Gitlab::Routing.url_helpers
include DiffHelper
delegate :namespace, :name_with_namespace, to: :project, prefix: :project delegate :namespace, :name_with_namespace, to: :project, prefix: :project
delegate :name, to: :author, prefix: :author delegate :name, to: :author, prefix: :author
...@@ -36,7 +37,7 @@ module Gitlab ...@@ -36,7 +37,7 @@ module Gitlab
end end
def diffs def diffs
@diffs ||= (compare.diffs if compare) @diffs ||= (safe_diff_files(compare.diffs, diff_refs) if compare)
end end
def diffs_count def diffs_count
...@@ -47,6 +48,10 @@ module Gitlab ...@@ -47,6 +48,10 @@ module Gitlab
@opts[:compare] @opts[:compare]
end end
def diff_refs
@opts[:diff_refs]
end
def compare_timeout def compare_timeout
diffs.overflow? if diffs diffs.overflow? if diffs
end end
......
...@@ -57,7 +57,7 @@ describe Gitlab::Email::Message::RepositoryPush do ...@@ -57,7 +57,7 @@ describe Gitlab::Email::Message::RepositoryPush do
describe '#diffs' do describe '#diffs' do
subject { message.diffs } subject { message.diffs }
it { is_expected.to all(be_an_instance_of Gitlab::Git::Diff) } it { is_expected.to all(be_an_instance_of Gitlab::Diff::File) }
end end
describe '#diffs_count' do describe '#diffs_count' do
......
...@@ -693,8 +693,9 @@ describe Notify do ...@@ -693,8 +693,9 @@ describe Notify do
let(:commits) { Commit.decorate(compare.commits, nil) } let(:commits) { Commit.decorate(compare.commits, nil) }
let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) } let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
let(:send_from_committer_email) { false } let(:send_from_committer_email) { false }
let(:diff_refs) { [project.merge_base_commit(sample_image_commit.id, sample_commit.id), project.commit(sample_commit.id)] }
subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, diff_refs: diff_refs, send_from_committer_email: send_from_committer_email) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -715,15 +716,15 @@ describe Notify do ...@@ -715,15 +716,15 @@ describe Notify do
is_expected.to have_body_text /Change some files/ is_expected.to have_body_text /Change some files/
end end
it 'includes diffs' do it 'includes diffs with character-level highlighting' do
is_expected.to have_body_text /def archive_formats_regex/ is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
end end
it 'contains a link to the diff' do it 'contains a link to the diff' do
is_expected.to have_body_text /#{diff_path}/ is_expected.to have_body_text /#{diff_path}/
end end
it 'doesn not contain the misleading footer' do it 'does not contain the misleading footer' do
is_expected.not_to have_body_text /you are a member of/ is_expected.not_to have_body_text /you are a member of/
end end
...@@ -797,8 +798,9 @@ describe Notify do ...@@ -797,8 +798,9 @@ describe Notify do
let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) } let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) }
let(:commits) { Commit.decorate(compare.commits, nil) } let(:commits) { Commit.decorate(compare.commits, nil) }
let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) } let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
let(:diff_refs) { [project.merge_base_commit(sample_commit.parent_id, sample_commit.id), project.commit(sample_commit.id)] }
subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, diff_refs: diff_refs) }
it_behaves_like 'it should show Gmail Actions View Commit link' it_behaves_like 'it should show Gmail Actions View Commit link'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -819,8 +821,8 @@ describe Notify do ...@@ -819,8 +821,8 @@ describe Notify do
is_expected.to have_body_text /Change some files/ is_expected.to have_body_text /Change some files/
end end
it 'includes diffs' do it 'includes diffs with character-level highlighting' do
is_expected.to have_body_text /def archive_formats_regex/ is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
end end
it 'contains a link to the diff' do it 'contains a link to the diff' 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