From d9601892e3f4e7eebe5874bbb17472fd1e8994c2 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Mon, 13 Jun 2022 21:17:52 -0400
Subject: [PATCH 1/4] Backend support for manipulating web-hook variables

Hook controllers accept new parameters to set url variables.

Changelog: changed
---
 app/controllers/admin/hooks_controller.rb     | 15 ++++---
 .../concerns/integrations/hooks_execution.rb  | 20 ++++++++++
 app/controllers/projects/hooks_controller.rb  | 10 +----
 ee/app/controllers/groups/hooks_controller.rb | 18 +++------
 .../groups/hooks_controller_spec.rb           | 34 +++++++++++-----
 .../admin/hooks_controller_spec.rb            | 36 +++++++++++++++--
 .../projects/hooks_controller_spec.rb         | 39 +++++++++++++++++--
 7 files changed, 129 insertions(+), 43 deletions(-)

diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb
index 6f5475a4a787c1..fc4bb915462857 100644
--- a/app/controllers/admin/hooks_controller.rb
+++ b/app/controllers/admin/hooks_controller.rb
@@ -14,7 +14,7 @@ def index
   end
 
   def create
-    @hook = SystemHook.new(hook_params.to_h)
+    @hook = SystemHook.new(hook_params)
 
     if @hook.save
       redirect_to admin_hooks_path, notice: _('Hook was successfully created.')
@@ -60,12 +60,11 @@ def hook_logs
     @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
-  def hook_params
-    params.require(:hook).permit(
-      :enable_ssl_verification,
-      :token,
-      :url,
-      *SystemHook.triggers.values
-    )
+  def hook_param_names
+    %i[enable_ssl_verification token url]
+  end
+
+  def trigger_values
+    SystemHook.triggers.values
   end
 end
diff --git a/app/controllers/concerns/integrations/hooks_execution.rb b/app/controllers/concerns/integrations/hooks_execution.rb
index 6a9d3d51f9bf21..751498a2774818 100644
--- a/app/controllers/concerns/integrations/hooks_execution.rb
+++ b/app/controllers/concerns/integrations/hooks_execution.rb
@@ -5,6 +5,26 @@ module Integrations::HooksExecution
 
   private
 
+  def hook_params
+    permitted = hook_param_names + trigger_values
+    permitted << { url_variables: [:key, :value] }
+
+    ps = params.require(:hook).permit(*permitted)
+
+    ps[:url_variables] = ps[:url_variables].to_h { [_1[:key], _1[:value].presence] } if ps.key?(:url_variables)
+
+    if action_name == 'update' && ps.key?(:url_variables)
+      supplied = ps.permit(url_variables: {})[:url_variables].to_h
+      ps[:url_variables] = hook.url_variables.merge(supplied).compact
+    end
+
+    ps
+  end
+
+  def hook_param_names
+    %i[enable_ssl_verification token url push_events_branch_filter]
+  end
+
   def destroy_hook(hook)
     result = WebHooks::DestroyService.new(current_user).execute(hook)
 
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 99eba32e00faf5..e54beab1a2d004 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -69,13 +69,7 @@ def hook_logs
     @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
-  def hook_params
-    params.require(:hook).permit(
-      :enable_ssl_verification,
-      :token,
-      :url,
-      :push_events_branch_filter,
-      *ProjectHook.triggers.values
-    )
+  def trigger_values
+    ProjectHook.triggers.values
   end
 end
diff --git a/ee/app/controllers/groups/hooks_controller.rb b/ee/app/controllers/groups/hooks_controller.rb
index 58dd5dd6fcb5b3..a4ca2bef2946fe 100644
--- a/ee/app/controllers/groups/hooks_controller.rb
+++ b/ee/app/controllers/groups/hooks_controller.rb
@@ -7,7 +7,7 @@ class Groups::HooksController < Groups::ApplicationController
   before_action :group
   before_action :authorize_admin_group!
   before_action :check_group_webhooks_available!
-  before_action :set_hook, only: [:edit, :update, :test, :destroy]
+  before_action :hook, only: [:edit, :update, :test, :destroy]
   before_action -> { check_rate_limit!(:group_testing_hook, scope: [@group, current_user]) }, only: :test
 
   respond_to :html
@@ -38,7 +38,7 @@ def edit
   end
 
   def update
