Merge branch 'no_project_notes' into 'master'

Support notes without a project (personal snippets notes)

See merge request !8468

app/mailers/emails/notes.rb

@@ -38,6 +38,14 @@ module Emails
       mail_answer_thread(@snippet, note_thread_options(recipient_id))
     end
 
+    def note_personal_snippet_email(recipient_id, note_id)
+      setup_note_mail(note_id, recipient_id)
+
+      @snippet = @note.noteable
+      @target_url = snippet_url(@note.noteable)
+      mail_answer_thread(@snippet, note_thread_options(recipient_id))
+    end
+
     private
 
     def note_target_url_options

app/models/ability.rb

@@ -22,6 +22,17 @@ class Ability
       end
     end
 
+    # Given a list of users and a snippet this method returns the users that can
+    # read the given snippet.
+    def users_that_can_read_personal_snippet(users, snippet)
+      case snippet.visibility_level
+      when Snippet::INTERNAL, Snippet::PUBLIC
+        users
+      when Snippet::PRIVATE
+        users.include?(snippet.author) ? [snippet.author] : []
+      end
+    end
+
     # Returns an Array of Issues that can be read by the given user.
     #
     # issues - The issues to reduce down to those readable by the user.

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,7 +116,8 @@ module CacheMarkdownField
       invalidation_method = "#{html_field}_invalidated?".to_sym
 
       define_method(cache_method) do
-        html = Banzai::Renderer.cacheless_render_field(self, markdown_field)
+        options = { skip_project_check: skip_project_check? }
+        html = Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
         __send__("#{html_field}=", html)
         true
       end

app/models/concerns/mentionable.rb

@@ -49,7 +49,11 @@ module Mentionable
 
     self.class.mentionable_attrs.each do |attr, options|
       text    = __send__(attr)
-      options = options.merge(cache_key: [self, attr], author: author)
+      options = options.merge(
+        cache_key: [self, attr],
+        author: author,
+        skip_project_check: skip_project_check?
+      )
 
       extractor.analyze(text, options)
     end
@@ -121,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,6 +96,11 @@ module Participable
 
     participants.merge(ext.users)
 
-    Ability.users_that_can_read_project(participants.to_a, project)
+    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)
+    end
   end
 end

app/models/note.rb

@@ -43,7 +43,8 @@ class Note < ActiveRecord::Base
   delegate :name, :email, to: :author, prefix: true
   delegate :title, to: :noteable, allow_nil: true
 
-  validates :note, :project, presence: true
+  validates :note, presence: true
+  validates :project, presence: true, unless: :for_personal_snippet?
 
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
@@ -53,7 +54,7 @@ class Note < ActiveRecord::Base
   validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true
 
-  validate unless: [:for_commit?, :importing?] do |note|
+  validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note|
     unless note.noteable.try(:project) == note.project
       errors.add(:invalid_project, 'Note and noteable project mismatch')
     end
@@ -83,7 +84,7 @@ class Note < ActiveRecord::Base
   after_initialize :ensure_discussion_id
   before_validation :nullify_blank_type, :nullify_blank_line_code
   before_validation :set_discussion_id
-  after_save :keep_around_commit
+  after_save :keep_around_commit, unless: :for_personal_snippet?
 
   class << self
     def model_name
@@ -165,6 +166,14 @@ class Note < ActiveRecord::Base
     noteable_type == "Snippet"
   end
 
+  def for_personal_snippet?
+    noteable.is_a?(PersonalSnippet)
+  end
+
+  def skip_project_check?
+    for_personal_snippet?
+  end
+
   # override to return commits, which are not active record
   def noteable
     if for_commit?
@@ -220,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/create_service.rb

@@ -3,9 +3,10 @@ module Notes
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
 
-      note = project.notes.new(params)
-      note.author = current_user
-      note.system = false
+      note = Note.new(params)
+      note.project = project
+      note.author  = current_user
+      note.system  = false
 
       if note.award_emoji?
         noteable = note.noteable

app/services/notes/post_process_service.rb

@@ -10,6 +10,9 @@ module Notes
       # Skip system notes, like status changes and cross-references and awards
       unless @note.system?
         EventCreateService.new.leave_note(@note, @note.author)
+
+        return if @note.for_personal_snippet?
+
         @note.create_cross_references!
         execute_note_hooks
       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

@@ -178,8 +178,15 @@ class NotificationService
     recipients = []
 
     mentioned_users = note.mentioned_users
+
+    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?(:read_project, note.project)
+      user.can?(ability, subject)
     end
 
     # Add all users participating in the thread (author, assignee, comment authors)
@@ -192,11 +199,13 @@ class NotificationService
 
     recipients = recipients.concat(participants)
 
-    # Merge project watchers
-    recipients = add_project_watchers(recipients, note.project)
+    unless note.for_personal_snippet?
+      # Merge project watchers
+      recipients = add_project_watchers(recipients, note.project)
 
