Merge branch 'osw-markdown-bypass-for-commit-messages' into 'master'

Bypass markdown for commit titles on MR notes

Closes #37616

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

app/services/system_note_service.rb

@@ -22,8 +22,7 @@ module SystemNoteService
     commits_text = "#{total_count} commit".pluralize(total_count)
 
     body = "added #{commits_text}\n\n"
-    body << existing_commit_summary(noteable, existing_commits, oldrev)
-    body << new_commit_summary(new_commits).join("\n")
+    body << commits_list(noteable, new_commits, existing_commits, oldrev)
     body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})"
 
     create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count))
@@ -481,7 +480,7 @@ module SystemNoteService
   # Returns an Array of Strings
   def new_commit_summary(new_commits)
     new_commits.collect do |commit|
-      "* #{commit.short_id} - #{escape_html(commit.title)}"
+      content_tag('li', "#{commit.short_id} - #{commit.title}")
     end
   end
 
@@ -604,6 +603,16 @@ module SystemNoteService
     "#{cross_reference_note_prefix}#{gfm_reference}"
   end
 
+  # Builds a list of existing and new commits according to existing_commits and
+  # new_commits methods.
+  # Returns a String wrapped in `ul` and `li` tags.
+  def commits_list(noteable, new_commits, existing_commits, oldrev)
+    existing_commit_summary = existing_commit_summary(noteable, existing_commits, oldrev)
+    new_commit_summary = new_commit_summary(new_commits).join
+
+    content_tag('ul', "#{existing_commit_summary}#{new_commit_summary}".html_safe)
+  end
+
   # Build a single line summarizing existing commits being added in a merge
   # request
   #
@@ -640,11 +649,8 @@ module SystemNoteService
     branch = noteable.target_branch
     branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork?
 
-    "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n"
-  end
-
-  def escape_html(text)
-    Rack::Utils.escape_html(text)
+    branch_name = content_tag('code', branch)
+    content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe)
   end
 
   def url_helpers
@@ -661,4 +667,8 @@ module SystemNoteService
       start_sha: oldrev
     )
   end
+
+  def content_tag(*args)
+    ActionController::Base.helpers.content_tag(*args)
+  end
 end

changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml

@@ -0,0 +1,5 @@
+---
+title: Bypass commits title markdown on notes
+merge_request:
+author:
+type: fixed

spec/services/system_note_service_spec.rb

@@ -54,10 +54,11 @@ describe SystemNoteService do
           expect(note_lines[0]).to eq "added #{new_commits.size} commits"
         end
 
-        it 'adds a message line for each commit' do
-          new_commits.each_with_index do |commit, i|
-            # Skip the header
-            expect(HTMLEntities.new.decode(note_lines[i + 1])).to eq "* #{commit.short_id} - #{commit.title}"
+        it 'adds a message for each commit' do
+          decoded_note_content = HTMLEntities.new.decode(subject.note)
+
+          new_commits.each do |commit|
+            expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>")
           end
         end
       end
@@ -69,7 +70,7 @@ describe SystemNoteService do
           let(:old_commits) { [noteable.commits.last] }
 
           it 'includes the existing commit' do
-            expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`"
+            expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>")
           end
         end
 
@@ -79,22 +80,16 @@ describe SystemNoteService do
           context 'with oldrev' do
             let(:oldrev) { noteable.commits[2].id }
 
-            it 'includes a commit range' do
-              expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}"
-            end
-
-            it 'includes a commit count' do
-              expect(summary_line).to end_with " - 26 commits from branch `feature`"
+            it 'includes a commit range and count' do
+              expect(summary_line)
+                .to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>")
             end
           end
 
           context 'without oldrev' do
-            it 'includes a commit range' do
-              expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}"
-            end
-
-            it 'includes a commit count' do
-              expect(summary_line).to end_with " - 26 commits from branch `feature`"
+            it 'includes a commit range and count' do
+              expect(summary_line)
+                .to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>")
             end
           end
 
@@ -104,7 +99,7 @@ describe SystemNoteService do
             end
 
             it 'includes the project namespace' do
-              expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`"
+              expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>")
             end
           end
         end
@@ -693,7 +688,7 @@ describe SystemNoteService do
   describe '.new_commit_summary' do
     it 'escapes HTML titles' do
       commit = double(title: '<pre>This is a test</pre>', short_id: '12345678')
-      escaped = '&lt;pre&gt;This is a test&lt;&#x2F;pre&gt;'
+      escaped = '&lt;pre&gt;This is a test&lt;/pre&gt;'
 
       expect(described_class.new_commit_summary([commit])).to all(match(/- #{escaped}/))
     end