Commit 9ee3c5f1 authored by Allison Browne's avatar Allison Browne Committed by Bob Van Landuyt

Add status page image post processing

Use a new status page pipeline and banzai filter to
remove lazy loading of images from html
parent 4ee562ce
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module StatusPage module StatusPage
class IncidentCommentEntity < Grape::Entity 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 expose :created_at
end end
end end
...@@ -6,8 +6,8 @@ module StatusPage ...@@ -6,8 +6,8 @@ module StatusPage
class IncidentEntity < Grape::Entity class IncidentEntity < Grape::Entity
expose :iid, as: :id expose :iid, as: :id
expose :state, as: :status expose :state, as: :status
expose(:title) { |entity| StatusPage::Renderer.markdown(entity, :title) } expose(:title) { |entity| StatusPage::Renderer.markdown(entity, :title, issue_iid: entity.iid) }
expose(:description) { |entity| StatusPage::Renderer.markdown(entity, :description) } expose(:description) { |entity| StatusPage::Renderer.markdown(entity, :description, issue_iid: entity.iid) }
expose :updated_at expose :updated_at
expose :created_at expose :created_at
expose :user_notes, as: :comments, using: IncidentCommentEntity expose :user_notes, as: :comments, using: IncidentCommentEntity
......
...@@ -5,11 +5,11 @@ module StatusPage ...@@ -5,11 +5,11 @@ module StatusPage
entity IncidentEntity entity IncidentEntity
def represent_list(resource) def represent_list(resource)
{ incidents: represent(resource, except: [:comments]) } { incidents: represent(resource, except: [:comments, :description]) }
end end
def represent_details(resource, user_notes) def represent_details(resource, user_notes)
represent(resource, user_notes: user_notes) represent(resource, user_notes: user_notes, issue_iid: resource.iid)
end end
private :represent private :represent
......
...@@ -2,8 +2,12 @@ ...@@ -2,8 +2,12 @@
module StatusPage module StatusPage
module Renderer module Renderer
def self.markdown(object, field) def self.markdown(object, field, issue_iid:)
MarkupHelper.markdown_field(object, field) context = {
post_process_pipeline: StatusPage::Pipeline::PostProcessPipeline,
issue_iid: issue_iid
}
MarkupHelper.markdown_field(object, field, context)
end end
end end
end end
# 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
# frozen_string_literal: true
module StatusPage
module Pipeline
class PostProcessPipeline < ::Banzai::Pipeline::PostProcessPipeline
def self.filters
super + [StatusPage::Filter::ImageFilter]
end
end
end
end
...@@ -17,6 +17,16 @@ module StatusPage ...@@ -17,6 +17,16 @@ module StatusPage
"data/incident/#{id}.json" "data/incident/#{id}.json"
end 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 def self.list_path
'data/list.json' 'data/list.json'
end end
......
{ {
"type": "object", "type": "object",
"required": [ "required": ["id", "status", "title", "links", "updated_at", "created_at"],
"id",
"status",
"title",
"description",
"links",
"updated_at",
"created_at"
],
"properties": { "properties": {
"id": { "type": "integer" }, "id": { "type": "integer" },
"status": { "status": {
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
"allOf": [ "allOf": [
{ "$ref": "basic_incident.json" }, { "$ref": "basic_incident.json" },
{ {
"required": ["comments"], "required": ["comments", "description"],
"properties": { "properties": {
"type": "object", "type": "object",
"items": { "items": {
......
# 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
# 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
...@@ -15,6 +15,18 @@ describe StatusPage::Storage do ...@@ -15,6 +15,18 @@ describe StatusPage::Storage do
it { is_expected.to eq('data/list.json') } it { is_expected.to eq('data/list.json') }
end 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 it 'MAX_KEYS_PER_PAGE times MAX_PAGES establishes upload limit' do
# spec intended to fail if page related MAX constants change # spec intended to fail if page related MAX constants change
# In order to ensure change to documented MAX_IMAGE_UPLOADS is considered # In order to ensure change to documented MAX_IMAGE_UPLOADS is considered
......
...@@ -5,8 +5,9 @@ require 'spec_helper' ...@@ -5,8 +5,9 @@ require 'spec_helper'
describe StatusPage::IncidentCommentEntity do describe StatusPage::IncidentCommentEntity do
let_it_be(:note, reload: true) { create(:note, note: ':ok:') } let_it_be(:note, reload: true) { create(:note, note: ':ok:') }
let(:json) { subject.as_json } 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 it 'exposes JSON fields' do
expect(json).to eq( expect(json).to eq(
...@@ -21,5 +22,11 @@ describe StatusPage::IncidentCommentEntity do ...@@ -21,5 +22,11 @@ describe StatusPage::IncidentCommentEntity do
let(:field) { :note } let(:field) { :note }
let(:value) { json[:note] } let(:value) { json[:note] }
end end
it_behaves_like 'img upload tags for status page' do
let(:object) { note }
let(:field) { :note }
let(:value) { json[:note] }
end
end end
end end
...@@ -11,7 +11,7 @@ describe StatusPage::IncidentEntity do ...@@ -11,7 +11,7 @@ describe StatusPage::IncidentEntity do
let(:json) { subject.as_json } 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 it 'exposes JSON fields' do
expect(json).to eq( expect(json).to eq(
...@@ -26,6 +26,10 @@ describe StatusPage::IncidentEntity do ...@@ -26,6 +26,10 @@ describe StatusPage::IncidentEntity do
) )
end 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 describe 'field #title' do
it_behaves_like 'reference links for status page' do it_behaves_like 'reference links for status page' do
let(:object) { issue } let(:object) { issue }
...@@ -40,6 +44,12 @@ describe StatusPage::IncidentEntity do ...@@ -40,6 +44,12 @@ describe StatusPage::IncidentEntity do
let(:field) { :description } let(:field) { :description }
let(:value) { json[:description] } let(:value) { json[:description] }
end end
it_behaves_like 'img upload tags for status page' do
let(:object) { issue }
let(:field) { :description }
let(:value) { json[:description] }
end
end end
context 'with user notes' do context 'with user notes' do
...@@ -47,7 +57,7 @@ describe StatusPage::IncidentEntity do ...@@ -47,7 +57,7 @@ describe StatusPage::IncidentEntity do
create_list(:note, 1, noteable: issue, project: issue.project) create_list(:note, 1, noteable: issue, project: issue.project)
end 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 it 'exposes comments' do
expect(json).to include(:comments) expect(json).to include(:comments)
......
...@@ -7,12 +7,13 @@ describe StatusPage::Renderer do ...@@ -7,12 +7,13 @@ describe StatusPage::Renderer do
it 'delegates to MarkupHelper.markdown_field' do it 'delegates to MarkupHelper.markdown_field' do
object = Object.new object = Object.new
field = :field field = :field
issue_iid = 1
expect(MarkupHelper) expect(MarkupHelper)
.to receive(:markdown_field) .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 end
end end
# 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
...@@ -9,7 +9,7 @@ module Banzai ...@@ -9,7 +9,7 @@ module Banzai
# Examples: # Examples:
# Pipeline[nil] # => Banzai::Pipeline::FullPipeline # Pipeline[nil] # => Banzai::Pipeline::FullPipeline
# Pipeline[:label] # => Banzai::Pipeline::LabelPipeline # Pipeline[:label] # => Banzai::Pipeline::LabelPipeline
# Pipeline[StatusPage::PostProcessPipeline] # => StatusPage::PostProcessPipeline # Pipeline[StatusPage::Pipeline::PostProcessPipeline] # => StatusPage::Pipeline::PostProcessPipeline
# #
# Pipeline['label'] # => raises ArgumentError - unsupport type # Pipeline['label'] # => raises ArgumentError - unsupport type
# Pipeline[Project] # => raises ArgumentError - not a subclass of BasePipeline # Pipeline[Project] # => raises ArgumentError - not a subclass of BasePipeline
......
...@@ -138,15 +138,18 @@ module Banzai ...@@ -138,15 +138,18 @@ module Banzai
# #
# html - String to process # html - String to process
# context - Hash of options to customize output # context - Hash of options to customize output
# :pipeline - Symbol pipeline type # :pipeline - Symbol pipeline type - for context transform only, defaults to :full
# :project - Project # :project - Project
# :user - User object # :user - User object
# :post_process_pipeline - pipeline to use for post_processing - defaults to PostProcessPipeline
# #
# Returns an HTML-safe String # Returns an HTML-safe String
def self.post_process(html, context) def self.post_process(html, context)
context = Pipeline[context[:pipeline]].transform_context(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] if context[:xhtml]
pipeline.to_document(html, context).to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML) pipeline.to_document(html, context).to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML)
else else
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe Banzai::Renderer do describe Banzai::Renderer do
let(:renderer) { described_class }
def fake_object(fresh:) def fake_object(fresh:)
object = double('object') object = double('object')
...@@ -40,8 +42,6 @@ describe Banzai::Renderer do ...@@ -40,8 +42,6 @@ describe Banzai::Renderer do
end end
describe '#render_field' do describe '#render_field' do
let(:renderer) { described_class }
context 'without cache' do context 'without cache' do
let(:commit) { fake_cacheless_object } let(:commit) { fake_cacheless_object }
...@@ -83,4 +83,57 @@ describe Banzai::Renderer do ...@@ -83,4 +83,57 @@ describe Banzai::Renderer do
end end
end 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 end
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