-    if @hook.update(hook_params)
+    if hook.update(hook_params)
       flash[:notice] = _('Hook was successfully updated.')
       redirect_to group_hooks_path(@group)
     else
@@ -48,7 +48,7 @@ def update
 
   def test
     if @group.first_non_empty_project
-      service = TestHooks::ProjectService.new(@hook, current_user, params[:trigger] || 'push_events')
+      service = TestHooks::ProjectService.new(hook, current_user, params[:trigger] || 'push_events')
       service.project = @group.first_non_empty_project
       result = service.execute
 
@@ -68,18 +68,12 @@ def destroy
 
   private
 
-  def set_hook
+  def hook
     @hook ||= @group.hooks.find(params[:id])
   end
 
-  def hook_params
-    params.require(:hook).permit(
-      :enable_ssl_verification,
-      :token,
-      :url,
-      :push_events_branch_filter,
-      *GroupHook.triggers.values
-    )
+  def trigger_values
+    GroupHook.triggers.values
   end
 
   def check_group_webhooks_available!
diff --git a/ee/spec/controllers/groups/hooks_controller_spec.rb b/ee/spec/controllers/groups/hooks_controller_spec.rb
index 368c8c90a49d17..d740434375aeab 100644
--- a/ee/spec/controllers/groups/hooks_controller_spec.rb
+++ b/ee/spec/controllers/groups/hooks_controller_spec.rb
@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Groups::HooksController do
-  let(:user)  { create(:user) }
-  let(:group) { create(:group) }
+  let_it_be(:user)  { create(:user) }
+  let_it_be(:group) { create(:group) }
 
   before do
     group.add_owner(user)
@@ -44,14 +44,18 @@
           enable_ssl_verification: true,
           token: 'TEST TOKEN',
           url: 'http://example.com',
-          push_events_branch_filter: 'filter-branch'
+          push_events_branch_filter: 'filter-branch',
+          url_variables: [
+            { key: 'token', value: 'shh-secret!' }
+          ]
         }
 
         post :create, params: { group_id: group.to_param, hook: hook_params }
 
         expect(response).to have_gitlab_http_status(:found)
         expect(group.hooks.size).to eq(1)
-        expect(group.hooks.first).to have_attributes(hook_params)
+        expect(group.hooks.first).to have_attributes(hook_params.except(:url_variables))
+        expect(group.hooks.first.url_variables).to eq('token' => 'shh-secret!')
       end
     end
 
@@ -68,7 +72,7 @@
     end
 
     describe 'PATCH #update' do
-      let(:hook) { create(:group_hook, group: group) }
+      let_it_be(:hook) { create(:group_hook, group: group) }
 
       context 'valid params' do
         let(:hook_params) do
@@ -88,17 +92,28 @@
             deployment_events: true,
             releases_events: true,
             member_events: true,
-            subgroup_events: true
+            subgroup_events: true,
+            url_variables: [
+              { key: 'a', value: 'alpha' },
+              { key: 'b', value: nil },
+              { key: 'c', value: 'gamma' }
+            ]
           }
         end
 
-        it 'is successfull' do
+        it 'is successful' do
+          hook.update!(url_variables: { 'a' => 'x', 'b' => 'z' })
+
           patch :update, params: { group_id: group.to_param, id: hook, hook: hook_params }
 
           expect(response).to have_gitlab_http_status(:found)
           expect(response).to redirect_to(group_hooks_path(group))
           expect(group.hooks.size).to eq(1)
-          expect(group.hooks.first).to have_attributes(hook_params)
+          expect(hook.reload).to have_attributes(hook_params.except(:url_variables))
+          expect(hook.url_variables).to eq(
+            'a' => 'alpha',
+            'c' => 'gamma'
+          )
         end
       end
 
@@ -113,6 +128,7 @@
           patch :update, params: { group_id: group.to_param, id: hook, hook: hook_params }
 
           expect(response).to have_gitlab_http_status(:ok)
+          expect(flash[:notice]).to be_nil
           expect(response).to render_template(:edit)
           expect(group.hooks.size).to eq(1)
           expect(group.hooks.first).not_to have_attributes(hook_params)
@@ -120,7 +136,7 @@
       end
     end
 
-    describe 'POST #test' do
+    describe 'POST #test', :clean_gitlab_redis_shared_state do
       let(:hook) { create(:group_hook, group: group) }
 
       context 'when group does not have a project' do
diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb
index 17c4222530d818..14f4a2f40e7295 100644
--- a/spec/controllers/admin/hooks_controller_spec.rb
+++ b/spec/controllers/admin/hooks_controller_spec.rb
@@ -17,16 +17,46 @@
         url: "http://example.com",
 
         push_events: true,
-        tag_push_events: true,
+        tag_push_events: false,
         repository_update_events: true,
-        merge_requests_events: true
+        merge_requests_events: false,
+        url_variables: [{ key: 'token', value: 'some secret value' }]
       }
 
       post :create, params: { hook: hook_params }
 
       expect(response).to have_gitlab_http_status(:found)
       expect(SystemHook.all.size).to eq(1)
-      expect(SystemHook.first).to have_attributes(hook_params)
+      expect(SystemHook.first).to have_attributes(hook_params.except(:url_variables))
+      expect(SystemHook.first).to have_attributes(url_variables: { 'token' => 'some secret value' })
+    end
+  end
+
+  describe 'POST #update' do
+    let!(:hook) { create(:system_hook) }
+
+    it 'sets all parameters' do
+      hook.update!(url_variables: { 'foo' => 'bar', 'baz' => 'woo' })
+
+      hook_params = {
+        url: 'http://example.com/{baz}?token={token}',
+        enable_ssl_verification: false,
+        url_variables: [
+          { key: 'token', value: 'some secret value' },
+          { key: 'foo', value: nil }
+        ]
+      }
+
+      put :update, params: { id: hook.id, hook: hook_params }
+
+      hook.reload
+
+      expect(response).to have_gitlab_http_status(:found)
+      expect(flash[:notice]).to include('successfully updated')
+      expect(hook).to have_attributes(hook_params.except(:url_variables))
+      expect(hook).to have_attributes(
+        url_variables: { 'token' => 'some secret value', 'baz' => 'woo' }
+      )
     end
   end
 
diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb
index ebcf35a7ecd08e..c29b5f1e5e6c06 100644
--- a/spec/controllers/projects/hooks_controller_spec.rb
+++ b/spec/controllers/projects/hooks_controller_spec.rb
@@ -20,6 +20,36 @@
     end
   end
 
+  describe '#update' do
+    let_it_be(:hook) { create(:project_hook, project: project) }
+
+    let(:params) do
+      { namespace_id: project.namespace, project_id: project, id: hook.id }
+    end
+
+    it 'adds, updates and deletes URL variables' do
+      hook.update!(url_variables: { 'a' => 'bar', 'b' => 'woo' })
+
+      params[:hook] = {
+        url_variables: [
+          { key: 'a', value: 'updated' },
+          { key: 'b', value: nil },
+          { key: 'c', value: 'new' }
+        ]
+      }
+
+      put :update, params: params
+
+      expect(response).to have_gitlab_http_status(:found)
+      expect(flash[:notice]).to include('successfully updated')
+
+      expect(hook.reload.url_variables).to eq(
+        'a' => 'updated',
+        'c' => 'new'
+      )
+    end
+  end
+
   describe '#edit' do
     let_it_be(:hook) { create(:project_hook, project: project) }
 
@@ -87,14 +117,17 @@ def it_renders_correctly
         job_events: true,
         pipeline_events: true,
         wiki_page_events: true,
-        deployment_events: true
+        deployment_events: true,
+
+        url_variables: [{ key: 'token', value: 'some secret value' }]
       }
 
       post :create, params: { namespace_id: project.namespace, project_id: project, hook: hook_params }
 
       expect(response).to have_gitlab_http_status(:found)
-      expect(ProjectHook.all.size).to eq(1)
-      expect(ProjectHook.first).to have_attributes(hook_params)
+      expect(ProjectHook.count).to eq(1)
+      expect(ProjectHook.first).to have_attributes(hook_params.except(:url_variables))
+      expect(ProjectHook.first).to have_attributes(url_variables: { 'token' => 'some secret value' })
     end
   end
 
-- 
GitLab


From 43385e2275053987d7741942d37f73b8fbd69bd4 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Thu, 16 Jun 2022 15:27:26 -0400
Subject: [PATCH 2/4] Refactor hooks controllers

Prior to this change, most logic was re-implemented in the three
main hooks controllers (project/group/admin).

