From ad14ed5e49a956511ae8f4d3bf377457fdb1075d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 6 Mar 2015 08:31:49 -0800 Subject: [PATCH] Add tag_push event notification to HipChat and Slack services. Normalize output to use: - User name instead of username - Include first line of title in message description - Link to "Issue #X" instead of "#X" --- CHANGELOG | 6 +-- .../project_services/hipchat_service.rb | 38 +++++++++------- app/models/project_services/slack_service.rb | 4 +- .../slack_service/issue_message.rb | 8 ++-- .../slack_service/merge_message.rb | 14 ++++-- .../slack_service/note_message.rb | 12 ++--- .../slack_service/push_message.rb | 19 +++++--- .../project_services/hipchat_service_spec.rb | 45 +++++++++++++++---- .../slack_service/issue_message_spec.rb | 11 ++--- .../slack_service/merge_message_spec.rb | 13 +++--- .../slack_service/note_message_spec.rb | 8 ++-- .../slack_service/push_message_spec.rb | 20 +++++++++ 12 files changed, 134 insertions(+), 64 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 611c6c77d54..c0d38a9abf0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,8 +1,9 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.9.0 (unreleased) - - Added comment notification events to HipChat and Slack services (Stan Hu) - - Added issue and merge request events to HipChat and Slack services (Stan Hu) + - Add tag push notifications and normalize HipChat and Slack messages to be consistent (Stan Hu) + - Add comment notification events to HipChat and Slack services (Stan Hu) + - Add issue and merge request events to HipChat and Slack services (Stan Hu) - Fix merge request URL passed to Webhooks. (Stan Hu) - Fix bug that caused a server error when editing a comment to "+1" or "-1" (Stan Hu) - Move labels/milestones tabs to sidebar @@ -35,7 +36,6 @@ v 7.8.2 - Fix response of push to repository to return "Not found" if user doesn't have access - Fix check if user is allowed to view the file attachment - Fix import check for case sensetive namespaces - - Added issue and merge request events to Slack service (Stan Hu) - Increase timeout for Git-over-HTTP requests to 1 hour since large pulls/pushes can take a long time. v 7.8.1 diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index d24351a7b13..90ba7e080f1 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -45,7 +45,7 @@ def fields end def supported_events - %w(push issue merge_request note) + %w(push issue merge_request note tag_push) end def execute(data) @@ -67,7 +67,7 @@ def create_message(data) message = \ case object_kind - when "push" + when "push", "tag_push" create_push_message(data) when "issue" create_issue_message(data) unless is_update?(data) @@ -79,21 +79,27 @@ def create_message(data) end def create_push_message(push) - ref = push[:ref].gsub("refs/heads/", "") + if push[:ref].starts_with?('refs/tags/') + ref_type = 'tag' + ref = push[:ref].gsub('refs/tags/', '') + else + ref_type = 'branch' + ref = push[:ref].gsub('refs/heads/', '') + end + before = push[:before] after = push[:after] message = "" message << "#{push[:user_name]} " if before.include?('000000') - message << "pushed new branch #{ref}"\ - " to "\ - "#{project_url}\n" + " to #{project_link}\n" elsif after.include?('000000') - message << "removed branch #{ref} from #{project.name_with_namespace.gsub!(/\s/,'')} \n" + message << "removed #{ref_type} #{ref} from #{project_name} \n" else - message << "pushed to branch #{ref} " message << "of #{project.name_with_namespace.gsub!(/\s/,'')} " message << "(Compare changes)" @@ -119,7 +125,7 @@ def format_body(body) end def create_issue_message(data) - username = data[:user][:username] + user_name = data[:user][:name] obj_attr = data[:object_attributes] obj_attr = HashWithIndifferentAccess.new(obj_attr) @@ -129,8 +135,8 @@ def create_issue_message(data) issue_url = obj_attr[:url] description = obj_attr[:description] - issue_link = "##{issue_iid}" - message = "#{username} #{state} issue #{issue_link} in #{project_link}: #{title}" + issue_link = "issue ##{issue_iid}" + message = "#{user_name} #{state} #{issue_link} in #{project_link}: #{title}" if description description = format_body(description) @@ -141,7 +147,7 @@ def create_issue_message(data) end def create_merge_request_message(data) - username = data[:user][:username] + user_name = data[:user][:name] obj_attr = data[:object_attributes] obj_attr = HashWithIndifferentAccess.new(obj_attr) @@ -153,8 +159,8 @@ def create_merge_request_message(data) title = obj_attr[:title] merge_request_url = "#{project_url}/merge_requests/#{merge_request_id}" - merge_request_link = "##{merge_request_id}" - message = "#{username} #{state} merge request #{merge_request_link} in " \ + merge_request_link = "merge request ##{merge_request_id}" + message = "#{user_name} #{state} #{merge_request_link} in " \ "#{project_link}: #{title}" if description @@ -171,7 +177,7 @@ def format_title(title) def create_note_message(data) data = HashWithIndifferentAccess.new(data) - username = data[:user][:username] + user_name = data[:user][:name] repo_attr = HashWithIndifferentAccess.new(data[:repository]) @@ -208,7 +214,7 @@ def create_note_message(data) end subject_html = "#{subject_type} #{subject_desc}" - message = "#{username} commented on #{subject_html} in #{project_link}: " + message = "#{user_name} commented on #{subject_html} in #{project_link}: " message << title if note diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index a58840116f4..36d9874edd3 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -44,7 +44,7 @@ def fields end def supported_events - %w(push issue merge_request note) + %w(push issue merge_request note tag_push) end def execute(data) @@ -64,7 +64,7 @@ def execute(data) message = \ case object_kind - when "push" + when "push", "tag_push" PushMessage.new(data) when "issue" IssueMessage.new(data) unless is_update?(data) diff --git a/app/models/project_services/slack_service/issue_message.rb b/app/models/project_services/slack_service/issue_message.rb index e2fed0bb1b9..5af24a80609 100644 --- a/app/models/project_services/slack_service/issue_message.rb +++ b/app/models/project_services/slack_service/issue_message.rb @@ -1,6 +1,6 @@ class SlackService class IssueMessage < BaseMessage - attr_reader :username + attr_reader :user_name attr_reader :title attr_reader :project_name attr_reader :project_url @@ -11,7 +11,7 @@ class IssueMessage < BaseMessage attr_reader :description def initialize(params) - @username = params[:user][:username] + @user_name = params[:user][:name] @project_name = params[:project_name] @project_url = params[:project_url] @@ -34,7 +34,7 @@ def attachments private def message - "#{username} #{state} issue #{issue_link} in #{project_link}: #{title}" + "#{user_name} #{state} #{issue_link} in #{project_link}: *#{title}*" end def opened_issue? @@ -50,7 +50,7 @@ def project_link end def issue_link - "[##{issue_iid}](#{issue_url})" + "[issue ##{issue_iid}](#{issue_url})" end end end diff --git a/app/models/project_services/slack_service/merge_message.rb b/app/models/project_services/slack_service/merge_message.rb index 4dcce1d15a0..e792c258f73 100644 --- a/app/models/project_services/slack_service/merge_message.rb +++ b/app/models/project_services/slack_service/merge_message.rb @@ -1,15 +1,16 @@ class SlackService class MergeMessage < BaseMessage - attr_reader :username + attr_reader :user_name attr_reader :project_name attr_reader :project_url attr_reader :merge_request_id attr_reader :source_branch attr_reader :target_branch attr_reader :state + attr_reader :title def initialize(params) - @username = params[:user][:username] + @user_name = params[:user][:name] @project_name = params[:project_name] @project_url = params[:project_url] @@ -19,6 +20,7 @@ def initialize(params) @source_branch = obj_attr[:source_branch] @target_branch = obj_attr[:target_branch] @state = obj_attr[:state] + @title = format_title(obj_attr[:title]) end def pretext @@ -31,6 +33,10 @@ def attachments private + def format_title(title) + '*' + title.lines.first.chomp + '*' + end + def message merge_request_message end @@ -40,11 +46,11 @@ def project_link end def merge_request_message - "#{username} #{state} merge request #{merge_request_link} in #{project_link}" + "#{user_name} #{state} #{merge_request_link} in #{project_link}: #{title}" end def merge_request_link - "[##{merge_request_id}](#{merge_request_url})" + "[merge request ##{merge_request_id}](#{merge_request_url})" end def merge_request_url diff --git a/app/models/project_services/slack_service/note_message.rb b/app/models/project_services/slack_service/note_message.rb index f93dc358f61..074478b292d 100644 --- a/app/models/project_services/slack_service/note_message.rb +++ b/app/models/project_services/slack_service/note_message.rb @@ -1,7 +1,7 @@ class SlackService class NoteMessage < BaseMessage attr_reader :message - attr_reader :username + attr_reader :user_name attr_reader :project_name attr_reader :project_link attr_reader :note @@ -10,7 +10,7 @@ class NoteMessage < BaseMessage def initialize(params) params = HashWithIndifferentAccess.new(params) - @username = params[:user][:username] + @user_name = params[:user][:name] @project_name = params[:project_name] @project_url = params[:project_url] @@ -47,28 +47,28 @@ def create_commit_note(commit) commit_sha = Commit.truncate_sha(commit_sha) commit_link = "[commit #{commit_sha}](#{@note_url})" title = format_title(commit[:message]) - @message = "#{@username} commented on #{commit_link} in #{project_link}: *#{title}*" + @message = "#{@user_name} commented on #{commit_link} in #{project_link}: *#{title}*" end def create_issue_note(issue) issue_iid = issue[:iid] note_link = "[issue ##{issue_iid}](#{@note_url})" title = format_title(issue[:title]) - @message = "#{@username} commented on #{note_link} in #{project_link}: *#{title}*" + @message = "#{@user_name} commented on #{note_link} in #{project_link}: *#{title}*" end def create_merge_note(merge_request) merge_request_id = merge_request[:iid] merge_request_link = "[merge request ##{merge_request_id}](#{@note_url})" title = format_title(merge_request[:title]) - @message = "#{@username} commented on #{merge_request_link} in #{project_link}: *#{title}*" + @message = "#{@user_name} commented on #{merge_request_link} in #{project_link}: *#{title}*" end def create_snippet_note(snippet) snippet_id = snippet[:id] snippet_link = "[snippet ##{snippet_id}](#{@note_url})" title = format_title(snippet[:title]) - @message = "#{@username} commented on #{snippet_link} in #{project_link}: *#{title}*" + @message = "#{@user_name} commented on #{snippet_link} in #{project_link}: *#{title}*" end def description_message diff --git a/app/models/project_services/slack_service/push_message.rb b/app/models/project_services/slack_service/push_message.rb index 2e566bc317b..3dc2df04764 100644 --- a/app/models/project_services/slack_service/push_message.rb +++ b/app/models/project_services/slack_service/push_message.rb @@ -6,7 +6,8 @@ class PushMessage < BaseMessage attr_reader :project_name attr_reader :project_url attr_reader :ref - attr_reader :username + attr_reader :ref_type + attr_reader :user_name def initialize(params) @after = params[:after] @@ -14,8 +15,14 @@ def initialize(params) @commits = params.fetch(:commits, []) @project_name = params[:project_name] @project_url = params[:project_url] - @ref = params[:ref].gsub('refs/heads/', '') - @username = params[:user_name] + if params[:ref].starts_with?('refs/tags/') + @ref_type = 'tag' + @ref = params[:ref].gsub('refs/tags/', '') + else + @ref_type = 'branch' + @ref = params[:ref].gsub('refs/heads/', '') + end + @user_name = params[:user_name] end def pretext @@ -45,15 +52,15 @@ def format(string) end def new_branch_message - "#{username} pushed new branch #{branch_link} to #{project_link}" + "#{user_name} pushed new #{ref_type} #{branch_link} to #{project_link}" end def removed_branch_message - "#{username} removed branch #{ref} from #{project_link}" + "#{user_name} removed #{ref_type} #{ref} from #{project_link}" end def push_message - "#{username} pushed to branch #{branch_link} of #{project_link} (#{compare_link})" + "#{user_name} pushed to #{ref_type} #{branch_link} of #{project_link} (#{compare_link})" end def commit_messages diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb index 95ce4f8e4aa..b9f2bee148d 100644 --- a/spec/models/project_services/hipchat_service_spec.rb +++ b/spec/models/project_services/hipchat_service_spec.rb @@ -50,6 +50,35 @@ expect(WebMock).to have_requested(:post, api_url).once end + + it "should create a push message" do + message = hipchat.send(:create_push_message, push_sample_data) + + obj_attr = push_sample_data[:object_attributes] + branch = push_sample_data[:ref].gsub('refs/heads/', '') + expect(message).to include("#{user.name} pushed to branch " \ + "#{branch} of " \ + "#{project_name}") + end + end + + context 'tag_push events' do + let(:push_sample_data) { Gitlab::PushDataBuilder.build(project, user, '000000', '111111', 'refs/tags/test', []) } + + it "should call Hipchat API for tag push events" do + hipchat.execute(push_sample_data) + + expect(WebMock).to have_requested(:post, api_url).once + end + + it "should create a tag push message" do + message = hipchat.send(:create_push_message, push_sample_data) + + obj_attr = push_sample_data[:object_attributes] + expect(message).to eq("#{user.name} pushed new tag " \ + "test to " \ + "#{project_name}\n") + end end context 'issue events' do @@ -67,8 +96,8 @@ message = hipchat.send(:create_issue_message, issues_sample_data) obj_attr = issues_sample_data[:object_attributes] - expect(message).to eq("#{user.username} opened issue " \ - "##{obj_attr["iid"]} in " \ + expect(message).to eq("#{user.name} opened " \ + "issue ##{obj_attr["iid"]} in " \ "#{project_name}: " \ "Awesome issue" \ "
please fix
") @@ -91,8 +120,8 @@ merge_sample_data) obj_attr = merge_sample_data[:object_attributes] - expect(message).to eq("#{user.username} opened merge request " \ - "##{obj_attr["iid"]} in " \ + expect(message).to eq("#{user.name} opened " \ + "merge request ##{obj_attr["iid"]} in " \ "#{project_name}: " \ "Awesome merge request" \ "
please fix
") @@ -122,7 +151,7 @@ commit_id = Commit.truncate_sha(data[:commit][:id]) title = hipchat.send(:format_title, data[:commit][:message]) - expect(message).to eq("#{user.username} commented on " \ + expect(message).to eq("#{user.name} commented on " \ "commit #{commit_id} in " \ "#{project_name}: " \ "#{title}" \ @@ -141,7 +170,7 @@ merge_id = data[:merge_request]['iid'] title = data[:merge_request]['title'] - expect(message).to eq("#{user.username} commented on " \ + expect(message).to eq("#{user.name} commented on " \ "merge request ##{merge_id} in " \ "#{project_name}: " \ "#{title}" \ @@ -158,7 +187,7 @@ issue_id = data[:issue]['iid'] title = data[:issue]['title'] - expect(message).to eq("#{user.username} commented on " \ + expect(message).to eq("#{user.name} commented on " \ "issue ##{issue_id} in " \ "#{project_name}: " \ "#{title}" \ @@ -177,7 +206,7 @@ snippet_id = data[:snippet]['id'] title = data[:snippet]['title'] - expect(message).to eq("#{user.username} commented on " \ + expect(message).to eq("#{user.name} commented on " \ "snippet ##{snippet_id} in " \ "#{project_name}: " \ "#{title}" \ diff --git a/spec/models/project_services/slack_service/issue_message_spec.rb b/spec/models/project_services/slack_service/issue_message_spec.rb index a23a7cc068e..8bca1fef44c 100644 --- a/spec/models/project_services/slack_service/issue_message_spec.rb +++ b/spec/models/project_services/slack_service/issue_message_spec.rb @@ -6,7 +6,8 @@ let(:args) { { user: { - username: 'username' + name: 'Test User', + username: 'Test User' }, project_name: 'project_name', project_url: 'somewhere.com', @@ -29,8 +30,8 @@ context 'open' do it 'returns a message regarding opening of issues' do expect(subject.pretext).to eq( - 'username opened issue in : '\ - 'Issue title') + 'Test User opened in : '\ + '*Issue title*') expect(subject.attachments).to eq([ { text: "issue description", @@ -47,8 +48,8 @@ end it 'returns a message regarding closing of issues' do expect(subject.pretext). to eq( - 'username closed issue in : '\ - 'Issue title') + 'Test User closed in : '\ + '*Issue title*') expect(subject.attachments).to be_empty end end diff --git a/spec/models/project_services/slack_service/merge_message_spec.rb b/spec/models/project_services/slack_service/merge_message_spec.rb index 25d03cd8736..aeb408aa766 100644 --- a/spec/models/project_services/slack_service/merge_message_spec.rb +++ b/spec/models/project_services/slack_service/merge_message_spec.rb @@ -6,13 +6,14 @@ let(:args) { { user: { - username: 'username' + name: 'Test User', + username: 'Test User' }, project_name: 'project_name', project_url: 'somewhere.com', object_attributes: { - title: 'Issue title', + title: "Issue title\nSecond line", id: 10, iid: 100, assignee_id: 1, @@ -30,8 +31,8 @@ context 'open' do it 'returns a message regarding opening of merge requests' do expect(subject.pretext).to eq( - 'username opened merge request '\ - 'in ') + 'Test User opened '\ + 'in : *Issue title*') expect(subject.attachments).to be_empty end end @@ -42,8 +43,8 @@ end it 'returns a message regarding closing of merge requests' do expect(subject.pretext).to eq( - 'username closed merge request '\ - 'in ') + 'Test User closed '\ + 'in : *Issue title*') expect(subject.attachments).to be_empty end end diff --git a/spec/models/project_services/slack_service/note_message_spec.rb b/spec/models/project_services/slack_service/note_message_spec.rb index f2516c10008..21fb575480b 100644 --- a/spec/models/project_services/slack_service/note_message_spec.rb +++ b/spec/models/project_services/slack_service/note_message_spec.rb @@ -37,7 +37,7 @@ it 'returns a message regarding notes on commits' do message = SlackService::NoteMessage.new(@args) - expect(message.pretext).to eq("username commented on " \ + expect(message.pretext).to eq("Test User commented on " \ " in : " \ "*Added a commit message*") expected_attachments = [ @@ -62,7 +62,7 @@ end it 'returns a message regarding notes on a merge request' do message = SlackService::NoteMessage.new(@args) - expect(message.pretext).to eq("username commented on " \ + expect(message.pretext).to eq("Test User commented on " \ " in : " \ "*merge request title*") expected_attachments = [ @@ -89,7 +89,7 @@ it 'returns a message regarding notes on an issue' do message = SlackService::NoteMessage.new(@args) expect(message.pretext).to eq( - "username commented on " \ + "Test User commented on " \ " in : " \ "*issue title*") expected_attachments = [ @@ -114,7 +114,7 @@ it 'returns a message regarding notes on a project snippet' do message = SlackService::NoteMessage.new(@args) - expect(message.pretext).to eq("username commented on " \ + expect(message.pretext).to eq("Test User commented on " \ " in : " \ "*snippet title*") expected_attachments = [ diff --git a/spec/models/project_services/slack_service/push_message_spec.rb b/spec/models/project_services/slack_service/push_message_spec.rb index ef0e7a6ee30..3ef065459d8 100644 --- a/spec/models/project_services/slack_service/push_message_spec.rb +++ b/spec/models/project_services/slack_service/push_message_spec.rb @@ -39,6 +39,26 @@ end end + context 'tag push' do + let(:args) { + { + after: 'after', + before: '000000', + project_name: 'project_name', + ref: 'refs/tags/new_tag', + user_name: 'user_name', + project_url: 'url' + } + } + + it 'returns a message regarding pushes' do + expect(subject.pretext).to eq('user_name pushed new tag ' \ + ' to ' \ + '') + expect(subject.attachments).to be_empty + end + end + context 'new branch' do before do args[:before] = '000000' -- GitLab