Merge branch '58065-uniform-html-txt-email' into 'master'

Make HTML and text emails for new issues uniform and add the mail to mailer previews

Closes #58065

See merge request gitlab-org/gitlab-ce!29886

9 files changed, 59 insertions, 85 deletions

diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml
index 83c7f548975..dc5529b489b 100644
--- a/app/views/notify/_note_email.html.haml
+++ b/app/views/notify/_note_email.html.haml
@@ -5,27 +5,21 @@
 
 - discussion = note.discussion if note.part_of_discussion?
 
-- if discussion
-  %p{ style: "color: #777777;" }
-    = succeed ':' do
-      = link_to note.author_name, user_url(note.author)
+%p{ style: "color: #777777;" }
+  = succeed ':' do
+    = link_to note.author_name, user_url(note.author)
+    - if discussion.nil?
+      commented
+    - else
+      - if discussion.new_discussion?
+        started a new
+      - else
+        commented on a
 
       - if discussion&.diff_discussion?
-        - if discussion.new_discussion?
-          started a new discussion
-        - else
-          commented on a discussion
-
-        on #{link_to discussion.file_path, target_url}
+        discussion on #{link_to(discussion.file_path, target_url)}
       - else
-        - if discussion.new_discussion?
-          started a new discussion
-        - else
-          commented on a #{link_to 'discussion', target_url}
-
-- elsif Gitlab::CurrentSettings.email_author_in_body
-  %p.details
-    #{link_to note.author_name, user_url(note.author)} commented:
+        = link_to 'discussion', target_url
 
 - if discussion&.diff_discussion? && discussion.on_text?
   = content_for :head do
diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb
index fae8fa3ccf3..a25daad8458 100644
--- a/app/views/notify/_note_email.text.erb
+++ b/app/views/notify/_note_email.text.erb
@@ -1,29 +1,25 @@
 <% note = local_assigns.fetch(:note, @note) -%>
 <% diff_limit = local_assigns.fetch(:diff_limit, nil) -%>
 <% target_url = local_assigns.fetch(:target_url, @target_url) -%>
+<% discussion = note.discussion if note.part_of_discussion? -%>
 
-<%  discussion = note.discussion if note.part_of_discussion? -%>
-<%  if discussion && !discussion.individual_note? -%>
-<%=   sanitize_name(note.author_name) -%>
+<%= sanitize_name(note.author_name) -%>
+<%  if discussion.nil? -%>
+ <%= 'commented' -%>:
+<%  else -%>
 <%    if discussion.new_discussion? -%>
-<%=     " started a new discussion" -%>
+ <%=    'started a new discussion' -%>
 <%    else -%>
-<%=     " commented on a discussion" -%>
+ <%=    'commented on a discussion' -%>
 <%    end -%>
 <%    if discussion.diff_discussion? -%>
-<%=     " on #{discussion.file_path}" -%>
+ <%=    "on #{discussion.file_path}" -%>
 <%    end -%>
-<%=   ":" -%>
-<%    if discussion.diff_discussion? || !discussion.new_discussion? -%>
-<%=     " #{target_url}" -%>
-<%    end -%>
-
-
-<%  elsif Gitlab::CurrentSettings.email_author_in_body -%>
-<%=     "#{sanitize_name(note.author_name)} commented:" -%>
+<%=   ':' -%>
+<%=   " #{target_url}" -%>
+<%  end -%>
 
 
-<%  end -%>
 <%  if discussion&.diff_discussion? && discussion.on_text? -%>
 <%    discussion.truncated_diff_lines(highlight: false, diff_limit: diff_limit).each do |line| -%>
 <%=     "> #{line.text}\n" -%>
diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml
index 8aa7939dd0b..78afb42c9cf 100644
--- a/app/views/notify/new_issue_email.html.haml
+++ b/app/views/notify/new_issue_email.html.haml
@@ -1,6 +1,5 @@
-- if Gitlab::CurrentSettings.email_author_in_body
-  %p.details
-    #{link_to @issue.author_name, user_url(@issue.author)} created an issue:
+%p.details
+  #{link_to @issue.author_name, user_url(@issue.author)} created an issue:
 
 - if @issue.assignees.any?
   %p
diff --git a/app/views/notify/new_issue_email.text.erb b/app/views/notify/new_issue_email.text.erb
index ff258711b48..b93d95ef02f 100644
--- a/app/views/notify/new_issue_email.text.erb
+++ b/app/views/notify/new_issue_email.text.erb
@@ -1,7 +1,9 @@
-New Issue was created.
+<%= sanitize_name(@issue.author_name) %> <%= 'created an issue:' %>
 
-Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %>
-Author:    <%= sanitize_name(@issue.author_name) %>
-<%= assignees_label(@issue) %>
+<% if @issue.assignees.any? -%>
+  <%= assignees_label(@issue) %>
+<% end %>
 
-<%= @issue.description %>
+<% if @issue.description -%>
+  <%= @issue.description %>
+<% end %>
diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml
index 9ab648e2a64..2ddea0b9f16 100644
--- a/app/views/notify/new_merge_request_email.html.haml
+++ b/app/views/notify/new_merge_request_email.html.haml
@@ -1,15 +1,15 @@
-- if Gitlab::CurrentSettings.email_author_in_body
-  %p.details
-    #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request:
-
 %p.details
-  = merge_path_description(@merge_request, '→')
+  #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request:
 
-- if @merge_request.assignees.any?
-  %p
+%p
+  .branch
+    = merge_path_description(@merge_request, 'to')
+  .author
+    Author #{@merge_request.author_name}
+  .assignee
     = assignees_label(@merge_request)
-
-= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
+  .approvers
+    = render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
 
 - if @merge_request.description
   %div
diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb
index e6c42f1cf5f..c3f2902c78a 100644
--- a/app/views/notify/new_merge_request_email.text.erb
+++ b/app/views/notify/new_merge_request_email.text.erb
@@ -1,9 +1,7 @@
-New Merge Request <%= @merge_request.to_reference %>
-
-<%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %>
+<%= @merge_request.author_name %> <%= 'created a merge request:' %> <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %>
 
 <%= merge_path_description(@merge_request, 'to') %>
-Author:    <%= @merge_request.author_name %>
+<%= 'Author' %>:    <%= @merge_request.author_name %>
 <%= assignees_label(@merge_request) %>
 <%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %>
 
diff --git a/changelogs/unreleased/58065-uniform-html-txt-email.yml b/changelogs/unreleased/58065-uniform-html-txt-email.yml
new file mode 100644
index 00000000000..be34e93ff83
--- /dev/null
+++ b/changelogs/unreleased/58065-uniform-html-txt-email.yml
@@ -0,0 +1,5 @@
+---
+title: Always shows author of created issue/started discussion/comment in HTML body and text of email
+merge_request: 29886
+author: Frank van Rest
+type: fixed
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index cbbb22ad78c..11af6837dab 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -99,15 +99,9 @@ describe Notify do
           end
         end
 
-        context 'when enabled email_author_in_body' do
-          before do
-            stub_application_setting(email_author_in_body: true)
-          end
-
-          it 'contains a link to note author' do
-            is_expected.to have_body_text(issue.author_name)
-            is_expected.to have_body_text 'created an issue:'
-          end
+        it 'contains a link to issue author' do
+          is_expected.to have_body_text(issue.author_name)
+          is_expected.to have_body_text 'created an issue:'
         end
       end
 
@@ -314,15 +308,9 @@ describe Notify do
           end
         end
 
-        context 'when enabled email_author_in_body' do
-          before do
-            stub_application_setting(email_author_in_body: true)
-          end
-
-          it 'contains a link to note author' do
-            is_expected.to have_body_text merge_request.author_name
-            is_expected.to have_body_text 'created a merge request:'
-          end
+        it 'contains a link to merge request author' do
+          is_expected.to have_body_text merge_request.author_name
+          is_expected.to have_body_text 'created a merge request:'
         end
       end
 
@@ -907,7 +895,9 @@ describe Notify do
         end
 
         it 'contains an introduction' do
-          is_expected.to have_body_text 'started a new discussion'
+          issuable_url = "project_#{note.noteable_type.underscore}_url"
+
+          is_expected.to have_body_text "started a new <a href=\"#{public_send(issuable_url, project, note.noteable, anchor: "note_#{note.id}")}\">discussion</a>"
         end
 
         context 'when a comment on an existing discussion' do
diff --git a/spec/support/shared_examples/notify_shared_examples.rb b/spec/support/shared_examples/notify_shared_examples.rb
index 897c9106d77..6894a63ce42 100644
--- a/spec/support/shared_examples/notify_shared_examples.rb
+++ b/spec/support/shared_examples/notify_shared_examples.rb
@@ -281,18 +281,8 @@ shared_examples 'a note email' do
     is_expected.to have_body_text note.note
   end
 
-  it 'does not contain note author' do
-    is_expected.not_to have_body_text note.author_name
-  end
-
-  context 'when enabled email_author_in_body' do
-    before do
-      stub_application_setting(email_author_in_body: true)
-    end
-
-    it 'contains a link to note author' do
-      is_expected.to have_body_text note.author_name
-    end
+  it 'contains a link to note author' do
+    is_expected.to have_body_text note.author_name
   end
 end