-    # Merge project with custom notification
-    recipients = add_custom_notifications(recipients, note.project, :new_note)
+      # Merge project with custom notification
+      recipients = add_custom_notifications(recipients, note.project, :new_note)
+    end
 
     # Reject users with Mention notification level, except those mentioned in _this_ note.
     recipients = reject_mention_users(recipients - mentioned_users, note.project)
@@ -211,8 +220,7 @@ class NotificationService
     recipients.delete(note.author)
     recipients = recipients.uniq
 
-    # build notify method like 'note_commit_email'
-    notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
+    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

+= render 'note_message'

app/views/notify/note_personal_snippet_email.text.erb

+New comment for Snippet <%= @snippet.id %>
+
+<%= url_for(snippet_url(@snippet, anchor: "note_#{@note.id}")) %>
+
+
+Author: <%= @note.author_name %>
+
+<%= @note.note %>

changelogs/unreleased/no_project_notes.yml

+---
+title: Support notes when a project is not specified (personal snippet notes)
+merge_request: 8468
+author:

lib/banzai/filter/reference_filter.rb

@@ -53,6 +53,10 @@ module Banzai
         context[:project]
       end
 
+      def skip_project_check?
+        context[:skip_project_check]
+      end
+
       def reference_class(type)
         "gfm gfm-#{type} has-tooltip"
       end

lib/banzai/filter/user_reference_filter.rb

@@ -24,7 +24,7 @@ module Banzai
       end
 
       def call
-        return doc if project.nil?
+        return doc if project.nil? && !skip_project_check?
 
         ref_pattern = User.reference_pattern
         ref_pattern_start = /\A#{ref_pattern}\z/
@@ -58,7 +58,7 @@ module Banzai
       # have `gfm` and `gfm-project_member` class names attached for styling.
       def user_link_filter(text, link_content: nil)
         self.class.references_in(text) do |match, username|
-          if username == 'all'
+          if username == 'all' && !skip_project_check?
             link_to_all(link_content: link_content)
           elsif namespace = namespaces[username]
             link_to_namespace(namespace, link_content: link_content) || match

lib/banzai/renderer.rb

@@ -52,9 +52,9 @@ module Banzai
     end
 
     # Same as +render_field+, but without consulting or updating the cache field
-    def cacheless_render_field(object, field)
+    def cacheless_render_field(object, field, options = {})
       text = object.__send__(field)
-      context = object.banzai_render_context(field)
+      context = object.banzai_render_context(field).merge(options)
 
       cacheless_render(text, context)
     end

spec/factories/notes.rb

@@ -13,6 +13,7 @@ FactoryGirl.define do
     factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_merge_request,      traits: [:on_merge_request]
     factory :note_on_project_snippet,    traits: [:on_project_snippet]
+    factory :note_on_personal_snippet,   traits: [:on_personal_snippet]
     factory :system_note,                traits: [:system]
 
     factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
@@ -70,6 +71,11 @@ FactoryGirl.define do
       noteable { create(:project_snippet, project: project) }
     end
 
+    trait :on_personal_snippet do
+      noteable { create(:personal_snippet) }
+      project nil
+    end
+
     trait :system do
       system true
     end

spec/lib/banzai/filter/user_reference_filter_spec.rb

@@ -152,6 +152,30 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
     end
   end
 
+  context 'when a project is not specified' do
+    let(:project) { nil }
+
+    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
+  end
+
   describe '#namespaces' do
     it 'returns a Hash containing all Namespaces' do
       document = Nokogiri::HTML.fragment("<p>#{user.to_reference}</p>")

spec/models/ability_spec.rb

@@ -171,6 +171,33 @@ describe Ability, lib: true do
     end
   end
 
+  describe '.users_that_can_read_personal_snippet' do
+    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] }
+
+    it 'private snippet is readable only by its author' do
+      snippet = create(:personal_snippet, :private, author: author)
+
+      expect(users_for_snippet(snippet)).to match_array([author])
+    end
+
+    it 'internal snippet is readable by all registered users' do
+      snippet = create(:personal_snippet, :public, author: author)
+
+      expect(users_for_snippet(snippet)).to match_array(users)
+    end
+
+    it 'public snippet is readable by all users' do
+      snippet = create(:personal_snippet, :public, author: author)
+
+      expect(users_for_snippet(snippet)).to match_array(users)
+    end
+  end
+
   describe '.issues_readable_by_user' do
     context 'with an admin user' do
       it 'returns all given issues' do

spec/models/concerns/mentionable_spec.rb

@@ -30,12 +30,20 @@ describe Issue, "Mentionable" do
   describe '#mentioned_users' do
     let!(:user) { create(:user, username: 'stranger') }
     let!(:user2) { create(:user, username: 'john') }
-    let!(:issue) { create(:issue, description: "#{user.to_reference} mentioned") }
+    let!(:user3) { create(:user, username: 'jim') }
+    let(:issue) { create(:issue, description: "#{user.to_reference} mentioned") }
 
     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 { expect(subject).to contain_exactly(user, user3) }
+    end
   end
 
   describe '#referenced_mentionables' do
