diff options
author | Rémy Coutable <remy@rymai.me> | 2016-10-20 14:51:41 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-10-20 14:51:41 +0000 |
commit | b101e92bde99098bff6ff05d9bd190fe77f1d1af (patch) | |
tree | c6b8f8245df383480126a82d33eb287186eeb4e7 | |
parent | 030c82267fab507999f914b9d9e88651f1b7966a (diff) | |
parent | db7c227fc64de0221d061d58387a93c6925cded7 (diff) | |
download | gitlab-ce-b101e92bde99098bff6ff05d9bd190fe77f1d1af.tar.gz |
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
-rw-r--r-- | CHANGELOG.md | 1 | ||||
-rw-r--r-- | app/models/project_services/hipchat_service.rb | 58 | ||||
-rw-r--r-- | spec/models/project_services/hipchat_service_spec.rb | 16 |
3 files changed, 45 insertions, 30 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f163f323e6..0bfdf23b959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) - 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 ## 8.13.0 (2016-10-22) diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index afebd3b6a12..660a8ae3421 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -1,5 +1,12 @@ class HipchatService < Service + include ActionView::Helpers::SanitizeHelper + 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 boolean_accessor :notify_only_broken_builds @@ -88,6 +95,10 @@ class HipchatService < Service end end + def render_line(text) + markdown(text.lines.first.chomp, pipeline: :single_line) if text + end + def create_push_message(push) ref_type = Gitlab::Git.tag_ref?(push[:ref]) ? 'tag' : 'branch' ref = Gitlab::Git.ref_name(push[:ref]) @@ -110,7 +121,7 @@ class HipchatService < Service message << "(<a href=\"#{project.web_url}/compare/#{before}...#{after}\">Compare changes</a>)" 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 if push[:commits].count > MAX_COMMITS @@ -121,12 +132,22 @@ class HipchatService < Service message end - def format_body(body) - if body - body = body.truncate(200, separator: ' ', omission: '...') - end + def markdown(text, options = {}) + return "" unless text + + 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 def create_issue_message(data) @@ -134,7 +155,7 @@ class HipchatService < Service obj_attr = data[:object_attributes] obj_attr = HashWithIndifferentAccess.new(obj_attr) - title = obj_attr[:title] + title = render_line(obj_attr[:title]) state = obj_attr[:state] issue_iid = obj_attr[:iid] issue_url = obj_attr[:url] @@ -143,10 +164,7 @@ class HipchatService < Service issue_link = "<a href=\"#{issue_url}\">issue ##{issue_iid}</a>" message = "#{user_name} #{state} #{issue_link} in #{project_link}: <b>#{title}</b>" - if description - description = format_body(description) - message << description - end + message << "<pre>#{markdown(description)}</pre>" message end @@ -159,23 +177,20 @@ class HipchatService < Service merge_request_id = obj_attr[:iid] state = obj_attr[:state] 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_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_id}</a>" message = "#{user_name} #{state} #{merge_request_link} in " \ "#{project_link}: <b>#{title}</b>" - if description - description = format_body(description) - message << description - end + message << "<pre>#{markdown(description)}</pre>" message end def format_title(title) - "<b>" + title.lines.first.chomp + "</b>" + "<b>#{render_line(title)}</b>" end def create_note_message(data) @@ -186,11 +201,13 @@ class HipchatService < Service note = obj_attr[:note] note_url = obj_attr[:url] noteable_type = obj_attr[:noteable_type] + commit_id = nil case noteable_type when "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_type = "commit" title = format_title(commit_attr[:message]) @@ -218,10 +235,7 @@ class HipchatService < Service message = "#{user_name} commented on #{subject_html} in #{project_link}: " message << title - if note - note = format_body(note) - message << note - end + message << "<pre>#{markdown(note, ref: commit_id)}</pre>" message end diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb index 26dd95bdfec..2da3a9cb09f 100644 --- a/spec/models/project_services/hipchat_service_spec.rb +++ b/spec/models/project_services/hipchat_service_spec.rb @@ -117,7 +117,7 @@ describe HipchatService, models: true do end 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(:issues_sample_data) { issue_service.hook_data(issue, 'open') } @@ -135,12 +135,12 @@ describe HipchatService, models: true do "<a href=\"#{obj_attr[:url]}\">issue ##{obj_attr["iid"]}</a> in " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<b>Awesome issue</b>" \ - "<pre>please fix</pre>") + "<pre><strong>please</strong> fix</pre>") end end 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_sample_data) { merge_service.hook_data(merge_request, 'open') } @@ -159,7 +159,7 @@ describe HipchatService, models: true do "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr["iid"]}</a> in " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<b>Awesome merge request</b>" \ - "<pre>please fix</pre>") + "<pre><strong>please</strong> fix</pre>") end end @@ -203,7 +203,7 @@ describe HipchatService, models: true do let(:merge_request_note) do create(:note_on_merge_request, noteable: merge_request, project: project, - note: "merge request note") + note: "merge request **note**") end it "calls Hipchat API for merge request comment events" do @@ -222,7 +222,7 @@ describe HipchatService, models: true do "<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<b>#{title}</b>" \ - "<pre>merge request note</pre>") + "<pre>merge request <strong>note</strong></pre>") end end @@ -230,7 +230,7 @@ describe HipchatService, models: true do let(:issue) { create(:issue, project: project) } let(:issue_note) do create(:note_on_issue, noteable: issue, project: project, - note: "issue note") + note: "issue **note**") end it "calls Hipchat API for issue comment events" do @@ -247,7 +247,7 @@ describe HipchatService, models: true do "<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \ "<a href=\"#{project.web_url}\">#{project_name}</a>: " \ "<b>#{title}</b>" \ - "<pre>issue note</pre>") + "<pre>issue <strong>note</strong></pre>") end end |