Commit ace833b3 authored by Adam Buckland's avatar Adam Buckland Committed by Douwe Maan

Add indication for closed or merged issuables in GFM

Example: for issues that are closed, the links will now show '[closed]'
following the issue number. This is done as post-process after the markdown has
been loaded from the cache as the status of the issue may change between
the cache being populated and the content being displayed.

In order to avoid N+1 queries problem when rendering notes ObjectRenderer
populates the cache of referenced issuables for all notes at once,
before the post processing phase.

As a part of this change, the Banzai BaseParser#grouped_objects_for_nodes
method has been refactored to return a Hash utilising the node itself as the
key, since this was a common pattern of usage for this method.
parent 5d444915
---
title: Add indication for closed or merged issuables in GFM
merge_request: 9462
author: Adam Buckland
module Banzai
module Filter
# HTML filter that appends state information to issuable links.
# Runs as a post-process filter as issuable state might change whilst
# Markdown is in the cache.
#
# This filter supports cross-project references.
class IssuableStateFilter < HTML::Pipeline::Filter
VISIBLE_STATES = %w(closed merged).freeze
def call
extractor = Banzai::IssuableExtractor.new(project, current_user)
issuables = extractor.extract([doc])
issuables.each do |node, issuable|
if VISIBLE_STATES.include?(issuable.state)
node.children.last.content += " [#{issuable.state}]"
end
end
doc
end
private
def current_user
context[:current_user]
end
def project
context[:project]
end
end
end
end
...@@ -7,7 +7,7 @@ module Banzai ...@@ -7,7 +7,7 @@ module Banzai
# #
class RedactorFilter < HTML::Pipeline::Filter class RedactorFilter < HTML::Pipeline::Filter
def call def call
Redactor.new(project, current_user).redact([doc]) Redactor.new(project, current_user).redact([doc]) unless context[:skip_redaction]
doc doc
end end
......
module Banzai
# Extract references to issuables from multiple documents
# This populates RequestStore cache used in Banzai::ReferenceParser::IssueParser
# and Banzai::ReferenceParser::MergeRequestParser
# Populating the cache should happen before processing documents one-by-one
# so we can avoid N+1 queries problem
class IssuableExtractor
QUERY = %q(
descendant-or-self::a[contains(concat(" ", @class, " "), " gfm ")]
[@data-reference-type="issue" or @data-reference-type="merge_request"]
).freeze
attr_reader :project, :user
def initialize(project, user)
@project = project
@user = user
end
# Returns Hash in the form { node => issuable_instance }
def extract(documents)
nodes = documents.flat_map do |document|
document.xpath(QUERY)
end
issue_parser = Banzai::ReferenceParser::IssueParser.new(project, user)
merge_request_parser = Banzai::ReferenceParser::MergeRequestParser.new(project, user)
issue_parser.issues_for_nodes(nodes).merge(
merge_request_parser.merge_requests_for_nodes(nodes)
)
end
end
end
...@@ -31,7 +31,8 @@ module Banzai ...@@ -31,7 +31,8 @@ module Banzai
# #
# Returns the same input objects. # Returns the same input objects.
def render(objects, attribute) def render(objects, attribute)
documents = render_objects(objects, attribute) documents = render_documents(objects, attribute)
documents = post_process_documents(documents, objects, attribute)
redacted = redact_documents(documents) redacted = redact_documents(documents)
objects.each_with_index do |object, index| objects.each_with_index do |object, index|
...@@ -41,9 +42,24 @@ module Banzai ...@@ -41,9 +42,24 @@ module Banzai
end end
end end
# Renders the attribute of every given object. private
def render_objects(objects, attribute)
render_attributes(objects, attribute) def render_documents(objects, attribute)
pipeline = HTML::Pipeline.new([])
objects.map do |object|
pipeline.to_document(Banzai.render_field(object, attribute))
end
end
def post_process_documents(documents, objects, attribute)
# Called here to populate cache, refer to IssuableExtractor docs
IssuableExtractor.new(project, user).extract(documents)
documents.zip(objects).map do |document, object|
context = context_for(object, attribute)
Banzai::Pipeline[:post_process].to_document(document, context)
end
end end
# Redacts the list of documents. # Redacts the list of documents.
...@@ -57,25 +73,15 @@ module Banzai ...@@ -57,25 +73,15 @@ module Banzai
# Returns a Banzai context for the given object and attribute. # Returns a Banzai context for the given object and attribute.
def context_for(object, attribute) def context_for(object, attribute)
context = base_context.dup base_context.merge(object.banzai_render_context(attribute))
context = context.merge(object.banzai_render_context(attribute))
context
end
# Renders the attributes of a set of objects.
#
# Returns an Array of `Nokogiri::HTML::Document`.
def render_attributes(objects, attribute)
objects.map do |object|
string = Banzai.render_field(object, attribute)
context = context_for(object, attribute)
Banzai::Pipeline[:relative_link].to_document(string, context)
end
end end
def base_context def base_context
@base_context ||= @redaction_context.merge(current_user: user, project: project) @base_context ||= @redaction_context.merge(
current_user: user,
project: project,
skip_redaction: true
)
end end
end end
end end
...@@ -4,6 +4,7 @@ module Banzai ...@@ -4,6 +4,7 @@ module Banzai
def self.filters def self.filters
FilterArray[ FilterArray[
Filter::RelativeLinkFilter, Filter::RelativeLinkFilter,
Filter::IssuableStateFilter,
Filter::RedactorFilter Filter::RedactorFilter
] ]
end end
......
...@@ -62,8 +62,7 @@ module Banzai ...@@ -62,8 +62,7 @@ module Banzai
nodes.select do |node| nodes.select do |node|
if node.has_attribute?(project_attr) if node.has_attribute?(project_attr)
node_id = node.attr(project_attr).to_i can_read_reference?(user, projects[node])
can_read_reference?(user, projects[node_id])
else else
true true
end end
...@@ -112,12 +111,12 @@ module Banzai ...@@ -112,12 +111,12 @@ module Banzai
per_project per_project
end end
# Returns a Hash containing objects for an attribute grouped per their # Returns a Hash containing objects for an attribute grouped per the
# IDs. # nodes that reference them.
# #
# The returned Hash uses the following format: # The returned Hash uses the following format:
# #
# { id value => row } # { node => row }
# #
# nodes - An Array of HTML nodes to process. # nodes - An Array of HTML nodes to process.
# #
...@@ -132,9 +131,14 @@ module Banzai ...@@ -132,9 +131,14 @@ module Banzai
return {} if nodes.empty? return {} if nodes.empty?
ids = unique_attribute_values(nodes, attribute) ids = unique_attribute_values(nodes, attribute)
rows = collection_objects_for_ids(collection, ids) collection_objects = collection_objects_for_ids(collection, ids)
objects_by_id = collection_objects.index_by(&:id)
rows.index_by(&:id) nodes.each_with_object({}) do |node, hash|
if node.has_attribute?(attribute)
hash[node] = objects_by_id[node.attr(attribute).to_i]
end
end
end end
# Returns an Array containing all unique values of an attribute of the # Returns an Array containing all unique values of an attribute of the
...@@ -201,7 +205,7 @@ module Banzai ...@@ -201,7 +205,7 @@ module Banzai
# #
# The returned Hash uses the following format: # The returned Hash uses the following format:
# #
# { project ID => project } # { node => project }
# #
def projects_for_nodes(nodes) def projects_for_nodes(nodes)
@projects_for_nodes ||= @projects_for_nodes ||=
......
...@@ -13,14 +13,14 @@ module Banzai ...@@ -13,14 +13,14 @@ module Banzai
issues_readable_by_user(issues.values, user).to_set issues_readable_by_user(issues.values, user).to_set
nodes.select do |node| nodes.select do |node|
readable_issues.include?(issue_for_node(issues, node)) readable_issues.include?(issues[node])
end end
end end
def referenced_by(nodes) def referenced_by(nodes)
issues = issues_for_nodes(nodes) issues = issues_for_nodes(nodes)
nodes.map { |node| issue_for_node(issues, node) }.uniq nodes.map { |node| issues[node] }.compact.uniq
end end
def issues_for_nodes(nodes) def issues_for_nodes(nodes)
...@@ -44,12 +44,6 @@ module Banzai ...@@ -44,12 +44,6 @@ module Banzai
self.class.data_attribute self.class.data_attribute
) )
end end
private
def issue_for_node(issues, node)
issues[node.attr(self.class.data_attribute).to_i]
end
end end
end end
end end
...@@ -3,6 +3,14 @@ module Banzai ...@@ -3,6 +3,14 @@ module Banzai
class MergeRequestParser < BaseParser class MergeRequestParser < BaseParser
self.reference_type = :merge_request self.reference_type = :merge_request
def merge_requests_for_nodes(nodes)
@merge_requests_for_nodes ||= grouped_objects_for_nodes(
nodes,
MergeRequest.all,
self.class.data_attribute
)
end
def references_relation def references_relation
MergeRequest.includes(:author, :assignee, :target_project) MergeRequest.includes(:author, :assignee, :target_project)
end end
......
...@@ -49,7 +49,7 @@ module Banzai ...@@ -49,7 +49,7 @@ module Banzai
# Check if project belongs to a group which # Check if project belongs to a group which
# user can read. # user can read.
def can_read_group_reference?(node, user, groups) def can_read_group_reference?(node, user, groups)
node_group = groups[node.attr('data-group').to_i] node_group = groups[node]
node_group && can?(user, :read_group, node_group) node_group && can?(user, :read_group, node_group)
end end
...@@ -74,8 +74,8 @@ module Banzai ...@@ -74,8 +74,8 @@ module Banzai
if project && project_id && project.id == project_id.to_i if project && project_id && project.id == project_id.to_i
true true
elsif project_id && user_id elsif project_id && user_id
project = projects[project_id.to_i] project = projects[node]
user = users[user_id.to_i] user = users[node]
project && user ? project.team.member?(user) : false project && user ? project.team.member?(user) : false
else else
......
...@@ -44,6 +44,10 @@ FactoryGirl.define do ...@@ -44,6 +44,10 @@ FactoryGirl.define do
state :reopened state :reopened
end end
trait :locked do
state :locked
end
trait :simple do trait :simple do
source_branch "feature" source_branch "feature"
target_branch "master" target_branch "master"
......
require 'spec_helper'
describe Banzai::Filter::IssuableStateFilter, lib: true do
include ActionView::Helpers::UrlHelper
include FilterSpecHelper
let(:user) { create(:user) }
def create_link(data)
link_to('text', '', class: 'gfm has-tooltip', data: data)
end
it 'ignores non-GFM links' do
html = %(See <a href="https://google.com/">Google</a>)
doc = filter(html, current_user: user)
expect(doc.css('a').last.text).to eq('Google')
end
it 'ignores non-issuable links' do
project = create(:empty_project, :public)
link = create_link(project: project, reference_type: 'issue')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
context 'for issue references' do
it 'ignores open issue references' do
issue = create(:issue)
link = create_link(issue: issue.id, reference_type: 'issue')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
it 'ignores reopened issue references' do
reopened_issue = create(:issue, :reopened)
link = create_link(issue: reopened_issue.id, reference_type: 'issue')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
it 'appends [closed] to closed issue references' do
closed_issue = create(:issue, :closed)
link = create_link(issue: closed_issue.id, reference_type: 'issue')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text [closed]')
end
end
context 'for merge request references' do
it 'ignores open merge request references' do
mr = create(:merge_request)
link = create_link(merge_request: mr.id, reference_type: 'merge_request')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
it 'ignores reopened merge request references' do
mr = create(:merge_request, :reopened)
link = create_link(merge_request: mr.id, reference_type: 'merge_request')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
it 'ignores locked merge request references' do
mr = create(:merge_request, :locked)
link = create_link(merge_request: mr.id, reference_type: 'merge_request')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text')
end
it 'appends [closed] to closed merge request references' do
mr = create(:merge_request, :closed)
link = create_link(merge_request: mr.id, reference_type: 'merge_request')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text [closed]')
end
it 'appends [merged] to merged merge request references' do
mr = create(:merge_request, :merged)
link = create_link(merge_request: mr.id, reference_type: 'merge_request')
doc = filter(link, current_user: user)
expect(doc.css('a').last.text).to eq('text [merged]')
end
end
end
...@@ -15,6 +15,16 @@ describe Banzai::Filter::RedactorFilter, lib: true do ...@@ -15,6 +15,16 @@ describe Banzai::Filter::RedactorFilter, lib: true do
link_to('text', '', class: 'gfm', data: data) link_to('text', '', class: 'gfm', data: data)
end end
it 'skips when the skip_redaction flag is set' do
user = create(:user)
project = create(:empty_project)
link = reference_link(project: project.id, reference_type: 'test')
doc = filter(link, current_user: user, skip_redaction: true)
expect(doc.css('a').length).to eq 1
end
context 'with data-project' do context 'with data-project' do
let(:parser_class) do let(:parser_class) do
Class.new(Banzai::ReferenceParser::BaseParser) do Class.new(Banzai::ReferenceParser::BaseParser) do
......
require 'spec_helper'
describe Banzai::IssuableExtractor, lib: true do
let(:project) { create(:empty_project) }
let(:user) { create(:user) }
let(:extractor) { described_class.new(project, user) }
let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:issue_link) do
html_to_node(
"<a href='' data-issue='#{issue.id}' data-reference-type='issue' class='gfm'>text</a>"
)
end
let(:merge_request_link) do
html_to_node(
"<a href='' data-merge-request='#{merge_request.id}' data-reference-type='merge_request' class='gfm'>text</a>"
)
end
def html_to_node(html)
Nokogiri::HTML.fragment(
html
).children[0]
end
it 'returns instances of issuables for nodes with references' do
result = extractor.extract([issue_link, merge_request_link])
expect(result).to eq(issue_link => issue, merge_request_link => merge_request)
end
describe 'caching' do
before do
RequestStore.begin!
end
after do
RequestStore.end!
RequestStore.clear!
end
it 'saves records to cache' do
extractor.extract([issue_link, merge_request_link])
second_call_queries = ActiveRecord::QueryRecorder.new do
extractor.extract([issue_link, merge_request_link])
end.count
expect(second_call_queries).to eq 0
end
end
end
...@@ -3,128 +3,51 @@ require 'spec_helper' ...@@ -3,128 +3,51 @@ require 'spec_helper'
describe Banzai::ObjectRenderer do describe Banzai::ObjectRenderer do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:user) { project.owner } let(:user) { project.owner }
let(:renderer) { described_class.new(project, user, custom_value: 'value') }
def fake_object(attrs = {}) let(:object) { Note.new(note: 'hello', note_html: '<p>hello</p>') }
object = double(attrs.merge("new_record?" => true, "destroyed?" => true))
allow(object).to receive(:markdown_cache_field_for).with(:note).and_return(:note_html)
allow(object).to receive(:banzai_render_context).with(:note).and_return(project: nil, author: nil)
allow(object).to receive(:update_column).with(:note_html, anything).and_return(true)
object
end
describe '#render' do describe '#render' do
it 'renders and redacts an Array of objects' do it 'renders and redacts an Array of objects' do
renderer = described_class.new(project, user)
object = fake_object(note: 'hello', note_html: nil)
expect(renderer).to receive(:render_objects).with([object], :note).
and_call_original
expect(renderer).to receive(:redact_documents).
with(an_instance_of(Array)).
and_call_original
expect(object).to receive(:redacted_note_html=).with('<p dir="auto">hello</p>')
expect(object).to receive(:user_visible_reference_count=).with(0)
renderer.render([object], :note) renderer.render([object], :note)
end
end
describe '#render_objects' do
it 'renders an Array of objects' do
object = fake_object(note: 'hello', note_html: nil)
renderer = described_class.new(project, user)
expect(renderer).to receive(:render_attributes).with([object], :note). expect(object.redacted_note_html).to eq '<p>hello</p>'
and_call_original expect(object.user_visible_reference_count).to eq 0
rendered = renderer.render_objects([object], :note)
expect(rendered).to be_an_instance_of(Array)
expect(rendered[0]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
end
end
describe '#redact_documents' do
it 'redacts a set of documents and returns them as an Array of Hashes' do
doc = Nokogiri::HTML.fragment('<p>hello</p>')
renderer = described_class.new(project, user)
expect_any_instance_of(Banzai::Redactor).to receive(:redact).
with([doc]).
and_call_original
redacted = renderer.redact_documents([doc])
expect(redacted.count).to eq(1)
expect(redacted.first[:visible_reference_count]).to eq(0)
expect(redacted.first[:document].to_html).to eq('<p>hello</p>')
end end
end
describe '#context_for' do it 'calls Banzai::Redactor to perform redaction' do
let(:object) { fake_object(note: 'hello') } expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original
let(:renderer) { described_class.new(project, user) }
it 'returns a Hash' do renderer.render([object], :note)
expect(renderer.context_for(object, :note)).to be_an_instance_of(Hash)
end
it 'includes the banzai render context for the object' do
expect(object).to receive(:banzai_render_context).with(:note).and_return(foo: :bar)
context = renderer.context_for(object, :note)
expect(context).to have_key(:foo)
expect(context[:foo]).to eq(:bar)
end
end
describe '#render_attributes' do
it 'renders the attribute of a list of objects' do
objects = [fake_object(note: 'hello', note_html: nil), fake_object(note: 'bye', note_html: nil)]
renderer = described_class.new(project, user)
objects.each do |object|
expect(Banzai).to receive(:render_field).with(object, :note).and_call_original
end
docs = renderer.render_attributes(objects, :note)
expect(docs[0]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
expect(docs[0].to_html).to eq('<p dir="auto">hello</p>')
expect(docs[1]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
expect(docs[1].to_html).to eq('<p dir="auto">bye</p>')
end
it 'returns when no objects to render' do
objects = []
renderer = described_class.new(project, user, pipeline: :note)
expect(renderer.render_attributes(objects, :note)).to eq([])
end end
end
describe '#base_context' do it 'retrieves field content using Banzai.render_field' do
let(:context) do expect(Banzai).to receive(:render_field).with(object, :note).and_call_original
described_class.new(project, user, foo: :bar).base_context
end
it 'returns a Hash' do renderer.render([object], :note)
expect(context).to be_an_instance_of(Hash)
end
it 'includes the custom attributes' do
expect(context[:foo]).to eq(:bar)
end end
it 'includes the current user' do it 'passes context to PostProcessPipeline' do
expect(context[:current_user]).to eq(user) another_user = create(:user)
end another_project = create(:empty_project)
object = Note.new(
note: 'hello',
note_html: 'hello',
author: another_user,
project: another_project
)
expect(Banzai::Pipeline::PostProcessPipeline).to receive(:to_document).with(
anything,
hash_including(
skip_redaction: true,
current_user: user,
project: another_project,
author: another_user,
custom_value: 'value'
)
).and_call_original
it 'includes the current project' do renderer.render([object], :note)
expect(context[:project]).to eq(project)
end end
end end
end end
...@@ -92,16 +92,26 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do ...@@ -92,16 +92,26 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do
end end
describe '#grouped_objects_for_nodes' do describe '#grouped_objects_for_nodes' do
it 'returns a Hash grouping objects per ID' do it 'returns a Hash grouping objects per node' do
nodes = [double(:node)] link = double(:link)
expect(link).to receive(:has_attribute?).
with('data-user').
and_return(true)
expect(link).to receive(:attr).
with('data-user').
and_return(user.id.to_s)
nodes = [link]
expect(subject).to receive(:unique_attribute_values). expect(subject).to receive(:unique_attribute_values).
with(nodes, 'data-user'). with(nodes, 'data-user').
and_return([user.id]) and_return([user.id.to_s])
hash = subject.grouped_objects_for_nodes(nodes, User, 'data-user') hash = subject.grouped_objects_for_nodes(nodes, User, 'data-user')
expect(hash).to eq({ user.id => user }) expect(hash).to eq({ link => user })
end end
it 'returns an empty Hash when the list of nodes is empty' do it 'returns an empty Hash when the list of nodes is empty' do
......
...@@ -67,6 +67,16 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do ...@@ -67,6 +67,16 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
expect(subject.referenced_by([])).to eq([]) expect(subject.referenced_by([])).to eq([])
end end
end end
context 'when issue with given ID does not exist' do
before do
link['data-issue'] = '-1'
end
it 'returns an empty Array' do
expect(subject.referenced_by([link])).to eq([])
end
end
end end
end end
...@@ -75,7 +85,7 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do ...@@ -75,7 +85,7 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
link['data-issue'] = issue.id.to_s link['data-issue'] = issue.id.to_s
nodes = [link] nodes = [link]
expect(subject.issues_for_nodes(nodes)).to eq({ issue.id => issue }) expect(subject.issues_for_nodes(nodes)).to eq({ link => issue })
end end
end end
end end
...@@ -180,6 +180,15 @@ describe Banzai::ReferenceParser::UserParser, lib: true do ...@@ -180,6 +180,15 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
expect(subject.nodes_user_can_reference(user, [link])).to eq([]) expect(subject.nodes_user_can_reference(user, [link])).to eq([])
end end
it 'returns the nodes if the project attribute value equals the current project ID' do
other_user = create(:user)
link['data-project'] = project.id.to_s
link['data-author'] = other_user.id.to_s
expect(subject.nodes_user_can_reference(user, [link])).to eq([link])
end
end end
context 'when the link does not have a data-author attribute' do context 'when the link does not have a data-author attribute' do
......
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