Commit af3727b3 authored by Stan Hu's avatar Stan Hu

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
parent 734e44ee
...@@ -4,6 +4,7 @@ v 8.10.0 (unreleased) ...@@ -4,6 +4,7 @@ v 8.10.0 (unreleased)
- Fix commit builds API, return all builds for all pipelines for given commit. !4849 - Fix commit builds API, return all builds for all pipelines for given commit. !4849
- Replace Haml with Hamlit to make view rendering faster. !3666 - Replace Haml with Hamlit to make view rendering faster. !3666
- Refactor repository paths handling to allow multiple git mount points - 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 - Add Application Setting to configure default Repository Path for new projects
- Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
- Align flash messages with left side of page content !4959 (winniehell) - Align flash messages with left side of page content !4959 (winniehell)
......
...@@ -10,6 +10,10 @@ class Note < ActiveRecord::Base ...@@ -10,6 +10,10 @@ class Note < ActiveRecord::Base
# Banzai::ObjectRenderer. # Banzai::ObjectRenderer.
attr_accessor :note_html 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 default_value_for :system, false
attr_mentionable :note, pipeline: :note attr_mentionable :note, pipeline: :note
...@@ -193,7 +197,15 @@ class Note < ActiveRecord::Base ...@@ -193,7 +197,15 @@ class Note < ActiveRecord::Base
end end
def cross_reference_not_visible_for?(user) 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 end
def award_emoji? def award_emoji?
......
...@@ -31,10 +31,10 @@ module Banzai ...@@ -31,10 +31,10 @@ module Banzai
redacted = redact_documents(documents) redacted = redact_documents(documents)
objects.each_with_index do |object, index| 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 end
objects
end end
# Renders the attribute of every given object. # Renders the attribute of every given object.
...@@ -50,9 +50,7 @@ module Banzai ...@@ -50,9 +50,7 @@ module Banzai
def redact_documents(documents) def redact_documents(documents)
redactor = Redactor.new(project, user) redactor = Redactor.new(project, user)
redactor.redact(documents).map do |document| redactor.redact(documents)
document.to_html.html_safe
end
end end
# Returns a Banzai context for the given object and attribute. # Returns a Banzai context for the given object and attribute.
......
...@@ -19,29 +19,36 @@ module Banzai ...@@ -19,29 +19,36 @@ module Banzai
# #
# Returns the documents passed as the first argument. # Returns the documents passed as the first argument.
def redact(documents) def redact(documents)
nodes = documents.flat_map do |document| all_document_nodes = document_nodes(documents)
Querying.css(document, 'a.gfm[data-reference-type]')
end
redact_nodes(nodes)
documents redact_document_nodes(all_document_nodes)
end end
# Redacts the given nodes # Redacts the given node documents
# #
# nodes - An Array of HTML nodes to redact. # data - An Array of a Hashes mapping an HTML document to nodes to redact.
def redact_nodes(nodes) def redact_document_nodes(all_document_nodes)
visible = nodes_visible_to_user(nodes) all_nodes = all_document_nodes.map { |x| x[:nodes] }.flatten
visible = nodes_visible_to_user(all_nodes)
metadata = []
nodes.each do |node| all_document_nodes.each do |entry|
unless visible.include?(node) 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, # The reference should be replaced by the original text,
# which is not always the same as the rendered text. # which is not always the same as the rendered text.
text = node.attr('data-original') || node.text text = node.attr('data-original') || node.text
node.replace(text) node.replace(text)
end end
end end
metadata
end end
# Returns the nodes visible to the current user. # Returns the nodes visible to the current user.
...@@ -65,5 +72,11 @@ module Banzai ...@@ -65,5 +72,11 @@ module Banzai
visible visible
end end
def document_nodes(documents)
documents.map do |document|
{ document: document, nodes: Querying.css(document, 'a.gfm[data-reference-type]') }
end
end
end end
end end
...@@ -135,6 +135,28 @@ describe 'Comments', feature: true do ...@@ -135,6 +135,28 @@ describe 'Comments', feature: true do
end end
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 describe 'On a merge request diff', js: true, feature: true do
let(:merge_request) { create(:merge_request) } let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project } let(:project) { merge_request.source_project }
......
...@@ -17,6 +17,7 @@ describe Banzai::ObjectRenderer do ...@@ -17,6 +17,7 @@ describe Banzai::ObjectRenderer do
and_call_original and_call_original
expect(object).to receive(:note_html=).with('<p>hello</p>') expect(object).to receive(:note_html=).with('<p>hello</p>')
expect(object).to receive(:user_visible_reference_count=).with(0)
renderer.render([object], :note) renderer.render([object], :note)
end end
...@@ -25,6 +26,7 @@ describe Banzai::ObjectRenderer do ...@@ -25,6 +26,7 @@ describe Banzai::ObjectRenderer do
describe '#render_objects' do describe '#render_objects' do
it 'renders an Array of objects' do it 'renders an Array of objects' do
object = double(:object, note: 'hello') object = double(:object, note: 'hello')
renderer = described_class.new(project, user) renderer = described_class.new(project, user)
expect(renderer).to receive(:render_attribute).with(object, :note). expect(renderer).to receive(:render_attribute).with(object, :note).
...@@ -38,7 +40,7 @@ describe Banzai::ObjectRenderer do ...@@ -38,7 +40,7 @@ describe Banzai::ObjectRenderer do
end end
describe '#redact_documents' do 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>') doc = Nokogiri::HTML.fragment('<p>hello</p>')
renderer = described_class.new(project, user) renderer = described_class.new(project, user)
...@@ -48,7 +50,9 @@ describe Banzai::ObjectRenderer do ...@@ -48,7 +50,9 @@ describe Banzai::ObjectRenderer do
redacted = renderer.redact_documents([doc]) 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
end end
......
...@@ -15,11 +15,31 @@ describe Banzai::Redactor do ...@@ -15,11 +15,31 @@ describe Banzai::Redactor do
expect(redactor).to receive(:nodes_visible_to_user).and_return([]) 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(doc1.to_html).to eq('foo')
expect(doc2.to_html).to eq('bar') expect(doc2.to_html).to eq('bar')
end 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 end
describe '#redact_nodes' do describe '#redact_nodes' do
...@@ -31,7 +51,7 @@ describe Banzai::Redactor do ...@@ -31,7 +51,7 @@ describe Banzai::Redactor do
with([node]). with([node]).
and_return(Set.new) and_return(Set.new)
redactor.redact_nodes([node]) redactor.redact_document_nodes([{ document: doc, nodes: [node] }])
expect(doc.to_html).to eq('foo') expect(doc.to_html).to eq('foo')
end end
......
...@@ -226,6 +226,20 @@ describe Note, models: true do ...@@ -226,6 +226,20 @@ describe Note, models: true do
it "returns false" do it "returns false" do
expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
end 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 end
describe 'clear_blank_line_code!' do describe 'clear_blank_line_code!' 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