Merge branch 'ab-205166-post-process-status-page-images' into 'master'

Post Process Status Page Images

See merge request gitlab-org/gitlab!30181

ee/app/serializers/status_page/incident_comment_entity.rb

@@ -2,7 +2,7 @@
 
 module StatusPage
   class IncidentCommentEntity < Grape::Entity
-    expose(:note) { |entity| StatusPage::Renderer.markdown(entity, :note) }
+    expose(:note) { |entity| StatusPage::Renderer.markdown(entity, :note, issue_iid: options[:issue_iid]) }
     expose :created_at
   end
 end

ee/app/serializers/status_page/incident_entity.rb

@@ -6,8 +6,8 @@ module StatusPage
   class IncidentEntity < Grape::Entity
     expose :iid, as: :id
     expose :state, as: :status
-    expose(:title) { |entity| StatusPage::Renderer.markdown(entity, :title) }
-    expose(:description) { |entity| StatusPage::Renderer.markdown(entity, :description) }
+    expose(:title) { |entity| StatusPage::Renderer.markdown(entity, :title, issue_iid: entity.iid) }
+    expose(:description) { |entity| StatusPage::Renderer.markdown(entity, :description, issue_iid: entity.iid) }
     expose :updated_at
     expose :created_at
     expose :user_notes, as: :comments, using: IncidentCommentEntity

ee/app/serializers/status_page/incident_serializer.rb

@@ -5,11 +5,11 @@ module StatusPage
     entity IncidentEntity
 
     def represent_list(resource)
-      { incidents: represent(resource, except: [:comments]) }
+      { incidents: represent(resource, except: [:comments, :description]) }
     end
 
     def represent_details(resource, user_notes)
-      represent(resource, user_notes: user_notes)
+      represent(resource, user_notes: user_notes, issue_iid: resource.iid)
     end
 
     private :represent

ee/app/serializers/status_page/renderer.rb

@@ -2,8 +2,12 @@
 
 module StatusPage
   module Renderer
-    def self.markdown(object, field)
-      MarkupHelper.markdown_field(object, field)
+    def self.markdown(object, field, issue_iid:)
+      context = {
+        post_process_pipeline: StatusPage::Pipeline::PostProcessPipeline,
+        issue_iid: issue_iid
+      }
+      MarkupHelper.markdown_field(object, field, context)
     end
   end
 end

ee/lib/status_page/filter/image_filter.rb

+# frozen_string_literal: true
+
+module StatusPage
+  # HTML filter that converts lazy loaded img nodes to standard HTML spec img nodes
+  # We do not need to lazy load images on the Status Page
+  module Filter
+    class ImageFilter < HTML::Pipeline::Filter
+      # Part of FileUploader::MARKDOWN_PATTERN but with a non-greedy file name matcher (?<file>.*) vs (?<file>.*?)
+      NON_GREEDY_UPLOAD_FILE_PATH_PATTERN = %r{/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*)}.freeze
+
+      def call
+        doc.css('img').each do |image_node|
+          image_node['class'] = 'gl-image'
+
+          original_src = image_node.delete('data-src').value
+          matches = NON_GREEDY_UPLOAD_FILE_PATH_PATTERN.match(original_src)
+          next unless matches && matches[:secret] && matches[:file]
+
+          change_image_path!(image_node, matches)
+        end
+
+        doc.to_html
+      end
+
+      def change_image_path!(image_node, matches)
+        new_src = ::StatusPage::Storage.upload_path(
+          context[:issue_iid],
+          matches[:secret],
+          matches[:file]
+        )
+        image_node['src'] = new_src
+        image_node.parent['href'] = new_src
+      end
+
+      def validate
+        raise ArgumentError unless context[:issue_iid]
+      end
+    end
+  end
+end

ee/lib/status_page/pipeline/post_process_pipeline.rb

+# frozen_string_literal: true
+
+module StatusPage
+  module Pipeline
+    class PostProcessPipeline < ::Banzai::Pipeline::PostProcessPipeline
+      def self.filters
+        super + [StatusPage::Filter::ImageFilter]
+      end
+    end
+  end
+end

