From 3faaa8521d3e394cd246031275e024021760eaac Mon Sep 17 00:00:00 2001
From: Karim Semmoud <114252883+karim-semmoud@users.noreply.github.com>
Date: Thu, 22 Jun 2023 02:18:15 +0200
Subject: [PATCH 1/3] Render markdown tables in legislation draft
* Add Tables option to Redcarpet in Legislation draft
* Allow table tags in Admin Legislation Sanitizer
* Add Test to render markdown tables in Legislation drafts
* Add Test for Admin Legislation Sanitizer
We include test for image, table and h1 to h6 tags and additional tests to strengthen the allowed and disallowed parameters
* Add Table from markdown test in System and Factories
* Add test to render tables for admin user
* Remove comment line about Redcarpet options
* Edit custom css for legislation draft table to make it responsive
---
.../stylesheets/legislation_process.scss | 4 +
app/helpers/application_helper.rb | 1 -
app/models/legislation/draft_version.rb | 10 ++-
lib/admin_legislation_sanitizer.rb | 2 +-
spec/factories/legislations.rb | 9 +++
spec/lib/admin_legislation_sanitizer_spec.rb | 65 +++++++++++++++
spec/models/legislation/draft_version_spec.rb | 80 +++++++++++++++++++
.../system/legislation/draft_versions_spec.rb | 26 ++++++
8 files changed, 193 insertions(+), 4 deletions(-)
create mode 100644 spec/lib/admin_legislation_sanitizer_spec.rb
diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss
index c605af7e7..a18266d97 100644
--- a/app/assets/stylesheets/legislation_process.scss
+++ b/app/assets/stylesheets/legislation_process.scss
@@ -459,6 +459,10 @@
padding: rem-calc(32) rem-calc(32) rem-calc(32) rem-calc(48);
}
+ table {
+ @include table-scroll;
+ }
+
h2 {
font-weight: 400;
margin-bottom: rem-calc(32);
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index a0ff6f621..3b2b25315 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -13,7 +13,6 @@ module ApplicationHelper
def markdown(text)
return text if text.blank?
- # See https://github.com/vmg/redcarpet for options
render_options = {
filter_html: false,
hard_wrap: true,
diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb
index c7576c421..7dd36dfa0 100644
--- a/app/models/legislation/draft_version.rb
+++ b/app/models/legislation/draft_version.rb
@@ -22,9 +22,15 @@ class Legislation::DraftVersion < ApplicationRecord
scope :published, -> { where(status: "published").order("id DESC") }
def body_html
- renderer = Redcarpet::Render::HTML.new(with_toc_data: true)
+ render_options = {
+ with_toc_data: true
+ }
+ renderer = Redcarpet::Render::HTML.new(render_options)
+ extensions = {
+ tables: true
+ }
- Redcarpet::Markdown.new(renderer).render(body)
+ Redcarpet::Markdown.new(renderer, extensions).render(body)
end
def toc_html
diff --git a/lib/admin_legislation_sanitizer.rb b/lib/admin_legislation_sanitizer.rb
index 103b305b6..358de0199 100644
--- a/lib/admin_legislation_sanitizer.rb
+++ b/lib/admin_legislation_sanitizer.rb
@@ -1,6 +1,6 @@
class AdminLegislationSanitizer < WYSIWYGSanitizer
def allowed_tags
- super + %w[img h1 h4 h5 h6]
+ super + %w[img h1 h4 h5 h6 table thead tbody tr th td]
end
def allowed_attributes
diff --git a/spec/factories/legislations.rb b/spec/factories/legislations.rb
index 069899884..3c1dbe41b 100644
--- a/spec/factories/legislations.rb
+++ b/spec/factories/legislations.rb
@@ -127,6 +127,15 @@ FactoryBot.define do
trait :final_version do
final_version { true }
end
+
+ trait :with_table do
+ body { <<~BODY_MARKDOWN }
+ | id | name | age | gender |
+ |----|---------|-----|--------|
+ | 1 | Roberta | 39 | M |
+ | 2 | Oliver | 25 | F |
+ BODY_MARKDOWN
+ end
end
factory :legislation_annotation, class: "Legislation::Annotation" do
diff --git a/spec/lib/admin_legislation_sanitizer_spec.rb b/spec/lib/admin_legislation_sanitizer_spec.rb
new file mode 100644
index 000000000..bc467ba84
--- /dev/null
+++ b/spec/lib/admin_legislation_sanitizer_spec.rb
@@ -0,0 +1,65 @@
+require "rails_helper"
+
+describe AdminLegislationSanitizer do
+ let(:sanitizer) { AdminLegislationSanitizer.new }
+
+ describe "#sanitize" do
+ it "allows images" do
+ html = 'Dangerous
image'
+ expect(sanitizer.sanitize(html)).to eq(html)
+ end
+
+ it "allows h1 to h6" do
+ html = 'Heading 1
+ Heading 2
+ Heading 3
+ Heading 4
+ Heading 5
+ Heading 6
'
+ expect(sanitizer.sanitize(html)).to eq(html)
+ end
+
+ it "allows tables" do
+ html = '
+
+
+ | id |
+ name |
+ age |
+ gender |
+
+
+
+
+ | 1 |
+ Roberta |
+ 39 |
+ M |
+
+
+ | 2 |
+ Oliver |
+ 25 |
+ F |
+
+
+
'
+ expect(sanitizer.sanitize(html)).to eq(html)
+ end
+
+ it "allows alt src and id" do
+ html = 'Dangerous image'
+ expect(sanitizer.sanitize(html)).to eq(html)
+ end
+
+ it "doesn't allow style" do
+ html = 'Dangerous image'
+ expect(sanitizer.sanitize(html)).not_to eq(html)
+ end
+
+ it "doesn't allow class" do
+ html = 'Dangerous image'
+ expect(sanitizer.sanitize(html)).not_to eq(html)
+ end
+ end
+end
diff --git a/spec/models/legislation/draft_version_spec.rb b/spec/models/legislation/draft_version_spec.rb
index 910a15f65..59e29d67f 100644
--- a/spec/models/legislation/draft_version_spec.rb
+++ b/spec/models/legislation/draft_version_spec.rb
@@ -29,6 +29,15 @@ describe Legislation::DraftVersion do
expect(legislation_draft_version.toc_html).to eq(toc_html)
end
+ it "renders the tables from the markdown body field" do
+ legislation_draft_version.body = body_with_table_markdown
+
+ legislation_draft_version.save!
+
+ expect(legislation_draft_version.body_html).to eq(body_with_table_html)
+ expect(legislation_draft_version.toc_html).to eq(toc_html)
+ end
+
def body_markdown
<<~BODY_MARKDOWN
# Title 1
@@ -50,6 +59,32 @@ describe Legislation::DraftVersion do
BODY_MARKDOWN
end
+ def body_with_table_markdown
+ <<~BODY_MARKDOWN
+ # Title 1
+
+ Some paragraph.
+
+ A list:
+
+ - item 1
+ - item 2
+
+ ## Subtitle
+
+ Another paragraph.
+
+ # Title 2
+
+ Something about this.
+
+ | id | name | age | gender |
+ |----|---------|-----|--------|
+ | 1 | Roberta | 39 | M |
+ | 2 | Oliver | 25 | F |
+ BODY_MARKDOWN
+ end
+
def body_html
<<~BODY_HTML
Title 1
@@ -73,6 +108,51 @@ describe Legislation::DraftVersion do
BODY_HTML
end
+ def body_with_table_html
+ <<~BODY_HTML
+ Title 1
+
+ Some paragraph.
+
+ A list:
+
+
+
+ Subtitle
+
+ Another paragraph.
+
+ Title 2
+
+ Something about this.
+
+
+
+ | id |
+ name |
+ age |
+ gender |
+
+
+
+ | 1 |
+ Roberta |
+ 39 |
+ M |
+
+
+ | 2 |
+ Oliver |
+ 25 |
+ F |
+
+
+ BODY_HTML
+ end
+
def toc_html
<<~TOC_HTML
diff --git a/spec/system/legislation/draft_versions_spec.rb b/spec/system/legislation/draft_versions_spec.rb
index ec0880f40..b3d69f30e 100644
--- a/spec/system/legislation/draft_versions_spec.rb
+++ b/spec/system/legislation/draft_versions_spec.rb
@@ -397,4 +397,30 @@ describe "Legislation Draft Versions" do
expect(page).to have_content "my other annotation"
end
end
+
+ context "See table from markdown" do
+ let(:draft_version) { create(:legislation_draft_version, :published, :with_table) }
+ let(:path) do
+ edit_admin_legislation_process_draft_version_path(draft_version.process, draft_version)
+ end
+
+ scenario "See table as a user" do
+ visit legislation_process_draft_version_path(draft_version.process, draft_version)
+
+ expect(page).to have_css("table")
+ expect(page).to have_content "Roberta"
+ expect(page).to have_content "25"
+ end
+
+ scenario "See table as an admin" do
+ login_as(administrator)
+
+ visit path
+ click_link "Launch text editor"
+
+ expect(page).to have_css("table")
+ expect(page).to have_content "Roberta"
+ expect(page).to have_content "25"
+ end
+ end
end
From 79120209da24f444a2bd6b6d9c01678c7612d00e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?=
Date: Thu, 29 Jun 2023 17:25:27 +0200
Subject: [PATCH 2/3] Use the same extensions in all markdown renderers
We were using two different sets of extensions but, since the markdown
code is always written by administrators, IMHO it makes sense to be
consistent and always render markdown code the same way.
---
app/helpers/application_helper.rb | 10 ++++++----
app/models/legislation/draft_version.rb | 10 +---------
spec/models/legislation/draft_version_spec.rb | 9 ++++++---
3 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 3b2b25315..d5afea8cc 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -10,22 +10,24 @@ module ApplicationHelper
%i[ar fa he].include?(locale)
end
- def markdown(text)
+ def markdown(text, **render_options)
return text if text.blank?
- render_options = {
+ default_render_options = {
filter_html: false,
hard_wrap: true,
link_attributes: { target: "_blank" }
}
- renderer = Redcarpet::Render::HTML.new(render_options)
+ renderer = Redcarpet::Render::HTML.new(default_render_options.merge(render_options))
+
extensions = {
autolink: true,
fenced_code_blocks: true,
lax_spacing: true,
no_intra_emphasis: true,
strikethrough: true,
- superscript: true
+ superscript: true,
+ tables: true
}
AdminLegislationSanitizer.new.sanitize(Redcarpet::Markdown.new(renderer, extensions).render(text))
diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb
index 7dd36dfa0..f470a51f1 100644
--- a/app/models/legislation/draft_version.rb
+++ b/app/models/legislation/draft_version.rb
@@ -22,15 +22,7 @@ class Legislation::DraftVersion < ApplicationRecord
scope :published, -> { where(status: "published").order("id DESC") }
def body_html
- render_options = {
- with_toc_data: true
- }
- renderer = Redcarpet::Render::HTML.new(render_options)
- extensions = {
- tables: true
- }
-
- Redcarpet::Markdown.new(renderer, extensions).render(body)
+ ApplicationController.helpers.markdown(body, with_toc_data: true)
end
def toc_html
diff --git a/spec/models/legislation/draft_version_spec.rb b/spec/models/legislation/draft_version_spec.rb
index 59e29d67f..58123c837 100644
--- a/spec/models/legislation/draft_version_spec.rb
+++ b/spec/models/legislation/draft_version_spec.rb
@@ -129,14 +129,16 @@ describe Legislation::DraftVersion do
Something about this.
-
+
+
| id |
name |
age |
gender |
-
+
+
| 1 |
Roberta |
@@ -149,7 +151,8 @@ describe Legislation::DraftVersion do
25 |
F |
-
+
+
BODY_HTML
end
From af618eaa458b769804ad0a405703c96fc62e38aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?=
Date: Thu, 29 Jun 2023 17:39:51 +0200
Subject: [PATCH 3/3] Extract markdown helper logic to a class
This way it'll be easier for other Consul installations to overwrite
parts of the code, like the default options.
---
app/helpers/application_helper.rb | 21 +----------
app/models/legislation/draft_version.rb | 6 ++--
lib/markdown_converter.rb | 48 +++++++++++++++++++++++++
3 files changed, 51 insertions(+), 24 deletions(-)
create mode 100644 lib/markdown_converter.rb
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index d5afea8cc..1377b27d2 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -11,26 +11,7 @@ module ApplicationHelper
end
def markdown(text, **render_options)
- return text if text.blank?
-
- default_render_options = {
- filter_html: false,
- hard_wrap: true,
- link_attributes: { target: "_blank" }
- }
- renderer = Redcarpet::Render::HTML.new(default_render_options.merge(render_options))
-
- extensions = {
- autolink: true,
- fenced_code_blocks: true,
- lax_spacing: true,
- no_intra_emphasis: true,
- strikethrough: true,
- superscript: true,
- tables: true
- }
-
- AdminLegislationSanitizer.new.sanitize(Redcarpet::Markdown.new(renderer, extensions).render(text))
+ MarkdownConverter.new(text, **render_options).render
end
def wysiwyg(text)
diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb
index f470a51f1..b36cb688a 100644
--- a/app/models/legislation/draft_version.rb
+++ b/app/models/legislation/draft_version.rb
@@ -22,13 +22,11 @@ class Legislation::DraftVersion < ApplicationRecord
scope :published, -> { where(status: "published").order("id DESC") }
def body_html
- ApplicationController.helpers.markdown(body, with_toc_data: true)
+ MarkdownConverter.new(body, with_toc_data: true).render
end
def toc_html
- renderer = Redcarpet::Render::HTML_TOC.new(with_toc_data: true)
-
- Redcarpet::Markdown.new(renderer).render(body)
+ MarkdownConverter.new(body).render_toc
end
def display_title
diff --git a/lib/markdown_converter.rb b/lib/markdown_converter.rb
new file mode 100644
index 000000000..d5d9845f0
--- /dev/null
+++ b/lib/markdown_converter.rb
@@ -0,0 +1,48 @@
+class MarkdownConverter
+ attr_reader :text, :render_options
+
+ def initialize(text, **render_options)
+ @text = text
+ @render_options = render_options
+ end
+
+ def render
+ return text if text.blank?
+
+ AdminLegislationSanitizer.new.sanitize(Redcarpet::Markdown.new(renderer, extensions).render(text))
+ end
+
+ def render_toc
+ Redcarpet::Markdown.new(toc_renderer).render(text)
+ end
+
+ private
+
+ def renderer
+ Redcarpet::Render::HTML.new(default_render_options.merge(render_options))
+ end
+
+ def toc_renderer
+ Redcarpet::Render::HTML_TOC.new(with_toc_data: true)
+ end
+
+ def default_render_options
+ {
+ filter_html: false,
+ hard_wrap: true,
+ link_attributes: { target: "_blank" }
+ }
+ end
+
+ def extensions
+ {
+ autolink: true,
+ fenced_code_blocks: true,
+ lax_spacing: true,
+ no_intra_emphasis: true,
+ strikethrough: true,
+ superscript: true,
+ tables: true
+ }
+ end
+end