diff options
author | Douwe Maan <douwe@selenight.nl> | 2016-05-21 00:23:04 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2016-05-21 00:23:04 +0300 |
commit | fc1910ddc5db25e608921f69c3dc28d830e6ea03 (patch) | |
tree | 1f82bd43de18fdb6ec1d01801d7d75c08bdf7928 | |
parent | 63f53730c0483b4c9e6fa9d90413d40a8c74a243 (diff) | |
parent | 5341b16fc92425d557a1c62d0329b276170eba73 (diff) |
Merge branch 'adambutler/gitlab-ce-feature/support-diff-of-issue-title-rename'
# Conflicts:
# app/services/system_note_service.rb
-rw-r--r-- | CHANGELOG | 2 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/files.scss | 16 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/typography.scss | 8 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/variables.scss | 1 | ||||
-rw-r--r-- | app/helpers/diff_helper.rb | 4 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 9 | ||||
-rw-r--r-- | app/views/projects/diffs/_file.html.haml | 6 | ||||
-rw-r--r-- | doc/markdown/markdown.md | 14 | ||||
-rw-r--r-- | lib/banzai/filter/inline_diff_filter.rb | 22 | ||||
-rw-r--r-- | lib/banzai/pipeline/gfm_pipeline.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/diff/inline_diff_marker.rb | 36 | ||||
-rw-r--r-- | spec/features/markdown_spec.rb | 4 | ||||
-rw-r--r-- | spec/fixtures/markdown.md.erb | 13 | ||||
-rw-r--r-- | spec/helpers/diff_helper_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/banzai/filter/inline_diff_filter_spec.rb | 68 | ||||
-rw-r--r-- | spec/services/issues/update_service_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/matchers/markdown_matchers.rb | 10 |
19 files changed, 192 insertions, 38 deletions
diff --git a/CHANGELOG b/CHANGELOG index 7cbb7901f85..470813fb9e9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -9,6 +9,8 @@ v 8.8.0 (unreleased) - Use a case-insensitive comparison in sanitizing URI schemes - Toggle sign-up confirmation emails in application settings - Make it possible to prevent tagged runner from picking untagged jobs + - Added `InlineDiffFilter` to the markdown parser. (Adam Butler) + - Added inline diff styling for `change_title` system notes. (Adam Butler) - Project#open_branches has been cleaned up and no longer loads entire records into memory. - Escape HTML in commit titles in system note messages - Fix creation of Ci::Commit object which can lead to pending, failed in some scenarios diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 61d9954c6c8..8c96c7a9c31 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -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) { color: $gl-dark-link-color; } diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 2779cd56788..3575984b229 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -269,3 +269,11 @@ h1, h2, h3, h4 { text-align: right; } } + +.idiff.deletion { + background: $line-removed-dark; +} + +.idiff.addition { + background: $line-added-dark; +} diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 7360bdbcc82..c5a4dbe372c 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -178,6 +178,7 @@ $table-border-gray: #f0f0f0; $line-target-blue: #eaf3fc; $line-select-yellow: #fcf8e7; $line-select-yellow-dark: #f0e2bd; + /* * Fonts */ diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 5f311f3780a..ea383f9b0f6 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -2,8 +2,8 @@ module DiffHelper def mark_inline_diffs(old_line, new_line) 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_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_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, mode: :addition) [marked_old_line, marked_new_line] end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 972f8b2012d..4e8fa0818b9 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -169,7 +169,14 @@ class SystemNoteService # # Returns the created Note object def self.change_title(noteable, project, author, old_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) end diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index f10b5094eaf..e5983c58039 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -11,11 +11,9 @@ = link_to "#diff-#{i}" do - if diff_file.renamed_file - 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 %span = diff_file.new_path diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index b10fef7179e..236eb7b12c4 100644 --- a/doc/markdown/markdown.md +++ b/doc/markdown/markdown.md @@ -8,6 +8,7 @@ * [Multiple underscores in words](#multiple-underscores-in-words) * [URL auto-linking](#url-auto-linking) * [Code and Syntax Highlighting](#code-and-syntax-highlighting) +* [Inline Diff](#inline-diff) * [Emoji](#emoji) * [Special GitLab references](#special-gitlab-references) * [Task lists](#task-lists) @@ -153,6 +154,19 @@ s = "There is no highlighting for this." But let's throw in a <b>tag</b>. ``` +## Inline Diff + +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 cannot be mixed as such: + +- {+ additions +] +- [+ additions +} +- {- deletions -] +- [- deletions -} + ## Emoji Sometimes you want to :monkey: around a bit and add some :star2: to your :speech_balloon:. Well we have a gift for you: diff --git a/lib/banzai/filter/inline_diff_filter.rb b/lib/banzai/filter/inline_diff_filter.rb new file mode 100644 index 00000000000..9e75edd4d4c --- /dev/null +++ b/lib/banzai/filter/inline_diff_filter.rb @@ -0,0 +1,22 @@ +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 diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index ed3cfd6b023..b27ecf3c923 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -23,7 +23,8 @@ module Banzai Filter::LabelReferenceFilter, Filter::MilestoneReferenceFilter, - Filter::TaskListFilter + Filter::TaskListFilter, + Filter::InlineDiffFilter ] end diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index dccb717e95d..87a9b1e23ac 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -1,6 +1,11 @@ module Gitlab module Diff class InlineDiffMarker + MARKDOWN_SYMBOLS = { + addition: "+", + deletion: "-" + } + attr_accessor :raw_line, :rich_line def initialize(raw_line, rich_line = raw_line) @@ -8,7 +13,7 @@ module Gitlab @rich_line = ERB::Util.html_escape(rich_line) end - def mark(line_inline_diffs) + def mark(line_inline_diffs, mode: nil, markdown: false) return rich_line unless line_inline_diffs marker_ranges = [] @@ -20,13 +25,22 @@ module Gitlab end 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 = + if markdown + "{#{MARKDOWN_SYMBOLS[mode]}" + else + "<span class='#{html_class_names(marker_ranges, mode, index)}'>" + end + after_content = + if markdown + "#{MARKDOWN_SYMBOLS[mode]}}" + else + "</span>" + end + offset = insert_around_range(rich_line, range, before_content, after_content, offset) end rich_line.html_safe @@ -34,6 +48,14 @@ module Gitlab 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 def position_mapping @position_mapping ||= begin diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index 0148c87084a..1d892fe1a55 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -278,6 +278,10 @@ describe 'GitLab Markdown', feature: true do it 'includes GollumTagsFilter' do expect(doc).to parse_gollum_tags end + + it 'includes InlineDiffFilter' do + expect(doc).to parse_inline_diffs + end end # Fake a `current_user` helper diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index 3e777a5e92b..34ce7c4f033 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -243,3 +243,16 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e - [[link-text|http://example.com/pdfs/gollum.pdf]] - [[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 -} diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index b7810185d16..52764f41e0d 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -93,9 +93,9 @@ describe DiffHelper do it "returns strings with marked inline diffs" do 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'>'def'</span>") + expect(marked_old_line).to eq("abc <span class='idiff left right deletion'>'def'</span>") expect(marked_old_line).to be_html_safe - expect(marked_new_line).to eq("abc <span class='idiff left right'>"def"</span>") + expect(marked_new_line).to eq("abc <span class='idiff left right addition'>"def"</span>") expect(marked_new_line).to be_html_safe end end diff --git a/spec/lib/banzai/filter/inline_diff_filter_spec.rb b/spec/lib/banzai/filter/inline_diff_filter_spec.rb new file mode 100644 index 00000000000..9e526371294 --- /dev/null +++ b/spec/lib/banzai/filter/inline_diff_filter_spec.rb @@ -0,0 +1,68 @@ +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 {+<script>alert('I steal cookies')</script>+} END" + expect(filter(doc).to_html).to eq("START <span class=\"idiff left right addition\"><script>alert('I steal cookies')</script></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 diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 810a1f2d666..be19be17151 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -75,10 +75,10 @@ describe Issues::UpdateService, services: true do end 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.note).to eq 'Title changed from **Old title** to **New title**' + expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**' end it 'creates system note about confidentiality change' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 213e8c2eb3a..56ee3e43056 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -90,10 +90,10 @@ describe MergeRequests::UpdateService, services: true do end 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.note).to eq 'Title changed from **Old title** to **New title**' + expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**' end it 'creates system note about branch change' do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index bffad59b8b6..ee976eb2926 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -241,7 +241,7 @@ describe SystemNoteService, services: true do it 'sets the note text' do expect(subject.note). - to eq "Title changed from **Old title** to **#{noteable.title}**" + to eq "Changed title: **{-Old title-}** → **{+#{noteable.title}+}**" end end end diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb index d921f9bb2bc..e005058ba5b 100644 --- a/spec/support/matchers/markdown_matchers.rb +++ b/spec/support/matchers/markdown_matchers.rb @@ -168,6 +168,16 @@ module MarkdownMatchers expect(actual).to have_selector('input[checked]', count: 3) 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 # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for |