From be63f48d76615179f0072875d5061d8a2683087c Mon Sep 17 00:00:00 2001
From: Joshua Wehner <joshua@forem.com>
Date: Wed, 14 Dec 2022 15:30:26 +0100
Subject: [PATCH] Extract article markdown rendering to service (#18754)

* Beginning, needs cleanup & tests

* Minor refactor/cleanup

* Wondering if this factory ever worked?

* Minor tweak for blank body processing

* Better error handling for ContentRenderer

* Added some tests around ContentRenderer error conditions

* This test seems fine?

* Mocking internal dependencies

* Use new extraction for has_frontmatter?

* FeatureFlag :consistent_rendering

* Try to enable per-actor feature flag
---
 app/models/article.rb                  |  73 +++++++++++++----
 app/services/content_renderer.rb       |  56 +++++++++++++
 app/services/feature_flag.rb           |  10 +++
 spec/factories/articles.rb             |   3 +-
 spec/models/article_spec.rb            |   4 +-
 spec/services/articles/creator_spec.rb |   2 +
 spec/services/articles/updater_spec.rb |   2 +
 spec/services/content_renderer_spec.rb | 107 +++++++++++++++++++++++++
 8 files changed, 241 insertions(+), 16 deletions(-)
 create mode 100644 app/services/content_renderer.rb
 create mode 100644 spec/services/content_renderer_spec.rb

diff --git a/app/models/article.rb b/app/models/article.rb
index f0e36cc05..197dedfe6 100644
--- a/app/models/article.rb
+++ b/app/models/article.rb
@@ -445,6 +445,16 @@ class Article < ApplicationRecord
   end
 
   def has_frontmatter?
+    if FeatureFlag.enabled?(:consistent_rendering, FeatureFlag::Actor[user])
+      front_matter.any? && front_matter["title"].present?
+    else
+      original_has_frontmatter?
+    end
+  rescue ContentRenderer::ContentParsingError
+    true
+  end
+
+  def original_has_frontmatter?
     fixed_body_markdown = MarkdownProcessor::Fixer::FixAll.call(body_markdown)
     begin
       parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown)
@@ -620,7 +630,41 @@ class Article < ApplicationRecord
       .strip
   end
 
+  def processed_content
+    return @processed_content if @processed_content && !body_markdown_changed?
+    return unless user
+
+    @processed_content = ContentRenderer.new(body_markdown, source: self, user: user)
+  end
+
+  delegate :front_matter, to: :processed_content
+
   def evaluate_markdown
+    if FeatureFlag.enabled?(:consistent_rendering, FeatureFlag::Actor[user])
+      extracted_evaluate_markdown
+    else
+      original_evaluate_markdown
+    end
+  end
+
+  def extracted_evaluate_markdown
+    return unless processed_content
+
+    self.reading_time = processed_content.calculate_reading_time
+    self.processed_html = processed_content.finalize
+
+    if front_matter.any?
+      evaluate_front_matter
+    elsif tag_list.any?
+      set_tag_list(tag_list)
+    end
+
+    self.description = processed_description if description.blank?
+  rescue ContentRenderer::ContentParsingError => e
+    errors.add(:base, ErrorMessages::Clean.call(e.message))
+  end
+
+  def original_evaluate_markdown
     fixed_body_markdown = MarkdownProcessor::Fixer::FixAll.call(body_markdown || "")
     parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown)
     parsed_markdown = MarkdownProcessor::Parser.new(parsed.content, source: self, user: user)
@@ -686,25 +730,26 @@ class Article < ApplicationRecord
     Articles::BustMultipleCachesWorker.perform_bulk(job_params)
   end
 
-  def evaluate_front_matter(front_matter)
-    self.title = front_matter["title"] if front_matter["title"].present?
-    set_tag_list(front_matter["tags"]) if front_matter["tags"].present?
-    self.published = front_matter["published"] if %w[true false].include?(front_matter["published"].to_s)
+  # TODO: The param can be removed after with FeatureFlag :consistent_rendering
+  def evaluate_front_matter(hash = front_matter)
+    self.title = hash["title"] if hash["title"].present?
+    set_tag_list(hash["tags"]) if hash["tags"].present?
+    self.published = hash["published"] if %w[true false].include?(hash["published"].to_s)
 
