From a59d719a19d5d3dca70f79f612c1e4ac1986241c Mon Sep 17 00:00:00 2001 From: Ravishankar Date: Sat, 30 May 2020 17:51:29 +0530 Subject: [PATCH] Send notification for merge when pipeline succeeds Filling up the code logic Fix wrong arguments Adding template and notification test cases Add feature check and lint error fix Add Documentation Translation fix Static analysis fix Review comments addressed Modify documentation Make the action change Making the method private and moving the feature check to child class Add spec for feature check Review comments Moving back the notify Review comments Modifying the email template to provide pipeline information Rubocop and test case fix change build pipeline to create pipeline Review comments addressed Review comments on the notify Change to small letter Adding checks to present sending notification on update params Fix the test case Review comments Change back to approval format Fix translation and build Fix static analysis Lint fix Lint fix Changing the spec name Review comments Remove the feature from documentation --- app/mailers/emails/merge_requests.rb | 7 + app/mailers/previews/notify_preview.rb | 4 + app/services/auto_merge/base_service.rb | 6 +- .../merge_when_pipeline_succeeds_service.rb | 8 + app/services/notification_service.rb | 8 + ...rge_when_pipeline_succeeds_email.html.haml | 161 ++++++++++++++++++ ...rge_when_pipeline_succeeds_email.text.haml | 8 + locale/gitlab.pot | 6 + spec/mailers/emails/merge_requests_spec.rb | 16 ++ ...rge_when_pipeline_succeeds_service_spec.rb | 14 ++ spec/services/notification_service_spec.rb | 20 +++ 11 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 app/views/notify/merge_when_pipeline_succeeds_email.html.haml create mode 100644 app/views/notify/merge_when_pipeline_succeeds_email.text.haml diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 76b1c2d234ce33..c709c2950d6090 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -92,6 +92,13 @@ def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_u mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id, reason)) end + def merge_when_pipeline_succeeds_email(recipient_id, merge_request_id, mwps_set_by_user_id, reason = nil) + setup_merge_request_mail(merge_request_id, recipient_id) + + @mwps_set_by = ::User.find(mwps_set_by_user_id) + mail_answer_thread(@merge_request, merge_request_thread_options(mwps_set_by_user_id, recipient_id, reason)) + end + private def setup_merge_request_mail(merge_request_id, recipient_id, present: false) diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index f3a4076e69ca17..c70ac1428cdf8c 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -177,6 +177,10 @@ def service_desk_thank_you_email Notify.service_desk_thank_you_email(issue.id).message end + def merge_when_pipeline_succeeds_email + Notify.merge_when_pipeline_succeeds_email(user.id, merge_request.id, user.id).message + end + private def project diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index c4109765a1c326..5c63dc34cb1d4f 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -11,7 +11,7 @@ def execute(merge_request) yield if block_given? end - # Notify the event that auto merge is enabled or merge param is updated + notify(merge_request) AutoMergeProcessWorker.perform_async(merge_request.id) strategy.to_sym @@ -62,6 +62,10 @@ def available_for?(merge_request) private + # Overridden in child classes + def notify(merge_request) + end + def strategy strong_memoize(:strategy) do self.class.name.demodulize.remove('Service').underscore diff --git a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb index 9ae5bd1b5ecb08..7e0298432ac5f8 100644 --- a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb +++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb @@ -34,5 +34,13 @@ def available_for?(merge_request) merge_request.actual_head_pipeline&.active? end end + + private + + def notify(merge_request) + return unless Feature.enabled?(:mwps_notification, project) + + notification_service.async.merge_when_pipeline_succeeds(merge_request, current_user) if merge_request.saved_change_to_auto_merge_enabled? + end end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 87664af3c1011c..a4e935a8cf57a9 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -582,6 +582,14 @@ def new_review(review) end end + def merge_when_pipeline_succeeds(merge_request, current_user) + recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'merge_when_pipeline_succeeds') + + recipients.each do |recipient| + mailer.merge_when_pipeline_succeeds_email(recipient.user.id, merge_request.id, current_user.id).deliver_later + end + end + protected def new_resource_email(target, method) diff --git a/app/views/notify/merge_when_pipeline_succeeds_email.html.haml b/app/views/notify/merge_when_pipeline_succeeds_email.html.haml new file mode 100644 index 00000000000000..54c4043f575138 --- /dev/null +++ b/app/views/notify/merge_when_pipeline_succeeds_email.html.haml @@ -0,0 +1,161 @@ + +%html{ lang: "en" } + %head + %meta{ content: "text/html; charset=UTF-8", "http-equiv" => "Content-Type" } + %meta{ content: "width=device-width, initial-scale=1", name: "viewport" } + %meta{ content: "IE=edge", "http-equiv" => "X-UA-Compatible" } + %title= message.subject + :css + /* CLIENT-SPECIFIC STYLES */ + body, table, td, a { -webkit-text-size-adjust: 100%; -ms-text-size-adjust: 100%; } + table, td { mso-table-lspace: 0pt; mso-table-rspace: 0pt; } + img { -ms-interpolation-mode: bicubic; } + + /* iOS BLUE LINKS */ + a[x-apple-data-detectors] { + color: inherit !important; + text-decoration: none !important; + font-size: inherit !important; + font-family: inherit !important; + font-weight: inherit !important; + line-height: inherit !important; + } + + /* ANDROID MARGIN HACK */ + body { margin:0 !important; } + div[style*="margin: 16px 0"] { margin:0 !important; } + + @media only screen and (max-width: 639px) { + body, #body { + min-width: 320px !important; + } + table.wrapper { + width: 100% !important; + min-width: 320px !important; + } + table.wrapper > tbody > tr > td { + border-left: 0 !important; + border-right: 0 !important; + border-radius: 0 !important; + padding-left: 10px !important; + padding-right: 10px !important; + } + } + + ul.assignees-list { + list-style: none; + padding: 0px; + display: block; + margin-top: 0px; + } + ul.assignees-list li { + display: inline-block; + padding-right: 12px; + padding-top: 8px; + } + + %body{ style: "background-color:#fafafa;margin:0;padding:0;text-align:center;min-width:640px;width:100%;height:100%;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;" } + %table#body{ border: "0", cellpadding: "0", cellspacing: "0", style: "background-color:#fafafa;margin:0;padding:0;text-align:center;min-width:640px;width:100%;" } + %tbody + %tr.line + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;background-color:#6b4fbb;height:4px;font-size:4px;line-height:4px;" } + %tr.header + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:25px 0;font-size:13px;line-height:1.6;color:#5c5c5c;" } + %img{ alt: "GitLab", height: "50", src: image_url('mailers/ci_pipeline_notif_v1/gitlab-logo.gif'), width: "55" } + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;" } + %table.wrapper{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:640px;margin:0 auto;border-collapse:separate;border-spacing:0;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;background-color:#ffffff;text-align:left;padding:18px 25px;border:1px solid #ededed;border-radius:3px;overflow:hidden;" } + %table.content{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;border-collapse:separate;border-spacing:0;" } + %tbody + %tr.success + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:10px;border-radius:3px;font-size:14px;line-height:1.3;text-align:center;overflow:hidden;color:#ffffff;background-color:#31af64;" } + %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;margin:0 auto;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;padding-right:5px;" } + %img{ alt: "✓", height: "13", src: image_url('mailers/ci_pipeline_notif_v1/icon-check-green-inverted.gif'), style: "display:block;", width: "13" } + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;" } + %span= _('Merge request was scheduled to merge after pipeline succeeds') + %tr.spacer + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;height:18px;font-size:18px;line-height:18px;" } +   + %tr.section + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;line-height:1.4;text-align:center;padding:0 15px;border:1px solid #ededed;border-radius:3px;overflow:hidden;" } + %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;width:100%;" } + %tbody + %tr{ style: 'width:100%;' } + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;text-align:center;" } + %img{ src: image_url('mailers/approval/icon-merge-request-gray.gif'), style: "height:18px;width:18px;margin-bottom:-4px;", alt: "Merge request icon" } + %span{ style: "font-weight: 600;color:#333333;" }= _('Merge request') + %a{ href: merge_request_url(@merge_request), style: "font-weight: 600;color:#3777b0;text-decoration:none" }= @merge_request.to_reference + %span= _('was scheduled to merge after pipeline succeeds by') + %img.avatar{ height: "24", src: avatar_icon_for_user(@mwps_set_by, 24, only_path: false), style: "border-radius:12px;margin:-7px 0 -7px 3px;", width: "24", alt: "Avatar" } + %a.muted{ href: user_url(@mwps_set_by), style: "color:#333333;text-decoration:none;" } + = @mwps_set_by.name + %tr.spacer + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;height:18px;font-size:18px;line-height:18px;" } +   + %tr.section + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:0 15px;border:1px solid #ededed;border-radius:3px;overflow:hidden;" } + %table.info{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;" }= _('Project') + -# haml-lint:disable NoPlainNodes + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;color:#333333;font-weight:400;width:75%;padding-left:5px;" } + - namespace_name = @project.group ? @project.group.name : @project.namespace.owner.name + - namespace_url = @project.group ? group_url(@project.group) : user_url(@project.namespace.owner) + %a.muted{ href: namespace_url, style: "color:#333333;text-decoration:none;" } + = namespace_name + \/ + %a.muted{ href: project_url(@project), style: "color:#333333;text-decoration:none;" } + = @project.name + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;border-top:1px solid #ededed;" }= _('Branch') + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;color:#333333;font-weight:400;width:75%;padding-left:5px;border-top:1px solid #ededed;" } + %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" } + %img{ height: "13", src: image_url('mailers/ci_pipeline_notif_v1/icon-branch-gray.gif'), style: "display:block;", width: "13", alt: "Branch icon" } + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" } + %span.muted{ style: "color:#333333;text-decoration:none;" } + = @merge_request.source_branch + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;border-top:1px solid #ededed;" }= _('Author') + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;color:#333333;font-weight:400;width:75%;padding-left:5px;border-top:1px solid #ededed;" } + %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" } + %img.avatar{ height: "24", src: avatar_icon_for_user(@merge_request.author, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" } + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" } + %a.muted{ href: user_url(@merge_request.author), style: "color:#333333;text-decoration:none;" } + = @merge_request.author.name + + - if @merge_request.assignees.any? + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;color:#8c8c8c;font-weight:300;padding:14px 0;margin:0;border-top:1px solid #ededed;" } + = assignees_label(@merge_request, include_value: false) + %td{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif; margin: 0; padding: 14px 0 0px 5px; font-size: 15px; line-height: 1.4; color: #333333; font-weight: 400; width: 75%; border-top-style: solid; border-top-color: #ededed; border-top-width: 1px; -webkit-text-size-adjust: 100%; -ms-text-size-adjust: 100%; mso-table-lspace: 0pt; mso-table-rspace: 0pt;" } + %ul.assignees-list{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif; font-size: 15px; line-height: 1.4; padding-right: 5px; -webkit-text-size-adjust: 100%; -ms-text-size-adjust: 100%; mso-table-lspace: 0pt; mso-table-rspace: 0pt;" } + - @merge_request.assignees.each do |assignee| + %li + %img.avatar{ alt: "Avatar", height: "24", src: avatar_icon_for_user(assignee, 24, only_path: false), style: "border-radius: 12px; max-width: 100%; height: auto; -ms-interpolation-mode: bicubic; margin: -2px 0;", width: "24" } + %a.muted{ href: user_url(assignee), style: "color: #333333; text-decoration: none; -webkit-text-size-adjust: 100%; -ms-text-size-adjust: 100%; vertical-align: top;" } + = assignee.name + + -# EE-specific start + = render 'layouts/mailer/additional_text' + -# EE-specific end + + %tr.footer + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:25px 0;font-size:13px;line-height:1.6;color:#5c5c5c;" } + %img{ alt: "GitLab", height: "33", src: image_url('mailers/ci_pipeline_notif_v1/gitlab-logo-full-horizontal.gif'), style: "display:block;margin:0 auto 1em;", width: "90" } + %div + - manage_notifications_link = link_to(_("Manage all notifications"), profile_notifications_url, style: "color:#3777b0;text-decoration:none;") + - help_link = link_to(_("Help"), help_url, style: "color:#3777b0;text-decoration:none;") + = _("You're receiving this email because of your account on %{host}. %{manage_notifications_link} · %{help_link}").html_safe % { host: Gitlab.config.gitlab.host, manage_notifications_link: manage_notifications_link, help_link: help_link } diff --git a/app/views/notify/merge_when_pipeline_succeeds_email.text.haml b/app/views/notify/merge_when_pipeline_succeeds_email.text.haml new file mode 100644 index 00000000000000..fdc23a6af0fb94 --- /dev/null +++ b/app/views/notify/merge_when_pipeline_succeeds_email.text.haml @@ -0,0 +1,8 @@ +Merge Request #{@merge_request.to_reference} was scheduled to merge after pipeline succeeds by #{sanitize_name(@mwps_set_by.name)} + +Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} + += merge_path_description(@merge_request, 'to') + +Author: #{sanitize_name(@merge_request.author_name)} += assignees_label(@merge_request) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 35afbe957f82ef..fb7d2fbba89401 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14546,6 +14546,9 @@ msgstr "" msgid "Merge request dependencies" msgstr "" +msgid "Merge request was scheduled to merge after pipeline succeeds" +msgstr "" + msgid "Merge requests" msgstr "" @@ -28914,6 +28917,9 @@ msgstr "" msgid "vulnerability|dismissed" msgstr "" +msgid "was scheduled to merge after pipeline succeeds by" +msgstr "" + msgid "wiki page" msgstr "" diff --git a/spec/mailers/emails/merge_requests_spec.rb b/spec/mailers/emails/merge_requests_spec.rb index fb523092f7ae96..477fb16400a407 100644 --- a/spec/mailers/emails/merge_requests_spec.rb +++ b/spec/mailers/emails/merge_requests_spec.rb @@ -17,4 +17,20 @@ expect(subject).to have_body_text current_user.name end end + + describe "#merge_when_pipeline_succeeds_email" do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:current_user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:title) { "Merge request #{merge_request.to_reference} was scheduled to merge after pipeline succeeds by #{current_user.name}" } + + subject { Notify.merge_when_pipeline_succeeds_email(user.id, merge_request.id, current_user.id) } + + it "has required details" do + expect(subject).to have_content title + expect(subject).to have_content merge_request.to_reference + expect(subject).to have_content current_user.name + end + end end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 092742276d3d4f..3bf59f6a2d1a65 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -69,6 +69,7 @@ before do allow(merge_request) .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) + expect(MailScheduler::NotificationServiceWorker).to receive(:perform_async).with('merge_when_pipeline_succeeds', merge_request, user).once service.execute(merge_request) end @@ -90,6 +91,18 @@ end end + context 'without feature enabled' do + it 'does not send notification' do + stub_feature_flags(mwps_notification: false) + + allow(merge_request) + .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) + expect(MailScheduler::NotificationServiceWorker).not_to receive(:perform_async) + + service.execute(merge_request) + end + end + context 'already approved' do let(:service) { described_class.new(project, user, should_remove_source_branch: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } @@ -106,6 +119,7 @@ it 'updates the merge params' do expect(SystemNoteService).not_to receive(:merge_when_pipeline_succeeds) + expect(MailScheduler::NotificationServiceWorker).not_to receive(:perform_async).with('merge_when_pipeline_succeeds', any_args) service.execute(mr_merge_if_green_enabled) expect(mr_merge_if_green_enabled.merge_params).to have_key('should_remove_source_branch') diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 9c837019d37110..2fe7a46de4b871 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2023,6 +2023,26 @@ def self.it_should_not_email! let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } end end + + describe '#merge_when_pipeline_succeeds' do + it 'send notification that merge will happen when pipeline succeeds' do + notification.merge_when_pipeline_succeeds(merge_request, assignee) + should_email(merge_request.author) + should_email(@u_watcher) + should_email(@subscriber) + end + + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.merge_when_pipeline_succeeds(merge_request, @u_disabled) } + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_when_pipeline_succeeds(merge_request, @u_disabled) } + end + end end describe 'Projects', :deliver_mails_inline do -- GitLab