Merge branch 'rs-remove-gitlab-pipeline' into 'master'

Remove html-pipeline-gitlab

Ports the custom emoji filter to the app itself.

See merge request !1781

Gemfile

@@ -88,7 +88,7 @@ gem "six"
 gem "seed-fu"
 
 # Markup pipeline for GitLab
-gem 'html-pipeline-gitlab', '~> 0.1'
+gem 'html-pipeline', '~> 1.11.0'
 
 # Markdown to HTML
 gem "github-markup"

Gemfile.lock

@@ -278,12 +278,6 @@ GEM
     html-pipeline (1.11.0)
       activesupport (>= 2)
       nokogiri (~> 1.4)
-    html-pipeline-gitlab (0.2.0)
-      actionpack (~> 4)
-      gitlab_emoji (~> 0.1)
-      html-pipeline (~> 1.11.0)
-      mime-types
-      sanitize (~> 2.1)
     http_parser.rb (0.5.3)
     httparty (0.13.3)
       json (~> 1.8)
@@ -717,7 +711,7 @@ DEPENDENCIES
   guard-spinach
   haml-rails
   hipchat (~> 1.5.0)
-  html-pipeline-gitlab (~> 0.1)
+  html-pipeline (~> 1.11.0)
   httparty
   jasmine (= 2.0.2)
   jquery-atwho-rails (~> 0.3.3)

lib/gitlab/markdown.rb

 require 'html/pipeline'
-require 'html/pipeline/gitlab'
 
 module Gitlab
   # Custom parser for GitLab-flavored Markdown
@@ -61,19 +60,24 @@ module Gitlab
         reference_only_path:  true
       )
 
