address comments

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
 

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

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

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)

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

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
 

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)

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

app/views/notify/note_personal_snippet_email.html.haml

@@ -1 +1 @@
-render 'note_message'
+= render 'note_message'

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 %>

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|

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

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
 

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
 

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

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