Merge branch '299758-improve-edit-view-of-runners-in-the-admin-area' into 'master'

Move the Runner edit page to an `/edit` path

See merge request gitlab-org/gitlab!77514

app/controllers/admin/runner_projects_controller.rb

@@ -9,9 +9,9 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
     @runner = Ci::Runner.find(params[:runner_project][:runner_id])
 
     if @runner.assign_to(@project, current_user)
-      redirect_to admin_runner_path(@runner), notice: s_('Runners|Runner assigned to project.')
+      redirect_to edit_admin_runner_url(@runner), notice: s_('Runners|Runner assigned to project.')
     else
-      redirect_to admin_runner_path(@runner), alert: 'Failed adding runner to project'
+      redirect_to edit_admin_runner_url(@runner), alert: 'Failed adding runner to project'
     end
   end
 
@@ -20,7 +20,7 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
     runner = rp.runner
     rp.destroy
 
-    redirect_to admin_runner_path(runner), status: :found, notice: s_('Runners|Runner unassigned from project.')
+    redirect_to edit_admin_runner_url(runner), status: :found, notice: s_('Runners|Runner unassigned from project.')
   end
 
   private

app/controllers/admin/runners_controller.rb

@@ -11,13 +11,21 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def show
+    # We will show runner details in a read-only view in
+    # future iterations. For now, this route will have a
+    # redirect until this new view is developed. See more:
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/347856
+    redirect_to edit_admin_runner_path(runner)
+  end
+
+  def edit
     assign_builds_and_projects
   end
 
   def update
     if Ci::UpdateRunnerService.new(@runner).update(runner_params)
       respond_to do |format|
-        format.html { redirect_to admin_runner_path(@runner) }
+        format.html { redirect_to edit_admin_runner_path(@runner) }
       end
     else
       assign_builds_and_projects

app/views/admin/runners/show.html.haml → app/views/admin/runners/edit.html.haml

 - add_page_specific_style 'page_bundles/ci_status'
 
-- breadcrumb_title @runner.short_sha
-- page_title "##{@runner.id} (#{@runner.short_sha})"
+- title = "##{@runner.id} (#{@runner.short_sha})"
+- breadcrumb_title title
+- page_title title
 - add_to_breadcrumbs _('Runners'), admin_runners_path
 
 #js-runner-details{ data: {runner_id: @runner.id} }
@@ -37,7 +38,7 @@
 
       %tr
         %td
-          = form_tag admin_runner_path(@runner), id: 'runner-projects-search', class: 'form-inline', method: :get do
+          = form_tag edit_admin_runner_path(@runner), id: 'runner-projects-search', class: 'form-inline', method: :get do
             .input-group
               = search_field_tag :search, params[:search], class: 'form-control gl-form-input', spellcheck: false
               .input-group-append

config/routes/admin.rb

@@ -159,7 +159,7 @@ namespace :admin do
 
   resources :labels
 
-  resources :runners, only: [:index, :show, :update, :destroy] do
+  resources :runners, only: [:index, :show, :edit, :update, :destroy] do
     member do
       post :resume
       post :pause

spec/controllers/admin/runner_projects_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Admin::RunnerProjectsController do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+
+  before do
+    sign_in(create(:admin))
+  end
+
+  describe '#create' do
+    let(:project_id) { project.path }
+
+    subject do
+      post :create, params: {
+        namespace_id: group.path,
+        project_id: project_id,
+        runner_project: { runner_id: project_runner.id }
+      }
+    end
+
+    context 'assigning runner to same project' do
+      let(:project_runner) { create(:ci_runner, :project, projects: [project]) }
+
+      it 'redirects to the admin runner edit page' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to redirect_to edit_admin_runner_url(project_runner)
+      end
+    end
+
+    context 'assigning runner to another project' do
+      let(:project_runner) { create(:ci_runner, :project, projects: [source_project]) }
+      let(:source_project) { create(:project) }
+
+      it 'redirects to the admin runner edit page' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to redirect_to edit_admin_runner_url(project_runner)
+      end
+    end
+
+    context 'for unknown project' do
+      let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project]) }
+
+      let(:project_id) { 0 }
+
+      it 'shows 404 for unknown project' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+end

