Merge branch 'reply-by-email-inline-diff' into 'master'

Email on push inline diff

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/3087 and https://gitlab.com/gitlab-org/gitlab-ce/issues/12821

I will backport 16745c1bb386443d8ea3395d23048b1d40ea0b56 and 92c2effbb334246bfed15a0fd78a58f1cfc8be34 to CE.

In a regular browser:

![Screen_Shot_2016-01-28_at_18.11.36](/uploads/662106793138493165e532ebe575b5f1/Screen_Shot_2016-01-28_at_18.11.36.png)

In Gmail (in Chrome):

![chromegmailnew-vertical-allowed-1366-1](/uploads/7887d702577a16056ba8585220d50e04/chromegmailnew-vertical-allowed-1366-1.png)

Gmail doesn't like monospace fonts, apparently, but that's acceptable.

See merge request !151

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 8.6.0 (unreleased)
   - Handle duplicate appearances table creation issue with upgrade from CE to EE
   - Add confidential issues
   - Improve weight filter for issues
+  - Add full diff highlighting to Email on push
   - Clear "stuck" mirror updates before periodically updating all mirrors.
   - [Elastic] Add elastic checker to gitlab:check
   - [Elastic] Added UPDATE_INDEX option to rake task

Gemfile

@@ -229,6 +229,8 @@ gem "gitlab-license", "~> 0.0.4"
 # Sentry integration
 gem 'sentry-raven', '~> 0.15'
 
+gem 'premailer-rails', '~> 1.9.0'
+
 # Metrics
 group :metrics do
   gem 'allocations', '~> 1.0', require: false, platform: :mri

Gemfile.lock

@@ -150,6 +150,8 @@ GEM
     crack (0.4.3)
       safe_yaml (~> 1.0.0)
     creole (0.5.0)
+    css_parser (1.3.7)
+      addressable
     d3_rails (3.5.11)
       railties (>= 3.1.0)
     daemons (1.2.3)
@@ -444,6 +446,7 @@ GEM
       haml (~> 4.0.0)
       nokogiri (~> 1.6.0)
       ruby_parser (~> 3.5)
+    htmlentities (4.3.4)
     http-cookie (1.0.2)
       domain_name (~> 0.5)
     http_parser.rb (0.5.3)
@@ -583,6 +586,12 @@ GEM
       websocket-driver (>= 0.2.0)
     posix-spawn (0.3.11)
     powerpack (0.1.1)
+    premailer (1.8.6)
+      css_parser (>= 1.3.6)
+      htmlentities (>= 4.0.0)
+    premailer-rails (1.9.0)
+      actionmailer (>= 3, < 5)
+      premailer (~> 1.7, >= 1.7.9)
     pry (0.10.3)
       coderay (~> 1.1.0)
       method_source (~> 0.8.1)
@@ -1018,6 +1027,7 @@ DEPENDENCIES
   paranoia (~> 2.0)
   pg (~> 0.18.2)
   poltergeist (~> 1.9.0)
+  premailer-rails (~> 1.9.0)
   pry-rails
   quiet_assets (~> 1.0.2)
   rack-attack (~> 4.3.1)

app/assets/stylesheets/mailers/repository_push_email.scss

+@import "framework/variables";
+
+table.code {
+  width: 100%;
+  font-family: $monospace_font;
+  border: none;
+  border-collapse: separate;
+  margin: 0px;
+  padding: 0px;
+  -premailer-cellpadding: 0;
+  -premailer-cellspacing: 0;
+  -premailer-width: 100%;
+
+  td {
+    line-height: $code_line_height;
+    font-family: $monospace_font;
+    font-size: $code_font_size;
+  }
+
+  td.diff-line-num {
+    margin: 0px;
+    padding: 0px;
+    border: none;
+    background: $background-color;
+    color: rgba(0, 0, 0, 0.3);
+    padding: 0px 5px;
+    border-right: 1px solid $border-color;
+    text-align: right;
+    min-width: 35px;
+    max-width: 50px;
+    width: 35px;
+  }
+
+  td.line_content {
+    display: block;
+    margin: 0px;
+    padding: 0px 0.5em;
+    border: none;
+    white-space: pre;
+  }
+}
+
+@import "highlight/white";

app/assets/stylesheets/notify.scss

