From b45ee2c314e2c26f4574f2e973dfa40204860c66 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Wed, 16 Dec 2015 10:08:05 -0400 Subject: [PATCH 01/12] better support for referencing and closing issues in asana_service.rb --- app/models/project_services/asana_service.rb | 28 +++++++++++++------ .../project_services/asana_service_spec.rb | 21 ++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index e6e16058d41..bbc508e8f8e 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -98,17 +98,29 @@ automatically inspected. Leave blank to include all branches.' task_list = [] close_list = [] - message.split("\n").each do |line| - # look for a task ID or a full Asana url - task_list.concat(line.scan(/#(\d+)/)) - task_list.concat(line.scan(/https:\/\/app\.asana\.com\/\d+\/\d+\/(\d+)/)) - # look for a word starting with 'fix' followed by a task ID - close_list.concat(line.scan(/(fix\w*)\W*#(\d+)/i)) + # matches either: + # - #1234 + # - https://app.asana.com/0/0/1234 + # optionally preceded with: + # - fix/ed/es/ing + # - close/s/d + # - closing + issue_finder = /(fix\w*|clos[ei]\w*+)?\W*(?:https:\/\/app\.asana\.com\/\d+\/\d+\/(\d+)|#(\d+))/i + + message.scan(issue_finder).each do |tuple| + # tuple will be + # [ 'fix', 'id_from_url', 'id_from_pound' ] + taskid = tuple[2] || tuple[1] + task_list.push(taskid) + + if tuple[0] + close_list.push(taskid) + end end # post commit to every taskid found task_list.each do |taskid| - task = Asana::Task.find(taskid[0]) + task = Asana::Task.find(taskid) if task task.create_story(text: push_msg + ' ' + message) @@ -117,7 +129,7 @@ automatically inspected. Leave blank to include all branches.' # close all tasks that had 'fix(ed/es/ing) #:id' in them close_list.each do |taskid| - task = Asana::Task.find(taskid.last) + task = Asana::Task.find(taskid) if task task.modify(completed: true) diff --git a/spec/models/project_services/asana_service_spec.rb b/spec/models/project_services/asana_service_spec.rb index 64bb92fba95..e368b03206e 100644 --- a/spec/models/project_services/asana_service_spec.rb +++ b/spec/models/project_services/asana_service_spec.rb @@ -62,5 +62,26 @@ describe AsanaService, models: true do @asana.check_commit('fix #456789', 'pushed') end + + it 'should be able to close via url' do + expect(Asana::Task).to receive(:find).with('42').twice + + @asana.check_commit('closes https://app.asana.com/19292/956299/42', 'pushed') + end + + it 'should allow multiple matches per line' do + expect(Asana::Task).to receive(:find).with('123').twice + expect(Asana::Task).to receive(:find).with('456').twice + expect(Asana::Task).to receive(:find).with('789').once + + expect(Asana::Task).to receive(:find).with('42').once + expect(Asana::Task).to receive(:find).with('12').twice + + message = <<-EOF + minor bigfix, refactoring, fixed #123 and Closes #456 work on #789 + ref https://app.asana.com/19292/956299/42 and closing https://app.asana.com/19292/956299/12 + EOF + @asana.check_commit(message, 'pushed') + end end end From 2cd2c54bd1c83f8545b5f903d7717c4848453f0c Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Tue, 29 Dec 2015 15:37:48 -0400 Subject: [PATCH 02/12] Update Asana field descriptions --- app/models/project_services/asana_service.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index bbc508e8f8e..a16adf10432 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -53,14 +53,12 @@ http://developer.asana.com/documentation/#api_keys' { type: 'text', name: 'api_key', - placeholder: 'User API token. User must have access to task, -all comments will be attributed to this user.' + placeholder: 'User Personal Access Token. User must have access to task, all comments will be attributed to this user.' }, { type: 'text', name: 'restrict_to_branch', - placeholder: 'Comma-separated list of branches which will be -automatically inspected. Leave blank to include all branches.' + placeholder: 'Comma-separated list of branches which will be automatically inspected. Leave blank to include all branches.' } ] end From 12ec1e3c407a4b72f670483a558a9cb95449c838 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Tue, 29 Dec 2015 15:39:58 -0400 Subject: [PATCH 03/12] Update Asana service to work with Personal Access Token, lessen number of requests to Asana API --- app/models/project_services/asana_service.rb | 58 ++++++++------------ 1 file changed, 24 insertions(+), 34 deletions(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index a16adf10432..111a60431b1 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -67,35 +67,34 @@ http://developer.asana.com/documentation/#api_keys' %w(push) end + def client + @_client ||= begin + Asana::Client.new do |c| + c.authentication :access_token, api_key + end + end + end + def execute(data) return unless supported_events.include?(data[:object_kind]) - Asana.configure do |client| - client.api_key = api_key - end - - user = data[:user_name] - branch = Gitlab::Git.ref_name(data[:ref]) - - branch_restriction = restrict_to_branch.to_s - # check the branch restriction is poplulated and branch is not included + branch = Gitlab::Git.ref_name(data[:ref]) + branch_restriction = restrict_to_branch.to_s if branch_restriction.length > 0 && branch_restriction.index(branch).nil? return end + user = data[:user_name] project_name = project.name_with_namespace - push_msg = user + ' pushed to branch ' + branch + ' of ' + project_name + push_msg = "#{user} pushed to branch #{branch} of #{project_name} ( #{commit[:url]} )" data[:commits].each do |commit| - check_commit(' ( ' + commit[:url] + ' ): ' + commit[:message], push_msg) + check_commit(commit[:message], push_msg) end end def check_commit(message, push_msg) - task_list = [] - close_list = [] - # matches either: # - #1234 # - https://app.asana.com/0/0/1234 @@ -109,28 +108,19 @@ http://developer.asana.com/documentation/#api_keys' # tuple will be # [ 'fix', 'id_from_url', 'id_from_pound' ] taskid = tuple[2] || tuple[1] - task_list.push(taskid) + + begin + task = Asana::Task.find_by_id(client, taskid) + rescue Exception => e + puts e.message + puts e.backtrace.inspect + next + end + + task.add_comment(text: "#{push_msg} #{message}") if tuple[0] - close_list.push(taskid) - end - end - - # post commit to every taskid found - task_list.each do |taskid| - task = Asana::Task.find(taskid) - - if task - task.create_story(text: push_msg + ' ' + message) - end - end - - # close all tasks that had 'fix(ed/es/ing) #:id' in them - close_list.each do |taskid| - task = Asana::Task.find(taskid) - - if task - task.modify(completed: true) + task.update(completed: true) end end end From 4c1f1c2c91a1a43f0ee38fc0f5e8c92dea996f04 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Tue, 29 Dec 2015 15:40:50 -0400 Subject: [PATCH 04/12] Update Asana specs --- .../project_services/asana_service_spec.rb | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/spec/models/project_services/asana_service_spec.rb b/spec/models/project_services/asana_service_spec.rb index e368b03206e..306d18171be 100644 --- a/spec/models/project_services/asana_service_spec.rb +++ b/spec/models/project_services/asana_service_spec.rb @@ -52,30 +52,54 @@ describe AsanaService, models: true do end it 'should call Asana service to created a story' do - expect(Asana::Task).to receive(:find).with('123456').once + d1 = double('Asana::Task', add_comment: true) + expect(d1).to receive(:add_comment) + expect(Asana::Task).to receive(:find_by_id).with(anything, '123456').once.and_return(d1) @asana.check_commit('related to #123456', 'pushed') end it 'should call Asana service to created a story and close a task' do - expect(Asana::Task).to receive(:find).with('456789').twice + d1 = double('Asana::Task', add_comment: true) + expect(d1).to receive(:add_comment) + expect(d1).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '456789').once.and_return(d1) @asana.check_commit('fix #456789', 'pushed') end it 'should be able to close via url' do - expect(Asana::Task).to receive(:find).with('42').twice + d1 = double('Asana::Task', add_comment: true) + expect(d1).to receive(:add_comment) + expect(d1).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '42').once.and_return(d1) @asana.check_commit('closes https://app.asana.com/19292/956299/42', 'pushed') end it 'should allow multiple matches per line' do - expect(Asana::Task).to receive(:find).with('123').twice - expect(Asana::Task).to receive(:find).with('456').twice - expect(Asana::Task).to receive(:find).with('789').once + d1 = double('Asana::Task', add_comment: true) + expect(d1).to receive(:add_comment) + expect(d1).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '123').once.and_return(d1) - expect(Asana::Task).to receive(:find).with('42').once - expect(Asana::Task).to receive(:find).with('12').twice + d2 = double('Asana::Task', add_comment: true) + expect(d2).to receive(:add_comment) + expect(d2).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '456').once.and_return(d2) + + d3 = double('Asana::Task', add_comment: true) + expect(d3).to receive(:add_comment) + expect(Asana::Task).to receive(:find_by_id).with(anything, '789').once.and_return(d3) + + d4 = double('Asana::Task', add_comment: true) + expect(d4).to receive(:add_comment) + expect(Asana::Task).to receive(:find_by_id).with(anything, '42').once.and_return(d4) + + d5 = double('Asana::Task', add_comment: true) + expect(d5).to receive(:add_comment) + expect(d5).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '12').once.and_return(d5) message = <<-EOF minor bigfix, refactoring, fixed #123 and Closes #456 work on #789 From e8080fc8fb216fb6da7d3f056e8b4417f807d09e Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Tue, 29 Dec 2015 16:00:36 -0400 Subject: [PATCH 05/12] Fix error in Asana service --- app/models/project_services/asana_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index 111a60431b1..80c56b9097b 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -87,9 +87,9 @@ http://developer.asana.com/documentation/#api_keys' user = data[:user_name] project_name = project.name_with_namespace - push_msg = "#{user} pushed to branch #{branch} of #{project_name} ( #{commit[:url]} )" data[:commits].each do |commit| + push_msg = "#{user} pushed to branch #{branch} of #{project_name} ( #{commit[:url]} )" check_commit(commit[:message], push_msg) end end From 6eb273a11cf7521448f4bcb5f9376df24c2f9f3f Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Wed, 30 Dec 2015 00:50:44 -0400 Subject: [PATCH 06/12] Restore colon in Asana comment --- app/models/project_services/asana_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index 80c56b9097b..183ce2df787 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -89,7 +89,7 @@ http://developer.asana.com/documentation/#api_keys' project_name = project.name_with_namespace data[:commits].each do |commit| - push_msg = "#{user} pushed to branch #{branch} of #{project_name} ( #{commit[:url]} )" + push_msg = "#{user} pushed to branch #{branch} of #{project_name} ( #{commit[:url]} ):" check_commit(commit[:message], push_msg) end end From 7b98d0e1a24645df802ad65911c290ad057d1422 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Wed, 30 Dec 2015 00:51:05 -0400 Subject: [PATCH 07/12] Update Asana service documentation --- app/models/project_services/asana_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index 183ce2df787..ab5772356f1 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -40,8 +40,8 @@ get the commit comment added to it. You can also close a task with a message containing: `fix #123456`. -You can find your Api Keys here: -http://developer.asana.com/documentation/#api_keys' +You can create a Personal Access Token here: +http://app.asana.com/-/account_api' end def to_param From 9e7a88f089323964088945829523b798ea6b78b5 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Wed, 30 Dec 2015 00:52:56 -0400 Subject: [PATCH 08/12] Better handling of errors in Asana service [ci skip] --- app/models/project_services/asana_service.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index ab5772356f1..cb4f6ddb3a5 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -111,17 +111,16 @@ http://app.asana.com/-/account_api' begin task = Asana::Task.find_by_id(client, taskid) - rescue Exception => e - puts e.message - puts e.backtrace.inspect + task.add_comment(text: "#{push_msg} #{message}") + + if tuple[0] + task.update(completed: true) + end + rescue => e + Rails.logger.error(e.message) + Rails.logger.error(e.backtrace.join("\n")) next end - - task.add_comment(text: "#{push_msg} #{message}") - - if tuple[0] - task.update(completed: true) - end end end end From 90029a5caaef1fd9d41a8ac02a7e9840ce3ac7b5 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Thu, 31 Dec 2015 18:27:34 -0400 Subject: [PATCH 09/12] Actually test the posted comment in Asana service --- .../project_services/asana_service_spec.rb | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/spec/models/project_services/asana_service_spec.rb b/spec/models/project_services/asana_service_spec.rb index 306d18171be..a7b32ac07a9 100644 --- a/spec/models/project_services/asana_service_spec.rb +++ b/spec/models/project_services/asana_service_spec.rb @@ -40,6 +40,20 @@ describe AsanaService, models: true do let(:user) { create(:user) } let(:project) { create(:project) } + def create_data_for_commits(*messages) + data = { + object_kind: 'push', + ref: 'master', + user_name: user.name, + commits: messages.map do |m| + { + message: m, + url: 'https://gitlab.com/', + } + end + } + end + before do @asana = AsanaService.new allow(@asana).to receive_messages( @@ -51,12 +65,15 @@ describe AsanaService, models: true do ) end - it 'should call Asana service to created a story' do - d1 = double('Asana::Task', add_comment: true) - expect(d1).to receive(:add_comment) + it 'should call Asana service to create a story' do + data = create_data_for_commits('Message from commit. related to #123456') + expected_message = "#{data[:user_name]} pushed to branch #{data[:ref]} of #{project.name_with_namespace} ( #{data[:commits][0][:url]} ): #{data[:commits][0][:message]}" + + d1 = double('Asana::Task') + expect(d1).to receive(:add_comment).with(text: expected_message) expect(Asana::Task).to receive(:find_by_id).with(anything, '123456').once.and_return(d1) - @asana.check_commit('related to #123456', 'pushed') + @asana.execute(data) end it 'should call Asana service to created a story and close a task' do From 571df5f44bfec89b21bdce0f91f9acfdda6d7660 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Thu, 31 Dec 2015 18:29:00 -0400 Subject: [PATCH 10/12] Use `execute` in Asana specs --- .../project_services/asana_service_spec.rb | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/spec/models/project_services/asana_service_spec.rb b/spec/models/project_services/asana_service_spec.rb index a7b32ac07a9..0db48c75d1d 100644 --- a/spec/models/project_services/asana_service_spec.rb +++ b/spec/models/project_services/asana_service_spec.rb @@ -76,53 +76,56 @@ describe AsanaService, models: true do @asana.execute(data) end - it 'should call Asana service to created a story and close a task' do - d1 = double('Asana::Task', add_comment: true) + it 'should call Asana service to create a story and close a task' do + data = create_data_for_commits('fix #456789') + d1 = double('Asana::Task') expect(d1).to receive(:add_comment) expect(d1).to receive(:update).with(completed: true) expect(Asana::Task).to receive(:find_by_id).with(anything, '456789').once.and_return(d1) - @asana.check_commit('fix #456789', 'pushed') + @asana.execute(data) end it 'should be able to close via url' do - d1 = double('Asana::Task', add_comment: true) + data = create_data_for_commits('closes https://app.asana.com/19292/956299/42') + d1 = double('Asana::Task') expect(d1).to receive(:add_comment) expect(d1).to receive(:update).with(completed: true) expect(Asana::Task).to receive(:find_by_id).with(anything, '42').once.and_return(d1) - @asana.check_commit('closes https://app.asana.com/19292/956299/42', 'pushed') + @asana.execute(data) end it 'should allow multiple matches per line' do - d1 = double('Asana::Task', add_comment: true) - expect(d1).to receive(:add_comment) - expect(d1).to receive(:update).with(completed: true) - expect(Asana::Task).to receive(:find_by_id).with(anything, '123').once.and_return(d1) - - d2 = double('Asana::Task', add_comment: true) - expect(d2).to receive(:add_comment) - expect(d2).to receive(:update).with(completed: true) - expect(Asana::Task).to receive(:find_by_id).with(anything, '456').once.and_return(d2) - - d3 = double('Asana::Task', add_comment: true) - expect(d3).to receive(:add_comment) - expect(Asana::Task).to receive(:find_by_id).with(anything, '789').once.and_return(d3) - - d4 = double('Asana::Task', add_comment: true) - expect(d4).to receive(:add_comment) - expect(Asana::Task).to receive(:find_by_id).with(anything, '42').once.and_return(d4) - - d5 = double('Asana::Task', add_comment: true) - expect(d5).to receive(:add_comment) - expect(d5).to receive(:update).with(completed: true) - expect(Asana::Task).to receive(:find_by_id).with(anything, '12').once.and_return(d5) - message = <<-EOF minor bigfix, refactoring, fixed #123 and Closes #456 work on #789 ref https://app.asana.com/19292/956299/42 and closing https://app.asana.com/19292/956299/12 EOF - @asana.check_commit(message, 'pushed') + data = create_data_for_commits(message) + d1 = double('Asana::Task') + expect(d1).to receive(:add_comment) + expect(d1).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '123').once.and_return(d1) + + d2 = double('Asana::Task') + expect(d2).to receive(:add_comment) + expect(d2).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '456').once.and_return(d2) + + d3 = double('Asana::Task') + expect(d3).to receive(:add_comment) + expect(Asana::Task).to receive(:find_by_id).with(anything, '789').once.and_return(d3) + + d4 = double('Asana::Task') + expect(d4).to receive(:add_comment) + expect(Asana::Task).to receive(:find_by_id).with(anything, '42').once.and_return(d4) + + d5 = double('Asana::Task') + expect(d5).to receive(:add_comment) + expect(d5).to receive(:update).with(completed: true) + expect(Asana::Task).to receive(:find_by_id).with(anything, '12').once.and_return(d5) + + @asana.execute(data) end end end From bb6b793c55150ecbe072456bc4b151191764b642 Mon Sep 17 00:00:00 2001 From: Mike Wyatt Date: Mon, 4 Jan 2016 11:19:39 -0400 Subject: [PATCH 11/12] Don't log backtrace in Asana service --- app/models/project_services/asana_service.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index cb4f6ddb3a5..7d367e40037 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -118,7 +118,6 @@ http://app.asana.com/-/account_api' end rescue => e Rails.logger.error(e.message) - Rails.logger.error(e.backtrace.join("\n")) next end end From 0bab4788ef870945feccbb102834fd89433dfef2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 5 Jan 2016 16:31:05 +0100 Subject: [PATCH 12/12] Satisfy Rubocop --- spec/models/project_services/asana_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/project_services/asana_service_spec.rb b/spec/models/project_services/asana_service_spec.rb index 0db48c75d1d..f3d15f3c1ea 100644 --- a/spec/models/project_services/asana_service_spec.rb +++ b/spec/models/project_services/asana_service_spec.rb @@ -41,7 +41,7 @@ describe AsanaService, models: true do let(:project) { create(:project) } def create_data_for_commits(*messages) - data = { + { object_kind: 'push', ref: 'master', user_name: user.name,