diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 0d20c9092c40ca85ffdfbb72364fcc71f055c206..46fa6fd9f6deb88369e76349f7e88ba071fedb9e 100644 --- a/app/mailers/emails/notes.rb +++ b/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 diff --git a/app/models/ability.rb b/app/models/ability.rb index fa8f8bc3a5f8dca53e3340e53f2a90c54a51a3d6..5bad5c17747d8c968bcced1e535bfb3e29f2cca4 100644 --- a/app/models/ability.rb +++ b/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.select { |user| snippet.author == user } + 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. diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 70740c76e43ecc01ffc3234c065ae9b0c7f543be..5d8a223fc21002be9b35f7ee757e09b5f6cb692f 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -96,6 +96,10 @@ module Participable participants.merge(ext.users) - Ability.users_that_can_read_project(participants.to_a, project) + 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) + end end end diff --git a/app/models/note.rb b/app/models/note.rb index 0c1b05dabf28909b5aaedec606ebd1308b515a1d..cbf1d0adda78b49b789ac2d1d34de58e5ad56b69 100644 --- a/app/models/note.rb +++ b/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,10 @@ class Note < ActiveRecord::Base noteable_type == "Snippet" end + def for_personal_snippet? + noteable_type == "Snippet" && noteable.type == 'PersonalSnippet' + end + # override to return commits, which are not active record def noteable if for_commit? diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index cdd765c85eb66b0470b8bb555384a9b59d3dbe88..b4f8b33d564b40e3ae3c6b4813c73407b367713b 100644 --- a/app/services/notes/create_service.rb +++ b/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 diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index e4cd3fc7833867763cff187fac320dbc37c55f8d..45d916800f621d627392ea914935e415c73e67d3 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -10,8 +10,10 @@ module Notes # Skip system notes, like status changes and cross-references and awards unless @note.system? EventCreateService.new.leave_note(@note, @note.author) - @note.create_cross_references! - execute_note_hooks + unless @note.for_personal_snippet? + @note.create_cross_references! + execute_note_hooks + end end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index c3b61e68eab6da4163aad842b00e0832a8193e1f..2a467ade5429149f9ff592ae8396a35f4fccfc92 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -178,8 +178,15 @@ class NotificationService recipients = [] mentioned_users = note.mentioned_users - mentioned_users.select! do |user| - user.can?(:read_project, note.project) + + 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 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) @@ -212,7 +221,11 @@ class NotificationService recipients = recipients.uniq # build notify method like 'note_commit_email' - notify_method = "note_#{note.noteable_type.underscore}_email".to_sym + 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 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 new file mode 100644 index 0000000000000000000000000000000000000000..3da199095d808ca96625e49a3a39486070cf45a7 --- /dev/null +++ b/app/views/notify/note_personal_snippet_email.html.haml @@ -0,0 +1 @@ +render 'note_message' diff --git a/changelogs/unreleased/no_project_notes.yml b/changelogs/unreleased/no_project_notes.yml new file mode 100644 index 0000000000000000000000000000000000000000..6106c027360788635b60365aa6eafe76114888d5 --- /dev/null +++ b/changelogs/unreleased/no_project_notes.yml @@ -0,0 +1,4 @@ +--- +title: Support notes when a project is not specified (personal snippet notes) +merge_request: 8468 +author: diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index a10ba6297609638f510f4b8924350aabf842c0fd..476f2a377931621e66ba83d25939fb45b7634bfe 100644 --- a/spec/factories/notes.rb +++ b/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 diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 1bdf005c8237dbaf87f9f8167bb2667dcccc4e5c..0b138405f02098fccfd5af0402d1649c7aa25a93 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -171,6 +171,36 @@ describe Ability, lib: true do 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 context 'with an admin user' do it 'returns all given issues' do diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index 132858950d54c5ac1464208c514c34ec6951597f..8321e0b89f845b8ad5febaacdf68088b73f4b888 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -138,6 +138,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:) diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 310fecd8a5cc5f05a190730814ca89ce1e6f9b4c..ddd62d7f0e67c8abb10f9a61899761fc4ce514a7 100644 --- a/spec/models/note_spec.rb +++ b/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 { create(:note_on_personal_snippet) } + + it 'is valid without project' do + is_expected.to be_valid + end + end end describe "Commit notes" do diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index b0cc3ce5f5a5c74b9845e7039070120f0ea5244c..b99cdfbae2d808e7134c8f06010c94df9851b8f4 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -32,9 +32,15 @@ describe Notes::CreateService, services: true do expect(note.note).to eq(opts[:note]) 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 - 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) @@ -42,8 +48,8 @@ describe Notes::CreateService, services: true do 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) @@ -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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f3e80ac22a07468c0b78a4044cf3f10fb7518402..0ba210fdc5b93bf8c5d1069fc75e9b90dc3de3cf 100644 --- a/spec/services/notification_service_spec.rb +++ b/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) + + # 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 let(:project) { create(:project, :public) } let(:note) { create(:note_on_commit, project: project) }