@@ -138,6 +146,16 @@ describe Issue, "Mentionable" do
         issue.update_attributes(description: issues[1].to_reference)
         issue.create_new_cross_references!
       end
+
+      it 'notifies new references from project snippet note' do
+        snippet = create(:snippet, project: project)
+        note = create(:note, note: issues[0].to_reference, noteable: snippet, project: project, author: author)
+
+        expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args)
+
+        note.update_attributes(note: issues[1].to_reference)
+        note.create_new_cross_references!
+      end
     end
 
     def create_issue(description:)

spec/models/note_spec.rb

@@ -52,6 +52,19 @@ describe Note, models: true do
       subject { create(:note) }
       it { is_expected.to be_valid }
     end
+
+    context 'when project is missing for a project related note' do
+      subject { build(:note, project: nil, noteable: build_stubbed(:issue)) }
+      it { is_expected.to be_invalid }
+    end
+
+    context 'when noteable is a personal snippet' do
+      subject { build(:note_on_personal_snippet) }
+
+      it 'is valid without project' do
+        is_expected.to be_valid
+      end
+    end
   end
 
   describe "Commit notes" do
@@ -139,6 +152,7 @@ describe Note, models: true do
         with([{
           text: note1.note,
           context: {
+            skip_project_check: false,
             pipeline: :note,
             cache_key: [note1, "note"],
             project: note1.project,
@@ -150,6 +164,7 @@ describe Note, models: true do
         with([{
           text: note2.note,
           context: {
+            skip_project_check: false,
             pipeline: :note,
             cache_key: [note2, "note"],
             project: note2.project,
@@ -306,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,39 +15,45 @@ 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 = described_class.new(project, user, opts).execute
+
+        expect(note.project).to eq(project)
+      end
+
       it 'TodoService#new_note is called' do
-        note = build(:note)
-        allow(project).to receive_message_chain(:notes, :new).with(opts) { note }
+        note = build(:note, project: project)
+        allow(Note).to receive(:new).with(opts) { note }
 
         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
-        note = build(:note, id: 999)
-        allow(project).to receive_message_chain(:notes, :new).with(opts) { note }
+        note = build(:note, id: 999, project: project)
+        allow(Note).to receive(:new).with(opts) { note }
 
         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
 
@@ -75,6 +81,27 @@ describe Notes::CreateService, services: true do
         end
       end
     end
+
+    describe 'personal snippet note' do
+      subject { described_class.new(nil, user, params).execute }
+
+      let(:snippet) { create(:personal_snippet) }
+      let(:params) do
+        { note: 'comment', noteable_type: 'Snippet', noteable_id: snippet.id }
+      end
+
+      it 'returns a valid note' do
+        expect(subject).to be_valid
+      end
+
+      it 'returns a persisted note' do
+        expect(subject).to be_persisted
+      end
+
+      it 'note has valid content' do
+        expect(subject.note).to eq(params[:note])
+      end
+    end
   end
 
   describe "award emoji" do
@@ -88,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')
@@ -100,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])
@@ -115,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

spec/services/notification_service_spec.rb

@@ -269,6 +269,55 @@ describe NotificationService, services: true do
       end
     end
 
+    context 'personal snippet note' do
+      let(:snippet) { create(:personal_snippet, :public, author: @u_snippet_author) }
+      let(:note)    { create(:note_on_personal_snippet, noteable: snippet, note: '@mentioned note', author: @u_note_author) }
+
+      before do
+        @u_watcher               = create_global_setting_for(create(:user), :watch)
+        @u_participant           = create_global_setting_for(create(:user), :participating)
+        @u_disabled              = create_global_setting_for(create(:user), :disabled)
+        @u_mentioned             = create_global_setting_for(create(:user, username: 'mentioned'), :mention)
+        @u_mentioned_level       = create_global_setting_for(create(:user, username: 'participator'), :mention)
+        @u_note_author           = create(:user, username: 'note_author')
+        @u_snippet_author        = create(:user, username: 'snippet_author')
+        @u_not_mentioned         = create_global_setting_for(create(:user, username: 'regular'), :participating)
+
+        reset_delivered_emails!
+      end
+
+      let!(:notes) do
+        [
+          create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_watcher),
+          create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_participant),
+          create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_mentioned),
+          create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_disabled),
+          create(:note_on_personal_snippet, noteable: snippet, note: 'note', author: @u_note_author),
+        ]
+      end
+
+      describe '#new_note' do
+        it 'notifies the participants' do
+          notification.new_note(note)
+
+          # it emails participants
+          should_email(@u_watcher)
+          should_email(@u_participant)
+          should_email(@u_watcher)
+          should_email(@u_snippet_author)
+
+          # it emails mentioned users
+          should_email(@u_mentioned)
+
+          # it does not email participants with mention notification level
+          should_not_email(@u_mentioned_level)
+
+          # it does not email note author
+          should_not_email(@u_note_author)
+        end
+      end
+    end
+
     context 'commit note' do
       let(:project) { create(:project, :public) }
       let(:note) { create(:note_on_commit, project: project) }