diff --git a/app/models/ability.rb b/app/models/ability.rb
index 5bad5c17747d8c968bcced1e535bfb3e29f2cca4..ad6c588202e531d0ab2759b9211293d562d62e12 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -29,7 +29,7 @@ class Ability
when Snippet::INTERNAL, Snippet::PUBLIC
users
when Snippet::PRIVATE
- users.select { |user| snippet.author == user }
+ users.include?(snippet.author) ? [snippet.author] : []
end
end
diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb
index 25970158e705e72cb7ad739f39877f1e74bb97b3..a600f9c14c5a5dd17ad4f029b2eb04ac4da8c9fd 100644
--- a/app/models/concerns/cache_markdown_field.rb
+++ b/app/models/concerns/cache_markdown_field.rb
@@ -51,6 +51,10 @@ module CacheMarkdownField
CACHING_CLASSES.map(&:constantize)
end
+ def skip_project_check?
+ false
+ end
+
extend ActiveSupport::Concern
included do
@@ -112,9 +116,7 @@ module CacheMarkdownField
invalidation_method = "#{html_field}_invalidated?".to_sym
define_method(cache_method) do
- options = {
- skip_project_check: is_a?(Note) && for_personal_snippet?
- }
+ options = { skip_project_check: skip_project_check? }
html = Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
__send__("#{html_field}=", html)
true
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 9ded015aad3e8daa6c427991f8d8ca64b5010b1e..ef2c1e5d41430f4badef5ff0804005d8f0af4d1a 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -52,7 +52,7 @@ module Mentionable
options = options.merge(
cache_key: [self, attr],
author: author,
- skip_project_check: is_a?(Note) && for_personal_snippet?
+ skip_project_check: skip_project_check?
)
extractor.analyze(text, options)
@@ -125,4 +125,8 @@ module Mentionable
def cross_reference_exists?(target)
SystemNoteService.cross_reference_exists?(target, local_reference)
end
+
+ def skip_project_check?
+ false
+ end
end
diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index 5d8a223fc21002be9b35f7ee757e09b5f6cb692f..4865c0a14b1721577702a65c2cbedabed4e4d272 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -96,7 +96,8 @@ module Participable
participants.merge(ext.users)
- if self.is_a?(PersonalSnippet)
+ case self
+ when PersonalSnippet
Ability.users_that_can_read_personal_snippet(participants.to_a, self)
else
Ability.users_that_can_read_project(participants.to_a, project)
diff --git a/app/models/note.rb b/app/models/note.rb
index cbf1d0adda78b49b789ac2d1d34de58e5ad56b69..bf090a0438c4768940f797bcd2946269286be962 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -167,7 +167,11 @@ class Note < ActiveRecord::Base
end
def for_personal_snippet?
- noteable_type == "Snippet" && noteable.type == 'PersonalSnippet'
+ noteable.is_a?(PersonalSnippet)
+ end
+
+ def skip_project_check?
+ for_personal_snippet?
end
# override to return commits, which are not active record
@@ -225,6 +229,10 @@ class Note < ActiveRecord::Base
note.match(Banzai::Filter::EmojiFilter.emoji_pattern)[1]
end
+ def to_ability_name
+ for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
+ end
+
private
def keep_around_commit
diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb
index 45d916800f621d627392ea914935e415c73e67d3..6a10e17248333fab9e6b3805cc38061af735874c 100644
--- a/app/services/notes/post_process_service.rb
+++ b/app/services/notes/post_process_service.rb
@@ -10,10 +10,11 @@ module Notes
# Skip system notes, like status changes and cross-references and awards
unless @note.system?
EventCreateService.new.leave_note(@note, @note.author)
- unless @note.for_personal_snippet?
- @note.create_cross_references!
- execute_note_hooks
- end
+
+ return if @note.for_personal_snippet?
+
+ @note.create_cross_references!
+ execute_note_hooks
end
end
diff --git a/app/services/notes/slash_commands_service.rb b/app/services/notes/slash_commands_service.rb
index aaea9717fc478d8e3f25283ca67e9f032e6f6f5a..56913568cae70e250caa8eb5269fd101ca46f21c 100644
--- a/app/services/notes/slash_commands_service.rb
+++ b/app/services/notes/slash_commands_service.rb
@@ -12,7 +12,7 @@ module Notes
def self.supported?(note, current_user)
noteable_update_service(note) &&
current_user &&
- current_user.can?(:"update_#{note.noteable_type.underscore}", note.noteable)
+ current_user.can?(:"update_#{note.to_ability_name}", note.noteable)
end
def supported?(note)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 2a467ade5429149f9ff592ae8396a35f4fccfc92..f74e6cac17478f00ca8622c83aa48b54d15ee897 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -179,14 +179,14 @@ class NotificationService
mentioned_users = note.mentioned_users
- if note.for_personal_snippet?
- mentioned_users.select! do |user|
- user.can?(:read_personal_snippet, note.noteable)
- end
- else
- mentioned_users.select! do |user|
- user.can?(:read_project, note.project)
- end
+ ability, subject = if note.for_personal_snippet?
+ [:read_personal_snippet, note.noteable]
+ else
+ [:read_project, note.project]
+ end
+
+ mentioned_users.select! do |user|
+ user.can?(ability, subject)
end
# Add all users participating in the thread (author, assignee, comment authors)
@@ -220,12 +220,7 @@ class NotificationService
recipients.delete(note.author)
recipients = recipients.uniq
- # build notify method like 'note_commit_email'
- if note.for_personal_snippet?
- notify_method = "note_personal_snippet_email".to_sym
- else
- notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
- end
+ notify_method = "note_#{note.to_ability_name}_email".to_sym
recipients.each do |recipient|
mailer.send(notify_method, recipient.id, note.id).deliver_later
diff --git a/app/views/notify/note_personal_snippet_email.html.haml b/app/views/notify/note_personal_snippet_email.html.haml
index 3da199095d808ca96625e49a3a39486070cf45a7..2fa2f7846611da52652a8307e29b3992409f1606 100644
--- a/app/views/notify/note_personal_snippet_email.html.haml
+++ b/app/views/notify/note_personal_snippet_email.html.haml
@@ -1 +1 @@
-render 'note_message'
+= render 'note_message'
diff --git a/app/views/notify/note_personal_snippet_email.text.erb b/app/views/notify/note_personal_snippet_email.text.erb
new file mode 100644
index 0000000000000000000000000000000000000000..b2a8809a23b4a23ea8d7754f34754f9b8b68118f
--- /dev/null
+++ b/app/views/notify/note_personal_snippet_email.text.erb
@@ -0,0 +1,8 @@
+New comment for Snippet <%= @snippet.id %>
+
+<%= url_for(snippet_url(@snippet, anchor: "note_#{@note.id}")) %>
+
+
+Author: <%= @note.author_name %>
+
+<%= @note.note %>
diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb
index 8173f88806212ad10d29b6da917edf08a0a39e02..1aa9355b256c439bb0efaa946e14cea70aa5ed6f 100644
--- a/lib/banzai/filter/user_reference_filter.rb
+++ b/lib/banzai/filter/user_reference_filter.rb
@@ -28,6 +28,7 @@ module Banzai
ref_pattern = User.reference_pattern
ref_pattern_start = /\A#{ref_pattern}\z/
+
nodes.each do |node|
if text_node?(node)
replace_text_when_pattern_matches(node, ref_pattern) do |content|
diff --git a/spec/lib/banzai/filter/user_reference_filter_spec.rb b/spec/lib/banzai/filter/user_reference_filter_spec.rb
index 2166350f579709a226f381de0479292068e8eee4..3e1ac9fb2b277f65b2c094e9dfc3782204721052 100644
--- a/spec/lib/banzai/filter/user_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/user_reference_filter_spec.rb
@@ -157,17 +157,20 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
it 'does not link a User' do
doc = reference_filter("Hey #{reference}")
+
expect(doc).not_to include('a')
end
context 'when skip_project_check set to true' do
it 'links to a User' do
doc = reference_filter("Hey #{reference}", skip_project_check: true)
+
expect(doc.css('a').first.attr('href')).to eq urls.user_url(user)
end
it 'does not link users using @all reference' do
doc = reference_filter("Hey #{User.reference_prefix}all", skip_project_check: true)
+
expect(doc).not_to include('a')
end
end
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index 0b138405f02098fccfd5af0402d1649c7aa25a93..4d57efd3c53b133af2df92d7c22e440db98e5acb 100644
--- a/spec/models/ability_spec.rb
+++ b/spec/models/ability_spec.rb
@@ -172,32 +172,29 @@ describe Ability, lib: true do
end
describe '.users_that_can_read_personal_snippet' do
- subject { Ability.users_that_can_read_personal_snippet(users, snippet) }
+ def users_for_snippet(snippet)
+ described_class.users_that_can_read_personal_snippet(users, snippet)
+ end
+
let(:users) { create_list(:user, 3) }
let(:author) { users[0] }
- context 'private snippet' do
- let(:snippet) { create(:personal_snippet, :private, author: author) }
+ it 'private snippet is readable only by its author' do
+ snippet = create(:personal_snippet, :private, author: author)
- it 'is readable only by its author' do
- expect(subject).to match_array([author])
- end
+ expect(users_for_snippet(snippet)).to match_array([author])
end
- context 'internal snippet' do
- let(:snippet) { create(:personal_snippet, :public, author: author) }
+ it 'internal snippet is readable by all registered users' do
+ snippet = create(:personal_snippet, :public, author: author)
- it 'is readable by all registered users' do
- expect(subject).to match_array(users)
- end
+ expect(users_for_snippet(snippet)).to match_array(users)
end
- context 'public snippet' do
- let(:snippet) { create(:personal_snippet, :public, author: author) }
+ it 'public snippet is readable by all users' do
+ snippet = create(:personal_snippet, :public, author: author)
- it 'is readable by all users' do
- expect(subject).to match_array(users)
- end
+ expect(users_for_snippet(snippet)).to match_array(users)
end
end
diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb
index 63eb7473c97a76cc680aaf5cba0e42cd92194dc0..b73028f0bc0b4cdb0af4094b912901ad55a62666 100644
--- a/spec/models/concerns/mentionable_spec.rb
+++ b/spec/models/concerns/mentionable_spec.rb
@@ -35,17 +35,14 @@ describe Issue, "Mentionable" do
subject { issue.mentioned_users }
- it { is_expected.to include(user) }
- it { is_expected.not_to include(user2) }
+ it { expect(subject).to contain_exactly(user) }
context 'when a note on personal snippet' do
let!(:note) { create(:note_on_personal_snippet, note: "#{user.to_reference} mentioned #{user3.to_reference}") }
subject { note.mentioned_users }
- it { is_expected.to include(user) }
- it { is_expected.to include(user3) }
- it { is_expected.not_to include(user2) }
+ it { expect(subject).to contain_exactly(user, user3) }
end
end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 8017bf7a13c19794a9e6859f8542415034c21377..1b8ae356f1ff5cd6f10f1633dd8785265e4dfb19 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -59,7 +59,7 @@ describe Note, models: true do
end
context 'when noteable is a personal snippet' do
- subject { create(:note_on_personal_snippet) }
+ subject { build(:note_on_personal_snippet) }
it 'is valid without project' do
is_expected.to be_valid
@@ -321,4 +321,70 @@ describe Note, models: true do
end
end
end
+
+ describe '#for_personal_snippet?' do
+ it 'returns false for a project snippet note' do
+ expect(build(:note_on_project_snippet).for_personal_snippet?).to be_falsy
+ end
+
+ it 'returns true for a personal snippet note' do
+ expect(build(:note_on_personal_snippet).for_personal_snippet?).to be_truthy
+ end
+ end
+
+ describe '#to_ability_name' do
+ it 'returns snippet for a project snippet note' do
+ expect(build(:note_on_project_snippet).to_ability_name).to eq('snippet')
+ end
+
+ it 'returns personal_snippet for a personal snippet note' do
+ expect(build(:note_on_personal_snippet).to_ability_name).to eq('personal_snippet')
+ end
+
+ it 'returns merge_request for an MR note' do
+ expect(build(:note_on_merge_request).to_ability_name).to eq('merge_request')
+ end
+
+ it 'returns issue for an issue note' do
+ expect(build(:note_on_issue).to_ability_name).to eq('issue')
+ end
+
+ it 'returns issue for a commit note' do
+ expect(build(:note_on_commit).to_ability_name).to eq('commit')
+ end
+ end
+
+ describe '#cache_markdown_field' do
+ let(:html) { '<p>some html</p>'}
+
+ context 'note for a project snippet' do
+ let(:note) { build(:note_on_project_snippet) }
+
+ before do
+ expect(Banzai::Renderer).to receive(:cacheless_render_field).
+ with(note, :note, { skip_project_check: false }).and_return(html)
+
+ note.save
+ end
+
+ it 'creates a note' do
+ expect(note.note_html).to eq(html)
+ end
+ end
+
+ context 'note for a personal snippet' do
+ let(:note) { build(:note_on_personal_snippet) }
+
+ before do
+ expect(Banzai::Renderer).to receive(:cacheless_render_field).
+ with(note, :note, { skip_project_check: true }).and_return(html)
+
+ note.save
+ end
+
+ it 'creates a note' do
+ expect(note.note_html).to eq(html)
+ end
+ end
+ end
end
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index b99cdfbae2d808e7134c8f06010c94df9851b8f4..9c92a5080c68584f33500088511e85acc92f05f5 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -15,25 +15,25 @@ describe Notes::CreateService, services: true do
context "valid params" do
it 'returns a valid note' do
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note).to be_valid
end
it 'returns a persisted note' do
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note).to be_persisted
end
it 'note has valid content' do
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note.note).to eq(opts[:note])
end
it 'note belongs to the correct project' do
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note.project).to eq(project)
end
@@ -44,7 +44,7 @@ describe Notes::CreateService, services: true do
expect_any_instance_of(TodoService).to receive(:new_note).with(note, user)
- Notes::CreateService.new(project, user, opts).execute
+ described_class.new(project, user, opts).execute
end
it 'enqueues NewNoteWorker' do
@@ -53,7 +53,7 @@ describe Notes::CreateService, services: true do
expect(NewNoteWorker).to receive(:perform_async).with(note.id)
- Notes::CreateService.new(project, user, opts).execute
+ described_class.new(project, user, opts).execute
end
end
@@ -115,7 +115,7 @@ describe Notes::CreateService, services: true do
noteable_type: 'Issue',
noteable_id: issue.id
}
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note).to be_valid
expect(note.name).to eq('smile')
@@ -127,7 +127,7 @@ describe Notes::CreateService, services: true do
noteable_type: 'Issue',
noteable_id: issue.id
}
- note = Notes::CreateService.new(project, user, opts).execute
+ note = described_class.new(project, user, opts).execute
expect(note).to be_valid
expect(note.note).to eq(opts[:note])
@@ -142,7 +142,7 @@ describe Notes::CreateService, services: true do
expect_any_instance_of(TodoService).to receive(:new_award_emoji).with(issue, user)
- Notes::CreateService.new(project, user, opts).execute
+ described_class.new(project, user, opts).execute
end
end
end