+img {
+  max-width: 100%;
+  height: auto;
+}
+p.details {
+  font-style:italic;
+  color:#777
+}
+.footer p {
+  font-size:small;
+  color:#777
+}
+pre.commit-message {
+  white-space: pre-wrap;
+}
+.file-stats a {
+  text-decoration: none;
+}
+.file-stats .new-file {
+  color: #090;
+}
+.file-stats .deleted-file {
+  color: #B00;
+}

app/helpers/emails_helper.rb

@@ -32,12 +32,6 @@ module EmailsHelper
     nil
   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
     valid_hours = Devise.reset_password_within / 60 / 60
     if valid_hours >= 24
@@ -56,4 +50,10 @@ module EmailsHelper
     msg = "This link is valid for #{password_reset_token_valid_time}.  "
     msg << "After it expires, you can #{link_tag}."
   end
+
+  # Overrides
+
+  def diff_hard_limit_enabled?
+    false
+  end
 end

app/mailers/notify.rb

@@ -11,6 +11,8 @@ class Notify < BaseMailer
   include Emails::Builds
 
   add_template_helper MergeRequestsHelper
+  add_template_helper DiffHelper
+  add_template_helper BlobHelper
   add_template_helper EmailsHelper
 
   def test_email(recipient_email, subject, body)

app/views/layouts/notify.html.haml

 %html{lang: "en"}
   %head
     %meta{content: "text/html; charset=utf-8", "http-equiv" => "Content-Type"}
-      %title
-        GitLab
-  :css
-    img {
-      max-width: 100%;
-      height: auto;
-    }
-    p.details {
-      font-style:italic;
-      color:#777
-    }
-    .footer p {
-      font-size:small;
-      color:#777
-    }
-    pre.commit-message {
-      white-space: pre-wrap;
-    }
-    .file-stats a {
-      text-decoration: none;
-    }
-    .file-stats .new-file {
-      color: #090;
-    }
-    .file-stats .deleted-file {
-      color: #B00;
-    }
+    %title
+      GitLab
+    = stylesheet_link_tag 'notify'
+    = yield :head
   %body
     %div.content
       = yield

app/views/notify/repository_push_email.html.haml

+= content_for :head do
+  = stylesheet_link_tag 'mailers/repository_push_email'
+
 %h3
   #{@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))}
@@ -43,25 +46,34 @@
             = diff.new_path
 
   - unless @message.disable_diffs?
+    - diff_files = safe_diff_files(@message.diffs, @message.diff_refs)
+
     %h4 Changes:
-    - @message.diffs.each_with_index do |diff, i|
+    - diff_files.each_with_index do |diff_file, i|
       %li{id: "diff-#{i}"}
         %a{href: @message.target_url + "#diff-#{i}"}
-          - if diff.deleted_file
+          - if diff_file.deleted_file
             %strong
-              = diff.old_path
+              = diff_file.old_path
             deleted
-          - elsif diff.renamed_file
+          - elsif diff_file.renamed_file
             %strong
-              = diff.old_path
+              = diff_file.old_path
             →
             %strong
-              = diff.new_path
+              = diff_file.new_path
           - else
             %strong
-              = diff.new_path
+              = diff_file.new_path
         %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
 
   - if @message.compare_timeout

app/views/projects/diffs/_file.html.haml

@@ -40,8 +40,8 @@
         = view_file_btn(diff_commit.id, diff_file, project)
 
   .diff-content.diff-wrap-lines
-    -# Skipp all non non-supported blobs
-    - return unless blob.respond_to?('text?')
+    -# Skip all non-supported blobs
+    - return unless blob.respond_to?(:text?)
     - if blob_text_viewable?(blob)
       - if diff_view == 'parallel'
         = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i

app/views/projects/diffs/_line.html.haml

+- type = line.type
+%tr.line_holder{id: line_code, class: type}
+  - case type
+  - when 'match'
+    = render "projects/diffs/match_line", {line: line.text,
+      line_old: line.old_pos, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file}
+  - when 'nonewline'
+    %td.old_line.diff-line-num
+    %td.new_line.diff-line-num
+    %td.line_content.match= line.text
+  - else
+    %td.old_line.diff-line-num{class: type}
+      - link_text = raw(type == "new" ? " " : line.old_pos)
+      - if defined?(plain) && plain
+        = link_text
+      - else
+        = link_to link_text, "##{line_code}", id: line_code
+      - if @comments_allowed && can?(current_user, :create_note, @project)
+        = link_to_new_diff_note(line_code)
+    %td.new_line.diff-line-num{class: type, data: {linenumber: line.new_pos}}
+      - link_text = raw(type == "old" ? " " : line.new_pos)
+      - if defined?(plain) && plain
+        = link_text
+      - else
+        = link_to link_text, "##{line_code}", id: line_code
+    %td.line_content{class: "noteable_line #{type} #{line_code}", data: { line_code: line_code }}= diff_line_content(line.text)

