From af3727b34a3e61668ffca8dc4db85e3c57ff2cc8 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 3 Jul 2016 22:31:43 -0700
Subject: [PATCH] Optimize system note visibility checking by hiding notes that
have been fully redacted and contain cross-project references.
The previous implementation relied on Note#cross_reference_not_visible_for?,
which essentially tries to render all the Markdown references in a system note
and only displays the note if the user can see the referring project. But this
duplicated the work that Banzai::NotesRenderer was doing already. Instead, for
each note we render, we memoize the number of visible user references and
use it later if it is available.
Improves #19273
---
CHANGELOG | 1 +
app/models/note.rb | 14 ++++++-
lib/banzai/object_renderer.rb | 10 ++---
lib/banzai/redactor.rb | 37 +++++++++++++------
spec/features/notes_on_merge_requests_spec.rb | 22 +++++++++++
spec/lib/banzai/object_renderer_spec.rb | 8 +++-
spec/lib/banzai/redactor_spec.rb | 24 +++++++++++-
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
--
GitLab