diff options
18 files changed, 354 insertions, 153 deletions
diff --git a/changelogs/unreleased/adam-finish-5993-closed-issuable.yml b/changelogs/unreleased/adam-finish-5993-closed-issuable.yml new file mode 100644 index 00000000000..b324566313f --- /dev/null +++ b/changelogs/unreleased/adam-finish-5993-closed-issuable.yml @@ -0,0 +1,4 @@ +--- +title: Add indication for closed or merged issuables in GFM +merge_request: 9462 +author: Adam Buckland diff --git a/lib/banzai/filter/issuable_state_filter.rb b/lib/banzai/filter/issuable_state_filter.rb new file mode 100644 index 00000000000..6b78aa795b4 --- /dev/null +++ b/lib/banzai/filter/issuable_state_filter.rb @@ -0,0 +1,35 @@ +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 diff --git a/lib/banzai/filter/redactor_filter.rb b/lib/banzai/filter/redactor_filter.rb index c59a80dd1c7..9f9882b3b40 100644 --- a/lib/banzai/filter/redactor_filter.rb +++ b/lib/banzai/filter/redactor_filter.rb @@ -7,7 +7,7 @@ module Banzai # class RedactorFilter < HTML::Pipeline::Filter def call - Redactor.new(project, current_user).redact([doc]) + Redactor.new(project, current_user).redact([doc]) unless context[:skip_redaction] doc end diff --git a/lib/banzai/issuable_extractor.rb b/lib/banzai/issuable_extractor.rb new file mode 100644 index 00000000000..c5ce360e172 --- /dev/null +++ b/lib/banzai/issuable_extractor.rb @@ -0,0 +1,36 @@ +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 diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index 9f8eb0931b8..002a3341ccd 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -31,7 +31,8 @@ module Banzai # # Returns the same input objects. 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) objects.each_with_index do |object, index| @@ -41,9 +42,24 @@ module Banzai end end - # Renders the attribute of every given object. - def render_objects(objects, attribute) - render_attributes(objects, attribute) + private + + 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 # Redacts the list of documents. @@ -57,25 +73,15 @@ module Banzai # Returns a Banzai context for the given object and attribute. def context_for(object, attribute) - context = base_context.dup - 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 + base_context.merge(object.banzai_render_context(attribute)) end 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 diff --git a/lib/banzai/pipeline/post_process_pipeline.rb b/lib/banzai/pipeline/post_process_pipeline.rb index ecff094b1e5..131ac3b0eec 100644 --- a/lib/banzai/pipeline/post_process_pipeline.rb +++ b/lib/banzai/pipeline/post_process_pipeline.rb @@ -4,6 +4,7 @@ module Banzai def self.filters FilterArray[ Filter::RelativeLinkFilter, + Filter::IssuableStateFilter, Filter::RedactorFilter ] end diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb index 52fdb9a2140..dabf71d6aeb 100644 --- a/lib/banzai/reference_parser/base_parser.rb +++ b/lib/banzai/reference_parser/base_parser.rb @@ -62,8 +62,7 @@ module Banzai nodes.select do |node| if node.has_attribute?(project_attr) - node_id = node.attr(project_attr).to_i - can_read_reference?(user, projects[node_id]) + can_read_reference?(user, projects[node]) else true end @@ -112,12 +111,12 @@ module Banzai per_project end - # Returns a Hash containing objects for an attribute grouped per their - # IDs. + # Returns a Hash containing objects for an attribute grouped per the + # nodes that reference them. # # The returned Hash uses the following format: # - # { id value => row } + # { node => row } # # nodes - An Array of HTML nodes to process. # @@ -132,9 +131,14 @@ module Banzai return {} if nodes.empty? 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 # Returns an Array containing all unique values of an attribute of the @@ -201,7 +205,7 @@ module Banzai # # The returned Hash uses the following format: # - # { project ID => project } + # { node => project } # def projects_for_nodes(nodes) @projects_for_nodes ||= diff --git a/lib/banzai/reference_parser/issue_parser.rb b/lib/banzai/reference_parser/issue_parser.rb index 6c20dec5734..e02b360924a 100644 --- a/lib/banzai/reference_parser/issue_parser.rb +++ b/lib/banzai/reference_parser/issue_parser.rb @@ -13,14 +13,14 @@ module Banzai issues_readable_by_user(issues.values, user).to_set nodes.select do |node| - readable_issues.include?(issue_for_node(issues, node)) + readable_issues.include?(issues[node]) end end def referenced_by(nodes) issues = issues_for_nodes(nodes) - nodes.map { |node| issue_for_node(issues, node) }.uniq + nodes.map { |node| issues[node] }.compact.uniq end def issues_for_nodes(nodes) @@ -44,12 +44,6 @@ module Banzai self.class.data_attribute ) end - - private - - def issue_for_node(issues, node) - issues[node.attr(self.class.data_attribute).to_i] - end end end end diff --git a/lib/banzai/reference_parser/merge_request_parser.rb b/lib/banzai/reference_parser/merge_request_parser.rb index 40451947e6c..7d7dcce017e 100644 --- a/lib/banzai/reference_parser/merge_request_parser.rb +++ b/lib/banzai/reference_parser/merge_request_parser.rb @@ -3,6 +3,14 @@ module Banzai class MergeRequestParser < BaseParser 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 MergeRequest.includes(:author, :assignee, :target_project) end diff --git a/lib/banzai/reference_parser/user_parser.rb b/lib/banzai/reference_parser/user_parser.rb index 7adaffa19c1..09b66cbd8fb 100644 --- a/lib/banzai/reference_parser/user_parser.rb +++ b/lib/banzai/reference_parser/user_parser.rb @@ -49,7 +49,7 @@ module Banzai # Check if project belongs to a group which # user can read. 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) end @@ -74,8 +74,8 @@ module Banzai if project && project_id && project.id == project_id.to_i true elsif project_id && user_id - project = projects[project_id.to_i] - user = users[user_id.to_i] + project = projects[node] + user = users[node] project && user ? project.team.member?(user) : false else diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index e36fe326e1c..361f9dac191 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -44,6 +44,10 @@ FactoryGirl.define do state :reopened end + trait :locked do + state :locked + end + trait :simple do source_branch "feature" target_branch "master" diff --git a/spec/lib/banzai/filter/issuable_state_filter_spec.rb b/spec/lib/banzai/filter/issuable_state_filter_spec.rb new file mode 100644 index 00000000000..603b79a323c --- /dev/null +++ b/spec/lib/banzai/filter/issuable_state_filter_spec.rb @@ -0,0 +1,95 @@ +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 diff --git a/spec/lib/banzai/filter/redactor_filter_spec.rb b/spec/lib/banzai/filter/redactor_filter_spec.rb index 0140a91c7ba..8a6fe1ad6a3 100644 --- a/spec/lib/banzai/filter/redactor_filter_spec.rb +++ b/spec/lib/banzai/filter/redactor_filter_spec.rb @@ -15,6 +15,16 @@ describe Banzai::Filter::RedactorFilter, lib: true do link_to('text', '', class: 'gfm', data: data) 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 let(:parser_class) do Class.new(Banzai::ReferenceParser::BaseParser) do diff --git a/spec/lib/banzai/issuable_extractor_spec.rb b/spec/lib/banzai/issuable_extractor_spec.rb new file mode 100644 index 00000000000..e5d332efb08 --- /dev/null +++ b/spec/lib/banzai/issuable_extractor_spec.rb @@ -0,0 +1,52 @@ +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 diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 6bcda87c999..4817fcd031a 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -3,128 +3,51 @@ require 'spec_helper' describe Banzai::ObjectRenderer do let(:project) { create(:empty_project) } let(:user) { project.owner } - - def fake_object(attrs = {}) - 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 + let(:renderer) { described_class.new(project, user, custom_value: 'value') } + let(:object) { Note.new(note: 'hello', note_html: '<p>hello</p>') } describe '#render' 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) - 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). - and_call_original - - 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>') + expect(object.redacted_note_html).to eq '<p>hello</p>' + expect(object.user_visible_reference_count).to eq 0 end - end - describe '#context_for' do - let(:object) { fake_object(note: 'hello') } - let(:renderer) { described_class.new(project, user) } + it 'calls Banzai::Redactor to perform redaction' do + expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original - it 'returns a Hash' do - 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([]) + renderer.render([object], :note) end - end - describe '#base_context' do - let(:context) do - described_class.new(project, user, foo: :bar).base_context - end + it 'retrieves field content using Banzai.render_field' do + expect(Banzai).to receive(:render_field).with(object, :note).and_call_original - it 'returns a Hash' do - expect(context).to be_an_instance_of(Hash) - end - - it 'includes the custom attributes' do - expect(context[:foo]).to eq(:bar) + renderer.render([object], :note) end - it 'includes the current user' do - expect(context[:current_user]).to eq(user) - end + it 'passes context to PostProcessPipeline' do + another_user = create(:user) + 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 - expect(context[:project]).to eq(project) + renderer.render([object], :note) end end end diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index aa127f0179d..a3141894c74 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -92,16 +92,26 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do end describe '#grouped_objects_for_nodes' do - it 'returns a Hash grouping objects per ID' do - nodes = [double(:node)] + it 'returns a Hash grouping objects per node' do + 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). with(nodes, 'data-user'). - and_return([user.id]) + and_return([user.id.to_s]) hash = subject.grouped_objects_for_nodes(nodes, User, 'data-user') - expect(hash).to eq({ user.id => user }) + expect(hash).to eq({ link => user }) end it 'returns an empty Hash when the list of nodes is empty' do diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb index 6873b7b85f9..7031c47231c 100644 --- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb @@ -67,6 +67,16 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do expect(subject.referenced_by([])).to eq([]) 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 @@ -75,7 +85,7 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do link['data-issue'] = issue.id.to_s 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 diff --git a/spec/lib/banzai/reference_parser/user_parser_spec.rb b/spec/lib/banzai/reference_parser/user_parser_spec.rb index 31ca9d27b0b..4ec998efe53 100644 --- a/spec/lib/banzai/reference_parser/user_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/user_parser_spec.rb @@ -180,6 +180,15 @@ describe Banzai::ReferenceParser::UserParser, lib: true do expect(subject.nodes_user_can_reference(user, [link])).to eq([]) 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 context 'when the link does not have a data-author attribute' do |