This change moves the common logic to the common hook execution concern.
Hooks controllers must implement the `relation` method.
---
 app/controllers/admin/hooks_controller.rb     | 38 ++--------------
 .../concerns/integrations/hooks_execution.rb  | 44 ++++++++++++++++++-
 app/controllers/projects/hooks_controller.rb  | 34 ++------------
 ee/app/controllers/groups/hooks_controller.rb | 37 ++--------------
 .../groups/hooks_controller_spec.rb           | 12 +++++
 locale/gitlab.pot                             |  6 ---
 .../projects/hooks_controller_spec.rb         | 13 ++++++
 7 files changed, 77 insertions(+), 107 deletions(-)

diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb
index fc4bb915462857..810801d420920d 100644
--- a/app/controllers/admin/hooks_controller.rb
+++ b/app/controllers/admin/hooks_controller.rb
@@ -8,40 +8,6 @@ class Admin::HooksController < Admin::ApplicationController
   feature_category :integrations
   urgency :low, [:test]
 
-  def index
-    @hooks = SystemHook.all.load
-    @hook = SystemHook.new
-  end
-
-  def create
-    @hook = SystemHook.new(hook_params)
-
-    if @hook.save
-      redirect_to admin_hooks_path, notice: _('Hook was successfully created.')
-    else
-      @hooks = SystemHook.all
-      render :index
-    end
-  end
-
-  def edit
-  end
-
-  def update
-    if hook.update(hook_params)
-      flash[:notice] = _('System hook was successfully updated.')
-      redirect_to admin_hooks_path
-    else
-      render 'edit'
-    end
-  end
-
-  def destroy
-    destroy_hook(hook)
-
-    redirect_to admin_hooks_path, status: :found
-  end
-
   def test
     result = TestHooks::SystemService.new(hook, current_user, params[:trigger]).execute
 
@@ -52,6 +18,10 @@ def test
 
   private
 
+  def relation
+    SystemHook
+  end
+
   def hook
     @hook ||= SystemHook.find(params[:id])
   end
diff --git a/app/controllers/concerns/integrations/hooks_execution.rb b/app/controllers/concerns/integrations/hooks_execution.rb
index 751498a2774818..fb26840168f092 100644
--- a/app/controllers/concerns/integrations/hooks_execution.rb
+++ b/app/controllers/concerns/integrations/hooks_execution.rb
@@ -3,18 +3,58 @@
 module Integrations::HooksExecution
   extend ActiveSupport::Concern
 
+  included do
+    attr_writer :hooks, :hook
+  end
+
+  def index
+    self.hooks = relation.select(&:persisted?)
+    self.hook = relation.new
+  end
+
+  def create
+    self.hook = relation.new(hook_params)
+    hook.save
+
+    unless hook.valid?
+      self.hooks = relation.select(&:persisted?)
+      flash[:alert] = hook.errors.full_messages.join.html_safe
+    end
+
+    redirect_to action: :index
+  end
+
+  def update
+    if hook.update(hook_params)
+      flash[:notice] = _('Hook was successfully updated.')
+      redirect_to action: :index
+    else
+      render 'edit'
+    end
+  end
+
+  def destroy
+    destroy_hook(hook)
+
+    redirect_to action: :index, status: :found
+  end
+
+  def edit
+    redirect_to(action: :index) unless hook
+  end
+
   private
 
   def hook_params
     permitted = hook_param_names + trigger_values
     permitted << { url_variables: [:key, :value] }
 
-    ps = params.require(:hook).permit(*permitted)
+    ps = params.require(:hook).permit(*permitted).to_h
 
     ps[:url_variables] = ps[:url_variables].to_h { [_1[:key], _1[:value].presence] } if ps.key?(:url_variables)
 
     if action_name == 'update' && ps.key?(:url_variables)
-      supplied = ps.permit(url_variables: {})[:url_variables].to_h
+      supplied = ps[:url_variables]
       ps[:url_variables] = hook.url_variables.merge(supplied).compact
     end
 
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index e54beab1a2d004..49f8d1c42af5f0 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -15,36 +15,10 @@ class Projects::HooksController < Projects::ApplicationController
   feature_category :integrations
   urgency :low, [:test]
 
-  def index
-    @hooks = @project.hooks.load
-    @hook = ProjectHook.new
-  end
-
-  def create
-    @hook = @project.hooks.new(hook_params)
-    @hook.save
-
-    unless @hook.valid?
-      @hooks = @project.hooks.select(&:persisted?)
-      flash[:alert] = @hook.errors.full_messages.join.html_safe
-    end
-
-    redirect_to action: :index
-  end
-
   def edit
     redirect_to(action: :index) unless hook
   end
 
