Add new render mode and template options for billboards (forem#20282)

* Add new render and template modes for billboards

* Add new fields to /admin

* Adjust test

* Adjust markup in form

* Add tests for script execution

* Add two script test

* Adjust scripts

* Adjust scripts

* Add new test

* Add new explicit tests

* Move JS functionaity around

app/controllers/admin/billboards_controller.rb

@@ -59,7 +59,8 @@ def destroy
     def billboard_params
       params.permit(:organization_id, :body_markdown, :placement_area, :target_geolocations,
                     :published, :approved, :name, :display_to, :tag_list, :type_of,
-                    :exclude_article_ids, :audience_segment_id, :priority)
+                    :exclude_article_ids, :audience_segment_id, :priority,
+                    :render_mode, :template)
     end
 
     def authorize_admin

app/javascript/__tests__/billboard.test.js

@@ -1,7 +1,10 @@
 import { getBillboard } from '../packs/billboard';
+import { executeBBScripts } from '../packs/billboardAfterRenderActions';
 
 describe('getBillboard', () => {
+  let originalFetch;
   beforeEach(() => {
+    originalFetch = global.fetch;
     global.Honeybadger = { notify: jest.fn() };
     document.body.innerHTML = `
       <div>
@@ -11,13 +14,143 @@ describe('getBillboard', () => {
     `;
   });
 
+  afterEach(() => {
+    global.fetch = originalFetch;
+  });
+
   test('should make a call to the correct placement url', async () => {
-    const fetchPromise = Promise.resolve('fetch response');
-    window.fetch = jest.fn(fetchPromise);
+    global.fetch = jest.fn(() =>
+      Promise.resolve({
+        text: () => Promise.resolve('<div>Some HTML content</div>'),
+      }),
+    );
+
+    await getBillboard();
+
+    expect(global.fetch).toHaveBeenCalledWith('/billboards/sidebar_left');
+    expect(global.fetch).toHaveBeenCalledWith('/billboards/sidebar_left_2');
+  });
+
+  test('should execute scripts in the fetched content', async () => {
+    global.fetch = jest.fn(() =>
+      Promise.resolve({
+        text: () =>
+          Promise.resolve(
+            '<script>window.someGlobalVar = "test";</script><script>window.someOtherGlobalVar = "test2";</script>',
+          ),
+      }),
+    );
 
     await getBillboard();
 
-    expect(window.fetch).toHaveBeenCalledWith('/billboards/sidebar_left');
-    expect(window.fetch).toHaveBeenCalledWith('/billboards/sidebar_left_2');
+    expect(window.someGlobalVar).toBe('test');
+    expect(window.someOtherGlobalVar).toBe('test2');
+  });
+
+  test('should handle fetch errors gracefully', async () => {
+    global.fetch = jest.fn(() => Promise.reject(new Error('NetworkError')));
+
+    await getBillboard();
+
+    expect(global.Honeybadger.notify).not.toHaveBeenCalled();
+  });
+
+  test('should report non-network errors to Honeybadger', async () => {
+    global.fetch = jest.fn(() => Promise.reject(new Error('Some other error')));
+
+    await getBillboard();
+
+    expect(global.Honeybadger.notify).toHaveBeenCalledWith(
+      new Error('Some other error'),
+    );
+  });
+
+  test('should clone and re-insert script tags in fetched content', async () => {
+    // Setup fetch to return a script tag
+    global.fetch = jest.fn(() =>
+      Promise.resolve({
+        text: () =>
+          Promise.resolve(
+            '<script type="text/javascript">console.log("test")</script>',
+          ),
+      }),
+    );
+
+    await getBillboard();
+
+    // Find the new script tags in the document
+    const scriptElements = document.querySelectorAll(
+      '.js-billboard-container script',
+    );
+
+    // Assert that the script tags are properly cloned and re-inserted
+    expect(scriptElements.length).toBe(2); // You have two .js-billboard-container in your setup, so expect two script tags
+    scriptElements.forEach((script) => {
+      expect(script.type).toEqual('text/javascript'); // Should retain attributes
+      expect(script.innerHTML).toEqual('console.log("test")'); // Should retain content
+    });
+  });
+});
+
+describe('executeBBScripts', () => {
+  let container;
+
+  beforeEach(() => {
+    container = document.createElement('div');
+    document.body.appendChild(container);
+  });
+
+  afterEach(() => {
+    document.body.removeChild(container);
+  });
+
+  test('should execute script when script tag is present', () => {
+    container.innerHTML = '<script>window.someGlobalVar = "executed";</script>';
+
+    executeBBScripts(container);
+
+    expect(window.someGlobalVar).toBe('executed');
+  });
+
+  test('should skip null or undefined script elements', () => {
+    container.innerHTML = '<script>window.someGlobalVar = "executed";</script>';
+    // Simulating an inconsistency in the returned HTMLCollection
+    const spiedGetElementsByTagName = jest
+      .spyOn(container, 'getElementsByTagName')
+      .mockReturnValue([null, undefined]);
+
+    executeBBScripts(container);
+
+    expect(spiedGetElementsByTagName).toBeCalled();
+  });
+
+  test('should copy attributes of original script element', () => {
+    container.innerHTML =
+      '<script type="text/javascript" async>window.someGlobalVar = "executed";</script>';
+
+    executeBBScripts(container);
+
+    const newScript = container.querySelector('script');
+    expect(newScript.type).toBe('text/javascript');
+  });
+
+  test('should remove the original script element', () => {
+    container.innerHTML = '<script>window.someGlobalVar = "executed";</script>';
+
+    executeBBScripts(container);
+
+    const allScripts = container.getElementsByTagName('script');
+    expect(allScripts.length).toBe(1);
+  });
+
+  test('should insert the new script element at the same position as the original', () => {
+    container.innerHTML =
+      '<div></div><script>window.someGlobalVar = "executed";</script><div></div>';
+
+    executeBBScripts(container);
+
+    const middleChild = container.children[1];
+    expect(middleChild.tagName).toBe('SCRIPT');
+    expect(middleChild.textContent).toBe('window.someGlobalVar = "executed";');
   });
 });

