summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYarNayar <YarTheGreat@gmail.com>2017-07-25 14:56:09 +0300
committerSean McGivern <sean@gitlab.com>2018-03-26 13:24:52 +0100
commit99b01e23598e6b0b2bcae891939ea28c67f7b2e9 (patch)
treec32d351419cd399dcb104eac54563b7028bbd1d8
parentbb9d360c0a7daed6aa08a0635e084c314c2c8b3e (diff)
downloadgitlab-ce-99b01e23598e6b0b2bcae891939ea28c67f7b2e9.tar.gz
Closes #23460
-rw-r--r--app/mailers/emails/merge_requests.rb8
-rw-r--r--app/models/commit.rb2
-rw-r--r--app/models/notification_recipient.rb15
-rw-r--r--app/models/notification_setting.rb7
-rw-r--r--app/services/merge_requests/refresh_service.rb8
-rw-r--r--app/services/notification_service.rb10
-rw-r--r--app/views/notify/push_to_merge_request_email.html.haml26
-rw-r--r--app/views/notify/push_to_merge_request_email.text.haml13
-rw-r--r--changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml5
-rw-r--r--db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb9
-rw-r--r--db/schema.rb3
-rw-r--r--doc/api/notification_settings.md4
-rw-r--r--doc/workflow/notifications.md3
-rw-r--r--spec/services/merge_requests/refresh_service_spec.rb21
-rw-r--r--spec/services/notification_service_spec.rb30
15 files changed, 154 insertions, 10 deletions
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 '&nbsp;- ' 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') }