Merge branch 'project-runners-page' into 'master'

Improved runner page for project

Tasks:

- [x] Ability to add runner to several projects
- [x] Render runner status (online, offline, disabled)
- [x] Two column style of page: specific and shared runners
- [x] Toggle shared runners for project
- [x] Fix tests
- [x] Write tests for new functionality
- [x] Remove runner instead disabling if it is last project for this runner
- [x] Cleanup and refactor code
- [x] Improve query for authorised runners (reject duplicates)
- [x] Improve UI based on https://dev.gitlab.org/gitlab/gitlab-ci/issues/185#note_41582

See merge request !61

21 files changed, 381 insertions, 91 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 30e972c..1fe97b4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 v7.11.0
   - Deploy Jobs API calls
   - Projects search on dashboard page
+  - Improved runners page
 
 v7.10.0
   - Projects sorting by last commit date
diff --git a/app/assets/stylesheets/generic/common.scss b/app/assets/stylesheets/generic/common.scss
index 1b6a90f..0c3d058 100644
--- a/app/assets/stylesheets/generic/common.scss
+++ b/app/assets/stylesheets/generic/common.scss
@@ -74,3 +74,35 @@
     line-height: 1.6;
   }
 }
+
+/** light list with border-bottom between li **/
+ul.bordered-list {
+  margin: 5px 0px;
+  padding: 0px;
+  li {
+    padding: 5px 0;
+    border-bottom: 1px solid #EEE;
+    overflow: hidden;
+    display: block;
+    margin: 0px;
+    &:last-child { border:none }
+    &.active {
+      background: #f9f9f9;
+      a { font-weight: bold; }
+    }
+  }
+
+  &.top-list {
+    li:first-child {
+      padding-top: 0;
+      h4, h5 {
+        margin-top: 0;
+      }
+    }
+  }
+}
+
+.underlined-title {
+  border-bottom: 1px solid #ccc;
+  padding: 0 0 3px 3px;
+}
diff --git a/app/assets/stylesheets/sections/runners.scss b/app/assets/stylesheets/sections/runners.scss
index ae098fe..a9111a7 100644
--- a/app/assets/stylesheets/sections/runners.scss
+++ b/app/assets/stylesheets/sections/runners.scss
@@ -10,3 +10,25 @@
     background: #3498db;
   }
 }
+
+.runner-status-online {
+  color: green;
+}
+
+.runner-status-offline {
+  color: gray;
+}
+
+.runner-status-paused {
+  color: red;
+}
+
+.runner {
+  .btn {
+    padding: 1px 6px;
+  }
+
+  h4 {
+    font-weight: normal;
+  }
+}
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index dfeb091..f10d42b 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -2,9 +2,9 @@ class ProjectsController < ApplicationController
   PROJECTS_PER_PAGE = 100
 
   before_filter :authenticate_user!, except: [:build, :badge, :index, :show]
-  before_filter :project, only: [:build, :integration, :show, :badge, :edit, :update, :destroy]
+  before_filter :project, only: [:build, :integration, :show, :badge, :edit, :update, :destroy, :toggle_shared_runners]
   before_filter :authorize_access_project!, except: [:build, :gitlab, :badge, :index, :show, :new, :create]
-  before_filter :authorize_manage_project!, only: [:edit, :integration, :update, :destroy]
+  before_filter :authorize_manage_project!, only: [:edit, :integration, :update, :destroy, :toggle_shared_runners]
   before_filter :authenticate_token!, only: [:build]
   before_filter :no_cache, only: [:badge]
   protect_from_forgery except: :build
@@ -65,7 +65,6 @@ class ProjectsController < ApplicationController
 
   def update
     if project.update_attributes(project_params)
-
       EventService.new.change_project_settings(current_user, project)
 
       redirect_to :back, notice: 'Project was successfully updated.'