-  def update
-    if hook.update(hook_params)
-      flash[:notice] = _('Hook was successfully updated.')
-      redirect_to action: :index
-    else
-      render 'edit'
-    end
-  end
-
   def test
     result = TestHooks::ProjectService.new(hook, current_user, params[:trigger]).execute
 
@@ -53,14 +27,12 @@ def test
     redirect_back_or_default(default: { action: :index })
   end
 
-  def destroy
-    destroy_hook(hook)
+  private
 
-    redirect_to action: :index, status: :found
+  def relation
+    @project.hooks
   end
 
-  private
-
   def hook
     @hook ||= @project.hooks.find(params[:id])
   end
diff --git a/ee/app/controllers/groups/hooks_controller.rb b/ee/app/controllers/groups/hooks_controller.rb
index a4ca2bef2946fe..febc3b558bf8bd 100644
--- a/ee/app/controllers/groups/hooks_controller.rb
+++ b/ee/app/controllers/groups/hooks_controller.rb
@@ -17,35 +17,6 @@ class Groups::HooksController < Groups::ApplicationController
   feature_category :integrations
   urgency :low, [:test]
 
-  def index
-    @hooks = @group.hooks.load
-    @hook = GroupHook.new
-  end
-
-  def create
-    @hook = @group.hooks.new(hook_params)
-    @hook.save
-
-    if @hook.valid?
-      redirect_to group_hooks_path(@group)
-    else
-      @hooks = @group.hooks.select(&:persisted?)
-      render :index
-    end
-  end
-
-  def edit
-  end
-
-  def update
-    if hook.update(hook_params)
-      flash[:notice] = _('Hook was successfully updated.')
-      redirect_to group_hooks_path(@group)
-    else
-      render 'edit'
-    end
-  end
-
   def test
     if @group.first_non_empty_project
       service = TestHooks::ProjectService.new(hook, current_user, params[:trigger] || 'push_events')
@@ -60,14 +31,12 @@ def test
     redirect_back_or_default(default: { action: 'index' })
   end
 
-  def destroy
-    destroy_hook(@hook)
+  private
 
-    redirect_to group_hooks_path(@group), status: :found
+  def relation
+    @group.hooks
   end
 
-  private
-
   def hook
     @hook ||= @group.hooks.find(params[:id])
   end
diff --git a/ee/spec/controllers/groups/hooks_controller_spec.rb b/ee/spec/controllers/groups/hooks_controller_spec.rb
index d740434375aeab..4e9a0086ff22ed 100644
--- a/ee/spec/controllers/groups/hooks_controller_spec.rb
+++ b/ee/spec/controllers/groups/hooks_controller_spec.rb
@@ -57,6 +57,18 @@
         expect(group.hooks.first).to have_attributes(hook_params.except(:url_variables))
         expect(group.hooks.first.url_variables).to eq('token' => 'shh-secret!')
       end
+
+      it 'alerts the user if the new hook is invalid' do
+        hook_params = {
+          token: "TEST\nTOKEN",
+          url: "http://example.com"
+        }
+
+        post :create, params: { group_id: group.to_param, hook: hook_params }
+
+        expect(flash[:alert]).to be_present
+        expect(group.hooks.count).to eq(0)
+      end
     end
 
     describe 'GET #edit' do
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 9f19cfd563a24c..b4b67cd21bf188 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -18989,9 +18989,6 @@ msgstr ""
 msgid "Hook execution failed. Ensure the group has a project with commits."
 msgstr ""
 
-msgid "Hook was successfully created."
-msgstr ""
-
 msgid "Hook was successfully updated."
 msgstr ""
 
@@ -37350,9 +37347,6 @@ msgstr ""
 msgid "System header and footer"
 msgstr ""
 
-msgid "System hook was successfully updated."
-msgstr ""
-
 msgid "System hooks are triggered on sets of events like creating a project or adding an SSH key. You can also enable extra triggers, such as push events."
 msgstr ""
 
diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb
index c29b5f1e5e6c06..343b54201d7393 100644
--- a/spec/controllers/projects/hooks_controller_spec.rb
+++ b/spec/controllers/projects/hooks_controller_spec.rb
@@ -125,10 +125,23 @@ def it_renders_correctly
       post :create, params: { namespace_id: project.namespace, project_id: project, hook: hook_params }
 
       expect(response).to have_gitlab_http_status(:found)
+      expect(flash[:alert]).to be_blank
       expect(ProjectHook.count).to eq(1)
       expect(ProjectHook.first).to have_attributes(hook_params.except(:url_variables))
       expect(ProjectHook.first).to have_attributes(url_variables: { 'token' => 'some secret value' })
     end
+
+    it 'alerts the user if the new hook is invalid' do
+      hook_params = {
+        token: "TEST\nTOKEN",
+        url: "http://example.com"
+      }
+
+      post :create, params: { namespace_id: project.namespace, project_id: project, hook: hook_params }
+
+      expect(flash[:alert]).to be_present
+      expect(ProjectHook.count).to eq(0)
+    end
   end
 
   describe 'DELETE #destroy' do
-- 
GitLab


From c666a69497e179c38242baa34d9d9afd712b6b99 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Fri, 17 Jun 2022 15:00:23 -0400
Subject: [PATCH 3/4] Remove refactored method

---
 app/controllers/projects/hooks_controller.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 49f8d1c42af5f0..83461c60d2f7ee 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -15,10 +15,6 @@ class Projects::HooksController < Projects::ApplicationController
   feature_category :integrations
   urgency :low, [:test]
 
-  def edit
-    redirect_to(action: :index) unless hook
-  end
-
   def test
     result = TestHooks::ProjectService.new(hook, current_user, params[:trigger]).execute
 
-- 
GitLab


From 6dfe6ef06cdb6bde9aef943931ad023a02cf4ccd Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Sun, 19 Jun 2022 15:34:56 -0400
Subject: [PATCH 4/4] Add spec for the behavior of failed tests

---
 app/controllers/projects/hooks_controller.rb  |  3 +-
 .../projects/hooks_controller_spec.rb         | 41 +++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index 83461c60d2f7ee..50f388324f1696 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -16,7 +16,8 @@ class Projects::HooksController < Projects::ApplicationController
   urgency :low, [:test]
 
   def test
-    result = TestHooks::ProjectService.new(hook, current_user, params[:trigger]).execute
+    trigger = params.fetch(:trigger, ::ProjectHook.triggers.each_value.first.to_s)
+    result = TestHooks::ProjectService.new(hook, current_user, trigger).execute
 
     set_hook_execution_notice(result)
 
diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb
index 343b54201d7393..a275bc2863151b 100644
--- a/spec/controllers/projects/hooks_controller_spec.rb
+++ b/spec/controllers/projects/hooks_controller_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Projects::HooksController do
+  include AfterNextHelpers
+
   let_it_be(:project) { create(:project) }
 
   let(:user) { project.first_owner }
@@ -155,6 +157,45 @@ def it_renders_correctly
   describe '#test' do
     let(:hook) { create(:project_hook, project: project) }
 
+    context 'when the hook executes successfully' do
+      before do
+        stub_request(:post, hook.url).to_return(status: 200)
+      end
+
+      it 'informs the user' do
+        post :test, params: { namespace_id: project.namespace, project_id: project, id: hook }
+
+        expect(flash[:notice]).to include('executed successfully')
+        expect(flash[:notice]).to include('HTTP 200')
+      end
+    end
+
+    context 'when the hook runs, but fails' do
+      before do
+        stub_request(:post, hook.url).to_return(status: 400)
+      end
+
+      it 'informs the user' do
+        post :test, params: { namespace_id: project.namespace, project_id: project, id: hook }
+
+        expect(flash[:alert]).to include('executed successfully but')
+        expect(flash[:alert]).to include('HTTP 400')
+      end
+    end
+
+    context 'when the hook fails completely' do
+      before do
+        allow_next(::TestHooks::ProjectService)
+          .to receive(:execute).and_return({ message: 'All is woe' })
+      end
+
+      it 'informs the user' do
+        post :test, params: { namespace_id: project.namespace, project_id: project, id: hook }
+
+        expect(flash[:alert]).to include('failed: All is woe')
+      end
+    end
+
     context 'when the endpoint receives requests above the limit', :freeze_time, :clean_gitlab_redis_rate_limiting do
       before do
         allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits)
-- 
GitLab