From 85f0229b157a26f4cd954ce5ca20588a9dae29f6 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 23 Dec 2016 11:13:30 +0100 Subject: [PATCH 1/4] Fix inconsistent return type --- app/controllers/projects/mattermosts_controller.rb | 5 ++++- .../project_services/mattermost_slash_commands_service.rb | 4 ++-- lib/mattermost/client.rb | 4 ++-- spec/controllers/projects/mattermosts_controller_spec.rb | 8 ++++---- .../mattermost_slash_commands_service_spec.rb | 5 +---- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/controllers/projects/mattermosts_controller.rb b/app/controllers/projects/mattermosts_controller.rb index 01d99c7df35..20161e1cfd8 100644 --- a/app/controllers/projects/mattermosts_controller.rb +++ b/app/controllers/projects/mattermosts_controller.rb @@ -34,7 +34,10 @@ class Projects::MattermostsController < Projects::ApplicationController end def teams - @teams ||= @service.list_teams(current_user) + @teams ||= begin + teams, error_message = @service.list_teams(current_user) + error_message ? error_message : teams + end end def service diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb index 50a011db74e..548163f38fa 100644 --- a/app/models/project_services/mattermost_slash_commands_service.rb +++ b/app/models/project_services/mattermost_slash_commands_service.rb @@ -30,8 +30,8 @@ class MattermostSlashCommandsService < ChatSlashCommandsService def list_teams(user) Mattermost::Team.new(user).all - rescue Mattermost::Error => e - [[], e.message] + rescue Mattermost::Error + [] end private diff --git a/lib/mattermost/client.rb b/lib/mattermost/client.rb index ec2903b7ec6..7552856f37a 100644 --- a/lib/mattermost/client.rb +++ b/lib/mattermost/client.rb @@ -27,12 +27,12 @@ module Mattermost end def json_response(response) - json_response = JSON.parse(response.body) - unless response.success? raise Mattermost::ClientError.new(json_response['message'] || 'Undefined error') end + json_response = JSON.parse(response.body) + json_response rescue JSON::JSONError raise Mattermost::ClientError.new('Cannot parse response') diff --git a/spec/controllers/projects/mattermosts_controller_spec.rb b/spec/controllers/projects/mattermosts_controller_spec.rb index 2ae635a1244..cae733f0cfb 100644 --- a/spec/controllers/projects/mattermosts_controller_spec.rb +++ b/spec/controllers/projects/mattermosts_controller_spec.rb @@ -13,13 +13,13 @@ describe Projects::MattermostsController do before do allow_any_instance_of(MattermostSlashCommandsService). to receive(:list_teams).and_return([]) - - get(:new, - namespace_id: project.namespace.to_param, - project_id: project.to_param) end it 'accepts the request' do + get(:new, + namespace_id: project.namespace.to_param, + project_id: project.to_param) + expect(response).to have_http_status(200) end end diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb index c879edddfdd..57ac63932e6 100644 --- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb +++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb @@ -113,10 +113,7 @@ describe MattermostSlashCommandsService, :models do end it 'shows error messages' do - teams, message = subject - - expect(teams).to be_empty - expect(message).to eq('Failed to get team list.') + expect(subject).to eq([]) end end end From 8bcc911b9bc9fcb2179860c5a98d8a1ad3ec34b0 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Tue, 24 Jan 2017 17:54:30 +0000 Subject: [PATCH 2/4] Added error message and test --- app/views/projects/mattermosts/_no_teams.html.haml | 3 +++ app/views/projects/mattermosts/new.html.haml | 2 +- .../projects/services/mattermost_slash_command_spec.rb | 9 +++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/views/projects/mattermosts/_no_teams.html.haml b/app/views/projects/mattermosts/_no_teams.html.haml index 605c7f61dee..1bb3642431e 100644 --- a/app/views/projects/mattermosts/_no_teams.html.haml +++ b/app/views/projects/mattermosts/_no_teams.html.haml @@ -1,3 +1,6 @@ += content_for :flash_message do + .alert.alert-danger= @teams if @teams.is_a?(String) + %p You aren’t a member of any team on the Mattermost instance at %strong= Gitlab.config.mattermost.host diff --git a/app/views/projects/mattermosts/new.html.haml b/app/views/projects/mattermosts/new.html.haml index 96b1d2aee61..82f596da0fe 100644 --- a/app/views/projects/mattermosts/new.html.haml +++ b/app/views/projects/mattermosts/new.html.haml @@ -2,7 +2,7 @@ .inline.pull-right = custom_icon('mattermost_logo', size: 48) %h3 Install Mattermost Command - - if @teams.empty? + - if @teams.is_a?(String) || @teams.empty? = render 'no_teams' - else = render 'team_selection' diff --git a/spec/features/projects/services/mattermost_slash_command_spec.rb b/spec/features/projects/services/mattermost_slash_command_spec.rb index 86a07b2c679..6737b85e0d1 100644 --- a/spec/features/projects/services/mattermost_slash_command_spec.rb +++ b/spec/features/projects/services/mattermost_slash_command_spec.rb @@ -99,6 +99,15 @@ feature 'Setup Mattermost slash commands', feature: true do expect(select_element.all('option').count).to eq(3) end + it 'shows an error alert with the error message if there is an error requesting teams' do + allow_any_instance_of(MattermostSlashCommandsService).to receive(:list_teams) { 'test mattermost error message' } + + click_link 'Add to Mattermost' + + expect(page).to have_selector('.alert') + expect(page).to have_content('test mattermost error message') + end + def stub_teams(count: 0) teams = create_teams(count) From b3de65bcc5627915511182a9ac9857b1fef14853 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 26 Jan 2017 09:16:07 +0100 Subject: [PATCH 3/4] Update #list_teams to propagate errors --- app/controllers/projects/mattermosts_controller.rb | 5 +---- .../project_services/mattermost_slash_commands_service.rb | 8 ++++---- app/views/projects/mattermosts/_no_teams.html.haml | 5 +++-- app/views/projects/mattermosts/new.html.haml | 2 +- lib/mattermost/client.rb | 4 ++-- .../mattermost_slash_commands_service_spec.rb | 2 +- 6 files changed, 12 insertions(+), 14 deletions(-) diff --git a/app/controllers/projects/mattermosts_controller.rb b/app/controllers/projects/mattermosts_controller.rb index 20161e1cfd8..38f7e6eb5e9 100644 --- a/app/controllers/projects/mattermosts_controller.rb +++ b/app/controllers/projects/mattermosts_controller.rb @@ -34,10 +34,7 @@ class Projects::MattermostsController < Projects::ApplicationController end def teams - @teams ||= begin - teams, error_message = @service.list_teams(current_user) - error_message ? error_message : teams - end + @teams, @teams_error_message = @service.list_teams(current_user) end def service diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb index 548163f38fa..b0f7a42f9a3 100644 --- a/app/models/project_services/mattermost_slash_commands_service.rb +++ b/app/models/project_services/mattermost_slash_commands_service.rb @@ -28,10 +28,10 @@ class MattermostSlashCommandsService < ChatSlashCommandsService [false, e.message] end - def list_teams(user) - Mattermost::Team.new(user).all - rescue Mattermost::Error - [] + def list_teams(current_user) + [Mattermost::Team.new(current_user).all, nil] + rescue Mattermost::Error => e + [[], e.message] end private diff --git a/app/views/projects/mattermosts/_no_teams.html.haml b/app/views/projects/mattermosts/_no_teams.html.haml index 1bb3642431e..aac74a25b75 100644 --- a/app/views/projects/mattermosts/_no_teams.html.haml +++ b/app/views/projects/mattermosts/_no_teams.html.haml @@ -1,5 +1,6 @@ -= content_for :flash_message do - .alert.alert-danger= @teams if @teams.is_a?(String) +- if @teams_error_message + = content_for :flash_message do + .alert.alert-danger= @teams_error_message %p You aren’t a member of any team on the Mattermost instance at diff --git a/app/views/projects/mattermosts/new.html.haml b/app/views/projects/mattermosts/new.html.haml index 82f596da0fe..96b1d2aee61 100644 --- a/app/views/projects/mattermosts/new.html.haml +++ b/app/views/projects/mattermosts/new.html.haml @@ -2,7 +2,7 @@ .inline.pull-right = custom_icon('mattermost_logo', size: 48) %h3 Install Mattermost Command - - if @teams.is_a?(String) || @teams.empty? + - if @teams.empty? = render 'no_teams' - else = render 'team_selection' diff --git a/lib/mattermost/client.rb b/lib/mattermost/client.rb index 7552856f37a..ec2903b7ec6 100644 --- a/lib/mattermost/client.rb +++ b/lib/mattermost/client.rb @@ -27,12 +27,12 @@ module Mattermost end def json_response(response) + json_response = JSON.parse(response.body) + unless response.success? raise Mattermost::ClientError.new(json_response['message'] || 'Undefined error') end - json_response = JSON.parse(response.body) - json_response rescue JSON::JSONError raise Mattermost::ClientError.new('Cannot parse response') diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb index 57ac63932e6..98f3d420c8a 100644 --- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb +++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb @@ -113,7 +113,7 @@ describe MattermostSlashCommandsService, :models do end it 'shows error messages' do - expect(subject).to eq([]) + expect(subject).to eq([[], "Failed to get team list."]) end end end From bd5245dba67b4aa4fccce819e4491259d568240b Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Mon, 30 Jan 2017 13:59:13 +0000 Subject: [PATCH 4/4] Updated test for new error result --- .../features/projects/services/mattermost_slash_command_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/projects/services/mattermost_slash_command_spec.rb b/spec/features/projects/services/mattermost_slash_command_spec.rb index 6737b85e0d1..042a1ccab51 100644 --- a/spec/features/projects/services/mattermost_slash_command_spec.rb +++ b/spec/features/projects/services/mattermost_slash_command_spec.rb @@ -100,7 +100,7 @@ feature 'Setup Mattermost slash commands', feature: true do end it 'shows an error alert with the error message if there is an error requesting teams' do - allow_any_instance_of(MattermostSlashCommandsService).to receive(:list_teams) { 'test mattermost error message' } + allow_any_instance_of(MattermostSlashCommandsService).to receive(:list_teams) { [[], 'test mattermost error message'] } click_link 'Add to Mattermost'