Commit 8a8b5497 authored by Adam Butler's avatar Adam Butler Committed by Rémy Coutable

Create DiffFilter and change SystemNoteService#change_title to use Gitlab::Diff::InlineDiff

parent 636b3ebb
...@@ -100,6 +100,10 @@ v 8.7.1 ...@@ -100,6 +100,10 @@ v 8.7.1
- Prevent users from deleting Webhooks via API they do not own - Prevent users from deleting Webhooks via API they do not own
- Fix Error 500 due to stale cache when projects are renamed or transferred - Fix Error 500 due to stale cache when projects are renamed or transferred
- Update width of search box to fix Safari bug. !3900 (Jedidiah) - Update width of search box to fix Safari bug. !3900 (Jedidiah)
- Added inline diff styling for `change_title` system notes. !3576 (Adam Butler)
- Added `InlineDiffFilter` to the markdown parser. !3576 (Adam Butler)
v 8.7.1 (unreleased)
- Use the `can?` helper instead of `current_user.can?` - Use the `can?` helper instead of `current_user.can?`
v 8.7.0 v 8.7.0
......
...@@ -36,22 +36,6 @@ ...@@ -36,22 +36,6 @@
} }
} }
.filename {
&.old {
display: inline-block;
span.idiff {
background-color: #f8cbcb;
}
}
&.new {
display: inline-block;
span.idiff {
background-color: #a6f3a6;
}
}
}
a:not(.btn) { a:not(.btn) {
color: $gl-dark-link-color; color: $gl-dark-link-color;
} }
......
...@@ -269,3 +269,11 @@ h1, h2, h3, h4 { ...@@ -269,3 +269,11 @@ h1, h2, h3, h4 {
text-align: right; text-align: right;
} }
} }
.idiff.deletion {
background: $gl-idiff-deletion;
}
.idiff.addition {
background: $gl-idiff-addition;
}
...@@ -178,6 +178,9 @@ $table-border-gray: #f0f0f0; ...@@ -178,6 +178,9 @@ $table-border-gray: #f0f0f0;
$line-target-blue: #eaf3fc; $line-target-blue: #eaf3fc;
$line-select-yellow: #fcf8e7; $line-select-yellow: #fcf8e7;
$line-select-yellow-dark: #f0e2bd; $line-select-yellow-dark: #f0e2bd;
$gl-idiff-deletion: #f8cbcb;
$gl-idiff-addition: #a6f3a6;
/* /*
* Fonts * Fonts
*/ */
......
...@@ -2,8 +2,8 @@ module DiffHelper ...@@ -2,8 +2,8 @@ module DiffHelper
def mark_inline_diffs(old_line, new_line) def mark_inline_diffs(old_line, new_line)
old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_line, new_line).inline_diffs old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_line, new_line).inline_diffs
marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs) marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs, mode: :deletion)
marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs) marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs, mode: :addition)
[marked_old_line, marked_new_line] [marked_old_line, marked_new_line]
end end
......
...@@ -171,7 +171,14 @@ class SystemNoteService ...@@ -171,7 +171,14 @@ class SystemNoteService
def self.change_title(noteable, project, author, old_title) def self.change_title(noteable, project, author, old_title)
return unless noteable.respond_to?(:title) return unless noteable.respond_to?(:title)
body = "Title changed from **#{old_title}** to **#{noteable.title}**" new_title = noteable.title.dup
old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs
marked_old_title = Gitlab::Diff::InlineDiffMarker.new(old_title).mark(old_diffs, mode: :deletion, markdown: true)
marked_new_title = Gitlab::Diff::InlineDiffMarker.new(new_title).mark(new_diffs, mode: :addition, markdown: true)
body = "Changed title: **#{marked_old_title}** → **#{marked_new_title}**"
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
......
...@@ -11,11 +11,9 @@ ...@@ -11,11 +11,9 @@
= link_to "#diff-#{i}" do = link_to "#diff-#{i}" do
- if diff_file.renamed_file - if diff_file.renamed_file
- old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
.filename.old = old_path
= old_path
→ →
.filename.new = new_path
= new_path
- else - else
%span %span
= diff_file.new_path = diff_file.new_path
......
module Banzai
module Filter
class InlineDiffFilter < HTML::Pipeline::Filter
IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set
def call
search_text_nodes(doc).each do |node|
next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS)
content = node.to_html
content = content.gsub(/(?:\[\-(.*?)\-\]|\{\-(.*?)\-\})/, '<span class="idiff left right deletion">\1\2</span>')
content = content.gsub(/(?:\[\+(.*?)\+\]|\{\+(.*?)\+\})/, '<span class="idiff left right addition">\1\2</span>')
next if html == content
node.replace(content)
end
doc
end
end
end
end
...@@ -23,7 +23,8 @@ module Banzai ...@@ -23,7 +23,8 @@ module Banzai
Filter::LabelReferenceFilter, Filter::LabelReferenceFilter,
Filter::MilestoneReferenceFilter, Filter::MilestoneReferenceFilter,
Filter::TaskListFilter Filter::TaskListFilter,
Filter::InlineDiffFilter
] ]
end end
......
module Gitlab module Gitlab
module Diff module Diff
class InlineDiffMarker class InlineDiffMarker
MARKDOWN_SYMBOLS = {
addition: "+",
deletion: "-"
}
attr_accessor :raw_line, :rich_line attr_accessor :raw_line, :rich_line
def initialize(raw_line, rich_line = raw_line) def initialize(raw_line, rich_line = raw_line)
...@@ -8,7 +13,7 @@ module Gitlab ...@@ -8,7 +13,7 @@ module Gitlab
@rich_line = ERB::Util.html_escape(rich_line) @rich_line = ERB::Util.html_escape(rich_line)
end end
def mark(line_inline_diffs) def mark(line_inline_diffs, mode: nil, markdown: false)
return rich_line unless line_inline_diffs return rich_line unless line_inline_diffs
marker_ranges = [] marker_ranges = []
...@@ -20,13 +25,12 @@ module Gitlab ...@@ -20,13 +25,12 @@ module Gitlab
end end
offset = 0 offset = 0
# Mark each range
marker_ranges.each_with_index do |range, i|
class_names = ["idiff"]
class_names << "left" if i == 0
class_names << "right" if i == marker_ranges.length - 1
offset = insert_around_range(rich_line, range, "<span class='#{class_names.join(" ")}'>", "</span>", offset) # Mark each range
marker_ranges.each_with_index do |range, index|
before_content = markdown ? "{#{MARKDOWN_SYMBOLS[mode]}" : "<span class='#{html_class_names(marker_ranges, mode, index)}'>"
after_content = markdown ? "#{MARKDOWN_SYMBOLS[mode]}}" : "</span>"
offset = insert_around_range(rich_line, range, before_content, after_content, offset)
end end
rich_line.html_safe rich_line.html_safe
...@@ -34,6 +38,14 @@ module Gitlab ...@@ -34,6 +38,14 @@ module Gitlab
private private
def html_class_names(marker_ranges, mode, index)
class_names = ["idiff"]
class_names << "left" if index == 0
class_names << "right" if index == marker_ranges.length - 1
class_names << mode if mode
class_names.join(" ")
end
# Mapping of character positions in the raw line, to the rich (highlighted) line # Mapping of character positions in the raw line, to the rich (highlighted) line
def position_mapping def position_mapping
@position_mapping ||= begin @position_mapping ||= begin
......
...@@ -278,6 +278,10 @@ describe 'GitLab Markdown', feature: true do ...@@ -278,6 +278,10 @@ describe 'GitLab Markdown', feature: true do
it 'includes GollumTagsFilter' do it 'includes GollumTagsFilter' do
expect(doc).to parse_gollum_tags expect(doc).to parse_gollum_tags
end end
it 'includes InlineDiffFilter' do
expect(doc).to parse_inline_diffs
end
end end
# Fake a `current_user` helper # Fake a `current_user` helper
......
...@@ -239,3 +239,16 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e ...@@ -239,3 +239,16 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e
- [[link-text|http://example.com/pdfs/gollum.pdf]] - [[link-text|http://example.com/pdfs/gollum.pdf]]
- [[images/example.jpg]] - [[images/example.jpg]]
- [[http://example.com/images/example.jpg]] - [[http://example.com/images/example.jpg]]
### Inline Diffs
With inline diffs tags you can display {+ additions +} or [- deletions -].
The wrapping tags can be either curly braces or square brackets [+ additions +] or {- deletions -}.
However the wrapping tags can not be mixed as such -
- {+ additions +]
- [+ additions +}
- {- delletions -]
- [- delletions -}
...@@ -93,9 +93,9 @@ describe DiffHelper do ...@@ -93,9 +93,9 @@ describe DiffHelper do
it "returns strings with marked inline diffs" do it "returns strings with marked inline diffs" do
marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line) marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line)
expect(marked_old_line).to eq("abc <span class='idiff left right'>&#39;def&#39;</span>") expect(marked_old_line).to eq("abc <span class='idiff left right deletion'>&#39;def&#39;</span>")
expect(marked_old_line).to be_html_safe expect(marked_old_line).to be_html_safe
expect(marked_new_line).to eq("abc <span class='idiff left right'>&quot;def&quot;</span>") expect(marked_new_line).to eq("abc <span class='idiff left right addition'>&quot;def&quot;</span>")
expect(marked_new_line).to be_html_safe expect(marked_new_line).to be_html_safe
end end
end end
......
require 'spec_helper'
describe Banzai::Filter::InlineDiffFilter, lib: true do
include FilterSpecHelper
it 'adds inline diff span tags for deletions when using square brackets' do
doc = "START [-something deleted-] END"
expect(filter(doc).to_html).to eq('START <span class="idiff left right deletion">something deleted</span> END')
end
it 'adds inline diff span tags for deletions when using curley braces' do
doc = "START {-something deleted-} END"
expect(filter(doc).to_html).to eq('START <span class="idiff left right deletion">something deleted</span> END')
end
it 'does not add inline diff span tags when a closing tag is not provided' do
doc = "START [- END"
expect(filter(doc).to_html).to eq(doc)
end
it 'adds inline span tags for additions when using square brackets' do
doc = "START [+something added+] END"
expect(filter(doc).to_html).to eq('START <span class="idiff left right addition">something added</span> END')
end
it 'adds inline span tags for additions when using curley braces' do
doc = "START {+something added+} END"
expect(filter(doc).to_html).to eq('START <span class="idiff left right addition">something added</span> END')
end
it 'does not add inline diff span tags when a closing addition tag is not provided' do
doc = "START {+ END"
expect(filter(doc).to_html).to eq(doc)
end
it 'does not add inline diff span tags when the tags do not match' do
examples = [
"{+ additions +]",
"[+ additions +}",
"{- delletions -]",
"[- delletions -}"
]
examples.each do |doc|
expect(filter(doc).to_html).to eq(doc)
end
end
it 'prevents user-land html being injected' do
doc = "START {+&lt;script&gt;alert('I steal cookies')&lt;/script&gt;+} END"
expect(filter(doc).to_html).to eq("START <span class=\"idiff left right addition\">&lt;script&gt;alert('I steal cookies')&lt;/script&gt;</span> END")
end
it 'preserves content inside pre tags' do
doc = "<pre>START {+something added+} END</pre>"
expect(filter(doc).to_html).to eq(doc)
end
it 'preserves content inside code tags' do
doc = "<code>START {+something added+} END</code>"
expect(filter(doc).to_html).to eq(doc)
end
it 'preserves content inside tt tags' do
doc = "<tt>START {+something added+} END</tt>"
expect(filter(doc).to_html).to eq(doc)
end
end
...@@ -79,10 +79,10 @@ describe Issues::UpdateService, services: true do ...@@ -79,10 +79,10 @@ describe Issues::UpdateService, services: true do
end end
it 'creates system note about title change' do it 'creates system note about title change' do
note = find_note('Title changed') note = find_note('Changed title:')
expect(note).not_to be_nil expect(note).not_to be_nil
expect(note.note).to eq 'Title changed from **Old title** to **New title**' expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
end end
end end
......
...@@ -90,10 +90,10 @@ describe MergeRequests::UpdateService, services: true do ...@@ -90,10 +90,10 @@ describe MergeRequests::UpdateService, services: true do
end end
it 'creates system note about title change' do it 'creates system note about title change' do
note = find_note('Title changed') note = find_note('Changed title:')
expect(note).not_to be_nil expect(note).not_to be_nil
expect(note.note).to eq 'Title changed from **Old title** to **New title**' expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
end end
it 'creates system note about branch change' do it 'creates system note about branch change' do
......
...@@ -241,7 +241,7 @@ describe SystemNoteService, services: true do ...@@ -241,7 +241,7 @@ describe SystemNoteService, services: true do
it 'sets the note text' do it 'sets the note text' do
expect(subject.note). expect(subject.note).
to eq "Title changed from **Old title** to **#{noteable.title}**" to eq "Changed title: **{-Old title-}** → **{+#{noteable.title}+}**"
end end
end end
......
...@@ -168,6 +168,16 @@ module MarkdownMatchers ...@@ -168,6 +168,16 @@ module MarkdownMatchers
expect(actual).to have_selector('input[checked]', count: 3) expect(actual).to have_selector('input[checked]', count: 3)
end end
end end
# InlineDiffFilter
matcher :parse_inline_diffs do
set_default_markdown_messages
match do |actual|
expect(actual).to have_selector('span.idiff.addition', count: 2)
expect(actual).to have_selector('span.idiff.deletion', count: 2)
end
end
end end
# Monkeypatch the matcher DSL so that we can reduce some noisy duplication for # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for
......
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