@@ -101,6 +100,11 @@ class ProjectsController < ApplicationController
     send_file image.path, filename: image.name, disposition: 'inline'
   end
 
+  def toggle_shared_runners
+    project.toggle!(:shared_runners_enabled)
+    redirect_to :back
+  end
+
   protected
 
   def project
diff --git a/app/controllers/runner_projects_controller.rb b/app/controllers/runner_projects_controller.rb
new file mode 100644
index 0000000..b27f888
--- /dev/null
+++ b/app/controllers/runner_projects_controller.rb
@@ -0,0 +1,32 @@
+class RunnerProjectsController < ApplicationController
+  before_filter :authenticate_user!
+  before_filter :project
+  before_filter :authorize_manage_project!
+
+  layout 'project'
+
+  def create
+    @runner = Runner.find(params[:runner_project][:runner_id])
+
+    return head(403) unless current_user.authorized_runners.include?(@runner)
+
+    if @runner.assign_to(project, current_user)
+      redirect_to project_runners_path(project)
+    else
+      redirect_to project_runners_path(project), alert: 'Failed adding runner to project'
+    end
+  end
+
+  def destroy
+    runner_project = project.runner_projects.find(params[:id])
+    runner_project.destroy
+
+    redirect_to project_runners_path(project)
+  end
+
+  private
+
+  def project
+    @project ||= Project.find(params[:project_id])
+  end
+end
diff --git a/app/controllers/runners_controller.rb b/app/controllers/runners_controller.rb
index 02465d6..12578b3 100644
--- a/app/controllers/runners_controller.rb
+++ b/app/controllers/runners_controller.rb
@@ -8,7 +8,11 @@ class RunnersController < ApplicationController
   layout 'project'
 
   def index
-    @runners = @project.runners.order('id DESC').page(params[:page]).per(20)
+    @runners = @project.runners.order('id DESC')
+    @specific_runners = current_user.authorized_runners.
+      where.not(id:  @runners).order('runners.id DESC').page(params[:page]).per(20)
+    @shared_runners = Runner.shared.active
+    @shared_runners_count = @shared_runners.count(:all)
   end
 
   def edit
diff --git a/app/helpers/runners_helper.rb b/app/helpers/runners_helper.rb
new file mode 100644
index 0000000..0d6e982
--- /dev/null
+++ b/app/helpers/runners_helper.rb
@@ -0,0 +1,20 @@
+module RunnersHelper
+  def runner_status_icon(runner)
+    unless runner.contacted_at
+      return content_tag :i, nil,
+        class: "icon-warning-sign",
+        title: "New runner. Has not connected yet"
+    end
+
+    status =
+      if runner.active?
+        runner.contacted_at > 3.hour.ago ? :online : :offline
+      else
+        :paused
+      end
+
+    content_tag :i, nil,
+      class: "icon-circle runner-status-#{status}",
+      title: "Runner is #{status}, last contact was #{time_ago_in_words(runner.contacted_at)} ago"
+  end
+end
diff --git a/app/models/runner.rb b/app/models/runner.rb
index bfd5a54..3747218 100644
--- a/app/models/runner.rb
+++ b/app/models/runner.rb
@@ -53,6 +53,10 @@ class Runner < ActiveRecord::Base
     is_shared
   end
 
+  def belongs_to_one_project?
+    runner_projects.count == 1
+  end
+
   def specific?
     !shared?
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 71b0c19..753717e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -65,6 +65,15 @@ class User
     end
   end
 
+  def authorized_runners
+    Runner.specific.includes(:runner_projects).
+      where(runner_projects: { project_id: authorized_projects } )
+  end
+
+  def authorized_projects
+    @authorized_projects ||= Project.where(gitlab_id: gitlab_projects.map(&:id))
+  end
+
   private
 
   def project_info(project_gitlab_id)
diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml
index 495dc6c..231bded 100644
--- a/app/views/layouts/project.html.haml
+++ b/app/views/layouts/project.html.haml
@@ -24,5 +24,4 @@
           .col-md-10
             = yield
       - else
