Commit 26de39fc authored by Sean McGivern's avatar Sean McGivern

Merge branch 'bw-commonmark-for-files' into 'master'

Enable CommonMark for files and wikis

See merge request gitlab-org/gitlab-ce!21228
parents 3ff54a95 e57d9994
Pipeline #29539829 passed with stages
in 56 minutes and 22 seconds
......@@ -107,23 +107,23 @@ module MarkupHelper
def markup(file_name, text, context = {})
context[:project] ||= @project
context[:markdown_engine] ||= :redcarpet
context[:markdown_engine] ||= :redcarpet unless commonmark_for_repositories_enabled?
html = context.delete(:rendered) || markup_unsafe(file_name, text, context)
prepare_for_rendering(html, context)
end
def render_wiki_content(wiki_page)
def render_wiki_content(wiki_page, context = {})
text = wiki_page.content
return '' unless text.present?
context = {
context.merge!(
pipeline: :wiki,
project: @project,
project_wiki: @project_wiki,
page_slug: wiki_page.slug,
issuable_state_filter_enabled: true,
markdown_engine: :redcarpet
}
issuable_state_filter_enabled: true
)
context[:markdown_engine] ||= :redcarpet unless commonmark_for_repositories_enabled?
html =
case wiki_page.format
......@@ -178,6 +178,10 @@ module MarkupHelper
end
end
def commonmark_for_repositories_enabled?
Feature.enabled?(:commonmark_for_repositories, default_enabled: true)
end
private
# Return +text+, truncated to +max_chars+ characters, excluding any HTML
......
......@@ -252,6 +252,10 @@ module ProjectsHelper
"xcode://clone?repo=#{CGI.escape(default_url_to_repo(project))}"
end
def legacy_render_context(params)
params[:legacy_render] ? { markdown_engine: :redcarpet } : {}
end
private
def get_project_nav_tabs(project, current_user)
......
......@@ -580,7 +580,12 @@ class Repository
end
def rendered_readme
MarkupHelper.markup_unsafe(readme.name, readme.data, project: project, markdown_engine: :redcarpet) if readme
return unless readme
context = { project: project }
context[:markdown_engine] = :redcarpet unless MarkupHelper.commonmark_for_repositories_enabled?
MarkupHelper.markup_unsafe(readme.name, readme.data, context)
end
cache_method :rendered_readme
......
......@@ -43,6 +43,10 @@ class PreviewMarkdownService < BaseService
end
def markdown_engine
CacheMarkdownField::MarkdownEngine.from_version(params[:markdown_version].to_i)
if params[:legacy_render]
:redcarpet
else
CacheMarkdownField::MarkdownEngine.from_version(params[:markdown_version].to_i)
end
end
end
......@@ -2,7 +2,7 @@
%div{ class: container_class }
.prepend-top-default.append-bottom-default
.wiki
= render_wiki_content(@wiki_home)
= render_wiki_content(@wiki_home, legacy_render_context(params))
- else
- can_create_wiki = can?(current_user, :create_wiki, @project)
.project-home-empty{ class: [('row-content-block' if can_create_wiki), ('content-block' unless can_create_wiki)] }
......
......@@ -21,7 +21,7 @@
Write
%li
= link_to '#preview', 'data-preview-url' => project_preview_blob_path(@project, @id) do
= link_to '#preview', 'data-preview-url' => project_preview_blob_path(@project, @id, legacy_render: params[:legacy_render]) do
= editing_preview_title(@blob.name)
= form_tag(project_update_blob_path(@project, @id), method: :put, class: 'js-quick-submit js-requires-input js-edit-blob-form', data: blob_editor_paths) do
......
......@@ -2,7 +2,7 @@
.diff-content
- if markup?(@blob.name)
.file-content.wiki
= markup(@blob.name, @content)
= markup(@blob.name, @content, legacy_render_context(params))
- else
.file-content.code.js-syntax-highlight
- unless @diff_lines.empty?
......
- blob = viewer.blob
- rendered_markup = blob.rendered_markup if blob.respond_to?(:rendered_markup)
- context = legacy_render_context(params)
- unless context[:markdown_engine] == :redcarpet
- context[:rendered] = blob.rendered_markup if blob.respond_to?(:rendered_markup)
.file-content.wiki
= markup(blob.name, blob.data, rendered: rendered_markup)
= markup(blob.name, blob.data, context)
- commit_message = @page.persisted? ? s_("WikiPageEdit|Update %{page_title}") : s_("WikiPageCreate|Create %{page_title}")
- commit_message = commit_message % { page_title: @page.title }
- if params[:legacy_render] || !commonmark_for_repositories_enabled?
- markdown_version = CacheMarkdownField::CACHE_REDCARPET_VERSION
- else
- markdown_version = 0
= form_for [@project.namespace.becomes(Namespace), @project, @page], method: @page.persisted? ? :put : :post,
html: { class: 'wiki-form common-note-form prepend-top-default js-quick-submit' },
data: { markdown_version: CacheMarkdownField::CACHE_REDCARPET_VERSION } do |f|
data: { markdown_version: markdown_version } do |f|
= form_errors(@page)
- if @page.persisted?
......
......@@ -12,7 +12,7 @@
.blocks-container
.block.block-first
- if @sidebar_page
= render_wiki_content(@sidebar_page)
= render_wiki_content(@sidebar_page, legacy_render_context(params))
- else
%ul.wiki-pages
= render @sidebar_wiki_entries, context: 'sidebar'
......
......@@ -26,6 +26,6 @@
.prepend-top-default.append-bottom-default
.wiki
= render_wiki_content(@page)
= render_wiki_content(@page, legacy_render_context(params))
= render 'sidebar'
......@@ -21,7 +21,7 @@
.file-content.wiki
- snippet_chunks.each do |chunk|
- unless chunk[:data].empty?
= markup(snippet.file_name, chunk[:data])
= markup(snippet.file_name, chunk[:data], legacy_render_context(params))
- else
.file-content.code
.nothing-here-block Empty file
......
---
title: Render files (`.md`) and wikis using CommonMark
merge_request: 21228
author:
type: changed
......@@ -58,13 +58,20 @@ Features that are developed and are intended to be merged behind a feature flag
should not include a changelog entry. The entry should be added in the merge
request removing the feature flags.
In the rare case that you need the feature flag to be on automatically, use
`default_enabled: true` when checking:
```ruby
Feature.enabled?(:feature_flag, project, default_enabled: true)
```
### Specs
In the test environment `Feature.enabled?` is stubbed to always respond to `true`,
so we make sure behavior under feature flag doesn't go untested in some non-specific
contexts.
If you need to test the feature flag in a different state, you need to stub it with:
If you need to test the feature flag in a different state, you need to stub it with:
```ruby
stub_feature_flags(my_feature_flag: false)
......
......@@ -42,13 +42,21 @@ class Feature
persisted_names.include?(feature.name.to_s)
end
def enabled?(key, thing = nil)
get(key).enabled?(thing)
# use `default_enabled: true` to default the flag to being `enabled`
# unless set explicitly. The default is `disabled`
def enabled?(key, thing = nil, default_enabled: false)
feature = Feature.get(key)
# If we're not default enabling the flag or the feature has been set, always evaluate.
# `persisted?` can potentially generate DB queries and also checks for inclusion
# in an array of feature names (177 at last count), possibly reducing performance by half.
# So we only perform the `persisted` check if `default_enabled: true`
!default_enabled || Feature.persisted?(feature) ? feature.enabled?(thing) : true
end
def disabled?(key, thing = nil)
def disabled?(key, thing = nil, default_enabled: false)
# we need to make different method calls to make it easy to mock / define expectations in test mode
thing.nil? ? !enabled?(key) : !enabled?(key, thing)
thing.nil? ? !enabled?(key, default_enabled: default_enabled) : !enabled?(key, thing, default_enabled: default_enabled)
end
def enable(key, thing = true)
......
......@@ -5,8 +5,8 @@ describe 'File blob', :js do
let(:project) { create(:project, :public, :repository) }
def visit_blob(path, anchor: nil, ref: 'master')
visit project_blob_path(project, File.join(ref, path), anchor: anchor)
def visit_blob(path, anchor: nil, ref: 'master', legacy_render: nil)
visit project_blob_path(project, File.join(ref, path), anchor: anchor, legacy_render: legacy_render)
wait_for_requests
end
......@@ -142,6 +142,52 @@ describe 'File blob', :js do
end
end
context 'Markdown rendering' do
before do
project.add_maintainer(project.creator)
Files::CreateService.new(
project,
project.creator,
start_branch: 'master',
branch_name: 'master',
commit_message: "Add RedCarpet and CommonMark Markdown ",
file_path: 'files/commonmark/file.md',
file_content: "1. one\n - sublist\n"
).execute
end
context 'when rendering default markdown' do
before do
visit_blob('files/commonmark/file.md')
wait_for_requests
end
it 'renders using CommonMark' do
aggregate_failures do
expect(page).to have_content("sublist")
expect(page).not_to have_xpath("//ol//li//ul")
end
end
end
context 'when rendering legacy markdown' do
before do
visit_blob('files/commonmark/file.md', legacy_render: 1)
wait_for_requests
end
it 'renders using RedCarpet' do
aggregate_failures do
expect(page).to have_content("sublist")
expect(page).to have_xpath("//ol//li//ul")
end
end
end
end
context 'Markdown file (stored in LFS)' do
before do
project.add_maintainer(project.creator)
......
......@@ -7,6 +7,7 @@ describe 'Editing file blob', :js do
let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') }
let(:branch) { 'master' }
let(:file_path) { project.repository.ls_files(project.repository.root_ref)[1] }
let(:readme_file_path) { 'README.md' }
context 'as a developer' do
let(:user) { create(:user) }
......@@ -20,14 +21,19 @@ describe 'Editing file blob', :js do
def edit_and_commit(commit_changes: true)
wait_for_requests
find('.js-edit-blob').click
find('#editor')
execute_script('ace.edit("editor").setValue("class NextFeature\nend\n")')
fill_editor(content: "class NextFeature\\nend\\n")
if commit_changes
click_button 'Commit changes'
end
end
def fill_editor(content: "class NextFeature\\nend\\n")
wait_for_requests
find('#editor')
execute_script("ace.edit('editor').setValue('#{content}')")
end
context 'from MR diff' do
before do
visit diffs_project_merge_request_path(project, merge_request)
......@@ -63,6 +69,30 @@ describe 'Editing file blob', :js do
expect(new_line_count).to be > 0
end
end
context 'when rendering the preview' do
it 'renders content with CommonMark' do
visit project_edit_blob_path(project, tree_join(branch, readme_file_path))
fill_editor(content: "1. one\\n - sublist\\n")
click_link 'Preview'
wait_for_requests
# the above generates two seperate lists (not embedded) in CommonMark
expect(page).to have_content("sublist")
expect(page).not_to have_xpath("//ol//li//ul")
end
it 'renders content with RedCarpet when legacy_render is set' do
visit project_edit_blob_path(project, tree_join(branch, readme_file_path), legacy_render: 1)
fill_editor(content: "1. one\\n - sublist\\n")
click_link 'Preview'
wait_for_requests
# the above generates a sublist list in RedCarpet
expect(page).to have_content("sublist")
expect(page).to have_xpath("//ol//li//ul")
end
end
end
context 'visit blob edit' do
......
......@@ -162,6 +162,34 @@ describe 'Projects > Wiki > User previews markdown changes', :js do
expect(page.html).to include("<a href=\"/#{project.full_path}/wikis/title%20with%20spaces\">spaced link</a>")
end
end
context 'when rendering the preview' do
it 'renders content with CommonMark' do
create_wiki_page 'a-page/b-page/c-page/common-mark'
click_link 'Edit'
fill_in :wiki_content, with: "1. one\n - sublist\n"
click_on "Preview"
# the above generates two seperate lists (not embedded) in CommonMark
expect(page).to have_content("sublist")
expect(page).not_to have_xpath("//ol//li//ul")
end
it 'renders content with RedCarpet when legacy_render is set' do
wiki_page = create(:wiki_page,
wiki: project.wiki,
attrs: { title: 'home', content: "Empty content" })
visit(project_wiki_edit_path(project, wiki_page, legacy_render: 1))
fill_in :wiki_content, with: "1. one\n - sublist\n"
click_on "Preview"
# the above generates a sublist list in RedCarpet
expect(page).to have_content("sublist")
expect(page).to have_xpath("//ol//li//ul")
end
end
end
it "does not linkify double brackets inside code blocks as expected" do
......
......@@ -68,23 +68,45 @@ describe 'Snippet', :js do
end
end
context 'with cached Redcarpet html' do
let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content, cached_markdown_version: CacheMarkdownField::CACHE_REDCARPET_VERSION) }
context 'Markdown rendering' do
let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content) }
let(:file_name) { 'test.md' }
let(:content) { "1. one\n - sublist\n" }
it 'renders correctly' do
expect(page).to have_xpath("//ol//li//ul")
context 'when rendering default markdown' do
it 'renders using CommonMark' do
expect(page).to have_content("sublist")
expect(page).not_to have_xpath("//ol//li//ul")
end
end
end
context 'with cached CommonMark html' do
let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
let(:file_name) { 'test.md' }
let(:content) { "1. one\n - sublist\n" }
context 'when rendering legacy markdown' do
before do
visit snippet_path(snippet, legacy_render: 1)
it 'renders correctly' do
expect(page).not_to have_xpath("//ol//li//ul")
wait_for_requests
end
it 'renders using RedCarpet' do
expect(page).to have_content("sublist")
expect(page).to have_xpath("//ol//li//ul")
end
end
context 'with cached CommonMark html' do
let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
it 'renders correctly' do
expect(page).not_to have_xpath("//ol//li//ul")
end
end
context 'with cached Redcarpet html' do
let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content, cached_markdown_version: CacheMarkdownField::CACHE_REDCARPET_VERSION) }
it 'renders correctly' do
expect(page).to have_xpath("//ol//li//ul")
end
end
end
......
......@@ -25,17 +25,17 @@ describe MarkupHelper do
let(:actual) { "#{merge_request.to_reference} -> #{commit.to_reference} -> #{issue.to_reference}" }
it "links to the merge request" do
expected = project_merge_request_path(project, merge_request)
expected = urls.project_merge_request_path(project, merge_request)
expect(helper.markdown(actual)).to match(expected)
end
it "links to the commit" do
expected = project_commit_path(project, commit)
expected = urls.project_commit_path(project, commit)
expect(helper.markdown(actual)).to match(expected)
end
it "links to the issue" do
expected = project_issue_path(project, issue)
expected = urls.project_issue_path(project, issue)
expect(helper.markdown(actual)).to match(expected)
end
end
......@@ -46,7 +46,7 @@ describe MarkupHelper do
let(:second_issue) { create(:issue, project: second_project) }
it 'links to the issue' do
expected = project_issue_path(second_project, second_issue)
expected = urls.project_issue_path(second_project, second_issue)
expect(markdown(actual, project: second_project)).to match(expected)
end
end
......@@ -93,7 +93,7 @@ describe MarkupHelper do
# First issue link
expect(doc.css('a')[1].attr('href'))
.to eq project_issue_path(project, issues[0])
.to eq urls.project_issue_path(project, issues[0])
expect(doc.css('a')[1].text).to eq issues[0].to_reference
# Internal commit link
......@@ -102,7 +102,7 @@ describe MarkupHelper do
# Second issue link
expect(doc.css('a')[3].attr('href'))
.to eq project_issue_path(project, issues[1])
.to eq urls.project_issue_path(project, issues[1])
expect(doc.css('a')[3].text).to eq issues[1].to_reference
# Trailing commit link
......@@ -128,7 +128,7 @@ describe MarkupHelper do
# First issue link
expect(doc.css('a')[1].attr('href'))
.to eq project_issue_path(project, issues[0])
.to eq urls.project_issue_path(project, issues[0])
expect(doc.css('a')[1].text).to eq issues[0].to_reference
# Internal commit link
......@@ -137,7 +137,7 @@ describe MarkupHelper do
# Second issue link
expect(doc.css('a')[3].attr('href'))
.to eq project_issue_path(project, issues[1])
.to eq urls.project_issue_path(project, issues[1])
expect(doc.css('a')[3].text).to eq issues[1].to_reference
# Trailing commit link
......@@ -183,7 +183,7 @@ describe MarkupHelper do
doc = Nokogiri::HTML.parse(rendered)
expect(doc.css('a')[0].attr('href'))
.to eq project_issue_path(project, issue)
.to eq urls.project_issue_path(project, issue)
expect(doc.css('a')[0].text).to eq issue.to_reference
wrapped = helper.link_to_html(rendered, link)
......@@ -205,6 +205,17 @@ describe MarkupHelper do
it "uses Wiki pipeline for markdown files" do
allow(@wiki).to receive(:format).and_return(:markdown)
expect(helper).to receive(:markdown_unsafe).with('wiki content',
pipeline: :wiki, project: project, project_wiki: @wiki, page_slug: "nested/page",
issuable_state_filter_enabled: true)
helper.render_wiki_content(@wiki)
end
it 'uses Wiki pipeline for markdown files with RedCarpet if feature disabled' do
stub_feature_flags(commonmark_for_repositories: false)
allow(@wiki).to receive(:format).and_return(:markdown)
expect(helper).to receive(:markdown_unsafe).with('wiki content',
pipeline: :wiki, project: project, project_wiki: @wiki, page_slug: "nested/page",
issuable_state_filter_enabled: true, markdown_engine: :redcarpet)
......@@ -259,10 +270,18 @@ describe MarkupHelper do
expect(helper.markup('foo.md', content, rendered: '<p>NOEL</p>')).to eq('<p>NOEL</p>')
end
it 'defaults to Redcarpet' do
expect(helper).to receive(:markdown_unsafe).with(content, hash_including(markdown_engine: :redcarpet)).and_return('NOEL')
it 'defaults to CommonMark' do
expect(helper.markup('foo.md', 'x^2')).to include('x^2')
end
expect(helper.markup('foo.md', content)).to eq('NOEL')
it 'honors markdown_engine for RedCarpet' do
expect(helper.markup('foo.md', 'x^2', { markdown_engine: :redcarpet })).to include('x<sup>2</sup>')
end
it 'uses RedCarpet if feature disabled' do
stub_feature_flags(commonmark_for_repositories: false)
expect(helper.markup('foo.md', 'x^2', { markdown_engine: :redcarpet })).to include('x<sup>2</sup>')
end
end
......@@ -414,4 +433,8 @@ describe MarkupHelper do
expect(helper.cross_project_reference(project, issue)).to include(project.full_path)
end
end
def urls
Gitlab::Routing.url_helpers
end
end
......@@ -470,4 +470,16 @@ describe ProjectsHelper do
end
end
end
describe '#legacy_render_context' do
it 'returns the redcarpet engine' do
params = { legacy_render: '1' }
expect(helper.legacy_render_context(params)).to include(markdown_engine: :redcarpet)
end
it 'returns nothing' do
expect(helper.legacy_render_context({})).to be_empty
end
end
end
......@@ -121,6 +121,13 @@ describe Banzai::Pipeline::WikiPipeline do
expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page\"")
end
it 'rewrites non-file links (with spaces) to be at the scope of the wiki root' do
markdown = "[Link to Page](page slug)"
output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page%20slug\"")
end
it "rewrites file links to be at the scope of the current directory" do
markdown = "[Link to Page](page.md)"
output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
......@@ -134,6 +141,13 @@ describe Banzai::Pipeline::WikiPipeline do
expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/start-page#title\"")
end
it 'rewrites links (with spaces) with anchor' do
markdown = '[Link to Header](start page#title)'
output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/start%20page#title\"")
end
end
describe "when creating root links" do
......
......@@ -119,6 +119,10 @@ describe Feature do
expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey
end
it 'returns true for undefined feature with default_enabled' do
expect(described_class.enabled?(:some_random_feature_flag, default_enabled: true)).to be_truthy
end
it 'returns false for existing disabled feature in the database' do
described_class.disable(:disabled_feature_flag)
......@@ -160,6 +164,10 @@ describe Feature do
expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy
end
it 'returns false for undefined feature with default_enabled' do
expect(described_class.disabled?(:some_random_feature_flag, default_enabled: true)).to be_falsey
end
it 'returns true for existing disabled feature in the database' do
described_class.disable(:disabled_feature_flag)
......
......@@ -101,4 +101,11 @@ describe PreviewMarkdownService do
expect(result[:markdown_engine]).to eq :common_mark
end
it 'honors the legacy_render parameter' do
service = described_class.new(project, user, { legacy_render: '1' })
result = service.execute
expect(result[:markdown_engine]).to eq :redcarpet
end
end
......@@ -4,8 +4,8 @@ module StubFeatureFlags
# @param [Hash] features where key is feature name and value is boolean whether enabled or not
def stub_feature_flags(features)
features.each do |feature_name, enabled|
allow(Feature).to receive(:enabled?).with(feature_name) { enabled }
allow(Feature).to receive(:enabled?).with(feature_name.to_s) { enabled }
allow(Feature).to receive(:enabled?).with(feature_name, any_args) { enabled }
allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args) { enabled }
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment