Merge branch '351214-create-markdown-pipeline-for-timeline-events' into 'master'

Add a timeline event pipeline filter to TimelineEvent

See merge request gitlab-org/gitlab!79852

ee/app/models/incident_management/timeline_event.rb

@@ -6,8 +6,7 @@ module IncidentManagement
 
     self.table_name = 'incident_management_timeline_events'
 
-    # TODO: Implement custom pipeline https://gitlab.com/gitlab-org/gitlab/-/issues/351214
-    cache_markdown_field :note, pipeline: :note, issuable_reference_expansion_enabled: true
+    cache_markdown_field :note, pipeline: :'incident_management/timeline_event', issuable_reference_expansion_enabled: true
 
     belongs_to :project
     belongs_to :author, class_name: 'User', foreign_key: :author_id

ee/lib/banzai/pipeline/incident_management/timeline_event_pipeline.rb

+# frozen_string_literal: true
+
+module Banzai
+  module Pipeline
+    module IncidentManagement
+      class TimelineEventPipeline < PlainMarkdownPipeline
+        ALLOWLIST = Banzai::Filter::SanitizationFilter::LIMITED.deep_dup.merge(
+          elements: %w(p b i strong em pre code a img)
+        ).freeze
+
+        def self.filters
+          @filters ||= FilterArray[
+            *super,
+            *Banzai::Pipeline::GfmPipeline.reference_filters,
+            Filter::EmojiFilter,
+            Filter::ExternalLinkFilter,
+            Filter::ImageLinkFilter,
+            Filter::SanitizationFilter
+          ]
+        end
+
+        def self.transform_context(context)
+          Filter::AssetProxyFilter.transform_context(context).merge(
+            only_path: true,
+            no_sourcepos: true,
+            allowlist: ALLOWLIST,
+            link_replaces_image: true
+          )
+        end
+      end
+    end
+  end
+end