-        .col-md-12
-          = yield
+        = yield
diff --git a/app/views/projects/_form.html.haml b/app/views/projects/_form.html.haml
index f04ca33..3702f7e 100644
--- a/app/views/projects/_form.html.haml
+++ b/app/views/projects/_form.html.haml
@@ -61,13 +61,6 @@
             = f.check_box :public
             %span.light Anyone can see project and builds
     .form-group
-      = f.label :shared_runners_enabled, 'Allow shared runners', class: 'control-label'
-      .col-sm-10
-        .checkbox
-          = f.label :shared_runners_enabled do
-            = f.check_box :shared_runners_enabled
-            %span.light Allow run builds on shared runners
-    .form-group
       = f.label :coverage_regex, "Test coverage parsing", class: 'control-label'
       .col-sm-10
         .input-group
diff --git a/app/views/runners/_runner.html.haml b/app/views/runners/_runner.html.haml
index 1c75058..4cb2879 100644
--- a/app/views/runners/_runner.html.haml
+++ b/app/views/runners/_runner.html.haml
@@ -1,37 +1,33 @@
-%tr{id: dom_id(runner)}
-  %td
-    %strong ##{runner.id}
-  %td
-    - if runner.active?
-      %span.label.label-success active
-    - else
-      %span.label.label-danger paused
-
-  %td
-    = runner.short_sha
-  %td
-    .runner-description
-      = runner.description
-  %td
-    - if build = runner.builds.last
-      = link_to "##{build.id}", [build.project,  build]
-  %td
-    - runner.tag_list.each do |tag|
-      %span.label.label-primary
-        = tag
-  %td
-    - if runner.contacted_at
-      #{time_ago_in_words(runner.contacted_at)} ago
-    - else
-      Never
-  %td
+%li.runner{id: dom_id(runner)}
+  %h4
+    = runner_status_icon(runner)
+    %span.monospace
+      = runner.short_sha
+      - if @runners.include?(runner)
+        %small
+          =link_to edit_project_runner_path(@project, runner) do
+            %i.fa.icon-edit.btn
+          
     .pull-right
-      = link_to 'Edit', edit_project_runner_path(@project, runner), class: 'btn btn-small'
-      &nbsp;
-      - if runner.active?
-        = link_to 'Pause', [:pause, @project, runner], data: { confirm: "Are you sure?" }, method: :get, class: 'btn btn-danger btn-small'
-      - else
-        = link_to 'Resume', [:resume, @project, runner], method: :get, class: 'btn btn-success btn-small'
-      - if runner.only_for?(@project)
-        = link_to 'Remove', [@project, runner], data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-small'
+      - if @runners.include?(runner)
+        - if runner.belongs_to_one_project?
+          = link_to 'Remove runner', [@project, runner], data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-small'
+        - else
+          - runner_project = @project.runner_projects.find_by(runner_id: runner)
+          = link_to 'Disable for this project', [@project, runner_project], data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-small'
+      - elsif runner.specific?
+        = form_for [@project, @project.runner_projects.new] do |f|
+          = f.hidden_field :runner_id, value: runner.id
+          = f.submit 'Enable for this project', class: 'btn btn-small'
+  .pull-right
+    %small.light
+      \##{runner.id}
+  - if runner.description.present?
+    %p.runner-description
+      = runner.description
+  - if runner.tag_list.present?
+    %p
+      - runner.tag_list.each do |tag|
+        %span.label.label-primary
+          = tag
 
