Ensure rendering parity on DisplayAd, Comment, & Article (#19091)

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)

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

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

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

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)
 

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

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 = /(?<!`)@_\w+_/
+
       def self.call(markdown)
         return unless markdown
 

app/services/markdown_processor/parser.rb

@@ -1,7 +1,5 @@
 module MarkdownProcessor
   class Parser
-    include ApplicationHelper
-
     BAD_XSS_REGEX = [
       /src=["'](data|&)/i,
       %r{data:text/html[,;][\sa-z0-9]*}i,
@@ -20,14 +18,16 @@ module MarkdownProcessor
     #
     # @see LiquidTagBase for more information regarding the liquid tag options.
 
-    def initialize(content, source: nil, user: nil, **liquid_tag_options)
+    def initialize(content, source: nil, user: nil,
+                   liquid_tag_options: {})
       @content = content
       @source = source
       @user = user
       @liquid_tag_options = liquid_tag_options.merge({ source: @source, user: @user })
     end
 
-    def finalize(link_attributes: {})
+    # @param prefix_images_options [Hash] params, that need to be passed further to HtmlParser#prefix_all_images
+    def finalize(link_attributes: {}, prefix_images_options: { width: 800, synchronous_detail_detection: false })
       options = { hard_wrap: true, filter_html: false, link_attributes: link_attributes }
       renderer = Redcarpet::Render::HTMLRouge.new(options)
       markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG)
@@ -35,7 +35,8 @@ module MarkdownProcessor
       code_tag_content = convert_code_tags_to_triple_backticks(@content)
       escaped_content = escape_liquid_tags_in_codeblock(code_tag_content)
       html = markdown.render(escaped_content)
-      sanitized_content = sanitize_rendered_markdown(html)
+      sanitized_content = ActionController::Base.helpers.sanitize html, { scrubber: RenderedMarkdownScrubber.new }
+
       begin
         # NOTE: [@rhymes] liquid 5.0.0 does not support ActiveSupport::SafeBuffer,
         # a String substitute, hence we force the conversion before passing it to Liquid::Template.
@@ -47,7 +48,7 @@ module MarkdownProcessor
         html = e.message
       end
 
-      parse_html(html)
+      parse_html(html, prefix_images_options)
     end
 
     def calculate_reading_time
@@ -125,13 +126,13 @@ module MarkdownProcessor
 
     private
 
-    def parse_html(html)
+    def parse_html(html, prefix_images_options)
       return html if html.blank?
 
       Html::Parser
         .new(html)
         .remove_nested_linebreak_in_list
-        .prefix_all_images
+        .prefix_all_images(**prefix_images_options)
         .wrap_all_images_in_links
         .add_control_class_to_codeblock
         .add_control_panel_to_codeblock

spec/models/display_ad_spec.rb

@@ -4,6 +4,8 @@ RSpec.describe DisplayAd do
   let(:organization) { build(:organization) }
   let(:display_ad) { build(:display_ad, organization: nil) }
 
+  before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
+
   it_behaves_like "Taggable"
 
   describe "validations" do
@@ -61,6 +63,17 @@ RSpec.describe DisplayAd do
     end
   end
 
+  context "when parsing liquid tags" do
+    it "renders username embed" do
+      user = create(:user)
+      url = "#{URL.url}/#{user.username}"
+      allow(UnifiedEmbed::Tag).to receive(:validate_link).with(any_args).and_return(url)
+      username_ad = create(:display_ad, body_markdown: "Hello! {% embed #{url}} %}")
+      expect(username_ad.processed_html).to include("/#{user.username}")
+      expect(username_ad.processed_html).to include("ltag__user__link")
+    end
+  end
+
   context "when callbacks are triggered before save" do
     before { display_ad.save! }
 
@@ -68,6 +81,14 @@ RSpec.describe DisplayAd do
       expect(display_ad.processed_html).to start_with("<p>Hello <em>hey</em> Hey hey")
     end
 
+    it "does not render <div>" do
+      div_html = "<div>Good morning, how are you?</div>"
+      p_html = "<p>Good morning, how are you?</p>"
+      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: "<style>body { color: black}</style> Hey hey")
       expect(display_ad.processed_html).to eq("<p>body { color: black} Hey hey</p>")
@@ -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 = "![Image description](#{image_url})<p style='margin-top:100px'>Hello <em>hey</em> Hey hey</p>"
+      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}")

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) { "<p>hello, hey</p>\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 }
-      <h1>
-        <a name="hey" href="#hey">
-        </a>
-        Hey
-      </h1>
-
-      <p>Hi, hello there, what's up?</p>
-
-    RESULT
+    let(:markdown) { "# Hey\n\nI'm a markdown" }
+    let(:expected_result) do
+      "<h1>\n  <a name=\"hey\" href=\"#hey\">\n  </a>\n  Hey\n</h1>\n\n<p>I'm a markdown</p>\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

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 = "<img src='https://image.com/image.jpg'>"
-        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

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(