Uniform html and text emails

Uniform new_issue_email html and text emails
Uniform note_email html and text emails
Uniform new_merge_request_email html and text emails

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;" }
+%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
+        discussion on #{link_to(discussion.file_path, target_url)}
       - else
-          commented on a 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

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? -%>
-<%  if discussion && !discussion.individual_note? -%>
+
 <%= sanitize_name(note.author_name) -%>
-<%    if discussion.new_discussion? -%>
-<%=     " started a new discussion" -%>
+<%  if discussion.nil? -%>
+ <%= 'commented' -%>:
 <%  else -%>
-<%=     " commented on a discussion" -%>
+<%    if discussion.new_discussion? -%>
+ <%=    'started a new discussion' -%>
+<%    else -%>
+ <%=    '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:" -%>
-
-
-<%  end -%>
 <%  if discussion&.diff_discussion? && discussion.on_text? -%>
 <%    discussion.truncated_diff_lines(highlight: false, diff_limit: diff_limit).each do |line| -%>
 <%=     "> #{line.text}\n" -%>

app/views/notify/new_issue_email.html.haml

@@ -1,5 +1,4 @@
-- if Gitlab::CurrentSettings.email_author_in_body
-  %p.details
+%p.details
   #{link_to @issue.author_name, user_url(@issue.author)} created an issue:
 
 - if @issue.assignees.any?

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 %>

app/views/notify/new_merge_request_email.html.haml

@@ -1,15 +1,15 @@
-- if Gitlab::CurrentSettings.email_author_in_body
-  %p.details
+%p.details
   #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request:
 
-%p.details
-  = merge_path_description(@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

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 %>
 

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

spec/mailers/notify_spec.rb

@@ -99,17 +99,11 @@ 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
+        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
-      end
 
       describe 'that are reassigned' do
         let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
@@ -314,17 +308,11 @@ 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
+        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
-      end
 
       describe 'that are reassigned' do
         let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
@@ -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

spec/support/shared_examples/notify_shared_examples.rb

@@ -281,19 +281,9 @@ 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
-  end
 end
 
 shared_examples 'appearance header and footer enabled' do