diff --git a/app/views/runners/_shared_runners.html.haml b/app/views/runners/_shared_runners.html.haml
new file mode 100644
index 0000000..ae803e2
--- /dev/null
+++ b/app/views/runners/_shared_runners.html.haml
@@ -0,0 +1,23 @@
+%h3 Shared runners
+
+.bs-callout.bs-callout-warning
+  GitLab Runners do not offer secure isolation between projects that they do builds for. You are TRUSTING all GitLab users who can push code to project A, B or C to run shell scripts on the machine hosting runner X.
+  %hr
+  - if @project.shared_runners_enabled
+    = link_to toggle_shared_runners_project_path(@project), class: 'btn btn-warning', method: :post do
+      Disable shared runners
+  - else
+    = link_to toggle_shared_runners_project_path(@project), class: 'btn btn-success', method: :post do
+      Enable shared runners
+  &nbsp; for this project
+
+- if @shared_runners_count.zero?
+  This application has no shared runners yet.
+  Please use specific runners or ask administrator to create one
+- else
+  %h4.underlined-title Available shared runners - #{@shared_runners_count}
+  %ul.bordered-list.available-shared-runners
+    = render @shared_runners.first(10)
+  - if @shared_runners_count > 10
+    .light
+      and #{@shared_runners_count - 10} more...
diff --git a/app/views/runners/_specific_runners.html.haml b/app/views/runners/_specific_runners.html.haml
new file mode 100644
index 0000000..be8525e
--- /dev/null
+++ b/app/views/runners/_specific_runners.html.haml
@@ -0,0 +1,29 @@
+%h3 Specific runners
+
+.bs-callout.help-callout
+  %h4 How to setup a new project specific runner
+
+  %ol
+    %li
+      Install GitLab Runner software.
+      Checkout the #{link_to 'GitLab Runner section', 'https://about.gitlab.com/gitlab-ci/#gitlab-runner', target: '_blank'} to install it
+    %li
+      Specify following URL during runner setup:
+      %code #{root_url(only_path: false)}
+    %li
+      Use the following registration token during setup:
+      %code #{@project.token}
+    %li
+      Start runner!
+
+
+- if @runners.any?
+  %h4.underlined-title Runners activated for this project
+  %ul.bordered-list.activated-specific-runners
+    = render @runners
+
+- if @specific_runners.any?
+  %h4.underlined-title Available specific runners
+  %ul.bordered-list.available-specific-runners
+    = render @specific_runners
+  = paginate @specific_runners
diff --git a/app/views/runners/index.html.haml b/app/views/runners/index.html.haml
index 6df2e8d..529fb9c 100644
--- a/app/views/runners/index.html.haml
+++ b/app/views/runners/index.html.haml
@@ -1,26 +1,11 @@
-%p.slead
-  A 'runner' is a process which runs a build.
-  You can setup as many runners as you need.
-  %br
-  Runners can be placed on separate users, servers, and even on your local machine.
+.light
+  %p
+    A 'runner' is a process which runs a build.
+    You can setup as many runners as you need.
+    %br
+    Runners can be placed on separate users, servers, and even on your local machine.
 
-.bs-callout.help-callout
-  %h4 How to setup a new project specific runner
-
-  %ol
-    %li
-      Install GitLab Runner software.
-      Checkout the #{link_to 'GitLab Runner section', 'https://about.gitlab.com/gitlab-ci/#gitlab-runner', target: '_blank'} to install it
-    %li
-      Specify following URL during runner setup:
-      %code #{root_url(only_path: false)}
-    %li
-      Use the following registration token during setup:
-      %code #{@project.token}
-    %li
-      Start runner!
-
-  %h4 Each runner can be in one of the following states:
+  %p Each runner can be in one of the following states:
   %div
     %ul
       %li
@@ -30,17 +15,11 @@
         %span.label.label-danger paused
         \- runner is paused and will not receive any new build
 
-%table.table
-  %thead
-    %tr
-      %th ID
-      %th Type
-      %th Runner token
-      %th Description
-      %th Last build
-      %th Tags
-      %th Last contact
-      %th
+%hr
 
