Commit b101e92b authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'render-hipchat-notification-descriptions' into 'master'

Render HipChat notification descriptions as HTML instead of raw markdown

## What does this MR do?

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/1621 for issues, merge requests and comments.

## Are there points in the code the reviewer needs to double check?

This MR is small enough to double check everything.

## Why was this MR needed?

Because seeing raw markdown in HipChat notifications is not nice :)

See merge request !6371
parents 030c8226 db7c227f
...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
## 8.14.0 (2016-11-22) ## 8.14.0 (2016-11-22)
- Adds user project membership expired event to clarify why user was removed (Callum Dryden) - Adds user project membership expired event to clarify why user was removed (Callum Dryden)
- Fix HipChat notifications rendering (airatshigapov, eisnerd)
- Simpler arguments passed to named_route on toggle_award_url helper method - Simpler arguments passed to named_route on toggle_award_url helper method
## 8.13.0 (2016-10-22) ## 8.13.0 (2016-10-22)
......
class HipchatService < Service class HipchatService < Service
include ActionView::Helpers::SanitizeHelper
MAX_COMMITS = 3 MAX_COMMITS = 3
HIPCHAT_ALLOWED_TAGS = %w[
a b i strong em br img pre code
table th tr td caption colgroup col thead tbody tfoot
ul ol li dl dt dd
]
prop_accessor :token, :room, :server, :notify, :color, :api_version prop_accessor :token, :room, :server, :notify, :color, :api_version
boolean_accessor :notify_only_broken_builds boolean_accessor :notify_only_broken_builds
...@@ -88,6 +95,10 @@ class HipchatService < Service ...@@ -88,6 +95,10 @@ class HipchatService < Service
end end
end end
def render_line(text)
markdown(text.lines.first.chomp, pipeline: :single_line) if text
end
def create_push_message(push) def create_push_message(push)
ref_type = Gitlab::Git.tag_ref?(push[:ref]) ? 'tag' : 'branch' ref_type = Gitlab::Git.tag_ref?(push[:ref]) ? 'tag' : 'branch'
ref = Gitlab::Git.ref_name(push[:ref]) ref = Gitlab::Git.ref_name(push[:ref])
...@@ -110,7 +121,7 @@ class HipchatService < Service ...@@ -110,7 +121,7 @@ class HipchatService < Service
message << "(<a href=\"#{project.web_url}/compare/#{before}...#{after}\">Compare changes</a>)" message << "(<a href=\"#{project.web_url}/compare/#{before}...#{after}\">Compare changes</a>)"
push[:commits].take(MAX_COMMITS).each do |commit| push[:commits].take(MAX_COMMITS).each do |commit|
message << "<br /> - #{commit[:message].lines.first} (<a href=\"#{commit[:url]}\">#{commit[:id][0..5]}</a>)" message << "<br /> - #{render_line(commit[:message])} (<a href=\"#{commit[:url]}\">#{commit[:id][0..5]}</a>)"
end end
if push[:commits].count > MAX_COMMITS if push[:commits].count > MAX_COMMITS
...@@ -121,12 +132,22 @@ class HipchatService < Service ...@@ -121,12 +132,22 @@ class HipchatService < Service
message message
end end
def format_body(body) def markdown(text, options = {})
if body return "" unless text
body = body.truncate(200, separator: ' ', omission: '...')
end context = {
project: project,
pipeline: :email
}
Banzai.render(text, context)
"<pre>#{body}</pre>" context.merge!(options)
html = Banzai.post_process(Banzai.render(text, context), context)
sanitized_html = sanitize(html, tags: HIPCHAT_ALLOWED_TAGS, attributes: %w[href title alt])
sanitized_html.truncate(200, separator: ' ', omission: '...')
end end
def create_issue_message(data) def create_issue_message(data)
...@@ -134,7 +155,7 @@ class HipchatService < Service ...@@ -134,7 +155,7 @@ class HipchatService < Service
obj_attr = data[:object_attributes] obj_attr = data[:object_attributes]
obj_attr = HashWithIndifferentAccess.new(obj_attr) obj_attr = HashWithIndifferentAccess.new(obj_attr)
title = obj_attr[:title] title = render_line(obj_attr[:title])
state = obj_attr[:state] state = obj_attr[:state]
issue_iid = obj_attr[:iid] issue_iid = obj_attr[:iid]
issue_url = obj_attr[:url] issue_url = obj_attr[:url]
...@@ -143,10 +164,7 @@ class HipchatService < Service ...@@ -143,10 +164,7 @@ class HipchatService < Service
issue_link = "<a href=\"#{issue_url}\">issue ##{issue_iid}</a>" issue_link = "<a href=\"#{issue_url}\">issue ##{issue_iid}</a>"
message = "#{user_name} #{state} #{issue_link} in #{project_link}: <b>#{title}</b>" message = "#{user_name} #{state} #{issue_link} in #{project_link}: <b>#{title}</b>"
if description message << "<pre>#{markdown(description)}</pre>"
description = format_body(description)
message << description
end
message message
end end
...@@ -159,23 +177,20 @@ class HipchatService < Service ...@@ -159,23 +177,20 @@ class HipchatService < Service
merge_request_id = obj_attr[:iid] merge_request_id = obj_attr[:iid]
state = obj_attr[:state] state = obj_attr[:state]
description = obj_attr[:description] description = obj_attr[:description]
title = obj_attr[:title] title = render_line(obj_attr[:title])
merge_request_url = "#{project_url}/merge_requests/#{merge_request_id}" merge_request_url = "#{project_url}/merge_requests/#{merge_request_id}"
merge_request_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_id}</a>" merge_request_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_id}</a>"
message = "#{user_name} #{state} #{merge_request_link} in " \ message = "#{user_name} #{state} #{merge_request_link} in " \
"#{project_link}: <b>#{title}</b>" "#{project_link}: <b>#{title}</b>"
if description message << "<pre>#{markdown(description)}</pre>"
description = format_body(description)
message << description
end
message message
end end
def format_title(title) def format_title(title)
"<b>" + title.lines.first.chomp + "</b>" "<b>#{render_line(title)}</b>"
end end
def create_note_message(data) def create_note_message(data)
...@@ -186,11 +201,13 @@ class HipchatService < Service ...@@ -186,11 +201,13 @@ class HipchatService < Service
note = obj_attr[:note] note = obj_attr[:note]
note_url = obj_attr[:url] note_url = obj_attr[:url]
noteable_type = obj_attr[:noteable_type] noteable_type = obj_attr[:noteable_type]
commit_id = nil
case noteable_type case noteable_type
when "Commit" when "Commit"
commit_attr = HashWithIndifferentAccess.new(data[:commit]) commit_attr = HashWithIndifferentAccess.new(data[:commit])
subject_desc = commit_attr[:id] commit_id = commit_attr[:id]
subject_desc = commit_id
subject_desc = Commit.truncate_sha(subject_desc) subject_desc = Commit.truncate_sha(subject_desc)
subject_type = "commit" subject_type = "commit"
title = format_title(commit_attr[:message]) title = format_title(commit_attr[:message])
...@@ -218,10 +235,7 @@ class HipchatService < Service ...@@ -218,10 +235,7 @@ class HipchatService < Service
message = "#{user_name} commented on #{subject_html} in #{project_link}: " message = "#{user_name} commented on #{subject_html} in #{project_link}: "
message << title message << title
if note message << "<pre>#{markdown(note, ref: commit_id)}</pre>"
note = format_body(note)
message << note
end
message message
end end
......
...@@ -117,7 +117,7 @@ describe HipchatService, models: true do ...@@ -117,7 +117,7 @@ describe HipchatService, models: true do
end end
context 'issue events' do context 'issue events' do
let(:issue) { create(:issue, title: 'Awesome issue', description: 'please fix') } let(:issue) { create(:issue, title: 'Awesome issue', description: '**please** fix') }
let(:issue_service) { Issues::CreateService.new(project, user) } let(:issue_service) { Issues::CreateService.new(project, user) }
let(:issues_sample_data) { issue_service.hook_data(issue, 'open') } let(:issues_sample_data) { issue_service.hook_data(issue, 'open') }
...@@ -135,12 +135,12 @@ describe HipchatService, models: true do ...@@ -135,12 +135,12 @@ describe HipchatService, models: true do
"<a href=\"#{obj_attr[:url]}\">issue ##{obj_attr["iid"]}</a> in " \ "<a href=\"#{obj_attr[:url]}\">issue ##{obj_attr["iid"]}</a> in " \
"<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
"<b>Awesome issue</b>" \ "<b>Awesome issue</b>" \
"<pre>please fix</pre>") "<pre><strong>please</strong> fix</pre>")
end end
end end
context 'merge request events' do context 'merge request events' do
let(:merge_request) { create(:merge_request, description: 'please fix', title: 'Awesome merge request', target_project: project, source_project: project) } let(:merge_request) { create(:merge_request, description: '**please** fix', title: 'Awesome merge request', target_project: project, source_project: project) }
let(:merge_service) { MergeRequests::CreateService.new(project, user) } let(:merge_service) { MergeRequests::CreateService.new(project, user) }
let(:merge_sample_data) { merge_service.hook_data(merge_request, 'open') } let(:merge_sample_data) { merge_service.hook_data(merge_request, 'open') }
...@@ -159,7 +159,7 @@ describe HipchatService, models: true do ...@@ -159,7 +159,7 @@ describe HipchatService, models: true do
"<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr["iid"]}</a> in " \ "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr["iid"]}</a> in " \
"<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
"<b>Awesome merge request</b>" \ "<b>Awesome merge request</b>" \
"<pre>please fix</pre>") "<pre><strong>please</strong> fix</pre>")
end end
end end
...@@ -203,7 +203,7 @@ describe HipchatService, models: true do ...@@ -203,7 +203,7 @@ describe HipchatService, models: true do
let(:merge_request_note) do let(:merge_request_note) do
create(:note_on_merge_request, noteable: merge_request, create(:note_on_merge_request, noteable: merge_request,
project: project, project: project,
note: "merge request note") note: "merge request **note**")
end end
it "calls Hipchat API for merge request comment events" do it "calls Hipchat API for merge request comment events" do
...@@ -222,7 +222,7 @@ describe HipchatService, models: true do ...@@ -222,7 +222,7 @@ describe HipchatService, models: true do
"<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \ "<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \
"<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
"<b>#{title}</b>" \ "<b>#{title}</b>" \
"<pre>merge request note</pre>") "<pre>merge request <strong>note</strong></pre>")
end end
end end
...@@ -230,7 +230,7 @@ describe HipchatService, models: true do ...@@ -230,7 +230,7 @@ describe HipchatService, models: true do
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
let(:issue_note) do let(:issue_note) do
create(:note_on_issue, noteable: issue, project: project, create(:note_on_issue, noteable: issue, project: project,
note: "issue note") note: "issue **note**")
end end
it "calls Hipchat API for issue comment events" do it "calls Hipchat API for issue comment events" do
...@@ -247,7 +247,7 @@ describe HipchatService, models: true do ...@@ -247,7 +247,7 @@ describe HipchatService, models: true do
"<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \ "<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \
"<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
"<b>#{title}</b>" \ "<b>#{title}</b>" \
"<pre>issue note</pre>") "<pre>issue <strong>note</strong></pre>")
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment