Commit 35ab5377 authored by Alexandru Croitor's avatar Alexandru Croitor

Rename methods for better clarity on purpose they serve

* Renamed Note#cross_reference to Note#system_note_with_references? to
reflect that it checks for system notes that may have references
regardless if references are cross project, group or not

* Renamed Note#cross_reference_visible_for?(user) to
Note#system_note_with_references_visible_for?(user) for clarity.
It should be easier to understand that the methods checks if system
notes have referenes and those references are  visible/readable
by prvided user.

* Renamed Epic#cross_reference?(from) to Epic#cross_referenced?(from)
as it does not check if `from` is a cross reference, but rather
if `from` is different from epic's group, which means epic is being
cross referenced.

* Renamed IssuableStateFilter#cross_reference?(issuable) to
IssuableStateFilter#cross_referenced?(issuable) for same reasons as
for Epic
parent 368a0240
...@@ -19,7 +19,7 @@ class Discussion ...@@ -19,7 +19,7 @@ class Discussion
:noteable_ability_name, :noteable_ability_name,
:to_ability_name, :to_ability_name,
:editable?, :editable?,
:cross_reference_visible_for?, :system_note_with_references_visible_for?,
:resource_parent, :resource_parent,
to: :first_note to: :first_note
......
...@@ -223,7 +223,7 @@ class Note < ApplicationRecord ...@@ -223,7 +223,7 @@ class Note < ApplicationRecord
end end
# rubocop: disable CodeReuse/ServiceClass # rubocop: disable CodeReuse/ServiceClass
def cross_reference? def system_note_with_references?
return unless system? return unless system?
if force_cross_reference_regex_check? if force_cross_reference_regex_check?
...@@ -339,9 +339,9 @@ class Note < ApplicationRecord ...@@ -339,9 +339,9 @@ class Note < ApplicationRecord
super super
end end
# This method is to be used for checking read permissions on a note instead of `visible_for?` # This method is to be used for checking read permissions on a note instead of `system_note_with_references_visible_for?`
def readable_by?(user) def readable_by?(user)
# note_policy accounts for #visible_for?(user) check when granting read access # note_policy accounts for #system_note_with_references_visible_for?(user) check when granting read access
Ability.allowed?(user, :read_note, self) Ability.allowed?(user, :read_note, self)
end end
...@@ -502,8 +502,8 @@ class Note < ApplicationRecord ...@@ -502,8 +502,8 @@ class Note < ApplicationRecord
noteable.user_mentions.where(note: self) noteable.user_mentions.where(note: self)
end end
def cross_reference_visible_for?(user) def system_note_with_references_visible_for?(user)
(!cross_reference? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user) (!system_note_with_references? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user)
end end
private private
......
...@@ -11,7 +11,7 @@ class NotePolicy < BasePolicy ...@@ -11,7 +11,7 @@ class NotePolicy < BasePolicy
condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") } condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
condition(:is_visible) { @subject.cross_reference_visible_for?(@user) } condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) }
rule { ~editable }.prevent :admin_note rule { ~editable }.prevent :admin_note
......
...@@ -283,7 +283,7 @@ class NotificationService ...@@ -283,7 +283,7 @@ class NotificationService
return true unless note.noteable_type.present? return true unless note.noteable_type.present?
# ignore gitlab service messages # ignore gitlab service messages
return true if note.cross_reference? && note.system? return true if note.system_note_with_references?
send_new_note_notifications(note) send_new_note_notifications(note)
end end
......
- return unless note.author - return unless note.author
- return if !note.readable_by?(current_user) - return unless note.readable_by?(current_user)
- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false) - show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
- note_editable = can?(current_user, :admin_note, note) - note_editable = can?(current_user, :admin_note, note)
......
...@@ -275,12 +275,12 @@ module EE ...@@ -275,12 +275,12 @@ module EE
def to_reference(from = nil, full: false) def to_reference(from = nil, full: false)
reference = "#{self.class.reference_prefix}#{iid}" reference = "#{self.class.reference_prefix}#{iid}"
return reference unless (cross_reference?(from) && !group.projects.include?(from)) || full return reference unless (cross_referenced?(from) && !group.projects.include?(from)) || full
"#{group.full_path}#{reference}" "#{group.full_path}#{reference}"
end end
def cross_reference?(from) def cross_referenced?(from)
from && from != group from && from != group
end end
......
...@@ -70,8 +70,8 @@ module EE ...@@ -70,8 +70,8 @@ module EE
noteable&.after_note_destroyed(self) noteable&.after_note_destroyed(self)
end end
override :cross_reference_visible_for? override :system_note_with_references_visible_for?
def cross_reference_visible_for?(user) def system_note_with_references_visible_for?(user)
return false unless super return false unless super
return true unless system_note_for_epic? && created_before_noteable? return true unless system_note_for_epic? && created_before_noteable?
...@@ -82,7 +82,7 @@ module EE ...@@ -82,7 +82,7 @@ module EE
private private
def system_note_for_epic? def system_note_for_epic?
for_epic? && system? system? && for_epic?
end end
def created_before_noteable? def created_before_noteable?
......
...@@ -40,12 +40,12 @@ describe Note do ...@@ -40,12 +40,12 @@ describe Note do
end end
it 'returns visible but not readable for a non-member user' do it 'returns visible but not readable for a non-member user' do
expect(note.cross_reference_visible_for?(non_member)).to be_truthy expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
expect(note.readable_by?(non_member)).to be_falsy expect(note.readable_by?(non_member)).to be_falsy
end end
it 'returns visible but not readable for a nil user' do it 'returns visible but not readable for a nil user' do
expect(note.cross_reference_visible_for?(nil)).to be_truthy expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
expect(note.readable_by?(nil)).to be_falsy expect(note.readable_by?(nil)).to be_falsy
end end
end end
...@@ -76,7 +76,7 @@ describe Note do ...@@ -76,7 +76,7 @@ describe Note do
end end
end end
describe '#cross_reference?' do describe '#system_note_with_references?' do
[:relate_epic, :unrelate_epic].each do |type| [:relate_epic, :unrelate_epic].each do |type|
it "delegates #{type} system note to the cross-reference regex" do it "delegates #{type} system note to the cross-reference regex" do
note = create(:note, :system) note = create(:note, :system)
...@@ -84,7 +84,7 @@ describe Note do ...@@ -84,7 +84,7 @@ describe Note do
expect(note).to receive(:matches_cross_reference_regex?).and_return(false) expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
note.cross_reference? note.system_note_with_references?
end end
end end
end end
......
...@@ -230,7 +230,7 @@ module API ...@@ -230,7 +230,7 @@ module API
.fresh .fresh
# Without RendersActions#prepare_notes_for_rendering, # Without RendersActions#prepare_notes_for_rendering,
# Note#cross_reference_visible_for? will attempt to render # Note#system_note_with_references_visible_for? will attempt to render
# Markdown references mentioned in the note to see whether they # Markdown references mentioned in the note to see whether they
# should be redacted. For notes that reference a commit, this # should be redacted. For notes that reference a commit, this
# would also incur a Gitaly call to verify the commit exists. # would also incur a Gitaly call to verify the commit exists.
......
...@@ -18,7 +18,7 @@ module Banzai ...@@ -18,7 +18,7 @@ module Banzai
issuables = extractor.extract([doc]) issuables = extractor.extract([doc])
issuables.each do |node, issuable| issuables.each do |node, issuable|
next if !can_read_cross_project? && cross_reference?(issuable) next if !can_read_cross_project? && cross_referenced?(issuable)
if VISIBLE_STATES.include?(issuable.state) && issuable_reference?(node.inner_html, issuable) if VISIBLE_STATES.include?(issuable.state) && issuable_reference?(node.inner_html, issuable)
state = moved_issue?(issuable) ? s_("IssuableStatus|moved") : issuable.state state = moved_issue?(issuable) ? s_("IssuableStatus|moved") : issuable.state
...@@ -39,7 +39,7 @@ module Banzai ...@@ -39,7 +39,7 @@ module Banzai
CGI.unescapeHTML(text) == issuable.reference_link_text(project || group) CGI.unescapeHTML(text) == issuable.reference_link_text(project || group)
end end
def cross_reference?(issuable) def cross_referenced?(issuable)
return true if issuable.project != project return true if issuable.project != project
return true if issuable.respond_to?(:group) && issuable.group != group return true if issuable.respond_to?(:group) && issuable.group != group
......
...@@ -285,9 +285,7 @@ describe Note do ...@@ -285,9 +285,7 @@ describe Note do
end end
end end
describe "#cross_reference_visible_for?" do describe "#system_note_with_references_visible_for?" do
using RSpec::Parameterized::TableSyntax
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user } let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user }
...@@ -311,12 +309,12 @@ describe Note do ...@@ -311,12 +309,12 @@ describe Note do
end end
it 'returns visible but not readable for non-member user' do it 'returns visible but not readable for non-member user' do
expect(note.cross_reference_visible_for?(non_member)).to be_truthy expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
expect(note.readable_by?(non_member)).to be_falsy expect(note.readable_by?(non_member)).to be_falsy
end end
it 'returns visible but not readable for a nil user' do it 'returns visible but not readable for a nil user' do
expect(note.cross_reference_visible_for?(nil)).to be_truthy expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
expect(note.readable_by?(nil)).to be_falsy expect(note.readable_by?(nil)).to be_falsy
end end
end end
...@@ -360,7 +358,7 @@ describe Note do ...@@ -360,7 +358,7 @@ describe Note do
end end
end end
describe "cross_reference_visible_for?" do describe "system_note_with_references_visible_for?" do
let_it_be(:private_user) { create(:user) } 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_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
let_it_be(:private_issue) { create(:issue, project: private_project) } let_it_be(:private_issue) { create(:issue, project: private_project) }
...@@ -370,11 +368,11 @@ describe Note do ...@@ -370,11 +368,11 @@ describe Note do
shared_examples "checks references" do shared_examples "checks references" do
it "returns true" do it "returns true" do
expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end end
it "returns false" do it "returns false" do
expect(note.cross_reference_visible_for?(private_user)).to be_truthy expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy
end end
it "returns false if user visible reference count set" do it "returns false if user visible reference count set" do
...@@ -382,14 +380,14 @@ describe Note do ...@@ -382,14 +380,14 @@ describe Note do
note.total_reference_count = 1 note.total_reference_count = 1
expect(note).not_to receive(:reference_mentionables) expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_visible_for?(ext_issue.author)).to be_truthy expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
end end
it "returns true if ref count is 0" do it "returns true if ref count is 0" do
note.user_visible_reference_count = 0 note.user_visible_reference_count = 0
expect(note).not_to receive(:reference_mentionables) expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end end
end end
...@@ -434,16 +432,16 @@ describe Note do ...@@ -434,16 +432,16 @@ describe Note do
note.total_reference_count = 2 note.total_reference_count = 2
expect(note).not_to receive(:reference_mentionables) expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_visible_for?(ext_issue.author)).to be_falsy expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end end
end end
end end
describe '#cross_reference?' do describe '#system_note_with_references?' do
it 'falsey for user-generated notes' do it 'falsey for user-generated notes' do
note = create(:note, system: false) note = create(:note, system: false)
expect(note.cross_reference?).to be_falsy expect(note.system_note_with_references?).to be_falsy
end end
context 'when the note might contain cross references' do context 'when the note might contain cross references' do
...@@ -454,7 +452,7 @@ describe Note do ...@@ -454,7 +452,7 @@ describe Note do
it 'delegates to the cross-reference regex' do it 'delegates to the cross-reference regex' do
expect(note).to receive(:matches_cross_reference_regex?).and_return(false) expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
note.cross_reference? note.system_note_with_references?
end end
end end
end end
...@@ -464,8 +462,8 @@ describe Note do ...@@ -464,8 +462,8 @@ describe Note do
let(:label_note) { build(:note, note: 'added ~2323232323', system: true) } let(:label_note) { build(:note, note: 'added ~2323232323', system: true) }
it 'scan for a `mentioned in` prefix' do it 'scan for a `mentioned in` prefix' do
expect(commit_note.cross_reference?).to be_truthy expect(commit_note.system_note_with_references?).to be_truthy
expect(label_note.cross_reference?).to be_falsy expect(label_note.system_note_with_references?).to be_falsy
end end
end end
...@@ -479,7 +477,7 @@ describe Note do ...@@ -479,7 +477,7 @@ describe Note do
it 'delegates to the system note service' do it 'delegates to the system note service' do
expect(SystemNotes::IssuablesService).to receive(:cross_reference?).with(note.note) expect(SystemNotes::IssuablesService).to receive(:cross_reference?).with(note.note)
note.cross_reference? note.system_note_with_references?
end end
end end
...@@ -491,7 +489,7 @@ describe Note do ...@@ -491,7 +489,7 @@ describe Note do
it 'delegates to the cross-reference regex' do it 'delegates to the cross-reference regex' do
expect(note).to receive(:matches_cross_reference_regex?) expect(note).to receive(:matches_cross_reference_regex?)
note.cross_reference? note.system_note_with_references?
end end
end end
...@@ -500,13 +498,13 @@ describe Note do ...@@ -500,13 +498,13 @@ describe Note do
it_behaves_like 'system_note_metadata includes note action' it_behaves_like 'system_note_metadata includes note action'
it { expect(note.cross_reference?).to be_falsy } it { expect(note.system_note_with_references?).to be_falsy }
context 'with cross reference label note' do context 'with cross reference label note' do
let(:label) { create(:label, project: issue.project)} let(:label) { create(:label, project: issue.project)}
let(:note) { create(:system_note, note: "added #{label.to_reference} label", noteable: issue, project: issue.project) } let(:note) { create(:system_note, note: "added #{label.to_reference} label", noteable: issue, project: issue.project) }
it { expect(note.cross_reference?).to be_truthy } it { expect(note.system_note_with_references?).to be_truthy }
end end
end end
...@@ -515,13 +513,13 @@ describe Note do ...@@ -515,13 +513,13 @@ describe Note do
it_behaves_like 'system_note_metadata includes note action' it_behaves_like 'system_note_metadata includes note action'
it { expect(note.cross_reference?).to be_falsy } it { expect(note.system_note_with_references?).to be_falsy }
context 'with cross reference milestone note' do context 'with cross reference milestone note' do
let(:milestone) { create(:milestone, project: issue.project)} let(:milestone) { create(:milestone, project: issue.project)}
let(:note) { create(:system_note, note: "added #{milestone.to_reference} milestone", noteable: issue, project: issue.project) } let(:note) { create(:system_note, note: "added #{milestone.to_reference} milestone", noteable: issue, project: issue.project) }
it { expect(note.cross_reference?).to be_truthy } it { expect(note.system_note_with_references?).to be_truthy }
end end
end end
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
shared_examples 'users with note access' do shared_examples 'users with note access' do
it 'returns true' do it 'returns true' do
users.each do |user| users.each do |user|
expect(note.cross_reference_visible_for?(user)).to be_truthy expect(note.system_note_with_references_visible_for?(user)).to be_truthy
expect(note.readable_by?(user)).to be_truthy expect(note.readable_by?(user)).to be_truthy
end end
end end
...@@ -12,7 +12,7 @@ end ...@@ -12,7 +12,7 @@ end
shared_examples 'users without note access' do shared_examples 'users without note access' do
it 'returns false' do it 'returns false' do
users.each do |user| users.each do |user|
expect(note.cross_reference_visible_for?(user)).to be_falsy expect(note.system_note_with_references_visible_for?(user)).to be_falsy
expect(note.readable_by?(user)).to be_falsy expect(note.readable_by?(user)).to be_falsy
end end
end end
......
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