Rename and combine some methods that run cross reference checks

Combine cross_reference_not_visible_for? and visible_for? methods
into a single one and adjust specs accordingly. Given that we moved
read access to policy check we do not need all the methods either.

app/models/discussion.rb

@@ -19,7 +19,7 @@ class Discussion
             :noteable_ability_name,
             :to_ability_name,
             :editable?,
-            :visible_for?,
+            :cross_reference_visible_for?,
             :resource_parent,
 
             to: :first_note

app/models/note.rb

@@ -502,14 +502,8 @@ class Note < ApplicationRecord
     noteable.user_mentions.where(note: self)
   end
 
-  def cross_reference_not_visible_for?(user)
-    cross_reference? && !all_referenced_mentionables_allowed?(user)
-  end
-
-  # Using this method for checking `read` access on a note may result in incorrect permission granting
-  # check `spec/models/note_spec.rb` and `ee/spec/models/note_spec.rb` for corresponding specs
-  def visible_for?(user)
-    !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user)
+  def cross_reference_visible_for?(user)
+    (!cross_reference? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user)
   end
 
   private

app/policies/note_policy.rb

@@ -11,7 +11,7 @@ class NotePolicy < BasePolicy
 
   condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
 
-  condition(:is_visible) { @subject.visible_for?(@user) }
+  condition(:is_visible) { @subject.cross_reference_visible_for?(@user) }
 
   rule { ~editable }.prevent :admin_note
 

ee/app/models/ee/note.rb

@@ -70,8 +70,8 @@ module EE
       noteable&.after_note_destroyed(self)
     end
 
-    override :visible_for?
-    def visible_for?(user)
+    override :cross_reference_visible_for?
+    def cross_reference_visible_for?(user)
       return false unless super
 
       return true unless system_note_for_epic? && created_before_noteable?

ee/spec/controllers/groups/epics/notes_controller_spec.rb

@@ -39,7 +39,7 @@ describe Groups::Epics::NotesController do
 
     context 'with cross-reference system note that is not visible to the current user', :request_store do
       it "does not return any note" do
-        expect_any_instance_of(Note).to receive(:cross_reference_not_visible_for?).and_return(true)
+        expect_any_instance_of(Note).to receive(:readable_by?).and_return(false)
 
         get :index, params: request_params
 

ee/spec/models/note_spec.rb

@@ -11,7 +11,7 @@ describe Note do
     let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
   end
 
-  describe '#visible_for?' do
+  describe '#readable_by?' do
     let(:owner) { create(:group_member, :owner, group: group, user: create(:user)).user }
     let(:guest) { create(:group_member, :guest, group: group, user: create(:user)).user }
     let(:reporter) { create(:group_member, :reporter, group: group, user: create(:user)).user }
@@ -40,12 +40,12 @@ describe Note do
         end
 
         it 'returns visible but not readable for a non-member user' do
-          expect(note.visible_for?(non_member)).to be_truthy
+          expect(note.cross_reference_visible_for?(non_member)).to be_truthy
           expect(note.readable_by?(non_member)).to be_falsy
         end
 
         it 'returns visible but not readable for a nil user' do
-          expect(note.visible_for?(nil)).to be_truthy
+          expect(note.cross_reference_visible_for?(nil)).to be_truthy
           expect(note.readable_by?(nil)).to be_falsy
         end
       end

lib/api/discussions.rb

@@ -230,7 +230,7 @@ module API
           .fresh
 
         # Without RendersActions#prepare_notes_for_rendering,
-        # Note#cross_reference_not_visible_for? will attempt to render
+        # Note#cross_reference_visible_for? will attempt to render
         # Markdown references mentioned in the note to see whether they
         # should be redacted. For notes that reference a commit, this
         # would also incur a Gitaly call to verify the commit exists.

spec/controllers/snippets/notes_controller_spec.rb

@@ -108,7 +108,7 @@ describe Snippets::NotesController do
 
         sign_in(user)
 
-        expect_any_instance_of(Note).to receive(:cross_reference_not_visible_for?).and_return(true)
+        expect_any_instance_of(Note).to receive(:readable_by?).and_return(false)
       end
 
       it "does not return any note" do

spec/models/note_spec.rb

@@ -285,7 +285,7 @@ describe Note do
     end
   end
 
-  describe "#visible_for?" do
+  describe "#cross_reference_visible_for?" do
     using RSpec::Parameterized::TableSyntax
 
     let(:project) { create(:project, :public) }
@@ -297,26 +297,6 @@ describe Note do
 
     let(:note) { create(:note, project: project) }
 
-    where(:cross_reference_visible, :system_note_viewable, :result) do
-      true  | true  | false
-      false | true  | true
-      false | false | false
-    end
-
-    with_them do
-      it "returns expected result" do
-        expect(note).to receive(:cross_reference_not_visible_for?).twice.and_return(cross_reference_visible)
-
-        unless cross_reference_visible
-          expect(note).to receive(:system_note_viewable_by?).twice
-                            .with(user).and_return(system_note_viewable)
-        end
-
-        expect(note.visible_for?(user)).to eq result
-        expect(note.readable_by?(user)).to eq result
-      end
-    end
-
     context 'when project is public' do
       it_behaves_like 'users with note access' do
         let(:users) { [reporter, maintainer, guest, non_member, nil] }
@@ -331,12 +311,12 @@ describe Note do
       end
 
       it 'returns visible but not readable for non-member user' do
-        expect(note.visible_for?(non_member)).to be_truthy
+        expect(note.cross_reference_visible_for?(non_member)).to be_truthy
         expect(note.readable_by?(non_member)).to be_falsy
       end
 
       it 'returns visible but not readable for a nil user' do
-        expect(note.visible_for?(nil)).to be_truthy
+        expect(note.cross_reference_visible_for?(nil)).to be_truthy
         expect(note.readable_by?(nil)).to be_falsy
       end
     end
@@ -380,7 +360,7 @@ describe Note do
     end
   end
 
-  describe "cross_reference_not_visible_for?" do
+  describe "cross_reference_visible_for?" do
     let_it_be(:private_user)    { create(:user) }
     let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
     let_it_be(:private_issue)   { create(:issue, project: private_project) }
@@ -390,11 +370,11 @@ describe Note do
 
     shared_examples "checks references" do
       it "returns true" do
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy
       end
 
       it "returns false" do
-        expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
+        expect(note.cross_reference_visible_for?(private_user)).to be_truthy
       end
 
       it "returns false if user visible reference count set" do
@@ -402,14 +382,14 @@ describe Note do
         note.total_reference_count = 1
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
+        expect(note.cross_reference_visible_for?(ext_issue.author)).to be_truthy
       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
+        expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy
       end
     end
 
@@ -454,7 +434,7 @@ describe Note do
         note.total_reference_count = 2
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy
       end
     end
   end

spec/support/shared_examples/models/note_access_check_shared_examples.rb

@@ -3,7 +3,7 @@
 shared_examples 'users with note access' do
   it 'returns true' do
     users.each do |user|
-      expect(note.visible_for?(user)).to be_truthy
+      expect(note.cross_reference_visible_for?(user)).to be_truthy
       expect(note.readable_by?(user)).to be_truthy
     end
   end
@@ -12,7 +12,7 @@ end
 shared_examples 'users without note access' do
   it 'returns false' do
     users.each do |user|
-      expect(note.visible_for?(user)).to be_falsy
+      expect(note.cross_reference_visible_for?(user)).to be_falsy
       expect(note.readable_by?(user)).to be_falsy
     end
   end