-      markdown_context = {
+      pipeline = HTML::Pipeline.new(filters)
+
+      context = {
+        # SanitizationFilter
+        whitelist:       sanitization_whitelist,
+
+        # EmojiFilter
         asset_root:      Gitlab.config.gitlab.url,
         asset_host:      Gitlab::Application.config.asset_host,
-        whitelist:       sanitization_whitelist,
-        reference_class: html_options[:class],
-        only_path:       options[:reference_only_path],
+
+        # ReferenceFilter
         current_user:    current_user,
-        project:         project
+        only_path:       options[:reference_only_path],
+        project:         project,
+        reference_class: html_options[:class]
       }
 
-      markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline
-
-      result = markdown_pipeline.call(text, markdown_context)
+      result = pipeline.call(text, context)
 
       save_options = 0
       if options[:xhtml]
@@ -91,7 +95,7 @@ module Gitlab
 
     private
 
-    # Custom filters for html-pipeline:
+    # Filters used in our pipeline
     #
     # SanitizationFilter should come first so that all generated reference HTML
     # goes through untouched.
@@ -101,6 +105,8 @@ module Gitlab
       [
         HTML::Pipeline::SanitizationFilter,
 
+        Gitlab::Markdown::EmojiFilter,
+
         Gitlab::Markdown::UserReferenceFilter,
         Gitlab::Markdown::IssueReferenceFilter,
         Gitlab::Markdown::ExternalIssueReferenceFilter,
@@ -109,8 +115,6 @@ module Gitlab
         Gitlab::Markdown::CommitRangeReferenceFilter,
         Gitlab::Markdown::CommitReferenceFilter,
         Gitlab::Markdown::LabelReferenceFilter,
-
-        HTML::Pipeline::Gitlab::GitlabEmojiFilter
       ]
     end
 

lib/gitlab/markdown/emoji_filter.rb

+require 'gitlab_emoji'
+require 'html/pipeline/filter'
+require 'action_controller'
+
+module Gitlab
+  module Markdown
+    # HTML filter that replaces :emoji: with images.
+    #
+    # Based on HTML::Pipeline::EmojiFilter
+    #
+    # Context options:
+    #   :asset_root
+    #   :asset_host
+    class EmojiFilter < HTML::Pipeline::Filter
+      IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set
+
+      def call
+        doc.search('text()').each do |node|
+          content = node.to_html
+          next unless content.include?(':')
+          next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS)
+
+          html = emoji_image_filter(content)
+
+          next if html == content
+
+          node.replace(html)
+        end
+
+        doc
+      end
+
+      # Replace :emoji: with corresponding images.
+      #
+      # text - String text to replace :emoji: in.
+      #
+      # Returns a String with :emoji: replaced with images.
+      def emoji_image_filter(text)
+        text.gsub(emoji_pattern) do |match|
+          name = $1
+          "<img class='emoji' title=':#{name}:' alt=':#{name}:' src='#{emoji_url(name)}' height='20' width='20' align='absmiddle' />"
+        end
+      end
+
+      private
+
+      def emoji_url(name)
+        emoji_path = "emoji/#{emoji_filename(name)}"
+        if context[:asset_host]
+          # Asset host is specified.
+          url_to_image(emoji_path)
+        elsif context[:asset_root]
+          # Gitlab url is specified
+          File.join(context[:asset_root], url_to_image(emoji_path))
+        else
+          # All other cases
+          url_to_image(emoji_path)
+        end
+      end
+
+      def url_to_image(image)
+        ActionController::Base.helpers.url_to_image(image)
+      end
+
+      # Build a regexp that matches all valid :emoji: names.
+      def self.emoji_pattern
+        @emoji_pattern ||= /:(#{Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/
+      end
+
+      def emoji_pattern
+        self.class.emoji_pattern
+      end
+
+      def emoji_filename(name)
+        "#{Emoji.emoji_filename(name)}.png"
+      end
+    end
+  end
+end

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -2,37 +2,21 @@ require 'spec_helper'
 
 describe GitlabMarkdownHelper do
   include ApplicationHelper
-  include IssuesHelper
-
-  # TODO: Properly test this
-  def can?(*)
-    true
-  end
 
   let!(:project) { create(:project) }
-  let(:empty_project) { create(:empty_project) }
 
   let(:user)          { create(:user, username: 'gfm') }
   let(:commit)        { project.repository.commit }
-  let(:earlier_commit){ project.repository.commit("HEAD~2") }
   let(:issue)         { create(:issue, project: project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:snippet)       { create(:project_snippet, project: project) }
-  let(:member)        { project.project_members.where(user_id: user).first }
 
   # Helper expects a current_user method.
   let(:current_user) { user }
 
-  def url_helper(image_name)
-    File.join(root_url, 'assets', image_name)
-  end
-
   before do
     # Helper expects a @project instance variable
     @project = project
-    @ref = 'markdown'
-    @repository = project.repository
-    @request.host = Gitlab.config.gitlab.host
   end
 
   describe "#gfm" do
@@ -60,53 +44,6 @@ describe GitlabMarkdownHelper do
       end
     end
 
-    # TODO (rspeicher): These tests belong in the emoji filter spec
-    describe "emoji" do
-      it "matches at the start of a string" do
-        expect(gfm(":+1:")).to match(/<img/)
-      end
-
-      it "matches at the end of a string" do
-        expect(gfm("This gets a :-1:")).to match(/<img/)
-      end
-
-      it "matches with adjacent text" do
-        expect(gfm("+1 (:+1:)")).to match(/<img/)
-      end
-
-      it "has a title attribute" do
-        expect(gfm(":-1:")).to match(/title=":-1:"/)
-      end
-
-      it "has an alt attribute" do
-        expect(gfm(":-1:")).to match(/alt=":-1:"/)
-      end
-
-      it "has an emoji class" do
-        expect(gfm(":+1:")).to match('class="emoji"')
-      end
-
-      it "sets height and width" do
-        actual = gfm(":+1:")
-        expect(actual).to match(/width="20"/)
-        expect(actual).to match(/height="20"/)
-      end
-
-      it "keeps whitespace intact" do
-        expect(gfm('This deserves a :+1: big time.')).
-          to match(/deserves a <img.+> big time/)
-      end
-
-      it "ignores invalid emoji" do
-        expect(gfm(":invalid-emoji:")).not_to match(/<img/)
-      end
-
-      it "should work independent of reference links (i.e. without @project being set)" do
-        @project = nil
-        expect(gfm(":+1:")).to match(/<img/)
-      end
-    end
-
     context 'parse_tasks: true' do
       before(:all) do
         @source_text_asterisk = <<-EOT.strip_heredoc
@@ -218,43 +155,48 @@ describe GitlabMarkdownHelper do
     end
   end
 
-  describe "#link_to_gfm" do
+  describe '#link_to_gfm' do
     let(:commit_path) { namespace_project_commit_path(project.namespace, project, commit) }
     let(:issues)      { create_list(:issue, 2, project: project) }
 
-    it "should handle references nested in links with all the text" do
+    it 'should handle references nested in links with all the text' do
       actual = link_to_gfm("This should finally fix ##{issues[0].iid} and ##{issues[1].iid} for real", commit_path)
+      doc = Nokogiri::HTML.parse(actual)
 
-      # Break the result into groups of links with their content, without
-      # closing tags
-      groups = actual.split("</a>")
+      # Make sure we didn't create invalid markup
+      expect(doc.errors).to be_empty
 
       # Leading commit link
-      expect(groups[0]).to match(/href="#{commit_path}"/)
-      expect(groups[0]).to match(/This should finally fix $/)
+      expect(doc.css('a')[0].attr('href')).to eq commit_path
+      expect(doc.css('a')[0].text).to eq 'This should finally fix '
 
       # First issue link
-      expect(groups[1]).
-        to match(/href="#{namespace_project_issue_path(project.namespace, project, issues[0])}"/)
-      expect(groups[1]).to match(/##{issues[0].iid}$/)
+      expect(doc.css('a')[1].attr('href')).
+        to eq namespace_project_issue_path(project.namespace, project, issues[0])
+      expect(doc.css('a')[1].text).to eq "##{issues[0].iid}"
 
       # Internal commit link
-      expect(groups[2]).to match(/href="#{commit_path}"/)
-      expect(groups[2]).to match(/ and /)
+      expect(doc.css('a')[2].attr('href')).to eq commit_path
+      expect(doc.css('a')[2].text).to eq ' and '
 
       # Second issue link
-      expect(groups[3]).
-        to match(/href="#{namespace_project_issue_path(project.namespace, project, issues[1])}"/)
-      expect(groups[3]).to match(/##{issues[1].iid}$/)
+      expect(doc.css('a')[3].attr('href')).
+        to eq namespace_project_issue_path(project.namespace, project, issues[1])
+      expect(doc.css('a')[3].text).to eq "##{issues[1].iid}"
 
       # Trailing commit link
-      expect(groups[4]).to match(/href="#{commit_path}"/)
-      expect(groups[4]).to match(/ for real$/)
+      expect(doc.css('a')[4].attr('href')).to eq commit_path
+      expect(doc.css('a')[4].text).to eq ' for real'
     end
 
-    it "should forward HTML options" do
+    it 'should forward HTML options' do
       actual = link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo')
-      expect(actual).to have_selector 'a.gfm.gfm-commit.foo'
+      doc = Nokogiri::HTML.parse(actual)
+
+      expect(doc.css('a')).to satisfy do |v|
+        # 'foo' gets added to all links
+        v.all? { |a| a.attr('class').match(/foo$/) }
+      end
     end
 
     it "escapes HTML passed in as the body" do
@@ -267,17 +209,6 @@ describe GitlabMarkdownHelper do
   describe "#markdown" do
     # TODO (rspeicher) - This block tests multiple different contexts. Break this up!
 
-    # REFERENCES (PART TWO: THE REVENGE) ---------------------------------------
-
-    it "should handle references in headers" do
-      actual = "\n# Working around ##{issue.iid}\n## Apply !#{merge_request.iid}"
-
-      expect(markdown(actual, no_header_anchors: true)).
-        to match(%r{<h1[^<]*>Working around <a.+>##{issue.iid}</a></h1>})
-      expect(markdown(actual, no_header_anchors: true)).
-        to match(%r{<h2[^<]*>Apply <a.+>!#{merge_request.iid}</a></h2>})
-    end
-
     it "should add ids and links to headers" do
       # Test every rule except nested tags.
       text = '..Ab_c-d. e..'
@@ -293,6 +224,17 @@ describe GitlabMarkdownHelper do
       )
     end
 
+    # REFERENCES (PART TWO: THE REVENGE) ---------------------------------------
+
+    it "should handle references in headers" do
+      actual = "\n# Working around ##{issue.iid}\n## Apply !#{merge_request.iid}"
+
+      expect(markdown(actual, no_header_anchors: true)).
+        to match(%r{<h1[^<]*>Working around <a.+>##{issue.iid}</a></h1>})
+      expect(markdown(actual, no_header_anchors: true)).
+        to match(%r{<h2[^<]*>Apply <a.+>!#{merge_request.iid}</a></h2>})
+    end
+
     it "should handle references in <em>" do
       actual = "Apply _!#{merge_request.iid}_ ASAP"
 
@@ -308,16 +250,15 @@ describe GitlabMarkdownHelper do
 
       target_html = "<pre class=\"code highlight white plaintext\"><code>some code from $#{snippet.id}\nhere too\n</code></pre>\n"
 
-      expect(helper.markdown("\n    some code from $#{snippet.id}\n    here too\n")).
+      expect(markdown("\n    some code from $#{snippet.id}\n    here too\n")).
         to eq(target_html)
-      expect(helper.markdown("\n```\nsome code from $#{snippet.id}\nhere too\n```\n")).
+      expect(markdown("\n```\nsome code from $#{snippet.id}\nhere too\n```\n")).
         to eq(target_html)
     end
 
     it "should leave inline code untouched" do
-      expect(markdown("\nDon't use `$#{snippet.id}` here.\n")).to eq(
-        "<p>Don't use <code>$#{snippet.id}</code> here.</p>\n"
-      )
+      expect(markdown("Don't use `$#{snippet.id}` here.")).
+        to eq "<p>Don't use <code>$#{snippet.id}</code> here.</p>\n"
     end
 
     # REF-LIKE AUTOLINKS? -----------------------------------------------------
@@ -335,35 +276,16 @@ describe GitlabMarkdownHelper do
       expect(markdown("screen shot: ![some image](http://example.tld/#!#{merge_request.iid})")).to eq("<p>screen shot: <img src=\"http://example.tld/#!#{merge_request.iid}\" alt=\"some image\"></p>\n")
     end
 
-    it "should generate absolute urls for refs" do
-      expect(markdown("##{issue.iid}")).to include(namespace_project_issue_path(project.namespace, project, issue))
-    end
-
-    # EMOJI -------------------------------------------------------------------
-
-    it "should generate absolute urls for emoji" do
-      # TODO (rspeicher): Why isn't this with the emoji tests?
-      expect(markdown(':smile:')).to(
-        include(%(src="#{Gitlab.config.gitlab.url}/assets/emoji/#{Emoji.emoji_filename('smile')}.png))
-      )
-    end
-
-    it "should generate absolute urls for emoji if relative url is present" do
-      # TODO (rspeicher): Why isn't this with the emoji tests?
-      allow(Gitlab.config.gitlab).to receive(:url).and_return('http://localhost/gitlab/root')
-      expect(markdown(":smile:")).to include("src=\"http://localhost/gitlab/root/assets/emoji/#{Emoji.emoji_filename('smile')}.png")
-    end
-
-    it "should generate absolute urls for emoji if asset_host is present" do
-      # TODO (rspeicher): Why isn't this with the emoji tests?
-      allow(Gitlab::Application.config).to receive(:asset_host).and_return("https://cdn.example.com")
-      ActionView::Base.any_instance.stub_chain(:config, :asset_host).and_return("https://cdn.example.com")
-      expect(markdown(":smile:")).to include("src=\"https://cdn.example.com/assets/emoji/#{Emoji.emoji_filename('smile')}.png")
-    end
-
     # RELATIVE URLS -----------------------------------------------------------
     # TODO (rspeicher): These belong in a relative link filter spec
 
+    context 'relative links' do
+      context 'with a valid repository' do
+        before do
+          @repository = project.repository
+          @ref = 'markdown'
+        end
+
         it "should handle relative urls for a file in master" do
           actual = "[GitLab API doc](doc/api/README.md)\n"
           expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md\">GitLab API doc</a></p>\n"
@@ -416,8 +338,24 @@ describe GitlabMarkdownHelper do
           actual = '<dl><dt>Term</dt><dd>Definition</dd></dl>'
           expect(markdown(actual)).to match(actual)
         end
+      end
+
+      context 'with an empty repository' do
+        before do
+          @project = create(:empty_project)
+          @repository = @project.repository
+        end
+
+        it "should not touch relative urls" do
+          actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n"
+          expected = "<p><a href=\"doc/api/README.md\">GitLab API doc</a></p>\n"
+          expect(markdown(actual)).to match(expected)
+        end
+      end
+    end
 
     # SANITIZATION ------------------------------------------------------------
+    # TODO (rspeicher): These are testing SanitizationFilter, not `markdown`
 
     it 'should sanitize tags that are not whitelisted' do
       actual = '<textarea>no inputs allowed</textarea> <blink>no blinks</blink>'
@@ -445,20 +383,6 @@ describe GitlabMarkdownHelper do
     end
   end
 
-  # TODO (rspeicher): This should be a context of relative link specs, not its own thing
-  describe 'markdown for empty repository' do
-    before do
-      @project = empty_project
-      @repository = empty_project.repository
-    end
-
-    it "should not touch relative urls" do
-      actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n"
-      expected = "<p><a href=\"doc/api/README.md\">GitLab API doc</a></p>\n"
-      expect(markdown(actual)).to match(expected)
-    end
-  end
-
   describe '#render_wiki_content' do
     before do
       @wiki = double('WikiPage')

spec/lib/gitlab/markdown/emoji_filter_spec.rb

+require 'spec_helper'
+
+module Gitlab::Markdown
+  describe EmojiFilter do
+    def filter(html, contexts = {})
+      described_class.call(html, contexts)
+    end
+
+    before do
+      ActionController::Base.asset_host = 'https://foo.com'
+    end
+
+    it 'replaces supported emoji' do
+      doc = filter('<p>:heart:</p>')
+      expect(doc.css('img').first.attr('src')).to eq 'https://foo.com/assets/emoji/2764.png'
+    end
+
+    it 'ignores unsupported emoji' do
+      exp = act = '<p>:foo:</p>'
+      doc = filter(act)
+      expect(doc.to_html).to match Regexp.escape(exp)
+    end
+
+    it 'correctly encodes the URL' do
+      doc = filter('<p>:+1:</p>')
+      expect(doc.css('img').first.attr('src')).to eq 'https://foo.com/assets/emoji/1F44D.png'
+    end
+
+    it 'matches at the start of a string' do
+      doc = filter(':+1:')
+      expect(doc.css('img').size).to eq 1
+    end
+
+    it 'matches at the end of a string' do
+      doc = filter('This gets a :-1:')
+      expect(doc.css('img').size).to eq 1
+    end
+
+    it 'matches with adjacent text' do
+      doc = filter('+1 (:+1:)')
+      expect(doc.css('img').size).to eq 1
+    end
+
+    it 'matches multiple emoji in a row' do
+      doc = filter(':see_no_evil::hear_no_evil::speak_no_evil:')
+      expect(doc.css('img').size).to eq 3
+    end
+
+    it 'has a title attribute' do
+      doc = filter(':-1:')
+      expect(doc.css('img').first.attr('title')).to eq ':-1:'
+    end
+
+    it 'has an alt attribute' do
+      doc = filter(':-1:')
+      expect(doc.css('img').first.attr('alt')).to eq ':-1:'
+    end
+
+    it 'has an align attribute' do
+      doc = filter(':8ball:')
+      expect(doc.css('img').first.attr('align')).to eq 'absmiddle'
+    end
+
+    it 'has an emoji class' do
+      doc = filter(':cat:')
+      expect(doc.css('img').first.attr('class')).to eq 'emoji'
+    end
+
+    it 'has height and width attributes' do
+      doc = filter(':dog:')
+      img = doc.css('img').first
+
+      expect(img.attr('width')).to eq '20'
+      expect(img.attr('height')).to eq '20'
+    end
+
+    it 'keeps whitespace intact' do
+      doc = filter('This deserves a :+1:, big time.')
+
+      expect(doc.to_html).to match(/^This deserves a <img.+>, big time\.\z/)
+    end
+
+    it 'uses a custom asset_root context' do
+      root = Gitlab.config.gitlab.url + 'gitlab/root'
+
+      doc = filter(':smile:', asset_root: root)
+      expect(doc.css('img').first.attr('src')).to start_with(root)
+    end
+
+    it 'uses a custom asset_host context' do
+      ActionController::Base.asset_host = 'https://cdn.example.com'
+
+      doc = filter(':frowning:', asset_host: 'https://this-is-ignored-i-guess?')
+      expect(doc.css('img').first.attr('src')).to start_with('https://cdn.example.com')
+    end
+  end
+end