-  = render @runners
-= paginate @runners
+%p.lead To start serving your builds you can either add specific runners to your project or use shared runners
+.row
+  .col-sm-6
+    = render 'specific_runners'
+  .col-sm-6
+    = render 'shared_runners'
diff --git a/config/routes.rb b/config/routes.rb
index a176114..01c665a 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -19,6 +19,7 @@ Rails.application.routes.draw do
       get :status, to: 'projects#badge'
       get :integration
       post :build
+      post :toggle_shared_runners
     end
 
     resources :services, only: [:index, :edit, :update] do
@@ -58,6 +59,8 @@ Rails.application.routes.draw do
       end
     end
 
+    resources :runner_projects, only: [:create, :destroy]
+
     resources :jobs, only: [:index] do
       collection do
         get :deploy_jobs
diff --git a/spec/factories/runners.rb b/spec/factories/runners.rb
index 5fe32a6..32af2ce 100644
--- a/spec/factories/runners.rb
+++ b/spec/factories/runners.rb
@@ -16,6 +16,16 @@
 
 FactoryGirl.define do
   factory :runner do
-    token "MyString"
+    sequence :description do |n|
+      "My runner#{n}"
+    end
+
+    factory :shared_runner do
+      is_shared true
+    end
+
+    factory :specific_runner do
+      is_shared false
+    end
   end
 end
diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb
index 888b202..36b7fab 100644
--- a/spec/features/runners_spec.rb
+++ b/spec/features/runners_spec.rb
@@ -3,15 +3,73 @@ require 'spec_helper'
 describe "Runners" do
   before do
     login_as :user
-    @project = FactoryGirl.create :project
   end
 
-  describe "GET /projects/:id/runners" do
+  describe "specific runners" do
     before do
+      @project = FactoryGirl.create :project
+      @project2 = FactoryGirl.create :project
+      stub_js_gitlab_calls
+
+      @shared_runner = FactoryGirl.create :shared_runner
+      @specific_runner = FactoryGirl.create :specific_runner
+      @specific_runner2 = FactoryGirl.create :specific_runner
+      @project.runners << @specific_runner
+      @project2.runners << @specific_runner2
+    end
+
+    it "places runners in right places" do
+      visit project_runners_path(@project)
+      page.find(".available-specific-runners").should have_content(@specific_runner2.display_name)
+      page.find(".activated-specific-runners").should have_content(@specific_runner.display_name)
+      page.find(".available-shared-runners").should have_content(@shared_runner.display_name)
+    end
+
+    it "enables specific runner for project" do
+      visit project_runners_path(@project)
+
+      within ".available-specific-runners" do
+        click_on "Enable for this project"
+      end
+
+      page.find(".activated-specific-runners").should have_content(@specific_runner2.display_name)
+    end
+
+    it "disables specific runner for project" do
+      @project2.runners << @specific_runner
+
       visit project_runners_path(@project)
+
+      within ".activated-specific-runners" do
+        click_on "Disable for this project"
+      end
+
+      page.find(".available-specific-runners").should have_content(@specific_runner.display_name)
     end
 
-    it { page.should have_content @project.name }
-    it { page.should have_content 'How to setup a new project specific runner' }
+    it "removes specific runner for project if this is last project for that runners" do
+      visit project_runners_path(@project)
+
+      within ".activated-specific-runners" do
+        click_on "Remove runner"
+      end
+
+      Runner.exists?(id: @specific_runner).should be_false
+    end
+  end
+
+  describe "shared runners" do
+    before do
+      @project = FactoryGirl.create :project
+      stub_js_gitlab_calls
+    end
+
+    it "enables shared runners" do
+      visit project_runners_path(@project)
+
+      click_on "Enable shared runners"
+
+      @project.reload.shared_runners_enabled.should be_true
+    end
   end
 end
