Commit d6b11daf authored by Jarka Kadlecova's avatar Jarka Kadlecova

Support notes without project

parent 270dc226
...@@ -38,6 +38,14 @@ module Emails ...@@ -38,6 +38,14 @@ module Emails
mail_answer_thread(@snippet, note_thread_options(recipient_id)) mail_answer_thread(@snippet, note_thread_options(recipient_id))
end 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 private
def note_target_url_options def note_target_url_options
......
...@@ -22,6 +22,17 @@ class Ability ...@@ -22,6 +22,17 @@ class Ability
end end
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.select { |user| snippet.author == user }
end
end
# Returns an Array of Issues that can be read by the given user. # 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. # issues - The issues to reduce down to those readable by the user.
......
...@@ -96,6 +96,10 @@ module Participable ...@@ -96,6 +96,10 @@ module Participable
participants.merge(ext.users) participants.merge(ext.users)
if self.is_a?(PersonalSnippet)
Ability.users_that_can_read_personal_snippet(participants.to_a, self)
else
Ability.users_that_can_read_project(participants.to_a, project) Ability.users_that_can_read_project(participants.to_a, project)
end end
end
end end
...@@ -43,7 +43,8 @@ class Note < ActiveRecord::Base ...@@ -43,7 +43,8 @@ class Note < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true delegate :name, :email, to: :author, prefix: true
delegate :title, to: :noteable, allow_nil: 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 # Attachments are deprecated and are handled by Markdown uploader
validates :attachment, file_size: { maximum: :max_attachment_size } validates :attachment, file_size: { maximum: :max_attachment_size }
...@@ -53,7 +54,7 @@ class Note < ActiveRecord::Base ...@@ -53,7 +54,7 @@ class Note < ActiveRecord::Base
validates :commit_id, presence: true, if: :for_commit? validates :commit_id, presence: true, if: :for_commit?
validates :author, presence: true 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 unless note.noteable.try(:project) == note.project
errors.add(:invalid_project, 'Note and noteable project mismatch') errors.add(:invalid_project, 'Note and noteable project mismatch')
end end
...@@ -83,7 +84,7 @@ class Note < ActiveRecord::Base ...@@ -83,7 +84,7 @@ class Note < ActiveRecord::Base
after_initialize :ensure_discussion_id after_initialize :ensure_discussion_id
before_validation :nullify_blank_type, :nullify_blank_line_code before_validation :nullify_blank_type, :nullify_blank_line_code
before_validation :set_discussion_id before_validation :set_discussion_id
after_save :keep_around_commit after_save :keep_around_commit, unless: :for_personal_snippet?
class << self class << self
def model_name def model_name
...@@ -165,6 +166,10 @@ class Note < ActiveRecord::Base ...@@ -165,6 +166,10 @@ class Note < ActiveRecord::Base
noteable_type == "Snippet" noteable_type == "Snippet"
end end
def for_personal_snippet?
noteable_type == "Snippet" && noteable.type == 'PersonalSnippet'
end
# override to return commits, which are not active record # override to return commits, which are not active record
def noteable def noteable
if for_commit? if for_commit?
......
...@@ -3,7 +3,8 @@ module Notes ...@@ -3,7 +3,8 @@ module Notes
def execute def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha) merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
note = project.notes.new(params) note = Note.new(params)
note.project = project
note.author = current_user note.author = current_user
note.system = false note.system = false
......
...@@ -10,10 +10,12 @@ module Notes ...@@ -10,10 +10,12 @@ module Notes
# Skip system notes, like status changes and cross-references and awards # Skip system notes, like status changes and cross-references and awards
unless @note.system? unless @note.system?
EventCreateService.new.leave_note(@note, @note.author) EventCreateService.new.leave_note(@note, @note.author)
unless @note.for_personal_snippet?
@note.create_cross_references! @note.create_cross_references!
execute_note_hooks execute_note_hooks
end end
end end
end
def hook_data def hook_data
Gitlab::DataBuilder::Note.build(@note, @note.author) Gitlab::DataBuilder::Note.build(@note, @note.author)
......
...@@ -178,9 +178,16 @@ class NotificationService ...@@ -178,9 +178,16 @@ class NotificationService
recipients = [] recipients = []
mentioned_users = note.mentioned_users 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| mentioned_users.select! do |user|
user.can?(:read_project, note.project) user.can?(:read_project, note.project)
end end
end
# Add all users participating in the thread (author, assignee, comment authors) # Add all users participating in the thread (author, assignee, comment authors)
participants = participants =
...@@ -192,11 +199,13 @@ class NotificationService ...@@ -192,11 +199,13 @@ class NotificationService
recipients = recipients.concat(participants) recipients = recipients.concat(participants)
unless note.for_personal_snippet?
# Merge project watchers # Merge project watchers
recipients = add_project_watchers(recipients, note.project) recipients = add_project_watchers(recipients, note.project)
# Merge project with custom notification # Merge project with custom notification
recipients = add_custom_notifications(recipients, note.project, :new_note) recipients = add_custom_notifications(recipients, note.project, :new_note)
end
# Reject users with Mention notification level, except those mentioned in _this_ note. # Reject users with Mention notification level, except those mentioned in _this_ note.
recipients = reject_mention_users(recipients - mentioned_users, note.project) recipients = reject_mention_users(recipients - mentioned_users, note.project)
...@@ -212,7 +221,11 @@ class NotificationService ...@@ -212,7 +221,11 @@ class NotificationService
recipients = recipients.uniq recipients = recipients.uniq
# build notify method like 'note_commit_email' # 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 notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
end
recipients.each do |recipient| recipients.each do |recipient|
mailer.send(notify_method, recipient.id, note.id).deliver_later mailer.send(notify_method, recipient.id, note.id).deliver_later
......
---
title: Support notes when a project is not specified (personal snippet notes)
merge_request: 8468
author:
...@@ -13,6 +13,7 @@ FactoryGirl.define do ...@@ -13,6 +13,7 @@ FactoryGirl.define do
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note] factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
factory :note_on_merge_request, traits: [:on_merge_request] factory :note_on_merge_request, traits: [:on_merge_request]
factory :note_on_project_snippet, traits: [:on_project_snippet] factory :note_on_project_snippet, traits: [:on_project_snippet]
factory :note_on_personal_snippet, traits: [:on_personal_snippet]
factory :system_note, traits: [:system] factory :system_note, traits: [:system]
factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
...@@ -70,6 +71,11 @@ FactoryGirl.define do ...@@ -70,6 +71,11 @@ FactoryGirl.define do
noteable { create(:project_snippet, project: project) } noteable { create(:project_snippet, project: project) }
end end
trait :on_personal_snippet do
noteable { create(:personal_snippet) }
project nil
end
trait :system do trait :system do
system true system true
end end
......
...@@ -171,6 +171,36 @@ describe Ability, lib: true do ...@@ -171,6 +171,36 @@ describe Ability, lib: true do
end end
end end
describe '.users_that_can_read_personal_snippet' do
subject { Ability.users_that_can_read_personal_snippet(users, snippet) }
let(:users) { create_list(:user, 3) }
let(:author) { users[0] }
context 'private snippet' do
let(:snippet) { create(:personal_snippet, :private, author: author) }
it 'is readable only by its author' do
expect(subject).to match_array([author])
end
end
context 'internal snippet' do
let(:snippet) { create(:personal_snippet, :public, author: author) }
it 'is readable by all registered users' do
expect(subject).to match_array(users)
end
end
context 'public snippet' do
let(:snippet) { create(:personal_snippet, :public, author: author) }
it 'is readable by all users' do
expect(subject).to match_array(users)
end
end
end
describe '.issues_readable_by_user' do describe '.issues_readable_by_user' do
context 'with an admin user' do context 'with an admin user' do
it 'returns all given issues' do it 'returns all given issues' do
......
...@@ -138,6 +138,16 @@ describe Issue, "Mentionable" do ...@@ -138,6 +138,16 @@ describe Issue, "Mentionable" do
issue.update_attributes(description: issues[1].to_reference) issue.update_attributes(description: issues[1].to_reference)
issue.create_new_cross_references! issue.create_new_cross_references!
end 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 end
def create_issue(description:) def create_issue(description:)
......
...@@ -52,6 +52,19 @@ describe Note, models: true do ...@@ -52,6 +52,19 @@ describe Note, models: true do
subject { create(:note) } subject { create(:note) }
it { is_expected.to be_valid } it { is_expected.to be_valid }
end 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 { create(:note_on_personal_snippet) }
it 'is valid without project' do
is_expected.to be_valid
end
end
end end
describe "Commit notes" do describe "Commit notes" do
......
...@@ -32,9 +32,15 @@ describe Notes::CreateService, services: true do ...@@ -32,9 +32,15 @@ describe Notes::CreateService, services: true do
expect(note.note).to eq(opts[:note]) expect(note.note).to eq(opts[:note])
end end
it 'note belongs to the correct project' do
note = Notes::CreateService.new(project, user, opts).execute
expect(note.project).to eq(project)
end
it 'TodoService#new_note is called' do it 'TodoService#new_note is called' do
note = build(:note) note = build(:note, project: project)
allow(project).to receive_message_chain(:notes, :new).with(opts) { note } allow(Note).to receive(:new).with(opts) { note }
expect_any_instance_of(TodoService).to receive(:new_note).with(note, user) expect_any_instance_of(TodoService).to receive(:new_note).with(note, user)
...@@ -42,8 +48,8 @@ describe Notes::CreateService, services: true do ...@@ -42,8 +48,8 @@ describe Notes::CreateService, services: true do
end end
it 'enqueues NewNoteWorker' do it 'enqueues NewNoteWorker' do
note = build(:note, id: 999) note = build(:note, id: 999, project: project)
allow(project).to receive_message_chain(:notes, :new).with(opts) { note } allow(Note).to receive(:new).with(opts) { note }
expect(NewNoteWorker).to receive(:perform_async).with(note.id) expect(NewNoteWorker).to receive(:perform_async).with(note.id)
...@@ -75,6 +81,27 @@ describe Notes::CreateService, services: true do ...@@ -75,6 +81,27 @@ describe Notes::CreateService, services: true do
end end
end 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 end
describe "award emoji" do describe "award emoji" do
......
...@@ -269,6 +269,55 @@ describe NotificationService, services: true do ...@@ -269,6 +269,55 @@ describe NotificationService, services: true do
end end
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)
# TODO: make mentions working for pesronal snippets
# should_email(@u_mentioned_level)
# 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 context 'commit note' do
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:note) { create(:note_on_commit, project: project) } let(:note) { create(:note_on_commit, project: project) }
......
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