app/views/projects/diffs/_text_file.html.haml

@@ -8,26 +8,9 @@
   - last_line = 0
   - raw_diff_lines = diff_file.diff_lines.to_a
   - diff_file.highlighted_diff_lines.each_with_index do |line, index|
-    - type = line.type
-    - last_line = line.new_pos
     - line_code = generate_line_code(diff_file.file_path, line)
-    - line_old = line.old_pos
-    %tr.line_holder{ id: line_code, class: "#{type}" }
-      - if type == "match"
-        = render "projects/diffs/match_line", {line: line.text,
-          line_old: line_old, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file}
-      - elsif type == 'nonewline'
-        %td.old_line.diff-line-num
-        %td.new_line.diff-line-num
-        %td.line_content.match= line.text
-      - else
-        %td.old_line.diff-line-num{class: type}
-          = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code
-          - if @comments_allowed && can?(current_user, :create_note, @project)
-            = link_to_new_diff_note(line_code)
-        %td.new_line.diff-line-num{class: type, data: {linenumber: line.new_pos}}
-          = link_to raw(type == "old" ? " " : line.new_pos), "##{line_code}", id: line_code
-        %td.line_content{class: "noteable_line #{type} #{line_code}", data: { line_code: line_code }}= diff_line_content(line.text)
+    - last_line = line.new_pos
+    = render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: line_code}
 
     - if @reply_allowed
       - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at)

app/workers/emails_on_push_worker.rb

@@ -25,15 +25,18 @@ class EmailsOnPushWorker
         :push
       end
 
+    diff_refs = nil
     compare = nil
     reverse_compare = false
     if action == :push
       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
 
       if compare.commits.empty?
         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
 
@@ -41,7 +44,7 @@ class EmailsOnPushWorker
       end
     end
 
-    recipients.split(" ").each do |recipient|
+    recipients.split.each do |recipient|
       begin
         Notify.repository_push_email(
           project_id,
@@ -51,6 +54,7 @@ class EmailsOnPushWorker
           action:                     action,
           compare:                    compare,
           reverse_compare:            reverse_compare,
+          diff_refs:                  diff_refs,
           send_from_committer_email:  send_from_committer_email,
           disable_diffs:              disable_diffs
         ).deliver_now

config/application.rb

@@ -49,6 +49,8 @@ module Gitlab
     config.assets.paths << Gemojione.index.images_path
     config.assets.precompile << "*.png"
     config.assets.precompile << "print.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
     config.assets.version = '1.0'

config/initializers/premailer.rb

+# See https://github.com/fphilipe/premailer-rails#configuration
+Premailer::Rails.config.merge!(
+  generate_text_part: false,
+  preserve_styles: true,
+  remove_comments: true,
+  remove_ids: true
+)

lib/gitlab/email/message/repository_push.rb

@@ -49,6 +49,10 @@ module Gitlab
           @opts[:compare]
         end
 
+        def diff_refs
+          @opts[:diff_refs]
+        end
+
         def compare_timeout
           diffs.overflow? if diffs
         end

spec/mailers/notify_spec.rb

@@ -679,8 +679,9 @@ describe Notify do
     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(: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, 'devs@company.name', 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, 'devs@company.name', 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 "a user cannot unsubscribe through footer link"
@@ -705,15 +706,15 @@ describe Notify do
       is_expected.to have_body_text /Change some files/
     end
 
-    it 'includes diffs' do
-      is_expected.to have_body_text /def archive_formats_regex/
+    it 'includes diffs with character-level highlighting' do
+      is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
     end
 
     it 'contains a link to the diff' do
       is_expected.to have_body_text /#{diff_path}/
     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/
     end
 
@@ -787,8 +788,9 @@ describe Notify do
     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(: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, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', 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 "a user cannot unsubscribe through footer link"
@@ -813,8 +815,8 @@ describe Notify do
       is_expected.to have_body_text /Change some files/
     end
 
-    it 'includes diffs' do
-      is_expected.to have_body_text /def archive_formats_regex/
+    it 'includes diffs with character-level highlighting' do
+      is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
     end
 
     it 'contains a link to the diff' do