ee/spec/lib/banzai/pipeline/incident_management/timeline_event_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Banzai::Pipeline::IncidentManagement::TimelineEventPipeline do
+  let_it_be(:project) { create(:project) }
+
+  describe '.filters' do
+    it 'contains required filters' do
+      expect(described_class.filters).to eq(
+        [
+          *Banzai::Pipeline::PlainMarkdownPipeline.filters,
+          *Banzai::Pipeline::GfmPipeline.reference_filters,
+          Banzai::Filter::EmojiFilter,
+          Banzai::Filter::ExternalLinkFilter,
+          Banzai::Filter::ImageLinkFilter,
+          Banzai::Filter::SanitizationFilter
+        ]
+      )
+    end
+  end
+
+  describe '.to_html' do
+    subject(:output) { described_class.to_html(markdown, project: project) }
+
+    context 'when markdown contains font style transformations' do
+      let(:markdown) { '**bold** _italic_ `code`' }
+
+      it { is_expected.to eq('<p><strong>bold</strong> <em>italic</em> <code>code</code></p>') }
+    end
+
+    context 'when markdown contains banned HTML tags' do
+      let(:markdown) { '<div>div</div><h1>h1</h1>' }
+
+      it 'filters out banned tags' do
+        is_expected.to eq(' div  h1 ')
+      end
+    end
+
+    context 'when markdown contains links' do
+      let(:markdown) { '[GitLab](https://gitlab.com)' }
+
+      it { is_expected.to eq(%q(<p><a href="https://gitlab.com" target="_blank">GitLab</a></p>)) }
+    end
+
+    context 'when markdown contains images' do
+      let(:markdown) { '![Name](/path/to/image.png)' }
+
+      it 'replaces image with a link to the image' do
+        is_expected.to eq(%q(<p><a class="with-attachment-icon" href="/path/to/image.png" target="_blank">Name</a></p>))
+      end
+    end
+
+    context 'when markdown contains emojis' do
+      let(:markdown) { ':+1:👍' }
+
+      it { is_expected.to eq('<p>👍👍</p>') }
+    end
+
+    context 'when markdown contains a reference to an issue' do
+      let!(:issue) { create(:issue, project: project) }
+      let(:markdown) { "issue ##{issue.iid}" }
+
+      it 'contains a link to the issue' do
+        is_expected.to match(%r(<p>issue <a href="[\w/]+-/issues/#{issue.iid}".*>##{issue.iid}</a></p>))
+      end
+    end
+
+    context 'when markdown contains a reference to a merge request' do
+      let!(:mr) { create(:merge_request, source_project: project, target_project: project) }
+      let(:markdown) { "MR !#{mr.iid}" }
+
+      it 'contains a link to the merge request' do
+        is_expected.to match(%r(<p>MR <a href="[\w/]+-/merge_requests/#{mr.iid}".*>!#{mr.iid}</a></p>))
+      end
+    end
+  end
+end

ee/spec/models/incident_management/timeline_event_spec.rb

@@ -41,8 +41,10 @@ RSpec.describe IncidentManagement::TimelineEvent do
   end
 
   describe '#cache_markdown_field' do
-    let(:note) { '<p>some html</p>' }
-    let(:expected_note_html) { '<p dir="auto">some html</p>' }
+    let(:note) { 'note **bold** _italic_ `code` ![image](/path/img.png) :+1:👍' }
+    let(:expected_note_html) do
+      '<p>note <strong>bold</strong> <em>italic</em> <code>code</code> <a class="with-attachment-icon" href="/path/img.png" target="_blank">image</a> 👍👍</p>'
+    end
 
     before do
       allow(Banzai::Renderer).to receive(:cacheless_render_field).and_call_original

lib/banzai/filter/image_link_filter.rb

@@ -8,11 +8,17 @@ module Banzai
       # Find every image that isn't already wrapped in an `a` tag, create
       # a new node (a link to the image source), copy the image as a child
       # of the anchor, and then replace the img with the link-wrapped version.
+      #
+      # If `link_replaces_image` context parameter is provided, the image is going
+      # to be replaced with a link to an image.
       def call
         doc.xpath('descendant-or-self::img[not(ancestor::a)]').each do |img|
+          link_replaces_image = !!context[:link_replaces_image]
+          html_class = link_replaces_image ? 'with-attachment-icon' : 'no-attachment-icon'
+
           link = doc.document.create_element(
             'a',
-            class: 'no-attachment-icon',
+            class: html_class,
             href: img['data-src'] || img['src'],
             target: '_blank',
             rel: 'noopener noreferrer'
@@ -21,7 +27,11 @@ module Banzai
           # make sure the original non-proxied src carries over to the link
           link['data-canonical-src'] = img['data-canonical-src'] if img['data-canonical-src']
 
-          link.children = img.clone
+          link.children = if link_replaces_image
+                            img['alt'] || img['data-src'] || img['src']
+                          else
+                            img.clone
+                          end
 
           img.replace(link)
         end

spec/lib/banzai/filter/image_link_filter_spec.rb

@@ -5,34 +5,82 @@ require 'spec_helper'
 RSpec.describe Banzai::Filter::ImageLinkFilter do
   include FilterSpecHelper
 
-  def image(path)
-    %(<img src="#{path}" />)
+  let(:path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' }
+  let(:context) { {} }
+
+  def image(path, alt: nil, data_src: nil)
+    alt_tag = alt ? %Q{alt="#{alt}"} : ""
+    data_src_tag = data_src ? %Q{data-src="#{data_src}"} : ""
+
+    %(<img src="#{path}" #{alt_tag} #{data_src_tag} />)
   end
 
   it 'wraps the image with a link to the image src' do
-    doc = filter(image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+    doc = filter(image(path), context)
+
     expect(doc.at_css('img')['src']).to eq doc.at_css('a')['href']
   end
 
   it 'does not wrap a duplicate link' do
-    doc = filter(%Q(<a href="/whatever">#{image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')}</a>))
+    doc = filter(%Q(<a href="/whatever">#{image(path)}</a>), context)
+
     expect(doc.to_html).to match %r{^<a href="/whatever"><img[^>]*></a>$}
   end
 
   it 'works with external images' do
-    doc = filter(image('https://i.imgur.com/DfssX9C.jpg'))
+    doc = filter(image('https://i.imgur.com/DfssX9C.jpg'), context)
+
     expect(doc.at_css('img')['src']).to eq doc.at_css('a')['href']
   end
 
   it 'works with inline images' do
-    doc = filter(%Q(<p>test #{image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')} inline</p>))
+    doc = filter(%Q(<p>test #{image(path)} inline</p>), context)
+
     expect(doc.to_html).to match %r{^<p>test <a[^>]*><img[^>]*></a> inline</p>$}
   end
 
   it 'keep the data-canonical-src' do
-    doc = filter(%q(<img src="http://assets.example.com/6cd/4d7" data-canonical-src="http://example.com/test.png" />))
+    doc = filter(%q(<img src="http://assets.example.com/6cd/4d7" data-canonical-src="http://example.com/test.png" />), context)
 
     expect(doc.at_css('img')['src']).to eq doc.at_css('a')['href']
     expect(doc.at_css('img')['data-canonical-src']).to eq doc.at_css('a')['data-canonical-src']
   end
+
+  it 'adds no-attachment icon class to the link' do
+    doc = filter(image(path), context)
+
+    expect(doc.at_css('a')['class']).to match(%r{no-attachment-icon})
+  end
+
+  context 'when :link_replaces_image is true' do
+    let(:context) { { link_replaces_image: true } }
+
+    it 'replaces the image with link to image src', :aggregate_failures do
+      doc = filter(image(path), context)
+
+      expect(doc.to_html).to match(%r{^<a[^>]*>#{path}</a>$})
+      expect(doc.at_css('a')['href']).to eq(path)
+    end
+
+    it 'uses image alt as a link text', :aggregate_failures do
+      doc = filter(image(path, alt: 'My image'), context)
+
+      expect(doc.to_html).to match(%r{^<a[^>]*>My image</a>$})
+      expect(doc.at_css('a')['href']).to eq(path)
+    end
+
+    it 'uses image data-src as a link text', :aggregate_failures do
+      data_src = '/uploads/data-src.png'
+      doc = filter(image(path, data_src: data_src), context)
+
+      expect(doc.to_html).to match(%r{^<a[^>]*>#{data_src}</a>$})
+      expect(doc.at_css('a')['href']).to eq(data_src)
+    end
+
+    it 'adds attachment icon class to the link' do
+      doc = filter(image(path), context)
+
+      expect(doc.at_css('a')['class']).to match(%r{with-attachment-icon})
+    end
+  end
 end