Improved team selection

Review changes

7 files changed, 53 insertions, 27 deletions

diff --git a/app/assets/stylesheets/framework/layout.scss b/app/assets/stylesheets/framework/layout.scss
index 29d55c44699..0a42b17c1f5 100644
--- a/app/assets/stylesheets/framework/layout.scss
+++ b/app/assets/stylesheets/framework/layout.scss
@@ -8,6 +8,19 @@ body {
   &.navless {
     background-color: $white-light !important;
   }
+
+  &.card-content {
+    background-color: $gray-darker;
+
+    .content-wrapper {
+      padding: 0;
+
+      .container-fluid,
+      .container-limited {
+        background-color: $gray-darker;
+      }
+    }
+  }
 }
 
 .container {
diff --git a/app/helpers/mattermost_helper.rb b/app/helpers/mattermost_helper.rb
index 49ac12db832..27ff4051c8d 100644
--- a/app/helpers/mattermost_helper.rb
+++ b/app/helpers/mattermost_helper.rb
@@ -1,9 +1,7 @@
 module MattermostHelper
   def mattermost_teams_options(teams)
-    teams_options = teams.map do |id, options|
-      [options['display_name'] || options['name'], id]
+    teams.map do |team|
+      [team['display_name'] || team['name'], team['id']]
     end
-
-    teams_options.compact.unshift(['Select team...', '0'])
   end
 end
diff --git a/app/views/layouts/application.html.haml b/app/views/layouts/application.html.haml
index 19bd9b6d5c9..36543edc040 100644
--- a/app/views/layouts/application.html.haml
+++ b/app/views/layouts/application.html.haml
@@ -1,7 +1,7 @@
 !!! 5
 %html{ lang: "en", class: "#{page_class}" }
   = render "layouts/head"
-  %body{ data: { page: body_data_page, project: "#{@project.path if @project}", group: "#{@group.path if @group}" } }
+  %body{ class: @body_class, data: { page: body_data_page, project: "#{@project.path if @project}", group: "#{@group.path if @group}" } }
     = Gon::Base.render_data
 
     = render "layouts/header/default", title: header_title
diff --git a/app/views/projects/mattermosts/_team_selection.html.haml b/app/views/projects/mattermosts/_team_selection.html.haml
index a80f9aa4c4a..04bd4e8b683 100644
--- a/app/views/projects/mattermosts/_team_selection.html.haml
+++ b/app/views/projects/mattermosts/_team_selection.html.haml
@@ -2,16 +2,15 @@
   This service will be installed on the Mattermost instance at
   %strong= link_to Gitlab.config.mattermost.host, Gitlab.config.mattermost.host
 %hr
-= form_for(:mattermost, method: :post, url: namespace_project_mattermost_path(@project.namespace, @project)) do |f|
+= form_for(:mattermost, method: :post, url: namespace_project_mattermost_path(@project.namespace, @project), html: { class: 'js-requires-input'} ) do |f|
   %h4 Team
   %p
     = @teams.one? ? 'The team' : 'Select the team'
     where the slash commands will be used in
-  - selected_id = @teams.one? ? @teams.keys.first : 0
-  - options = mattermost_teams_options(@teams)
-  - options = options_for_select(options, selected_id)
-  = f.select(:team_id, options, {}, { class: 'form-control', disabled: @teams.one?, selected: selected_id })
-  = f.hidden_field(:team_id, value: selected_id) if @teams.one?
+  - selected_id = @teams.one? ? @teams.first['id'] : nil
+  - options = options_for_select(mattermost_teams_options(@teams), selected_id)
+  = f.select(:team_id, options, { include_blank: 'Select team...'}, { class: 'form-control', disabled: @teams.one?, selected: selected_id, required: true })
+  = f.hidden_field(:team_id, value: selected_id, required: true) if @teams.one?
   .help-block
     - if @teams.one?
       This is the only available team.
@@ -25,7 +24,7 @@
   %hr
   %h4 Command trigger word
   %p Choose the word that will trigger commands
-  = f.text_field(:trigger, value: @project.path, class: 'form-control')
+  = f.text_field(:trigger, value: @project.path, class: 'form-control', required: true)
   .help-block
     %p
       Trigger word must be unique, and can't begin with a slash or contain any spaces.
diff --git a/app/views/projects/mattermosts/new.html.haml b/app/views/projects/mattermosts/new.html.haml
index 96b1d2aee61..15829a3f143 100644
--- a/app/views/projects/mattermosts/new.html.haml
+++ b/app/views/projects/mattermosts/new.html.haml
@@ -1,3 +1,5 @@
+- @body_class = 'card-content'
+
 .service-installation
   .inline.pull-right
     = custom_icon('mattermost_logo', size: 48)
diff --git a/lib/mattermost/team.rb b/lib/mattermost/team.rb
index 09dfd082b3a..afc152aa02e 100644
--- a/lib/mattermost/team.rb
+++ b/lib/mattermost/team.rb
@@ -1,7 +1,7 @@
 module Mattermost
   class Team < Client
     def all
-      session_get('/api/v3/teams/all')
+      session_get('/api/v3/teams/all').values
     end
   end
 end
diff --git a/spec/features/projects/services/mattermost_slash_command_spec.rb b/spec/features/projects/services/mattermost_slash_command_spec.rb
index f5adb53a2dc..24d22a092d4 100644
--- a/spec/features/projects/services/mattermost_slash_command_spec.rb
+++ b/spec/features/projects/services/mattermost_slash_command_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-feature 'Setup Mattermost slash commands', feature: true do
+feature 'Setup Mattermost slash commands', :feature, :js do
   let(:user) { create(:user) }
   let(:project) { create(:empty_project) }
   let(:service) { project.create_mattermost_slash_commands_service }
@@ -62,11 +62,11 @@ feature 'Setup Mattermost slash commands', feature: true do
 
       click_link 'Add to Mattermost'
 
-      team_name = teams.first[1]['display_name']
-      select_element = find('select#mattermost_team_id')
+      team_name = teams.first['display_name']
+      select_element = find('#mattermost_team_id')
       selected_option = select_element.find('option[selected]')
 
-      expect(select_element['disabled']).to eq('disabled')
+      expect(select_element['disabled']).to be(true)
       expect(selected_option).to have_content(team_name.to_s)
     end
 
@@ -75,7 +75,7 @@ feature 'Setup Mattermost slash commands', feature: true do
 
       click_link 'Add to Mattermost'
 
-      expect(find('input#mattermost_team_id', visible: false).value).to eq(teams.first[0].to_s)
+      expect(find('input#mattermost_team_id', visible: false).value).to eq(teams.first['id'])
     end
 
     it 'shows an explanation user is a member of multiple teams' do
@@ -92,12 +92,9 @@ feature 'Setup Mattermost slash commands', feature: true do
 
       click_link 'Add to Mattermost'
 
-      select_element = find('select#mattermost_team_id')
-      selected_option = select_element.find('option[selected]')
+      select_element = find('#mattermost_team_id')
 
-      expect(select_element['disabled']).to be(nil)
-      expect(selected_option).to have_content('Select team...')
-      # The 'Select team...' placeholder is item `0`.
+      expect(select_element['disabled']).to be(false)
       expect(select_element.all('option').count).to eq(3)
     end
 
@@ -110,20 +107,37 @@ feature 'Setup Mattermost slash commands', feature: true do
       expect(page).to have_content('test mattermost error message')
     end
 
+    it 'enables the submit button if the required fields are provided', :js do
+      stub_teams(count: 1)
+
+      click_link 'Add to Mattermost'
+
+      expect(find('input[type="submit"]')['disabled']).not_to be(true)
+    end
+
+    it 'disables the submit button if the required fields are not provided', :js do
+      stub_teams(count: 1)
+
+      click_link 'Add to Mattermost'
+
+      fill_in('mattermost_trigger', with: '')
+
+      expect(find('input[type="submit"]')['disabled']).to be(true)
+    end
+
     def stub_teams(count: 0)
       teams = create_teams(count)
 
-      allow_any_instance_of(MattermostSlashCommandsService).to receive(:list_teams) { teams }
+      allow_any_instance_of(MattermostSlashCommandsService).to receive(:list_teams) { [teams, nil] }
 
       teams
     end
 
     def create_teams(count = 0)
-      teams = {}
+      teams = []
 
       count.times do |i|
-        i += 1
-        teams[i] = { id: i, display_name: i }
+        teams.push({ "id" => "x#{i}", "display_name" => "x#{i}-name" })
       end
 
       teams