-    self.published_at = front_matter["published_at"] if front_matter["published_at"]
-    self.published_at ||= parse_date(front_matter["date"]) if published
+    self.published_at = hash["published_at"] if hash["published_at"]
+    self.published_at ||= parse_date(hash["date"]) if published
 
-    set_main_image(front_matter)
-    self.canonical_url = front_matter["canonical_url"] if front_matter["canonical_url"].present?
+    set_main_image
+    self.canonical_url = hash["canonical_url"] if hash["canonical_url"].present?
 
-    update_description = front_matter["description"].present? || front_matter["title"].present?
-    self.description = front_matter["description"] if update_description
+    update_description = hash["description"].present? || hash["title"].present?
+    self.description = hash["description"] if update_description
 
-    self.collection_id = nil if front_matter["title"].present?
-    self.collection_id = Collection.find_series(front_matter["series"], user).id if front_matter["series"].present?
+    self.collection_id = nil if hash["title"].present?
+    self.collection_id = Collection.find_series(hash["series"], user).id if hash["series"].present?
   end
 
-  def set_main_image(front_matter)
+  def set_main_image
     # At one point, we have set the main_image based on the front matter. Forever will that now dictate the behavior.
     if main_image_from_frontmatter?
       self.main_image = front_matter["cover_image"]
@@ -879,7 +924,7 @@ class Article < ApplicationRecord
   end
 
   def title_to_slug
-    "#{Sterile.sluggerize(title)}-#{rand(100_000).to_s(26)}" # rubocop:disable Rails/ToSWithArgument
+    "#{Sterile.sluggerize(title)}-#{rand(100_000).to_s(26)}"
   end
 
   def touch_actor_latest_article_updated_at(destroying: false)
diff --git a/app/services/content_renderer.rb b/app/services/content_renderer.rb
new file mode 100644
index 000000000..f8cea2507
--- /dev/null
+++ b/app/services/content_renderer.rb
@@ -0,0 +1,56 @@
+class ContentRenderer
+  class_attribute :fixer, default: MarkdownProcessor::Fixer::FixAll
+  class_attribute :front_matter_parser, default: FrontMatterParser::Parser.new(:md)
+  class_attribute :processor, default: MarkdownProcessor::Parser
+
+  class ContentParsingError < StandardError
+  end
+
+  delegate :calculate_reading_time, to: :processed
+  delegate :content, :front_matter, to: :parsed_input
+
+  attr_reader :input, :source, :user
+
+  def initialize(input, source:, user:)
+    @input = input || ""
+    @source = source
+    @user = user
+  end
+
+  def processed
+    @processed ||= processor.new(content, source: source, user: user)
+  # TODO: Replicating prior behaviour, but this swallows errors we probably shouldn't
+  rescue StandardError => e
+    raise ContentParsingError, e.message
+  end
+
+  def finalize
+    processed.finalize
+  # TODO: Replicating prior behaviour, but this swallows errors we probably shouldn't
+  rescue StandardError => e
+    raise ContentParsingError, e.message
+  end
+
+  private
+
+  def fix(markdown)
+    fixer.call(markdown)
+  # TODO: Replicating prior behaviour, but this swallows errors we probably shouldn't
+  rescue StandardError => e
+    raise ContentParsingError, e.message
+  end
+
+  def parse_front_matter(markdown)
+    front_matter_parser.call(markdown)
+  # TODO: Replicating prior behaviour, but this swallows errors we probably shouldn't
+  rescue StandardError => e
+    raise ContentParsingError, e.message
+  end
+
+  def parsed_input
+    @parsed_input = parse_front_matter(fix(input))
+  # TODO: Replicating prior behaviour, but this swallows errors we probably shouldn't
+  rescue StandardError => e
+    raise ContentParsingError, e.message
+  end
+end
diff --git a/app/services/feature_flag.rb b/app/services/feature_flag.rb
index 7aff2bc26..c75b4ad88 100644
--- a/app/services/feature_flag.rb
+++ b/app/services/feature_flag.rb
@@ -2,6 +2,16 @@
 #
 # @note A wrapper around the Flipper gem
 module FeatureFlag
