diff --git a/app/models/article.rb b/app/models/article.rb index b71a88ba0..be9d12b4c 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -667,10 +667,12 @@ class Article < ApplicationRecord content_renderer = processed_content return unless content_renderer - self.processed_html = content_renderer.process(calculate_reading_time: true) - self.reading_time = content_renderer.reading_time + result = content_renderer.process_article - front_matter = content_renderer.front_matter + self.processed_html = result.processed_html + self.reading_time = result.reading_time + + front_matter = result.front_matter if front_matter.any? evaluate_front_matter(front_matter) diff --git a/app/models/comment.rb b/app/models/comment.rb index e5a3782e9..e75ab5a3c 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -222,19 +222,11 @@ class Comment < ApplicationRecord end end - def processed_content - return @processed_content if @processed_content && !body_markdown_changed? + def extracted_evaluate_markdown return unless user - @processed_content = ContentRenderer.new(body_markdown, source: self, user: user) - end - - def extracted_evaluate_markdown - content_renderer = processed_content - - return unless content_renderer - - self.processed_html = content_renderer.process(link_attributes: { rel: "nofollow" }) + renderer = ContentRenderer.new(body_markdown, source: self, user: user) + self.processed_html = renderer.process(link_attributes: { rel: "nofollow" }).processed_html wrap_timestamps_if_video_present! if commentable shorten_urls! rescue ContentRenderer::ContentParsingError => e diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb index 2803d3ed8..7b974df20 100644 --- a/app/models/display_ad.rb +++ b/app/models/display_ad.rb @@ -108,6 +108,23 @@ class DisplayAd < ApplicationRecord end def process_markdown + return unless body_markdown_changed? + + if FeatureFlag.enabled?(:consistent_rendering) + extracted_process_markdown + else + original_process_markdown + end + end + + def extracted_process_markdown + renderer = ContentRenderer.new(body_markdown || "", source: self) + self.processed_html = renderer.process(prefix_images_options: { width: prefix_width, + synchronous_detail_detection: true }).processed_html + self.processed_html = processed_html.delete("\n") + end + + def original_process_markdown renderer = Redcarpet::Render::HTMLRouge.new(hard_wrap: true, filter_html: false) markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG) initial_html = markdown.render(body_markdown) @@ -116,7 +133,7 @@ class DisplayAd < ApplicationRecord attributes: MarkdownProcessor::AllowedAttributes::DISPLAY_AD html = stripped_html.delete("\n") self.processed_html = Html::Parser.new(html) - .prefix_all_images(prefix_width, synchronous_detail_detection: true).html + .prefix_all_images(width: prefix_width, synchronous_detail_detection: true).html end def prefix_width diff --git a/app/services/content_renderer.rb b/app/services/content_renderer.rb index e597c2de6..b8d34148f 100644 --- a/app/services/content_renderer.rb +++ b/app/services/content_renderer.rb @@ -1,27 +1,54 @@ +# renders markdown for Articles, DisplayAds, Comments class ContentRenderer - class_attribute :fixer, default: MarkdownProcessor::Fixer::FixAll - class_attribute :front_matter_parser, default: FrontMatterParser::Parser.new(:md) + Result = Struct.new(:front_matter, :reading_time, :processed_html, keyword_init: true) + class_attribute :processor, default: MarkdownProcessor::Parser + class_attribute :front_matter_parser, default: FrontMatterParser::Parser.new(:md) class ContentParsingError < StandardError end - attr_reader :input, :source, :user - attr_accessor :reading_time, :front_matter - - def initialize(input, source:, user:) + # @param input [String] body_markdown to process + # @param source [Article, Comment, DisplayAd] + # @param user [User, NilClass] article's or comment's user, nil for DisplayAd + # @param fixer [Object] fixes the input markdown + def initialize(input, source:, user: nil, fixer: MarkdownProcessor::Fixer::FixAll) @input = input || "" @source = source @user = user + @fixer = fixer end - def process(link_attributes: {}, calculate_reading_time: false) + # @param link_attributes [Hash] options passed further to RedCarpet::Render::HTMLRouge, example: { rel: "nofollow"} + # @param prefix_images_options [Hash] options for Html::Parser#prefix_all_images + # @return [ContentRenderer::Result] + def process(link_attributes: {}, + prefix_images_options: { width: 800, synchronous_detail_detection: false }) + fixed = fixer.call(input) + processed = processor.new(fixed, source: source, user: user) + + processed_html = processed.finalize(link_attributes: link_attributes, + prefix_images_options: prefix_images_options) + + Result.new(front_matter: nil, processed_html: processed_html, reading_time: 0) + rescue StandardError => e + raise ContentParsingError, e.message + end + + # processes Article markdown, calculates reading time and finds frontmatter (if it exists) + # + # @return [ContentRenderer::Result] + def process_article fixed = fixer.call(input) parsed = front_matter_parser.call(fixed) - self.front_matter = parsed.front_matter + front_matter = parsed.front_matter processed = processor.new(parsed.content, source: source, user: user) - self.reading_time = processed.calculate_reading_time if calculate_reading_time - processed.finalize(link_attributes: link_attributes) + + reading_time = processed.calculate_reading_time + + processed_html = processed.finalize + + Result.new(front_matter: front_matter, processed_html: processed_html, reading_time: reading_time) rescue StandardError => e raise ContentParsingError, e.message end @@ -29,9 +56,13 @@ class ContentRenderer def has_front_matter? fixed = fixer.call(input) parsed = front_matter_parser.call(fixed) - self.front_matter = parsed.front_matter + front_matter = parsed.front_matter front_matter.any? && front_matter["title"].present? rescue ContentRenderer::ContentParsingError true end + + private + + attr_reader :fixer, :input, :user, :source end diff --git a/app/services/html/parser.rb b/app/services/html/parser.rb index 323214c67..5de8a5ac0 100644 --- a/app/services/html/parser.rb +++ b/app/services/html/parser.rb @@ -32,7 +32,7 @@ module Html self end - def prefix_all_images(width = 880, synchronous_detail_detection: false) + def prefix_all_images(width: 880, synchronous_detail_detection: false) # wrap with Cloudinary or allow if from giphy or githubusercontent.com doc = Nokogiri::HTML.fragment(@html) diff --git a/app/services/markdown_processor.rb b/app/services/markdown_processor.rb index e97dfdd0e..e0493fa97 100644 --- a/app/services/markdown_processor.rb +++ b/app/services/markdown_processor.rb @@ -29,6 +29,7 @@ module MarkdownProcessor # In FEED but not DISPLAY_AD: [i iframe] # In DISPLAY_AD but not FEED: [abbr add figcaption hr kbd mark rp rt ruby source sub video] + # In DISPLAY_AD but not RENDERED_MARKDOWN_SCRUBBER: [div] DISPLAY_AD = %w[a abbr add b blockquote br center cite code col colgroup dd del div dl dt em figcaption h1 h2 h3 h4 h5 h6 hr img kbd li mark ol p pre q rp rt ruby small source span strong sub sup table tbody td tfoot th thead diff --git a/app/services/markdown_processor/fixer/base.rb b/app/services/markdown_processor/fixer/base.rb index adc42d9f5..70a7d4256 100644 --- a/app/services/markdown_processor/fixer/base.rb +++ b/app/services/markdown_processor/fixer/base.rb @@ -12,6 +12,7 @@ module MarkdownProcessor # Match @_username_ that is not preceded by backtick USERNAME_WITH_UNDERSCORE_REGEXP = /(?Hello hey Hey hey") end + it "does not render
Good morning, how are you?
" + display_ad.update(body_markdown: div_html) + display_ad.reload + expect(display_ad.processed_html).to eq(p_html) + end + it "does not render disallowed tags" do display_ad.update(body_markdown: " Hey hey") expect(display_ad.processed_html).to eq("body { color: black} Hey hey
") @@ -79,9 +100,36 @@ RSpec.describe DisplayAd do end end - describe "after_create callbacks" do + describe "#process_markdown" do + # FastImage.size is called when synchronous_detail_detection: true is passed to Html::Parser#prefix_all_images + # which should be the case for DisplayAd + # Images::Optimizer is also called with widht + it "calls Html::Parser#prefix_all_images with parameters" do + # Html::Parser.new(html).prefix_all_images(prefix_width, synchronous_detail_detection: true).html + image_url = "https://dummyimage.com/100x100" + allow(FastImage).to receive(:size) + allow(Images::Optimizer).to receive(:call).and_return(image_url) + image_md = "Hello hey Hey hey
" + create(:display_ad, body_markdown: image_md, placement_area: "post_comments") + expect(FastImage).to have_received(:size).with(image_url, { timeout: 10 }) + # width is display_ad.prefix_width + expect(Images::Optimizer).to have_received(:call).with(image_url, width: DisplayAd::POST_WIDTH) + # Images::Optimizer.call(source, width: width) + end + + it "keeps the same processed_html if markdown was not changed" do + display_ad = create(:display_ad) + html = display_ad.processed_html + display_ad.update(name: "Sample display ad") + display_ad.reload + expect(display_ad.processed_html).to eq(html) + end + end + + describe "after_save callbacks" do + let!(:display_ad) { create(:display_ad, name: nil) } + it "generates a name when one does not exist" do - display_ad = create(:display_ad, name: nil) display_ad_with_name = create(:display_ad, name: "Test") expect(display_ad.name).to eq("Display Ad #{display_ad.id}") diff --git a/spec/services/content_renderer_spec.rb b/spec/services/content_renderer_spec.rb index fcfe2d95d..71e2433bf 100644 --- a/spec/services/content_renderer_spec.rb +++ b/spec/services/content_renderer_spec.rb @@ -1,93 +1,88 @@ require "rails_helper" RSpec.describe ContentRenderer do + let(:markdown) { "hello, hey" } + let(:renderer) { described_class.new(markdown, source: nil, user: nil, fixer: MarkdownProcessor::Fixer::FixAll) } + + it "calls fixer" do + allow(MarkdownProcessor::Fixer::FixAll).to receive(:call).and_call_original + described_class.new(markdown, source: nil, user: nil, fixer: MarkdownProcessor::Fixer::FixAll).process + expect(MarkdownProcessor::Fixer::FixAll).to have_received(:call).with(markdown) + end + describe "#process" do - let(:markdown) { "hello, hey" } - let(:expected_result) { "hello, hey
\n\n" } - let(:mock_fixer) { class_double MarkdownProcessor::Fixer::FixAll } - let(:mock_front_matter_parser) { instance_double FrontMatterParser::Parser } - let(:mock_processor) { class_double MarkdownProcessor::Parser } - let(:fixed_markdown) { :fixed_markdown } - let(:parsed_contents) { Struct.new(:content, :front_matter).new(:parsed_content) } - let(:processed_contents) { instance_double MarkdownProcessor::Parser } + let(:parser) { instance_double(MarkdownProcessor::Parser) } - # rubocop:disable RSpec/InstanceVariable - before do - allow(mock_fixer).to receive(:call).and_return(fixed_markdown) - allow(mock_front_matter_parser).to receive(:call).with(fixed_markdown).and_return(parsed_contents) - allow(mock_processor).to receive(:new).and_return(processed_contents) - allow(processed_contents).to receive(:finalize).and_return(expected_result) - @original_fixer = described_class.fixer - @original_parser = described_class.front_matter_parser - @original_processor = described_class.processor + context "with double parser" do + before do + allow(MarkdownProcessor::Parser).to receive(:new).and_return(parser) + allow(parser).to receive(:finalize) + allow(parser).to receive(:calculate_reading_time).and_return(1) + end - described_class.fixer = mock_fixer - described_class.front_matter_parser = mock_front_matter_parser - described_class.processor = mock_processor + it "calls finalize with link_attributes" do + renderer.process(link_attributes: { rel: "nofollow" }) + finalize_attrs = { + link_attributes: { rel: "nofollow" }, + prefix_images_options: { width: 800, synchronous_detail_detection: false } + } + expect(parser).to have_received(:finalize).with(finalize_attrs) + end + end + end + + describe "#process_article" do + it "calculates reading time if processing an article" do + result = renderer.process_article + expect(result.reading_time).to eq(1) end - after do - described_class.fixer = @original_fixer - described_class.front_matter_parser = @original_parser - described_class.processor = @original_processor - end - # rubocop:enable RSpec/InstanceVariable - - it "is the result of fixing, parsing, and processing" do - result = described_class.new(markdown, source: nil, user: nil).process - expect(result).to eq(expected_result) - expect(mock_fixer).to have_received(:call) - expect(mock_front_matter_parser).to have_received(:call).with(fixed_markdown) - expect(mock_processor).to have_received(:new) - expect(processed_contents).to have_received(:finalize) + it "sets front_matter if it exists" do + frontmatter_markdown = <<~HEREDOC + --- + title: Hello + published: false + description: Hello Hello + --- + lalalalala + HEREDOC + md_renderer = described_class.new(frontmatter_markdown, source: nil, user: nil, + fixer: MarkdownProcessor::Fixer::FixAll) + result = md_renderer.process_article + expect(result.front_matter["title"]).to eq("Hello") + expect(result.front_matter["description"]).to eq("Hello Hello") end end context "when markdown is valid" do - let(:markdown) { "# Hey\n\nHi, hello there, what's up?" } - let(:expected_result) { <<~RESULT } -Hi, hello there, what's up?
- - RESULT + let(:markdown) { "# Hey\n\nI'm a markdown" } + let(:expected_result) do + "I'm a markdown
\n\n" + end it "processes markdown" do - result = described_class.new(markdown, source: nil, user: nil).process - expect(result).to eq(expected_result) - end - end - - context "when markdown has liquid tags that aren't allowed for user" do - let(:markdown) { "hello hey hey hey {% poll 123 %}" } - let(:article) { build(:article) } - let(:user) { instance_double(User) } - - before do - allow(user).to receive(:any_admin?).and_return(false) - end - - it "raises ContentParsingError" do - expect do - described_class.new(markdown, source: article, user: user).process - end.to raise_error(ContentRenderer::ContentParsingError, /User is not permitted to use this liquid tag/) + result = described_class.new(markdown, source: build(:comment), user: build(:user)).process + expect(result.processed_html).to eq(expected_result) end end context "when markdown has liquid tags that aren't allowed for source" do let(:markdown) { "hello hey hey hey {% poll 123 %}" } - let(:source) { build(:comment) } let(:user) { instance_double(User) } before do allow(user).to receive(:any_admin?).and_return(true) end - it "raises ContentParsingError" do + it "raises ContentParsingError for comment" do + source = build(:comment) + expect do + described_class.new(markdown, source: source, user: user).process + end.to raise_error(ContentRenderer::ContentParsingError, /This liquid tag can only be used in Articles/) + end + + it "raises ContentParsingError for display ad" do + source = build(:display_ad) expect do described_class.new(markdown, source: source, user: user).process end.to raise_error(ContentRenderer::ContentParsingError, /This liquid tag can only be used in Articles/) @@ -99,7 +94,7 @@ RSpec.describe ContentRenderer do it "raises ContentParsingError" do expect do - described_class.new(markdown, source: nil, user: nil).process + described_class.new(markdown, source: nil, user: nil).process_article end.to raise_error(ContentRenderer::ContentParsingError, /while scanning a simple key/) end end diff --git a/spec/services/html/parser_spec.rb b/spec/services/html/parser_spec.rb index fb20064fe..e801ef520 100644 --- a/spec/services/html/parser_spec.rb +++ b/spec/services/html/parser_spec.rb @@ -74,7 +74,7 @@ RSpec.describe Html::Parser, type: :service do it "detects height and width" do allow(FastImage).to receive(:size).and_return([100, 200]) html = "
"
- parsed_html = described_class.new(html).prefix_all_images(350, synchronous_detail_detection: true).html
+ parsed_html = described_class.new(html).prefix_all_images(width: 350, synchronous_detail_detection: true).html
expect(parsed_html).to include("height=\"200\"")
end
end
diff --git a/spec/services/markdown_processor/parser_spec.rb b/spec/services/markdown_processor/parser_spec.rb
index d961c5493..235c2bcd8 100644
--- a/spec/services/markdown_processor/parser_spec.rb
+++ b/spec/services/markdown_processor/parser_spec.rb
@@ -480,7 +480,7 @@ RSpec.describe MarkdownProcessor::Parser, type: :service do
"{% liquid example %}",
source: :my_source,
user: :my_user,
- policy: :my_policy,
+ liquid_tag_options: { policy: :my_policy },
).finalize
expect(Liquid::Template).to have_received(:parse)
.with(