From 99b01e23598e6b0b2bcae891939ea28c67f7b2e9 Mon Sep 17 00:00:00 2001 From: YarNayar Date: Tue, 25 Jul 2017 14:56:09 +0300 Subject: Send notification emails when push to a merge request Closes #23460 --- app/mailers/emails/merge_requests.rb | 8 ++++++ app/models/commit.rb | 2 +- app/models/notification_recipient.rb | 15 ++++++++--- app/models/notification_setting.rb | 7 ++++- app/services/merge_requests/refresh_service.rb | 8 +++--- app/services/notification_service.rb | 10 ++++++++ .../notify/push_to_merge_request_email.html.haml | 26 +++++++++++++++++++ .../notify/push_to_merge_request_email.text.haml | 13 ++++++++++ ...n-pushing-more-commits-to-the-merge-request.yml | 5 ++++ ...sh_to_merge_request_to_notification_settings.rb | 9 +++++++ db/schema.rb | 3 ++- doc/api/notification_settings.md | 4 +++ doc/workflow/notifications.md | 3 ++- .../merge_requests/refresh_service_spec.rb | 21 +++++++++++++++ spec/services/notification_service_spec.rb | 30 ++++++++++++++++++++++ 15 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 app/views/notify/push_to_merge_request_email.html.haml create mode 100644 app/views/notify/push_to_merge_request_email.text.haml create mode 100644 changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml create mode 100644 db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 5fe09cea83f..be99f3780cc 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -11,6 +11,14 @@ module Emails mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end + def push_to_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil, new_commits: [], existing_commits: []) + setup_merge_request_mail(merge_request_id, recipient_id) + @new_commits = new_commits + @existing_commits = existing_commits + + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) + end + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) diff --git a/app/models/commit.rb b/app/models/commit.rb index cceae5efb72..b64462fb768 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -175,7 +175,7 @@ class Commit if safe_message.blank? no_commit_message else - safe_message.split("\n", 2).first + safe_message.split(/[\r\n]/, 2).first end end diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index e95655e19f8..b3ffad00a07 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -48,7 +48,7 @@ class NotificationRecipient when :custom custom_enabled? || %i[participating mention].include?(@type) when :watch, :participating - !excluded_watcher_action? + !action_excluded? when :mention @type == :mention else @@ -96,13 +96,22 @@ class NotificationRecipient end end + def action_excluded? + excluded_watcher_action? || excluded_participating_action? + end + def excluded_watcher_action? - return false unless @custom_action - return false if notification_level == :custom + return false unless @custom_action && notification_level == :watch NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action) end + def excluded_participating_action? + return false unless @custom_action && notification_level == :participating + + NotificationSetting::EXCLUDED_PARTICIPATING_EVENTS.include?(@custom_action) + end + private def read_ability diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 245f8dddcf9..f6d9b0215fc 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -33,6 +33,7 @@ class NotificationSetting < ActiveRecord::Base :close_issue, :reassign_issue, :new_merge_request, + :push_to_merge_request, :reopen_merge_request, :close_merge_request, :reassign_merge_request, @@ -41,10 +42,14 @@ class NotificationSetting < ActiveRecord::Base :success_pipeline ].freeze - EXCLUDED_WATCHER_EVENTS = [ + EXCLUDED_PARTICIPATING_EVENTS = [ :success_pipeline ].freeze + EXCLUDED_WATCHER_EVENTS = [ + :push_to_merge_request + ].push(*EXCLUDED_PARTICIPATING_EVENTS).freeze + def self.find_or_create_for(source) setting = find_or_initialize_by(source: source) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 18c40ce8992..1fb1796b56c 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -21,7 +21,7 @@ module MergeRequests comment_mr_branch_presence_changed end - comment_mr_with_commits + notify_about_push mark_mr_as_wip_from_commits execute_mr_web_hooks @@ -141,8 +141,8 @@ module MergeRequests end end - # Add comment about pushing new commits to merge requests - def comment_mr_with_commits + # Add comment about pushing new commits to merge requests and send nofitication emails + def notify_about_push return unless @commits.present? merge_requests_for_source_branch.each do |merge_request| @@ -155,6 +155,8 @@ module MergeRequests SystemNoteService.add_commits(merge_request, merge_request.project, @current_user, new_commits, existing_commits, @oldrev) + + notification_service.push_to_merge_request(merge_request, @current_user, new_commits: new_commits, existing_commits: existing_commits) end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d7d2cde1004..f94c76cf3ac 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -113,6 +113,16 @@ class NotificationService new_resource_email(merge_request, :new_merge_request_email) end + def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: []) + new_commits = new_commits.map { |c| { short_id: c.short_id, title: c.title } } + existing_commits = existing_commits.map { |c| { short_id: c.short_id, title: c.title } } + recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: "push_to") + + recipients.each do |recipient| + mailer.send(:push_to_merge_request_email, recipient.user.id, merge_request.id, current_user.id, recipient.reason, new_commits: new_commits, existing_commits: existing_commits).deliver_later + end + end + # When merge request text is updated, we should send an email to: # # * newly mentioned project team members with notification level higher than Participating diff --git a/app/views/notify/push_to_merge_request_email.html.haml b/app/views/notify/push_to_merge_request_email.html.haml new file mode 100644 index 00000000000..5cc6f21c0f3 --- /dev/null +++ b/app/views/notify/push_to_merge_request_email.html.haml @@ -0,0 +1,26 @@ +%h3 + New commits were pushed to the merge request + = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)) + by #{@current_user.name} + +- if @existing_commits.any? + - count = @existing_commits.size + %ul + %li + - if count.one? + - commit_id = @existing_commits.first[:short_id] + = link_to(commit_id, project_commit_url(@merge_request.target_project, commit_id)) + - else + = link_to(project_compare_url(@merge_request.target_project, from: @existing_commits.first[:short_id], to: @existing_commits.last[:short_id])) do + #{@existing_commits.first[:short_id]}...#{@existing_commits.last[:short_id]} + = precede ' - ' do + - commits_text = "#{count} commit".pluralize(count) + #{commits_text} from branch `#{@merge_request.target_branch}` + +- if @new_commits.any? + %ul + - @new_commits.each do |commit| + %li + = link_to(commit[:short_id], project_commit_url(@merge_request.target_project, commit[:short_id])) + = precede ' - ' do + #{commit[:title]} diff --git a/app/views/notify/push_to_merge_request_email.text.haml b/app/views/notify/push_to_merge_request_email.text.haml new file mode 100644 index 00000000000..d7722e5f41f --- /dev/null +++ b/app/views/notify/push_to_merge_request_email.text.haml @@ -0,0 +1,13 @@ +New commits were pushed to the merge request #{@merge_request.to_reference} by #{@current_user.name} +\ +#{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))} +\ +- if @existing_commits.any? + - count = @existing_commits.size + - commits_id = count.one? ? @existing_commits.first[:short_id] : "#{@existing_commits.first[:short_id]}...#{@existing_commits.last[:short_id]}" + - commits_text = "#{count} commit".pluralize(count) + + * #{commits_id} - #{commits_text} from branch `#{@merge_request.target_branch}` +\ +- @new_commits.each do |commit| + * #{commit[:short_id]} - #{raw commit[:title]} diff --git a/changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml b/changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml new file mode 100644 index 00000000000..a62137ea2c9 --- /dev/null +++ b/changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml @@ -0,0 +1,5 @@ +--- +title: Send notification emails when push to a merge request +merge_request: 7610 +author: YarNayar +type: feature diff --git a/db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb b/db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb new file mode 100644 index 00000000000..12b8875d8dc --- /dev/null +++ b/db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb @@ -0,0 +1,9 @@ +class AddPushToMergeRequestToNotificationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :notification_settings, :push_to_merge_request, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 56116a2d241..394ccc872e3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180320182229) do +ActiveRecord::Schema.define(version: 20180323150945) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1296,6 +1296,7 @@ ActiveRecord::Schema.define(version: 20180320182229) do t.boolean "merge_merge_request" t.boolean "failed_pipeline" t.boolean "success_pipeline" + t.boolean "push_to_merge_request" end add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree diff --git a/doc/api/notification_settings.md b/doc/api/notification_settings.md index 3a2c398e355..f05ae647577 100644 --- a/doc/api/notification_settings.md +++ b/doc/api/notification_settings.md @@ -24,6 +24,7 @@ reopen_issue close_issue reassign_issue new_merge_request +push_to_merge_request reopen_merge_request close_merge_request reassign_merge_request @@ -75,6 +76,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab | `close_issue` | boolean | no | Enable/disable this notification | | `reassign_issue` | boolean | no | Enable/disable this notification | | `new_merge_request` | boolean | no | Enable/disable this notification | +| `push_to_merge_request` | boolean | no | Enable/disable this notification | | `reopen_merge_request` | boolean | no | Enable/disable this notification | | `close_merge_request` | boolean | no | Enable/disable this notification | | `reassign_merge_request` | boolean | no | Enable/disable this notification | @@ -141,6 +143,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab | `close_issue` | boolean | no | Enable/disable this notification | | `reassign_issue` | boolean | no | Enable/disable this notification | | `new_merge_request` | boolean | no | Enable/disable this notification | +| `push_to_merge_request` | boolean | no | Enable/disable this notification | | `reopen_merge_request` | boolean | no | Enable/disable this notification | | `close_merge_request` | boolean | no | Enable/disable this notification | | `reassign_merge_request` | boolean | no | Enable/disable this notification | @@ -164,6 +167,7 @@ Example responses: "close_issue": false, "reassign_issue": false, "new_merge_request": false, + "push_to_merge_request": false, "reopen_merge_request": false, "close_merge_request": false, "reassign_merge_request": false, diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 37265a5b771..c4095ee0f69 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -67,7 +67,7 @@ Below is the table of events users can be notified of: ### Issue / Merge request events -In all of the below cases, the notification will be sent to: +In most of the below cases, the notification will be sent to: - Participants: - the author and assignee of the issue/merge request - authors of comments on the issue/merge request @@ -87,6 +87,7 @@ In all of the below cases, the notification will be sent to: | Reassign issue | The above, plus the old assignee | | Reopen issue | | | New merge request | | +| Push to merge request | Participants and Custom notification level with this event selected | | Reassign merge request | The above, plus the old assignee | | Close merge request | | | Reopen merge request | | diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 903aa0a5078..2536c6e2514 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -24,6 +24,14 @@ describe MergeRequests::RefreshService do merge_when_pipeline_succeeds: true, merge_user: @user) + @another_merge_request = create(:merge_request, + source_project: @project, + source_branch: 'master', + target_branch: 'test', + target_project: @project, + merge_when_pipeline_succeeds: true, + merge_user: @user) + @fork_merge_request = create(:merge_request, source_project: @fork_project, source_branch: 'master', @@ -52,9 +60,11 @@ describe MergeRequests::RefreshService do context 'push to origin repo source branch' do let(:refresh_service) { service.new(@project, @user) } + let(:notification_service) { spy('notification_service') } before do allow(refresh_service).to receive(:execute_hooks) + allow(NotificationService).to receive(:new) { notification_service } end it 'executes hooks with update action' do @@ -64,6 +74,11 @@ describe MergeRequests::RefreshService do expect(refresh_service).to have_received(:execute_hooks) .with(@merge_request, 'update', old_rev: @oldrev) + expect(notification_service).to have_received(:push_to_merge_request) + .with(@merge_request, @user, new_commits: anything, existing_commits: anything) + expect(notification_service).to have_received(:push_to_merge_request) + .with(@another_merge_request, @user, new_commits: anything, existing_commits: anything) + expect(@merge_request.notes).not_to be_empty expect(@merge_request).to be_open expect(@merge_request.merge_when_pipeline_succeeds).to be_falsey @@ -119,11 +134,13 @@ describe MergeRequests::RefreshService do context 'push to origin repo source branch when an MR was reopened' do let(:refresh_service) { service.new(@project, @user) } + let(:notification_service) { spy('notification_service') } before do @merge_request.update(state: :reopened) allow(refresh_service).to receive(:execute_hooks) + allow(NotificationService).to receive(:new) { notification_service } refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end @@ -131,6 +148,10 @@ describe MergeRequests::RefreshService do it 'executes hooks with update action' do expect(refresh_service).to have_received(:execute_hooks) .with(@merge_request, 'update', old_rev: @oldrev) + expect(notification_service).to have_received(:push_to_merge_request) + .with(@merge_request, @user, new_commits: anything, existing_commits: anything) + expect(notification_service).to have_received(:push_to_merge_request) + .with(@another_merge_request, @user, new_commits: anything, existing_commits: anything) expect(@merge_request.notes).not_to be_empty expect(@merge_request).to be_open diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 3943148f0db..f8fa2540804 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1090,6 +1090,36 @@ describe NotificationService, :mailer do end end + describe '#push_to_merge_request' do + before do + update_custom_notification(:push_to_merge_request, @u_guest_custom, resource: project) + update_custom_notification(:push_to_merge_request, @u_custom_global) + end + + it do + notification.push_to_merge_request(merge_request, @u_disabled) + + should_email(merge_request.assignee) + should_email(@u_guest_custom) + should_email(@u_custom_global) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_not_email(@u_watcher) + should_not_email(@u_guest_watcher) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } + end + end + describe '#relabel_merge_request' do let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', merge_requests: [merge_request]) } let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') } -- cgit v1.2.1