Merge branch...

Merge branch '300750-add-missing-reviewers-information-to-approve-unapprove-merge-request-emails' into 'master'

Add reviewers detail to approve/unapprove MR emails

See merge request gitlab-org/gitlab!56290

ee/app/views/notify/approved_merge_request_email.html.haml

@@ -42,13 +42,13 @@
           }
       }
 
-      ul.assignees-list {
+      ul.users-list {
           list-style: none;
           padding: 0px;
           display: block;
           margin-top: 0px;
       }
-      ul.assignees-list li {
+      ul.users-list li {
           display: inline-block;
           padding-right: 12px;
           padding-top: 8px;
@@ -136,16 +136,10 @@
                                               = @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
+                                  = render 'users_list', users: @merge_request.assignees, user_label: assignees_label(@merge_request, include_value: false)
+
+                                - if @merge_request.reviewers.any?
+                                  = render 'users_list', users: @merge_request.reviewers, user_label: reviewers_label(@merge_request, include_value: false)
 
         -# EE-specific start
         = render 'layouts/mailer/additional_text'

ee/app/views/notify/approved_merge_request_email.text.haml

@@ -6,3 +6,4 @@ Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @m
 
 Author: #{sanitize_name(@merge_request.author_name)}
 = assignees_label(@merge_request)
+= reviewers_label(@merge_request)

ee/app/views/notify/unapproved_merge_request_email.html.haml

@@ -42,13 +42,13 @@
           }
       }
 
-      ul.assignees-list {
+      ul.users-list {
           list-style: none;
           padding: 0px;
           display: block;
           margin-top: 0px;
       }
-      ul.assignees-list li {
+      ul.users-list li {
           display: inline-block;
           padding-right: 12px;
           padding-top: 8px;
@@ -135,17 +135,10 @@
                                             %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
+                                  = render 'users_list', users: @merge_request.assignees, user_label: assignees_label(@merge_request, include_value: false)
 
+                                - if @merge_request.reviewers.any?
+                                  = render 'users_list', users: @merge_request.reviewers, user_label: reviewers_label(@merge_request, include_value: false)
         -# EE-specific start
         = render 'layouts/mailer/additional_text'
         -# EE-specific end

ee/app/views/notify/unapproved_merge_request_email.text.haml

@@ -6,3 +6,4 @@ Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @m
 
 Author: #{sanitize_name(@merge_request.author_name)}
 = assignees_label(@merge_request)
+= reviewers_label(@merge_request)

ee/changelogs/unreleased/300750-add-missing-reviewers-information-to-approve-unapprove-merge-reque.yml

+---
+title: Add reviewers detail to approved and unapproved merge request emails
+merge_request: 56290
+author:
+type: added

ee/spec/mailers/ee/emails/merge_requests_spec.rb

@@ -6,12 +6,15 @@ require 'email_spec'
 RSpec.describe EE::Emails::MergeRequests do
   include EmailSpec::Matchers
 
-  let(:user) { create(:user) }
-  let(:merge_request) { create(:merge_request) }
-  let(:current_user) { create(:user) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:assignee, reload: true) { create(:user, email: 'assignee@example.com', name: 'John Doe') }
+  let_it_be(:reviewer, reload: true) { create(:user, email: 'reviewer@example.com', name: 'Jane Doe') }
+  let_it_be(:merge_request) { create(:merge_request, assignees: [assignee], reviewers: [reviewer]) }
+
+  let(:recipient) { assignee }
 
   describe '#add_merge_request_approver_email' do
-    subject { Notify.add_merge_request_approver_email(user.id, merge_request.id, current_user.id) }
+    subject { Notify.add_merge_request_approver_email(recipient.id, merge_request.id, current_user.id) }
 
     context 'when email_author_in_body is set' do
       it 'includes the name of the person who added the approver' do
@@ -31,18 +34,32 @@ RSpec.describe EE::Emails::MergeRequests do
   end
 
   describe '#approved_merge_request_email' do
-    subject { Notify.approved_merge_request_email(user.id, merge_request.id, current_user.id) }
+    subject { Notify.approved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
 
-    it 'includes the name of the approver' do
-      expect(subject).to have_body_text(current_user.name)
+    it 'has the correct body' do
+      aggregate_failures do
+        is_expected.to have_body_text('was approved by')
+        is_expected.to have_body_text(current_user.name)
+        is_expected.to have_text_part_content(assignee.name)
+        is_expected.to have_html_part_content(assignee.name)
+        is_expected.to have_text_part_content(reviewer.name)
+        is_expected.to have_html_part_content(reviewer.name)
+      end
     end
   end
 
   describe '#unapproved_merge_request_email' do
-    subject { Notify.unapproved_merge_request_email(user.id, merge_request.id, current_user.id) }
+    subject { Notify.unapproved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
 
-    it 'includes the name of the person who removed their approval' do
-      expect(subject).to have_body_text(current_user.name)
+    it 'has the correct body' do
+      aggregate_failures do
+        is_expected.to have_body_text('was unapproved by')
+        is_expected.to have_body_text(current_user.name)
+        is_expected.to have_text_part_content(assignee.name)
+        is_expected.to have_html_part_content(assignee.name)
+        is_expected.to have_text_part_content(reviewer.name)
+        is_expected.to have_html_part_content(reviewer.name)
+      end
     end
   end
 end