app/javascript/packs/billboard.js

@@ -1,7 +1,10 @@
 import { setupBillboardDropdown } from '../utilities/billboardDropdown';
-import { observeBillboards } from './billboardAfterRenderActions';
+import {
+  observeBillboards,
+  executeBBScripts,
+} from './billboardAfterRenderActions';
 
-async function getBillboard() {
+export async function getBillboard() {
   const placeholderElements = document.getElementsByClassName(
     'js-billboard-container',
   );
@@ -23,6 +26,7 @@ async function generateBillboard(element) {
 
       element.innerHTML = '';
       element.appendChild(generatedElement);
+      executeBBScripts(element);
       setupBillboardDropdown();
       // This is called here because the ad is loaded asynchronously.
       // The original code is still in the asset pipeline, so is not importable.
@@ -38,5 +42,3 @@ async function generateBillboard(element) {
 }
 
 getBillboard();
-
-export { getBillboard };

app/javascript/packs/billboardAfterRenderActions.js

@@ -18,6 +18,30 @@ export function initializeBillboardVisibility() {
   });
 }
 
+export function executeBBScripts(el) {
+  const scriptElements = el.getElementsByTagName('script');
+  let originalElement, copyElement, parentNode, nextSibling, i;
+
+  for (i = 0; i < scriptElements.length; i++) {
+    originalElement = scriptElements[i];
+    if (!originalElement) {
+      continue;
+    }
+    copyElement = document.createElement('script');
+    for (let j = 0; j < originalElement.attributes.length; j++) {
+      copyElement.setAttribute(
+        originalElement.attributes[j].name,
+        originalElement.attributes[j].value,
+      );
+    }
+    copyElement.textContent = originalElement.textContent;
+    parentNode = originalElement.parentNode;
+    nextSibling = originalElement.nextSibling;
+    parentNode.removeChild(originalElement);
+    parentNode.insertBefore(copyElement, nextSibling);
+  }
+}
+
 export function observeBillboards() {
   const observer = new IntersectionObserver(
     (entries) => {

app/models/billboard.rb

@@ -30,6 +30,8 @@ class Billboard < ApplicationRecord
   attribute :target_geolocations, :geolocation_array
   enum display_to: { all: 0, logged_in: 1, logged_out: 2 }, _prefix: true
   enum type_of: { in_house: 0, community: 1, external: 2 }
+  enum render_mode: { forem_markdown: 0, raw: 1 }
+  enum template: { authorship_box: 0, plain: 1 }
 
   belongs_to :organization, optional: true
   has_many :billboard_events, foreign_key: :display_ad_id, inverse_of: :billboard, dependent: :destroy
@@ -214,10 +216,11 @@ def generate_billboard_name
   def process_markdown
     return unless body_markdown_changed?
 
-    if FeatureFlag.enabled?(:consistent_rendering)
+    if render_mode == "forem_markdown"
       extracted_process_markdown
-    else
-      original_process_markdown
+    else # raw
+      self.processed_html = Html::Parser.new(body_markdown)
+        .prefix_all_images(width: 880, synchronous_detail_detection: true).html
     end
   end
 
@@ -228,18 +231,6 @@ def extracted_process_markdown
     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)
-    stripped_html = ActionController::Base.helpers.sanitize initial_html,
-                                                            tags: MarkdownProcessor::AllowedTags::BILLBOARD,
-                                                            attributes: MarkdownProcessor::AllowedAttributes::BILLBOARD
-    html = stripped_html.delete("\n")
-    self.processed_html = Html::Parser.new(html)
-      .prefix_all_images(width: prefix_width, synchronous_detail_detection: true).html
-  end
-
   def prefix_width
     placement_area.include?("sidebar") ? SIDEBAR_WIDTH : POST_WIDTH
   end

app/views/admin/billboards/_form.html.erb

@@ -18,10 +18,20 @@
     </div>
 
     <div class="crayons-field">
-      <%= label_tag :body_markdown, "Body Markdown:", class: "crayons-field__label" %>
+      <%= label_tag :body_markdown, "Body Content:", class: "crayons-field__label" %>
       <%= text_area_tag :body_markdown, @billboard.body_markdown, size: "100x5", class: "crayons-textfield" %>
     </div>
 
+    <div class="crayons-field">
+      <%= label_tag :render_mode, "Render Mode:", class: "crayons-field__label" %>
+      <%= select_tag :render_mode, options_for_select([["Forem Markdown", "forem_markdown"], ["Raw", "raw"]], selected: @billboard.render_mode), class: "crayons-select" %>
+    </div>
+
+    <div class="crayons-field">
+      <%= label_tag :template, "Template:", class: "crayons-field__label" %>
+      <%= select_tag :template, options_for_select([["Authorship Box", "authorship_box"], ["Plain", "plain"]], selected: @billboard.template), class: "crayons-select" %>
+    </div>
+
     <div class="crayons-field">
       <%= label_tag :placement_area, "Placement Area:", class: "crayons-field__label" %>
       <%= select_tag :placement_area, options_for_select(billboards_placement_area_options_array, selected: @billboard.placement_area), include_blank: "Select...", class: "crayons-select js-placement-area" %>

app/views/shared/_billboard.html.erb

@@ -1,4 +1,13 @@
-<% if billboard.placement_area.start_with?("feed_") %>
+<% if billboard.template == "plain" %>
+  <div
+    data-display-unit data-id="<%= billboard.id %>"
+    data-category-click="<%= BillboardEvent::CATEGORY_CLICK %>"
+    data-category-impression="<%= BillboardEvent::CATEGORY_IMPRESSION %>"
+    data-context-type="<%= data_context_type %>"
+    data-type-of="<%= billboard.type_of %>">
+      <%= billboard.processed_html.html_safe %>
+  </div>
+<% elsif billboard.placement_area.start_with?("feed_") %>
   <div class="crayons-story crayons-story__billboard billboard js-billboard"
     data-display-unit data-id="<%= billboard.id %>"
     data-category-click="<%= BillboardEvent::CATEGORY_CLICK %>"

db/migrate/20231026152356_add_render_mode_to_billboards.rb

@@ -0,0 +1,7 @@
+class AddRenderModeToBillboards < ActiveRecord::Migration[7.0]
+  def change
+    # enums
+    add_column :display_ads, :render_mode, :integer, default: 0
+    add_column :display_ads, :template, :integer, default: 0
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_10_18_104337) do
+ActiveRecord::Schema[7.0].define(version: 2023_10_26_152356) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "ltree"
@@ -482,8 +482,10 @@
     t.boolean "priority", default: false
     t.text "processed_html"
     t.boolean "published", default: false
+    t.integer "render_mode", default: 0
     t.float "success_rate", default: 0.0
     t.ltree "target_geolocations", default: [], array: true
+    t.integer "template", default: 0
     t.integer "type_of", default: 0, null: false
     t.datetime "updated_at", precision: nil, null: false
     t.float "weight", default: 1.0, null: false

spec/models/billboard_spec.rb

@@ -5,8 +5,6 @@
   let(:billboard) { build(:billboard, organization: nil) }
   let(:audience_segment) { create(:audience_segment) }
 
-  before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
-
   it_behaves_like "Taggable"
 
   describe "validations" do
@@ -136,6 +134,28 @@
     end
   end
 
+  context "when render_mode is set to raw" do
+    it "outputs processed html that matches the body input" do
+      raw_input = "<style>.bb { color: red }</style><div class=\"bb\">This is a raw div</div>"
+      raw_billboard = create(:billboard, body_markdown: raw_input, render_mode: "raw")
+      expect(raw_billboard.processed_html).to eq raw_input
+    end
+
+    it "still processes images in raw mode" do
+      raw_input = "<div class=\"bb\"><img src=\"https://dummyimage.com/100x100\" /></div>"
+      raw_billboard = create(:billboard, body_markdown: raw_input, render_mode: "raw")
+      expect(raw_billboard.processed_html).to include "dummyimage.com/100x100\" width=\"\" height=\"\" loading=\"lazy\""
+    end
+  end
+
+  context "when render_mode is not set to raw" do
+    it "outputs processed html that sanitizes raw html as if it were markdown" do
+      raw_input = "<style>.bb { color: red }</style><div class=\"bb\">This is a raw div</div>"
+      raw_billboard = create(:billboard, body_markdown: raw_input) # default render mode
+      expect(raw_billboard.processed_html).to eq "<p>.bb { color: red }</p>This is a raw div"
+    end
+  end
+
   context "when callbacks are triggered before save" do
     before { billboard.save! }