diff --git a/spec/helpers/runners_helper_spec.rb b/spec/helpers/runners_helper_spec.rb
new file mode 100644
index 0000000..02d497b
--- /dev/null
+++ b/spec/helpers/runners_helper_spec.rb
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe RunnersHelper do
+  it "returns - not contacted yet" do
+    runner = FactoryGirl.build :runner
+    runner_status_icon(runner).should include("not connected yet")
+  end
+
+  it "returns offline text" do
+    runner = FactoryGirl.build(:runner, contacted_at: 1.day.ago, active: true)
+    runner_status_icon(runner).should include("Runner is offline")
+  end
+
+  it "returns online text" do
+    runner = FactoryGirl.build(:runner, contacted_at: 1.hour.ago, active: true)
+    runner_status_icon(runner).should include("Runner is online")
+  end
+end
diff --git a/spec/models/runner_spec.rb b/spec/models/runner_spec.rb
index 5440e9e..6b90367 100644
--- a/spec/models/runner_spec.rb
+++ b/spec/models/runner_spec.rb
@@ -22,8 +22,8 @@ describe Runner do
     end
 
     it 'should return the token if it does not have a description' do
-      runner = FactoryGirl.build(:runner)
-      expect(runner.display_name).to eq runner.token
+      runner = FactoryGirl.create(:runner)
+      expect(runner.display_name).to eq runner.description
     end
 
     it 'should return the token if the description is an empty string' do
@@ -34,7 +34,7 @@ describe Runner do
 
   describe :assign_to do
     let!(:project) { FactoryGirl.create :project }
-    let!(:shared_runner) { FactoryGirl.create(:runner, is_shared: true) }
+    let!(:shared_runner) { FactoryGirl.create(:shared_runner) }
 
     before { shared_runner.assign_to(project) }
 
@@ -42,4 +42,24 @@ describe Runner do
     it { shared_runner.projects.should == [project] }
     it { shared_runner.only_for?(project).should be_true }
   end
+
+  describe "belongs_to_one_project?" do
+    it "returns false if there are two projects runner assigned to" do
+      runner = FactoryGirl.create(:specific_runner)
+      project = FactoryGirl.create(:project)
+      project1 = FactoryGirl.create(:project)
+      project.runners << runner
+      project1.runners << runner
+
+      runner.belongs_to_one_project?.should be_false
+    end
+
+    it "returns true" do
+      runner = FactoryGirl.create(:specific_runner)
+      project = FactoryGirl.create(:project)
+      project.runners << runner
+
+      runner.belongs_to_one_project?.should be_true
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 44b260b..4a1e393 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -51,4 +51,38 @@ describe User do
       @user.has_developer_access?(1).should be_true
     end
   end
+
+  describe "authorized_projects" do
+    it "returns projects" do
+      project = FactoryGirl.create :project, gitlab_id: 1
+      project1 = FactoryGirl.create :project, gitlab_id: 2
+      gitlab_project = OpenStruct.new({id: 1})
+      gitlab_project1 = OpenStruct.new({id: 2})
+      User.any_instance.stub(:gitlab_projects).and_return([gitlab_project, gitlab_project1])
+      user = User.new({})
+
+      user.authorized_projects.count.should == 2
+    end
+  end
+
+  describe "authorized_runners" do
+    it "returns authorized runners" do
+      project = FactoryGirl.create :project, gitlab_id: 1
+      project1 = FactoryGirl.create :project, gitlab_id: 2
+      gitlab_project = OpenStruct.new({id: 1})
+      gitlab_project1 = OpenStruct.new({id: 2})
+      User.any_instance.stub(:gitlab_projects).and_return([gitlab_project, gitlab_project1])
+      user = User.new({})
+
+      runner = FactoryGirl.create :specific_runner
+      runner1 = FactoryGirl.create :specific_runner
+      runner2 = FactoryGirl.create :specific_runner
+
+      project.runners << runner
+      project1.runners << runner1
+
+      user.authorized_runners.should include(runner, runner1)
+      user.authorized_runners.should_not include(runner2)
+    end
+  end
 end