diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/models/note.rb | 14 | ||||
| -rw-r--r-- | lib/banzai/object_renderer.rb | 10 | ||||
| -rw-r--r-- | lib/banzai/redactor.rb | 37 | ||||
| -rw-r--r-- | spec/features/notes_on_merge_requests_spec.rb | 22 | ||||
| -rw-r--r-- | spec/lib/banzai/object_renderer_spec.rb | 8 | ||||
| -rw-r--r-- | spec/lib/banzai/redactor_spec.rb | 24 | ||||
| -rw-r--r-- | spec/models/note_spec.rb | 14 |
8 files changed, 107 insertions, 23 deletions
diff --git a/CHANGELOG b/CHANGELOG index 79ee72e330c..41fd3c8d22b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.10.0 (unreleased) - Fix commit builds API, return all builds for all pipelines for given commit. !4849 - Replace Haml with Hamlit to make view rendering faster. !3666 - Refactor repository paths handling to allow multiple git mount points + - Optimize system note visibility checking by memoizing the visible reference count !5070 - Add Application Setting to configure default Repository Path for new projects - Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Align flash messages with left side of page content !4959 (winniehell) diff --git a/app/models/note.rb b/app/models/note.rb index ffffd0c0838..8dca2ef09a8 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -10,6 +10,10 @@ class Note < ActiveRecord::Base # Banzai::ObjectRenderer. attr_accessor :note_html + # An Array containing the number of visible references as generated by + # Banzai::ObjectRenderer + attr_accessor :user_visible_reference_count + default_value_for :system, false attr_mentionable :note, pipeline: :note @@ -193,7 +197,15 @@ class Note < ActiveRecord::Base end def cross_reference_not_visible_for?(user) - cross_reference? && referenced_mentionables(user).empty? + cross_reference? && !has_referenced_mentionables?(user) + end + + def has_referenced_mentionables?(user) + if user_visible_reference_count.present? + user_visible_reference_count > 0 + else + referenced_mentionables(user).any? + end end def award_emoji? diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index f0e4f28bf12..dc83b87a6c1 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -31,10 +31,10 @@ module Banzai redacted = redact_documents(documents) objects.each_with_index do |object, index| - object.__send__("#{attribute}_html=", redacted.fetch(index)) + redacted_data = redacted[index] + object.__send__("#{attribute}_html=", redacted_data[:document].to_html.html_safe) + object.user_visible_reference_count = redacted_data[:visible_reference_count] end - - objects end # Renders the attribute of every given object. @@ -50,9 +50,7 @@ module Banzai def redact_documents(documents) redactor = Redactor.new(project, user) - redactor.redact(documents).map do |document| - document.to_html.html_safe - end + redactor.redact(documents) end # Returns a Banzai context for the given object and attribute. diff --git a/lib/banzai/redactor.rb b/lib/banzai/redactor.rb index ffd267d5e9a..0df3a72d1c4 100644 --- a/lib/banzai/redactor.rb +++ b/lib/banzai/redactor.rb @@ -19,29 +19,36 @@ module Banzai # # Returns the documents passed as the first argument. def redact(documents) - nodes = documents.flat_map do |document| - Querying.css(document, 'a.gfm[data-reference-type]') - end - - redact_nodes(nodes) + all_document_nodes = document_nodes(documents) - documents + redact_document_nodes(all_document_nodes) end - # Redacts the given nodes + # Redacts the given node documents # - # nodes - An Array of HTML nodes to redact. - def redact_nodes(nodes) - visible = nodes_visible_to_user(nodes) + # data - An Array of a Hashes mapping an HTML document to nodes to redact. + def redact_document_nodes(all_document_nodes) + all_nodes = all_document_nodes.map { |x| x[:nodes] }.flatten + visible = nodes_visible_to_user(all_nodes) + metadata = [] - nodes.each do |node| - unless visible.include?(node) + all_document_nodes.each do |entry| + nodes_for_document = entry[:nodes] + doc_data = { document: entry[:document], visible_reference_count: nodes_for_document.count } + metadata << doc_data + + nodes_for_document.each do |node| + next if visible.include?(node) + + doc_data[:visible_reference_count] -= 1 # The reference should be replaced by the original text, # which is not always the same as the rendered text. text = node.attr('data-original') || node.text node.replace(text) end end + + metadata end # Returns the nodes visible to the current user. @@ -65,5 +72,11 @@ module Banzai visible end + + def document_nodes(documents) + documents.map do |document| + { document: document, nodes: Querying.css(document, 'a.gfm[data-reference-type]') } + end + end end end diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index 5174168713c..64c6d9416f6 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -135,6 +135,28 @@ describe 'Comments', feature: true do end end + describe 'Handles cross-project system notes', js: true, feature: true do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:project2) { create(:project, :private) } + let(:issue) { create(:issue, project: project2) } + let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'markdown') } + let!(:note) { create(:note_on_merge_request, :system, noteable: merge_request, project: project, note: "mentioned in #{issue.to_reference(project)}") } + + it 'shows the system note' do + login_as :admin + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + + expect(page).to have_css('.system-note') + end + + it 'hides redacted system note' do + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + + expect(page).not_to have_css('.system-note') + end + end + describe 'On a merge request diff', js: true, feature: true do let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 44256b32bdc..d99175967af 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -17,6 +17,7 @@ describe Banzai::ObjectRenderer do and_call_original expect(object).to receive(:note_html=).with('<p>hello</p>') + expect(object).to receive(:user_visible_reference_count=).with(0) renderer.render([object], :note) end @@ -25,6 +26,7 @@ describe Banzai::ObjectRenderer do describe '#render_objects' do it 'renders an Array of objects' do object = double(:object, note: 'hello') + renderer = described_class.new(project, user) expect(renderer).to receive(:render_attribute).with(object, :note). @@ -38,7 +40,7 @@ describe Banzai::ObjectRenderer do end describe '#redact_documents' do - it 'redacts a set of documents and returns them as an Array of Strings' 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) @@ -48,7 +50,9 @@ describe Banzai::ObjectRenderer do redacted = renderer.redact_documents([doc]) - expect(redacted).to eq(['<p>hello</p>']) + 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 diff --git a/spec/lib/banzai/redactor_spec.rb b/spec/lib/banzai/redactor_spec.rb index 488f465bcda..254657a881d 100644 --- a/spec/lib/banzai/redactor_spec.rb +++ b/spec/lib/banzai/redactor_spec.rb @@ -15,11 +15,31 @@ describe Banzai::Redactor do expect(redactor).to receive(:nodes_visible_to_user).and_return([]) - expect(redactor.redact([doc1, doc2])).to eq([doc1, doc2]) + redacted_data = redactor.redact([doc1, doc2]) + expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2]) + expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([0, 0]) expect(doc1.to_html).to eq('foo') expect(doc2.to_html).to eq('bar') end + + it 'does not redact an Array of documents' do + doc1_html = '<a class="gfm" data-reference-type="issue">foo</a>' + doc1 = Nokogiri::HTML.fragment(doc1_html) + + doc2_html = '<a class="gfm" data-reference-type="issue">bar</a>' + doc2 = Nokogiri::HTML.fragment(doc2_html) + + nodes = redactor.document_nodes([doc1, doc2]).map { |x| x[:nodes] } + expect(redactor).to receive(:nodes_visible_to_user).and_return(nodes.flatten) + + redacted_data = redactor.redact([doc1, doc2]) + + expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2]) + expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([1, 1]) + expect(doc1.to_html).to eq(doc1_html) + expect(doc2.to_html).to eq(doc2_html) + end end describe '#redact_nodes' do @@ -31,7 +51,7 @@ describe Banzai::Redactor do with([node]). and_return(Set.new) - redactor.redact_nodes([node]) + redactor.redact_document_nodes([{ document: doc, nodes: [node] }]) expect(doc.to_html).to eq('foo') end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 6549791f675..7d0697dab42 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -226,6 +226,20 @@ describe Note, models: true do it "returns false" do expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy end + + it "returns false if user visible reference count set" do + note.user_visible_reference_count = 1 + + expect(note).not_to receive(:reference_mentionables) + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy + end + + it "returns true if ref count is 0" do + note.user_visible_reference_count = 0 + + expect(note).not_to receive(:reference_mentionables) + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + end end describe 'clear_blank_line_code!' do |