+  class Actor < SimpleDelegator
+    class << self
+      alias [] new
+    end
+
+    def flipper_id
+      respond_to?(:id) ? id : nil
+    end
+  end
+
   class << self
     delegate :add, :disable, :enable, :enabled?, :exist?, :remove, to: Flipper
 
diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb
index 0f2ed014d..ec3407eae 100644
--- a/spec/factories/articles.rb
+++ b/spec/factories/articles.rb
@@ -5,13 +5,14 @@ FactoryBot.define do
     published_at { Time.current }
 
     transient do
-      title { generate :title }
+      title { generate(:title) }
       published { true }
       date { "01/01/2015" }
       tags { "javascript, html, discuss" }
       canonical_url { Faker::Internet.url }
       with_canonical_url { false }
       with_main_image { true }
+      main_image_from_frontmatter { false }
       with_date { false }
       with_tags { true }
       with_hr_issue { false }
diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb
index 71677b8f3..10a6b556e 100644
--- a/spec/models/article_spec.rb
+++ b/spec/models/article_spec.rb
@@ -7,6 +7,8 @@ RSpec.describe Article do
     article
   end
 
+  before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
+
   let(:user) { create(:user) }
   let!(:article) { create(:article, user: user) }
 
@@ -206,7 +208,7 @@ RSpec.describe Article do
     end
 
     describe "liquid tags" do
-      xit "is not valid if it contains invalid liquid tags" do
+      it "is not valid if it contains invalid liquid tags" do
         body = "{% github /thepracticaldev/dev.to %}"
         article = build(:article, body_markdown: body)
         expect(article).not_to be_valid
diff --git a/spec/services/articles/creator_spec.rb b/spec/services/articles/creator_spec.rb
index f407f71c7..f025c846a 100644
--- a/spec/services/articles/creator_spec.rb
+++ b/spec/services/articles/creator_spec.rb
@@ -3,6 +3,8 @@ require "rails_helper"
 RSpec.describe Articles::Creator, type: :service do
   let(:user) { create(:user) }
 
+  before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
+
   context "when valid attributes" do
     let(:valid_attributes) { attributes_for(:article) }
 
diff --git a/spec/services/articles/updater_spec.rb b/spec/services/articles/updater_spec.rb
index 4462b119f..dd52fde92 100644
--- a/spec/services/articles/updater_spec.rb
+++ b/spec/services/articles/updater_spec.rb
@@ -6,6 +6,8 @@ RSpec.describe Articles::Updater, type: :service do
   let(:attributes) { { body_markdown: "sample" } }
   let(:draft) { create(:article, user: user, published: false, published_at: nil) }
 
+  before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
+
   it "updates an article" do
     described_class.call(user, article, attributes)
     article.reload
diff --git a/spec/services/content_renderer_spec.rb b/spec/services/content_renderer_spec.rb
new file mode 100644
index 000000000..a5c1c498a
--- /dev/null
+++ b/spec/services/content_renderer_spec.rb
@@ -0,0 +1,107 @@
+require "rails_helper"
+
+RSpec.describe ContentRenderer do
+  describe "#finalize" 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).new(:parsed_content) }
+    let(:processed_contents) { 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
+
+      described_class.fixer = mock_fixer
+      described_class.front_matter_parser = mock_front_matter_parser
+      described_class.processor = mock_processor
+    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).finalize
+      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)
+    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
+
+    it "processes markdown" do
+      result = described_class.new(markdown, source: nil, user: nil).finalize
+      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).finalize
+      end.to raise_error(ContentRenderer::ContentParsingError, /User is not permitted to use this liquid tag/)
+    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
+      expect do
+        described_class.new(markdown, source: source, user: user).finalize
+      end.to raise_error(ContentRenderer::ContentParsingError, /This liquid tag can only be used in Articles/)
+    end
+  end
+
+  context "when markdown has invalid frontmatter" do
+    let(:markdown) { "---\ntitle: Title\npublished: false\npublished_at:2022-12-05 18:00 +0300---\n\n" }
+
+    it "raises ContentParsingError" do
+      expect do
+        described_class.new(markdown, source: nil, user: nil).front_matter
+      end.to raise_error(ContentRenderer::ContentParsingError, /while scanning a simple key/)
+    end
+  end
+end