ee/lib/status_page/storage.rb

@@ -17,6 +17,16 @@ module StatusPage
       "data/incident/#{id}.json"
     end
 
+    def self.upload_path(issue_iid, secret, file_name)
+      uploads_path = self.uploads_path(issue_iid)
+
+      File.join(uploads_path, secret, file_name)
+    end
+
+    def self.uploads_path(issue_iid)
+      File.join('data', 'incident', issue_iid.to_s, '/')
+    end
+
     def self.list_path
       'data/list.json'
     end

ee/spec/fixtures/api/schemas/status_page/basic_incident.json

 {
   "type": "object",
-  "required": [
-    "id",
-    "status",
-    "title",
-    "description",
-    "links",
-    "updated_at",
-    "created_at"
-  ],
+  "required": ["id", "status", "title", "links", "updated_at", "created_at"],
   "properties": {
     "id": { "type": "integer" },
     "status": {

ee/spec/fixtures/api/schemas/status_page/incident_details.json

@@ -3,7 +3,7 @@
   "allOf": [
     { "$ref": "basic_incident.json" },
     {
-      "required": ["comments"],
+      "required": ["comments", "description"],
       "properties": {
         "type": "object",
         "items": {

ee/spec/lib/status_page/filter/image_filter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe StatusPage::Filter::ImageFilter do
+  include FilterSpecHelper
+
+  describe '.call' do
+    subject { filter(original_html, context_options) }
+
+    let(:issue_iid) { 1 }
+    let(:secret) { '50b7a196557cf72a98e86a7ab4b1ac3b' }
+    let(:filename) { 'tanuki.png'}
+    let(:original_source_path) { "/uploads/#{secret}/#{filename}" }
+    let(:expected_source_path) { StatusPage::Storage.upload_path(issue_iid, secret, filename) }
+    let(:original_html) { %Q{<a class="no-attachment-icon gfm" href="#{original_source_path}" target="_blank" rel="noopener noreferrer"><img class="lazy" data-src="#{original_source_path}"></a>} }
+    let(:context_options) { { post_process_pipeline: StatusPage::Pipeline::PostProcessPipeline, issue_iid: issue_iid } }
+    let(:img_tag) { Nokogiri::HTML(subject).css('img')[0] }
+    let(:link_tag) { img_tag.parent }
+
+    it { expect(img_tag['src']).to eq(expected_source_path) }
+    it { expect(img_tag['class']).to eq('gl-image') }
+    it { expect(link_tag['href']).to eq(expected_source_path) }
+
+    context 'when no issue_iid key' do
+      let(:context_options) { { post_process_pipeline: StatusPage::Pipeline::PostProcessPipeline } }
+
+      it 'raises error' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'when issue_iid is nil' do
+      let(:issue_iid) { nil }
+
+      it 'raises error' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'no image tags in original html' do
+      let(:original_html) { %{<a href="hello/world"></a>} }
+
+      it { is_expected.to eq(original_html) }
+    end
+  end
+end

ee/spec/lib/status_page/pipeline/post_process_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe StatusPage::Pipeline::PostProcessPipeline do
+  describe '.filters' do
+    subject { described_class.filters }
+
+    it { is_expected.to eq(::Banzai::Pipeline::PostProcessPipeline.filters.push(StatusPage::Filter::ImageFilter)) }
+  end
+end

ee/spec/lib/status_page/storage_spec.rb

@@ -15,6 +15,18 @@ describe StatusPage::Storage do
     it { is_expected.to eq('data/list.json') }
   end
 
+  describe '.upload_path' do
+    subject { described_class.upload_path(2, '50b7a196557cf72a98e86a7ab4b1ac3b', 'screenshot.png') }
+
+    it { is_expected.to eq('data/incident/2/50b7a196557cf72a98e86a7ab4b1ac3b/screenshot.png') }
+  end
+
+  describe '.uploads_path' do
+    subject { described_class.uploads_path(2) }
+
+    it { is_expected.to eq('data/incident/2/') }
+  end
+
   it 'MAX_KEYS_PER_PAGE times MAX_PAGES establishes upload limit' do
     # spec intended to fail if page related MAX constants change
     # In order to ensure change to documented MAX_IMAGE_UPLOADS is considered

ee/spec/serializers/status_page/incident_comment_entity_spec.rb

@@ -5,8 +5,9 @@ require 'spec_helper'
 describe StatusPage::IncidentCommentEntity do
   let_it_be(:note, reload: true) { create(:note, note: ':ok:') }
   let(:json) { subject.as_json }
+  let(:issue) { instance_double(Issue, iid: 1) }
 
-  subject { described_class.new(note) }
+  subject { described_class.new(note, issue_iid: issue.iid) }
 
   it 'exposes JSON fields' do
     expect(json).to eq(
@@ -21,5 +22,11 @@ describe StatusPage::IncidentCommentEntity do
       let(:field) { :note }
       let(:value) { json[:note] }
     end
+
+    it_behaves_like 'img upload tags for status page' do
+      let(:object) { note }
+      let(:field) { :note }
+      let(:value) { json[:note] }
+    end
   end
 end

ee/spec/serializers/status_page/incident_entity_spec.rb

@@ -11,7 +11,7 @@ describe StatusPage::IncidentEntity do
 
   let(:json) { subject.as_json }
 
-  subject { described_class.new(issue) }
+  subject { described_class.new(issue, user_notes: [], issue_iid: issue.iid) }
 
   it 'exposes JSON fields' do
     expect(json).to eq(
@@ -26,6 +26,10 @@ describe StatusPage::IncidentEntity do
     )
   end
 
+  it 'exposes correct data types' do
+    expect(json.to_json).to match_schema('status_page/incident_details', dir: 'ee')
+  end
+
   describe 'field #title' do
     it_behaves_like 'reference links for status page' do
       let(:object) { issue }
@@ -40,6 +44,12 @@ describe StatusPage::IncidentEntity do
       let(:field) { :description }
       let(:value) { json[:description] }
     end
+
+    it_behaves_like 'img upload tags for status page' do
+      let(:object) { issue }
+      let(:field) { :description }
+      let(:value) { json[:description] }
+    end
   end
 
   context 'with user notes' do
@@ -47,7 +57,7 @@ describe StatusPage::IncidentEntity do
       create_list(:note, 1, noteable: issue, project: issue.project)
     end
 
-    subject { described_class.new(issue, user_notes: user_notes) }
+    subject { described_class.new(issue, user_notes: user_notes, issue_iid: issue.iid) }
 
     it 'exposes comments' do
       expect(json).to include(:comments)

ee/spec/serializers/status_page/renderer_spec.rb

@@ -7,12 +7,13 @@ describe StatusPage::Renderer do
     it 'delegates to MarkupHelper.markdown_field' do
       object = Object.new
       field = :field
+      issue_iid = 1
 
       expect(MarkupHelper)
         .to receive(:markdown_field)
-        .with(object, field)
+        .with(object, field, issue_iid: issue_iid, post_process_pipeline: ::StatusPage::Pipeline::PostProcessPipeline)
 
-      described_class.markdown(object, field)
+      described_class.markdown(object, field, issue_iid: issue_iid)
     end
   end
 end

ee/spec/support/shared_examples/status_page/image_post_process_examples.rb

+# frozen_string_literal: true
+
+# This shared_example requires the following variables:
+# - object: The AR object
+# - field: The entity field/AR attribute which contains the GFM reference
+# - value: The resulting field representation
+RSpec.shared_examples "img upload tags for status page" do
+  it 'converts to html' do
+    secret = '50b7a196557cf72a98e86a7ab4b1ac3b'
+    filename = 'tanuki.png'
+    markdown = "![tanuki](/uploads/#{secret}/#{filename})"
+    object.send("#{field}=".to_sym, markdown)
+
+    result_img_tag = Nokogiri::HTML(json[field]).css('img')[0]
+    result_link_tag = result_img_tag.parent
+
+    expected_source_path = StatusPage::Storage.upload_path(issue.iid, secret, filename)
+
+    expect(result_img_tag['class']).to eq 'gl-image'
+    expect(result_img_tag['src']).to eq expected_source_path
+    expect(result_img_tag['alt']).to eq 'tanuki'
+    expect(result_link_tag['href']).to eq expected_source_path
+  end
+end

lib/banzai/pipeline.rb

@@ -9,7 +9,7 @@ module Banzai
     # Examples:
     #     Pipeline[nil] # => Banzai::Pipeline::FullPipeline
     #     Pipeline[:label] # => Banzai::Pipeline::LabelPipeline
-    #     Pipeline[StatusPage::PostProcessPipeline] # => StatusPage::PostProcessPipeline
+    #     Pipeline[StatusPage::Pipeline::PostProcessPipeline] # => StatusPage::Pipeline::PostProcessPipeline
     #
     #     Pipeline['label'] # => raises ArgumentError - unsupport type
     #     Pipeline[Project] # => raises ArgumentError - not a subclass of BasePipeline

lib/banzai/renderer.rb

@@ -138,15 +138,18 @@ module Banzai
     #
     # html     - String to process
     # context  - Hash of options to customize output
-    #            :pipeline  - Symbol pipeline type
+    #            :pipeline  - Symbol pipeline type - for context transform only, defaults to :full
     #            :project   - Project
     #            :user      - User object
+    #            :post_process_pipeline - pipeline to use for post_processing - defaults to PostProcessPipeline
     #
     # Returns an HTML-safe String
     def self.post_process(html, context)
       context = Pipeline[context[:pipeline]].transform_context(context)
 
-      pipeline = Pipeline[:post_process]
+      # Use a passed class for the pipeline or default to PostProcessPipeline
+      pipeline = context.delete(:post_process_pipeline) || ::Banzai::Pipeline::PostProcessPipeline
+
       if context[:xhtml]
         pipeline.to_document(html, context).to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML)
       else

spec/lib/banzai/renderer_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Banzai::Renderer do
+  let(:renderer) { described_class }
+
   def fake_object(fresh:)
     object = double('object')
 
@@ -40,8 +42,6 @@ describe Banzai::Renderer do
   end
 
   describe '#render_field' do
-    let(:renderer) { described_class }
-
     context 'without cache' do
       let(:commit) { fake_cacheless_object }
 
@@ -83,4 +83,57 @@ describe Banzai::Renderer do
       end
     end
   end
+
+  describe '#post_process' do
+    let(:context_options) { {} }
+    let(:html) { 'Consequatur aperiam et nesciunt modi aut assumenda quo id. '}
+    let(:post_processed_html) { double(html_safe: 'safe doc') }
+    let(:doc) { double(to_html: post_processed_html) }
+
+    subject { renderer.post_process(html, context_options) }
+
+    context 'when xhtml' do
+      let(:context_options) { { xhtml: ' ' } }
+
+      context 'without :post_process_pipeline key' do
+        it 'uses PostProcessPipeline' do
+          expect(::Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).and_return(doc)
+
+          subject
+        end
+      end
+
+      context 'with :post_process_pipeline key' do
+        let(:context_options) { { post_process_pipeline: Object, xhtml: ' ' } }
+
+        it 'uses passed post process pipeline' do
+          expect(Object).to receive(:to_document).and_return(doc)
+
+          subject
+        end
+      end
+    end
+
+    context 'when not xhtml' do
+      context 'without :post_process_pipeline key' do
+        it 'uses PostProcessPipeline' do
+          expect(::Banzai::Pipeline::PostProcessPipeline).to receive(:to_html)
+            .with(html, { only_path: true, disable_asset_proxy: true })
+            .and_return(post_processed_html)
+
+          subject
+        end
+      end
+
+      context 'with :post_process_pipeline key' do
+        let(:context_options) { { post_process_pipeline: Object } }
+
+        it 'uses passed post process pipeline' do
+          expect(Object).to receive(:to_html).and_return(post_processed_html)
+
+          subject
+        end
+      end
+    end
+  end
 end