Merge branch 'optimize-cross-ref-system-notes-check' into 'master'

Optimize cross ref system notes check

## What does this MR do?

This MR optimizes system note visibility checking by memoizing the visible reference count, reducing the overhead of calling `Note#cross_reference_not_visible_for?`.

## Are there points in the code the reviewer needs to double check?

Note that since a cross reference message contains, "Mentioned in XYZ", we EXPECT that there is at least one reference. That's why using the ref count > 0 works. 

## Why was this MR needed?

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. This shaves about 0.8 s
from the load time from https://gitlab.com/gitlab-com/operations/issues/42.

## What are the relevant issue numbers?

#19273

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5070

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
   - Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell)
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)

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?

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.

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

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 }

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
 

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

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