spec/controllers/admin/runners_controller_spec.rb

@@ -25,6 +25,23 @@ RSpec.describe Admin::RunnersController do
   describe '#show' do
     render_views
 
+    let_it_be(:project) { create(:project) }
+
+    before_all do
+      create(:ci_build, runner: runner, project: project)
+    end
+
+    it 'redirects to the runner edit page' do
+      get :show, params: { id: runner.id }
+
+      expect(response).to have_gitlab_http_status(:redirect)
+      expect(response).to redirect_to edit_admin_runner_path(runner)
+    end
+  end
+
+  describe '#edit' do
+    render_views
+
     let_it_be(:project) { create(:project) }
     let_it_be(:project_two) { create(:project) }
 
@@ -33,29 +50,29 @@ RSpec.describe Admin::RunnersController do
       create(:ci_build, runner: runner, project: project_two)
     end
 
-    it 'shows a particular runner' do
-      get :show, params: { id: runner.id }
+    it 'shows a runner edit page' do
+      get :edit, params: { id: runner.id }
 
       expect(response).to have_gitlab_http_status(:ok)
     end
 
     it 'shows 404 for unknown runner' do
-      get :show, params: { id: 0 }
+      get :edit, params: { id: 0 }
 
       expect(response).to have_gitlab_http_status(:not_found)
     end
 
     it 'avoids N+1 queries', :request_store do
-      get :show, params: { id: runner.id }
+      get :edit, params: { id: runner.id }
 
-      control_count = ActiveRecord::QueryRecorder.new { get :show, params: { id: runner.id } }.count
+      control_count = ActiveRecord::QueryRecorder.new { get :edit, params: { id: runner.id } }.count
 
       new_project = create(:project)
       create(:ci_build, runner: runner, project: new_project)
 
       # There is one additional query looking up subject.group in ProjectPolicy for the
       # needs_new_sso_session permission
-      expect { get :show, params: { id: runner.id } }.not_to exceed_query_limit(control_count + 1)
+      expect { get :edit, params: { id: runner.id } }.not_to exceed_query_limit(control_count + 1)
 
       expect(response).to have_gitlab_http_status(:ok)
     end

spec/features/admin/admin_runners_spec.rb

@@ -449,19 +449,21 @@ RSpec.describe "Admin Runners" do
     end
   end
 
-  describe "Runner show page" do
+  describe "Runner edit page" do
     let(:runner) { create(:ci_runner) }
 
     before do
       @project1 = create(:project)
       @project2 = create(:project)
-      visit admin_runner_path(runner)
+      visit edit_admin_runner_path(runner)
+
+      wait_for_requests
     end
 
     describe 'runner page breadcrumbs' do
-      it 'contains the current runner token' do
+      it 'contains the current runner id and token' do
         page.within '[data-testid="breadcrumb-links"]' do
-          expect(page.find('h2')).to have_content(runner.short_sha)
+          expect(page.find('h2')).to have_content("##{runner.id} (#{runner.short_sha})")
         end
       end
     end
@@ -510,7 +512,7 @@ RSpec.describe "Admin Runners" do
         let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
 
         before do
-          visit admin_runner_path(runner)
+          visit edit_admin_runner_path(runner)
         end
 
         it_behaves_like 'assignable runner'
@@ -520,7 +522,7 @@ RSpec.describe "Admin Runners" do
         let(:runner) { create(:ci_runner, :project, projects: [@project1], locked: true) }
 
         before do
-          visit admin_runner_path(runner)
+          visit edit_admin_runner_path(runner)
         end
 
         it_behaves_like 'assignable runner'
@@ -531,7 +533,7 @@ RSpec.describe "Admin Runners" do
 
         before do
           @project1.destroy!
-          visit admin_runner_path(runner)
+          visit edit_admin_runner_path(runner)
         end
 
         it_behaves_like 'assignable runner'
@@ -542,7 +544,7 @@ RSpec.describe "Admin Runners" do
       let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
 
       before do
-        visit admin_runner_path(runner)
+        visit edit_admin_runner_path(runner)
       end
 
       it 'removed specific runner from project' do