From 67b9a78f90775963993287c60fe079beec881852 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Wed, 1 Feb 2023 15:24:22 -0500
Subject: [PATCH 1/8] Add group web-hooks failed notifications

- Defines a new read_web_hooks policy for groups
- Adds a new web-hooks helper module for groups
- Defines `web_hook_dismissed` for group callouts
- Introduces `Project#any_hook_failed?` for cleaner modelling and
  testing
- Introduces `HasWebHooks` - a concern that objects that have
  a `hooks` relation can include.

Changelog: changed
---
 app/helpers/users/callouts_helper.rb          | 22 +++---
 app/helpers/web_hooks/web_hooks_helper.rb     |  2 +
 .../concerns/web_hooks/has_web_hooks.rb       | 10 ++-
 app/models/project.rb                         |  1 +
 app/models/users/group_callout.rb             |  3 +-
 app/policies/group_policy.rb                  |  2 +
 app/views/layouts/group.html.haml             |  1 +
 .../helpers/ee/web_hooks/web_hooks_helper.rb  | 31 ++++++++
 ee/app/models/ee/group.rb                     |  8 ++
 .../_group_web_hook_disabled_alert.html.haml  | 13 ++++
 .../web_hooks/web_hooks_helper_spec.rb        | 74 +++++++++++++++++++
 ee/spec/models/ee/group_spec.rb               |  8 ++
 ..._web_hook_disabled_alert.html.haml_spec.rb | 36 +++++++++
 locale/gitlab.pot                             |  3 +
 spec/helpers/users/callouts_helper_spec.rb    | 44 +++++++----
 .../web_hooks/web_hooks_helper_spec.rb        |  8 +-
 .../concerns/web_hooks/has_web_hooks_spec.rb  | 41 ++++++++++
 spec/models/project_spec.rb                   | 26 +++++++
 .../policies/group_policy_shared_context.rb   |  1 +
 19 files changed, 302 insertions(+), 32 deletions(-)
 create mode 100644 ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
 create mode 100644 ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml
 create mode 100644 ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
 create mode 100644 ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb
 create mode 100644 spec/models/concerns/web_hooks/has_web_hooks_spec.rb

diff --git a/app/helpers/users/callouts_helper.rb b/app/helpers/users/callouts_helper.rb
index 2b8368dd29fdfc..8f4a069f5ffb8d 100644
--- a/app/helpers/users/callouts_helper.rb
+++ b/app/helpers/users/callouts_helper.rb
@@ -59,17 +59,15 @@ def show_security_newsletter_user_callout?
         !user_dismissed?(SECURITY_NEWSLETTER_CALLOUT)
     end
 
-    def web_hook_disabled_dismissed?(project)
-      return false unless project
-
-      last_failure = Gitlab::Redis::SharedState.with do |redis|
-        key = "web_hooks:last_failure:project-#{project.id}"
-        redis.get(key)
+    def web_hook_disabled_dismissed?(object)
+      return false unless object.respond_to?(:last_failure_redis_key)
+
+      case object
+      when Project
+        user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, project: object)
+      when Group
+        user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, group: object)
       end
-
-      last_failure = DateTime.parse(last_failure) if last_failure
-
-      user_dismissed?(WEB_HOOK_DISABLED, last_failure, project: project)
     end
 
     def show_merge_request_settings_callout?(project)
@@ -84,13 +82,15 @@ def ultimate_feature_removal_banner_dismissed?(project)
 
     private
 
-    def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, project: nil)
+    def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, project: nil, group: nil)
       return false unless current_user
 
       query = { feature_name: feature_name, ignore_dismissal_earlier_than: ignore_dismissal_earlier_than }
 
       if project
         current_user.dismissed_callout_for_project?(project: project, **query)
+      elsif group
+        current_user.dismissed_callout_for_group?(group: group, **query)
       else
         current_user.dismissed_callout?(**query)
       end
diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index 514db6ba8a2ff2..024711fa2c9cab 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -20,3 +20,5 @@ def project_hook_page?
     end
   end
 end
+
+WebHooks::WebHooksHelper.prepend_mod
diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb
index 161ce106b9b7ce..61cfdd5c28b461 100644
--- a/app/models/concerns/web_hooks/has_web_hooks.rb
+++ b/app/models/concerns/web_hooks/has_web_hooks.rb
@@ -15,7 +15,7 @@ def web_hook_failure_redis_key
     end
 
     def last_failure_redis_key
-      "web_hooks:last_failure:project-#{id}"
+      "web_hooks:last_failure:#{self.class.name.underscore}-#{id}"
     end
 
     def get_web_hook_failure
@@ -42,5 +42,13 @@ def cache_web_hook_failure(state = any_hook_failed?)
         state
       end
     end
+
+    def last_webhook_failure
+      last_failure = Gitlab::Redis::SharedState.with do |redis|
+        redis.get(last_failure_redis_key)
+      end
+
+      DateTime.parse(last_failure) if last_failure
+    end
   end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index cd35332d7dfaec..43eef0178c6f03 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -19,6 +19,7 @@ class Project < ApplicationRecord
   include Presentable
   include HasRepository
   include HasWiki
+  include WebHooks::HasWebHooks
   include CanMoveRepositoryStorage
   include Routable
   include GroupDescendant
diff --git a/app/models/users/group_callout.rb b/app/models/users/group_callout.rb
index 2552407fa4cda8..863ebddad998d9 100644
--- a/app/models/users/group_callout.rb
+++ b/app/models/users/group_callout.rb
@@ -24,7 +24,8 @@ class GroupCallout < ApplicationRecord
       namespace_storage_limit_banner_error_threshold: 13, # EE-only
       usage_quota_trial_alert: 14, # EE-only
       preview_usage_quota_free_plan_alert: 15, # EE-only
-      enforcement_at_limit_alert: 16 # EE-only
+      enforcement_at_limit_alert: 16, # EE-only
+      web_hook_disabled: 17
     }
 
     validates :group, presence: true
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index ae736865e1772c..fb4887fd98543b 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -202,6 +202,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :maintainer_access
     enable :read_upload
     enable :destroy_upload
+    enable :admin_achievement
+    enable :read_web_hooks
   end
 
   rule { owner }.policy do
diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml
index 95934f43a5185d..28579c7f7eaaec 100644
--- a/app/views/layouts/group.html.haml
+++ b/app/views/layouts/group.html.haml
@@ -16,6 +16,7 @@
       :plain
         window.uploads_path = "#{group_uploads_path(@group)}";
 
+= dispensable_render_if_exists "shared/web_hooks/group_web_hook_disabled_alert"
 = dispensable_render_if_exists "shared/free_user_cap_alert", source: @group
 
 = render template: base_layout || "layouts/application"
diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
new file mode 100644
index 00000000000000..ab55cf18ef20d1
--- /dev/null
+++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module EE
+  module WebHooks
+    module WebHooksHelper
+      def show_group_hook_failed_callout?(group:)
+        return false if group_hook_page?
+        return false unless current_user
+
+        return false unless Ability.allowed?(current_user, :read_web_hooks, group)
+
+        # Assumes include of Users::CalloutsHelper
+        return false if web_hook_disabled_dismissed?(group)
+
+        any_group_hook_failed?(group) # Most expensive query last
+      end
+
+      private
+
+      def group_hook_page?
+        current_controller?('groups/hooks') || current_controller?('groups/hook_logs')
+      end
+
+      def any_group_hook_failed?(group)
+        any_hook_failed?("any_web_hook_failed:group:#{group.id}") do
+          group.any_web_hook_failed?
+        end
+      end
+    end
+  end
+end
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 85b6e3db55ca80..69975bde87b79d 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -13,6 +13,7 @@ module Group
       include TokenAuthenticatable
       include InsightsFeature
       include HasWiki
+      include ::WebHooks::HasWebHooks
       include CanMoveRepositoryStorage
       include ReactiveCaching
 
@@ -646,6 +647,13 @@ def execute_hooks(data, hooks_scope)
       end
     end
 
+    override :any_web_hook_failed?
+    def any_web_hook_failed?
+      # morally `hooks.disabled.exists?`, but since group hooks include
+      # Unstoppable, we simply return `false`.
+      false
+    end
+
     override :git_transfer_in_progress?
     def git_transfer_in_progress?
       reference_counter(type: ::Gitlab::GlRepository::WIKI).value > 0
diff --git a/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml b/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml
new file mode 100644
index 00000000000000..f227a2dfbe2b08
--- /dev/null
+++ b/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml
@@ -0,0 +1,13 @@
+- return unless show_group_hook_failed_callout?(group: @group)
+
+- content_for :after_flash_content do
+  = render Pajamas::AlertComponent.new(variant: :danger,
+    title: s_('Webhooks|Webhook disabled'),
+    alert_options: { class: 'gl-my-4 js-web-hook-disabled-callout',
+                      data: { feature_id: Users::CalloutsHelper::WEB_HOOK_DISABLED, dismiss_endpoint: group_callouts_path, group_id: @group.id, defer_links: 'true'} }) do |c|
+    = c.body do
+      = s_('Webhooks|A webhook in this group was automatically disabled after being retried multiple times.')
+      = succeed '.' do
+        = link_to _('Learn more'), help_page_path('user/project/integrations/webhooks', anchor: 'troubleshoot-webhooks'), target: '_blank', rel: 'noopener noreferrer'
+    = c.actions do
+      = link_to s_('Webhooks|Go to webhooks'), group_hooks_path(@group, anchor: 'webhooks-index'), class: 'btn gl-alert-action btn-confirm gl-button'
diff --git a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
new file mode 100644
index 00000000000000..2950a64503d2a2
--- /dev/null
+++ b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::WebHooksHelper, feature_category: :integrations do
+  let_it_be_with_reload(:group) { create(:group) } # rubocop: disable RSpec/FactoryBot/AvoidCreate
+
+  let(:current_user) { nil }
+  let(:callout_dismissed) { false }
+  let(:viewing_hooks) { true }
+  let(:a_hook_has_failed) { false }
+
+  before do
+    allow(helper).to receive(:current_user).and_return(current_user)
+    allow(helper).to receive(:web_hook_disabled_dismissed?).with(group).and_return(callout_dismissed)
+    allow(helper).to receive(:current_controller?).with('groups/hooks').and_return(viewing_hooks)
+    allow(helper).to receive(:current_controller?).with('groups/hook_logs').and_return(viewing_hooks)
+    allow(group).to receive(:any_web_hook_failed?).and_return(a_hook_has_failed)
+  end
+
+  shared_context 'when we are not viewing the group hooks or the logs' do
+    let(:viewing_hooks) { false }
+  end
+
+  shared_context 'when the user has permission' do
+    before do
+      group.add_maintainer(current_user) if current_user
+    end
+  end
+
+  shared_context 'when a user is logged in' do
+    let(:current_user) { create(:user) } # rubocop: disable RSpec/FactoryBot/AvoidCreate
+  end
+
+  shared_context 'when the user dismissed the callout' do
+    let(:callout_dismissed) { true }
+  end
+
+  shared_context 'when a hook has failed' do
+    let(:a_hook_has_failed) { true }
+  end
+
+  describe '#show_group_hook_failed_callout?' do
+    context 'when all conditions are met' do
+      include_context 'when a user is logged in'
+      include_context 'when the user has permission'
+      include_context 'when we are not viewing the group hooks or the logs'
+      include_context 'when a hook has failed'
+
+      it 'is true' do
+        expect(helper).to be_show_group_hook_failed_callout(group: group)
+      end
+    end
+
+    context 'when any one condition is not met' do
+      contexts = [
+        'when a user is logged in',
+        'when the user has permission',
+        'when we are not viewing the group hooks or the logs',
+        'when a hook has failed'
+      ]
+
+      contexts.each do |name|
+        context "namely #{name}" do
+          contexts.each { |ctx| include_context(ctx) unless ctx == name }
+
+          it 'is false' do
+            expect(helper).not_to be_show_group_hook_failed_callout(group: group)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index b0b459914aac6a..0a23af8e07616e 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -1773,6 +1773,14 @@
     end
   end
 
+  describe '#any_web_hook_failed?' do
+    let_it_be(:group) { create(:group) }
+
+    subject { group.any_web_hook_failed? }
+
+    it { is_expected.to eq(false) }
+  end
+
   describe "#execute_hooks" do
     context "group_webhooks", :request_store do
       let_it_be(:parent_group) { create(:group) }
diff --git a/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb b/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb
new file mode 100644
index 00000000000000..92fc62dead16ac
--- /dev/null
+++ b/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'shared/web_hooks/_group_web_hook_disabled_alert', feature_category: :integrations do
+  let_it_be(:group) { build_stubbed(:group) }
+
+  def after_flash_content
+    view.content_for(:after_flash_content)
+  end
+
+  before do
+    view.assign(group: group)
+    allow(view).to receive(:show_group_hook_failed_callout?).with(group: group).and_return(show_callout)
+  end
+
+  context 'when the helper returns true' do
+    let(:show_callout) { true }
+
+    it 'adds alert to `:after_flash_content`' do
+      view.render('shared/web_hooks/group_web_hook_disabled_alert')
+
+      expect(after_flash_content).to have_content('Webhook disabled')
+    end
+  end
+
+  context 'when helper returns false' do
+    let(:show_callout) { false }
+
+    it 'does not add alert to `:after_flash_content`' do
+      view.render('shared/web_hooks/group_web_hook_disabled_alert')
+
+      expect(after_flash_content).to be_nil
+    end
+  end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8eabffd78b7129..6df41c03ab4e20 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -48096,6 +48096,9 @@ msgstr ""
 msgid "Webhooks|A subgroup is created or removed."
 msgstr ""
 
+msgid "Webhooks|A webhook in this group was automatically disabled after being retried multiple times."
+msgstr ""
+
 msgid "Webhooks|A webhook in this project was automatically disabled after being retried multiple times."
 msgstr ""
 
diff --git a/spec/helpers/users/callouts_helper_spec.rb b/spec/helpers/users/callouts_helper_spec.rb
index 4cb179e4f60d64..b1c2a2587116c6 100644
--- a/spec/helpers/users/callouts_helper_spec.rb
+++ b/spec/helpers/users/callouts_helper_spec.rb
@@ -166,58 +166,72 @@
   end
 
   describe '#web_hook_disabled_dismissed?' do
-    context 'without a project' do
+    context 'without a container' do
       it 'is false' do
         expect(helper).not_to be_web_hook_disabled_dismissed(nil)
       end
     end
 
-    context 'with a project' do
-      let_it_be(:project) { create(:project) }
-
+    shared_examples 'web_hook_disabled shared examples' do
       context 'the web-hook failure callout has never been dismissed' do
         it 'is false' do
-          expect(helper).not_to be_web_hook_disabled_dismissed(project)
+          expect(helper).not_to be_web_hook_disabled_dismissed(container)
         end
       end
 
       context 'the web-hook failure callout has been dismissed', :freeze_time do
         before do
-          create(:project_callout,
+          create(factory,
                  feature_name: described_class::WEB_HOOK_DISABLED,
                  user: user,
-                 project: project,
-                 dismissed_at: 1.week.ago)
+                 dismissed_at: 1.week.ago,
+                 container_key => container)
         end
 
         it 'is true' do
-          expect(helper).to be_web_hook_disabled_dismissed(project)
+          expect(helper).to be_web_hook_disabled_dismissed(container)
         end
 
         context 'when there was an older failure', :clean_gitlab_redis_shared_state do
-          let(:key) { "web_hooks:last_failure:project-#{project.id}" }
-
           before do
             Gitlab::Redis::SharedState.with { |r| r.set(key, 1.month.ago.iso8601) }
           end
 
           it 'is true' do
-            expect(helper).to be_web_hook_disabled_dismissed(project)
+            expect(helper).to be_web_hook_disabled_dismissed(container)
           end
         end
 
         context 'when there has been a more recent failure', :clean_gitlab_redis_shared_state do
-          let(:key) { "web_hooks:last_failure:project-#{project.id}" }
-
           before do
             Gitlab::Redis::SharedState.with { |r| r.set(key, 1.day.ago.iso8601) }
           end
 
           it 'is false' do
-            expect(helper).not_to be_web_hook_disabled_dismissed(project)
+            expect(helper).not_to be_web_hook_disabled_dismissed(container)
           end
         end
       end
     end
+
+    context 'with a project' do
+      let_it_be(:project) { create(:project) }
+      let(:factory) { :project_callout }
+      let(:container_key) { :project }
+      let(:container) { project }
+      let(:key) { "web_hooks:last_failure:project-#{project.id}" }
+
+      include_examples 'web_hook_disabled shared examples'
+    end
+
+    context 'with a group' do
+      let_it_be(:group) { create(:group) }
+      let(:factory) { :group_callout }
+      let(:container_key) { :group }
+      let(:container) { group }
+      let(:key) { "web_hooks:last_failure:group-#{group.id}" }
+
+      include_examples 'web_hook_disabled shared examples'
+    end
   end
 end
diff --git a/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index fdd0be8095b2e8..df62669d59ec19 100644
--- a/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -7,10 +7,12 @@
 
   let(:current_user) { nil }
   let(:callout_dismissed) { false }
+  let(:any_hook_failed) { false }
 
   before do
     allow(helper).to receive(:current_user).and_return(current_user)
     allow(helper).to receive(:web_hook_disabled_dismissed?).with(project).and_return(callout_dismissed)
+    allow(project).to receive(:any_web_hook_failed?).and_return(any_hook_failed)
   end
 
   shared_context 'user is logged in' do
@@ -28,12 +30,10 @@
   end
 
   shared_context 'a hook has failed' do
-    before do
-      create(:project_hook, :permanently_disabled, project: project)
-    end
+    let(:any_hook_failed) { true }
   end
 
-  describe '#show_project_hook_failed_callout?' do
+  describe '#show_project_hook_failed_callout?', :use_clean_rails_memory_store_caching do
     context 'all conditions are met' do
       include_context 'user is logged in'
       include_context 'the user has permission'
diff --git a/spec/models/concerns/web_hooks/has_web_hooks_spec.rb b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb
new file mode 100644
index 00000000000000..cc08a01420ba1a
--- /dev/null
+++ b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::HasWebHooks, feature_category: :integrations do
+  let(:minimal_test_class) do
+    Class.new do
+      include HasWebHooks
+
+      def id
+        1
+      end
+    end
+  end
+
+  before do
+    stub_const('MinimalTestClass', minimal_test_class)
+  end
+
+  describe '#last_failure_redis_key' do
+    subject { MinimalTestClass.new.last_failure_redis_key }
+
+    it { is_expected.to eq('web_hooks:last_failure:minimal_test_class-1') }
+  end
+
+  describe 'last_webhook_failure', :clean_gitlab_redis_shared_state do
+    subject { MinimalTestClass.new.last_webhook_failure }
+
+    it { is_expected.to eq(nil) }
+
+    context 'when there was an older failure', :clean_gitlab_redis_shared_state do
+      let(:last_failure_date) { 1.month.ago.iso8601 }
+
+      before do
+        Gitlab::Redis::SharedState.with { |r| r.set('web_hooks:last_failure:minimal_test_class-1', last_failure_date) }
+      end
+
+      it { is_expected.to eq(last_failure_date) }
+    end
+  end
+end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index ab50a3aa4805f8..3e59e08c6ef74c 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5992,6 +5992,32 @@ def has_external_wiki
     end
   end
 
+  describe '#any_web_hook_failed?' do
+    let_it_be_with_reload(:project) { create(:project) }
+
+    subject { project.any_web_hook_failed? }
+
+    context 'there are no hooks' do
+      it { is_expected.to eq(false) }
+    end
+
+    context 'there are hooks' do
+      before do
+        create_list(:project_hook, 2, project: project)
+      end
+
+      it { is_expected.to eq(false) }
+
+      context 'when a hook has failed' do
+        before do
+          create(:project_hook, :permanently_disabled, project: project)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
+
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
 
diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
index ccfd403bebd25e..58f26894a473f6 100644
--- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb
+++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
@@ -60,6 +60,7 @@
       destroy_upload
       admin_achievement
       award_achievement
+      read_web_hooks
     ]
   end
 
-- 
GitLab


From afb8ea39a291baf8b35824fa3b14f81850c356c4 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Wed, 15 Feb 2023 19:06:18 +0100
Subject: [PATCH 2/8] Fix rebase related failures

---
 ee/app/helpers/ee/web_hooks/web_hooks_helper.rb      | 8 +-------
 ee/app/models/ee/group.rb                            | 4 ++--
 ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb   | 2 +-
 ee/spec/models/ee/group_spec.rb                      | 4 ++--
 spec/helpers/web_hooks/web_hooks_helper_spec.rb      | 8 ++++----
 spec/models/concerns/web_hooks/has_web_hooks_spec.rb | 2 +-
 spec/models/project_spec.rb                          | 4 ++--
 7 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
index ab55cf18ef20d1..a9ffd4b2cdc214 100644
--- a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
+++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
@@ -12,7 +12,7 @@ def show_group_hook_failed_callout?(group:)
         # Assumes include of Users::CalloutsHelper
         return false if web_hook_disabled_dismissed?(group)
 
-        any_group_hook_failed?(group) # Most expensive query last
+        group.fetch_web_hook_failure
       end
 
       private
@@ -20,12 +20,6 @@ def show_group_hook_failed_callout?(group:)
       def group_hook_page?
         current_controller?('groups/hooks') || current_controller?('groups/hook_logs')
       end
-
-      def any_group_hook_failed?(group)
-        any_hook_failed?("any_web_hook_failed:group:#{group.id}") do
-          group.any_web_hook_failed?
-        end
-      end
     end
   end
 end
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 69975bde87b79d..1830f9d9d0eadd 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -647,8 +647,8 @@ def execute_hooks(data, hooks_scope)
       end
     end
 
-    override :any_web_hook_failed?
-    def any_web_hook_failed?
+    override :any_hook_failed?
+    def any_hook_failed?
       # morally `hooks.disabled.exists?`, but since group hooks include
       # Unstoppable, we simply return `false`.
       false
diff --git a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index 2950a64503d2a2..5713af8b4e8b91 100644
--- a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -15,7 +15,7 @@
     allow(helper).to receive(:web_hook_disabled_dismissed?).with(group).and_return(callout_dismissed)
     allow(helper).to receive(:current_controller?).with('groups/hooks').and_return(viewing_hooks)
     allow(helper).to receive(:current_controller?).with('groups/hook_logs').and_return(viewing_hooks)
-    allow(group).to receive(:any_web_hook_failed?).and_return(a_hook_has_failed)
+    allow(group).to receive(:any_hook_failed?).and_return(a_hook_has_failed)
   end
 
   shared_context 'when we are not viewing the group hooks or the logs' do
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index 0a23af8e07616e..de967401cb7d02 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -1773,10 +1773,10 @@
     end
   end
 
-  describe '#any_web_hook_failed?' do
+  describe '#any_hook_failed?' do
     let_it_be(:group) { create(:group) }
 
-    subject { group.any_web_hook_failed? }
+    subject { group.any_hook_failed? }
 
     it { is_expected.to eq(false) }
   end
diff --git a/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index df62669d59ec19..fdd0be8095b2e8 100644
--- a/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -7,12 +7,10 @@
 
   let(:current_user) { nil }
   let(:callout_dismissed) { false }
-  let(:any_hook_failed) { false }
 
   before do
     allow(helper).to receive(:current_user).and_return(current_user)
     allow(helper).to receive(:web_hook_disabled_dismissed?).with(project).and_return(callout_dismissed)
-    allow(project).to receive(:any_web_hook_failed?).and_return(any_hook_failed)
   end
 
   shared_context 'user is logged in' do
@@ -30,10 +28,12 @@
   end
 
   shared_context 'a hook has failed' do
-    let(:any_hook_failed) { true }
+    before do
+      create(:project_hook, :permanently_disabled, project: project)
+    end
   end
 
-  describe '#show_project_hook_failed_callout?', :use_clean_rails_memory_store_caching do
+  describe '#show_project_hook_failed_callout?' do
     context 'all conditions are met' do
       include_context 'user is logged in'
       include_context 'the user has permission'
diff --git a/spec/models/concerns/web_hooks/has_web_hooks_spec.rb b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb
index cc08a01420ba1a..afb2406a96931d 100644
--- a/spec/models/concerns/web_hooks/has_web_hooks_spec.rb
+++ b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb
@@ -5,7 +5,7 @@
 RSpec.describe WebHooks::HasWebHooks, feature_category: :integrations do
   let(:minimal_test_class) do
     Class.new do
-      include HasWebHooks
+      include WebHooks::HasWebHooks
 
       def id
         1
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3e59e08c6ef74c..4ae209fadb4dcf 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5992,10 +5992,10 @@ def has_external_wiki
     end
   end
 
-  describe '#any_web_hook_failed?' do
+  describe '#any_hook_failed?' do
     let_it_be_with_reload(:project) { create(:project) }
 
-    subject { project.any_web_hook_failed? }
+    subject { project.any_hook_failed? }
 
     context 'there are no hooks' do
       it { is_expected.to eq(false) }
-- 
GitLab


From 35510f95db77e9e8aef5324851ec4fd2fec22554 Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Thu, 16 Feb 2023 09:38:39 +0100
Subject: [PATCH 3/8] Fixes specs and typos

---
 app/helpers/users/callouts_helper.rb          |  5 +-
 app/models/project.rb                         |  1 -
 ee/app/helpers/ee/users/callouts_helper.rb    |  1 +
 .../helpers/ee/users/callouts_helper_spec.rb  | 76 +++++++++++++++++++
 .../web_hooks/web_hooks_helper_spec.rb        |  2 +-
 spec/helpers/users/callouts_helper_spec.rb    | 44 ++++-------
 spec/models/project_spec.rb                   | 26 -------
 .../has_web_hooks_shared_examples.rb          |  2 +-
 8 files changed, 96 insertions(+), 61 deletions(-)

diff --git a/app/helpers/users/callouts_helper.rb b/app/helpers/users/callouts_helper.rb
index 8f4a069f5ffb8d..f0de16c3660acf 100644
--- a/app/helpers/users/callouts_helper.rb
+++ b/app/helpers/users/callouts_helper.rb
@@ -62,10 +62,9 @@ def show_security_newsletter_user_callout?
     def web_hook_disabled_dismissed?(object)
       return false unless object.respond_to?(:last_failure_redis_key)
 
-      case object
-      when Project
+      if object.is_a?(Project)
         user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, project: object)
-      when Group
+      else
         user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, group: object)
       end
     end
diff --git a/app/models/project.rb b/app/models/project.rb
index 43eef0178c6f03..f61b20c1cbf0a5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -42,7 +42,6 @@ class Project < ApplicationRecord
   include BlocksUnsafeSerialization
   include Subquery
   include IssueParent
-  include WebHooks::HasWebHooks
 
   extend Gitlab::Cache::RequestCache
   extend Gitlab::Utils::Override
diff --git a/ee/app/helpers/ee/users/callouts_helper.rb b/ee/app/helpers/ee/users/callouts_helper.rb
index 677662b38e95d0..1b75805698b40c 100644
--- a/ee/app/helpers/ee/users/callouts_helper.rb
+++ b/ee/app/helpers/ee/users/callouts_helper.rb
@@ -16,6 +16,7 @@ module CalloutsHelper
       EOA_BRONZE_PLAN_END_DATE = '2022-01-26'
       CL_SUBSCRIPTION_ACTIVATION = 'cloud_licensing_subscription_activation_banner'
       PROFILE_PERSONAL_ACCESS_TOKEN_EXPIRY = 'profile_personal_access_token_expiry'
+      WEB_HOOK_DISABLED = 'web_hook_disabled'
 
       def show_enable_hashed_storage_warning?
         return if hashed_storage_enabled?
diff --git a/ee/spec/helpers/ee/users/callouts_helper_spec.rb b/ee/spec/helpers/ee/users/callouts_helper_spec.rb
index 1a8b373ae6331c..c22e4642271fda 100644
--- a/ee/spec/helpers/ee/users/callouts_helper_spec.rb
+++ b/ee/spec/helpers/ee/users/callouts_helper_spec.rb
@@ -370,4 +370,80 @@
       end
     end
   end
+
+  describe '#web_hook_disabled_dismissed?' do
+    let_it_be(:user, refind: true) { create(:user) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    context 'without a project or a group' do
+      it 'is false' do
+        expect(helper).not_to be_web_hook_disabled_dismissed(nil)
+      end
+    end
+
+    shared_examples 'web_hook_disabled shared examples' do
+      context 'when the web-hook failure callout has never been dismissed' do
+        it 'is false' do
+          expect(helper).not_to be_web_hook_disabled_dismissed(container)
+        end
+      end
+
+      context 'when the web-hook failure callout has been dismissed', :freeze_time, :clean_gitlab_redis_shared_state do
+        before do
+          create(factory,
+            feature_name: described_class::WEB_HOOK_DISABLED,
+            user: user,
+            dismissed_at: 1.week.ago,
+            container_key => container)
+        end
+
+        it 'is true' do
+          expect(helper).to be_web_hook_disabled_dismissed(container)
+        end
+
+        context 'when there was an older failure' do
+          before do
+            Gitlab::Redis::SharedState.with { |r| r.set(key, 1.month.ago.iso8601) }
+          end
+
+          it 'is true' do
+            expect(helper).to be_web_hook_disabled_dismissed(container)
+          end
+        end
+
+        context 'when there has been a more recent failure' do
+          before do
+            Gitlab::Redis::SharedState.with { |r| r.set(key, 1.day.ago.iso8601) }
+          end
+
+          it 'is false' do
+            expect(helper).not_to be_web_hook_disabled_dismissed(container)
+          end
+        end
+      end
+    end
+
+    context 'with a project' do
+      let_it_be(:project) { create(:project) }
+      let(:factory) { :project_callout }
+      let(:container_key) { :project }
+      let(:container) { project }
+      let(:key) { "web_hooks:last_failure:project-#{project.id}" }
+
+      include_examples 'web_hook_disabled shared examples'
+    end
+
+    context 'with a group' do
+      let_it_be(:group) { create(:group) }
+      let(:factory) { :group_callout }
+      let(:container_key) { :group }
+      let(:container) { group }
+      let(:key) { "web_hooks:last_failure:group-#{group.id}" }
+
+      include_examples 'web_hook_disabled shared examples'
+    end
+  end
 end
diff --git a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index 5713af8b4e8b91..3b291df9cededf 100644
--- a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe WebHooks::WebHooksHelper, feature_category: :integrations do
+RSpec.describe WebHooks::WebHooksHelper, :clean_gitlab_redis_shared_state, feature_category: :integrations do
   let_it_be_with_reload(:group) { create(:group) } # rubocop: disable RSpec/FactoryBot/AvoidCreate
 
   let(:current_user) { nil }
diff --git a/spec/helpers/users/callouts_helper_spec.rb b/spec/helpers/users/callouts_helper_spec.rb
index b1c2a2587116c6..4cb179e4f60d64 100644
--- a/spec/helpers/users/callouts_helper_spec.rb
+++ b/spec/helpers/users/callouts_helper_spec.rb
@@ -166,72 +166,58 @@
   end
 
   describe '#web_hook_disabled_dismissed?' do
-    context 'without a container' do
+    context 'without a project' do
       it 'is false' do
         expect(helper).not_to be_web_hook_disabled_dismissed(nil)
       end
     end
 
-    shared_examples 'web_hook_disabled shared examples' do
+    context 'with a project' do
+      let_it_be(:project) { create(:project) }
+
       context 'the web-hook failure callout has never been dismissed' do
         it 'is false' do
-          expect(helper).not_to be_web_hook_disabled_dismissed(container)
+          expect(helper).not_to be_web_hook_disabled_dismissed(project)
         end
       end
 
       context 'the web-hook failure callout has been dismissed', :freeze_time do
         before do
-          create(factory,
+          create(:project_callout,
                  feature_name: described_class::WEB_HOOK_DISABLED,
                  user: user,
-                 dismissed_at: 1.week.ago,
-                 container_key => container)
+                 project: project,
+                 dismissed_at: 1.week.ago)
         end
 
         it 'is true' do
-          expect(helper).to be_web_hook_disabled_dismissed(container)
+          expect(helper).to be_web_hook_disabled_dismissed(project)
         end
 
         context 'when there was an older failure', :clean_gitlab_redis_shared_state do
+          let(:key) { "web_hooks:last_failure:project-#{project.id}" }
+
           before do
             Gitlab::Redis::SharedState.with { |r| r.set(key, 1.month.ago.iso8601) }
           end
 
           it 'is true' do
-            expect(helper).to be_web_hook_disabled_dismissed(container)
+            expect(helper).to be_web_hook_disabled_dismissed(project)
           end
         end
 
         context 'when there has been a more recent failure', :clean_gitlab_redis_shared_state do
+          let(:key) { "web_hooks:last_failure:project-#{project.id}" }
+
           before do
             Gitlab::Redis::SharedState.with { |r| r.set(key, 1.day.ago.iso8601) }
           end
 
           it 'is false' do
-            expect(helper).not_to be_web_hook_disabled_dismissed(container)
+            expect(helper).not_to be_web_hook_disabled_dismissed(project)
           end
         end
       end
     end
-
-    context 'with a project' do
-      let_it_be(:project) { create(:project) }
-      let(:factory) { :project_callout }
-      let(:container_key) { :project }
-      let(:container) { project }
-      let(:key) { "web_hooks:last_failure:project-#{project.id}" }
-
-      include_examples 'web_hook_disabled shared examples'
-    end
-
-    context 'with a group' do
-      let_it_be(:group) { create(:group) }
-      let(:factory) { :group_callout }
-      let(:container_key) { :group }
-      let(:container) { group }
-      let(:key) { "web_hooks:last_failure:group-#{group.id}" }
-
-      include_examples 'web_hook_disabled shared examples'
-    end
   end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4ae209fadb4dcf..ab50a3aa4805f8 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5992,32 +5992,6 @@ def has_external_wiki
     end
   end
 
-  describe '#any_hook_failed?' do
-    let_it_be_with_reload(:project) { create(:project) }
-
-    subject { project.any_hook_failed? }
-
-    context 'there are no hooks' do
-      it { is_expected.to eq(false) }
-    end
-
-    context 'there are hooks' do
-      before do
-        create_list(:project_hook, 2, project: project)
-      end
-
-      it { is_expected.to eq(false) }
-
-      context 'when a hook has failed' do
-        before do
-          create(:project_hook, :permanently_disabled, project: project)
-        end
-
-        it { is_expected.to eq(true) }
-      end
-    end
-  end
-
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
 
diff --git a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb
index cd6eb8c77faf1c..fa33db66c3bcf0 100644
--- a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb
+++ b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb
@@ -83,7 +83,7 @@
 
   describe '#fetch_web_hook_failure', :clean_gitlab_redis_shared_state do
     context 'when a value has not been stored' do
-      it 'does not call #any_hook_failed?' do
+      it 'calls #any_hook_failed?' do
         expect(object.get_web_hook_failure).to be_nil
         expect(object).to receive(:any_hook_failed?).and_return(true)
 
-- 
GitLab


From 639a8840951c85f07a71432d98750b582ccc2ffc Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Thu, 23 Feb 2023 11:03:51 +0100
Subject: [PATCH 4/8] Applies maintainer's feedback

---
 app/helpers/users/callouts_helper.rb          | 22 +++++++++----------
 app/helpers/web_hooks/web_hooks_helper.rb     | 16 +++++++++-----
 .../concerns/web_hooks/has_web_hooks.rb       |  2 --
 app/models/users/group_callout.rb             |  2 +-
 ee/app/helpers/ee/users/callouts_helper.rb    |  8 ++++++-
 .../helpers/ee/web_hooks/web_hooks_helper.rb  | 10 +--------
 ee/app/models/ee/group.rb                     |  4 ++--
 .../helpers/ee/users/callouts_helper_spec.rb  |  2 +-
 8 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/app/helpers/users/callouts_helper.rb b/app/helpers/users/callouts_helper.rb
index f0de16c3660acf..7a5dd8be77c264 100644
--- a/app/helpers/users/callouts_helper.rb
+++ b/app/helpers/users/callouts_helper.rb
@@ -60,13 +60,9 @@ def show_security_newsletter_user_callout?
     end
 
     def web_hook_disabled_dismissed?(object)
-      return false unless object.respond_to?(:last_failure_redis_key)
+      return false unless object.class < WebHooks::HasWebHooks
 
-      if object.is_a?(Project)
-        user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, project: object)
-      else
-        user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, group: object)
-      end
+      user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, object: object)
     end
 
     def show_merge_request_settings_callout?(project)
@@ -76,24 +72,26 @@ def show_merge_request_settings_callout?(project)
     def ultimate_feature_removal_banner_dismissed?(project)
       return false unless project
 
-      user_dismissed?(ULTIMATE_FEATURE_REMOVAL_BANNER, project: project)
+      user_dismissed?(ULTIMATE_FEATURE_REMOVAL_BANNER, object: project)
     end
 
     private
 
-    def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, project: nil, group: nil)
+    def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, object: nil)
       return false unless current_user
 
       query = { feature_name: feature_name, ignore_dismissal_earlier_than: ignore_dismissal_earlier_than }
 
-      if project
-        current_user.dismissed_callout_for_project?(project: project, **query)
-      elsif group
-        current_user.dismissed_callout_for_group?(group: group, **query)
+      if object
+        dismissed_callout?(object, query)
       else
         current_user.dismissed_callout?(**query)
       end
     end
+
+    def dismissed_callout?(object, query)
+      current_user.dismissed_callout_for_project?(project: object, **query)
+    end
   end
 end
 
diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index 024711fa2c9cab..7d4d266fc8bd75 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -4,17 +4,23 @@ module WebHooks
   module WebHooksHelper
     def show_project_hook_failed_callout?(project:)
       return false if project_hook_page?
+
+      show_hook_failed_callout?(project)
+    end
+
+    private
+
+    def show_hook_failed_callout?(object)
       return false unless current_user
-      return false unless Ability.allowed?(current_user, :read_web_hooks, project)
+
+      return false unless Ability.allowed?(current_user, :read_web_hooks, object)
 
       # Assumes include of Users::CalloutsHelper
-      return false if web_hook_disabled_dismissed?(project)
+      return false if web_hook_disabled_dismissed?(object)
 
-      project.fetch_web_hook_failure
+      object.fetch_web_hook_failure
     end
 
-    private
-
     def project_hook_page?
       current_controller?('projects/hooks') || current_controller?('projects/hook_logs')
     end
diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb
index 61cfdd5c28b461..2183cc3c44b0b2 100644
--- a/app/models/concerns/web_hooks/has_web_hooks.rb
+++ b/app/models/concerns/web_hooks/has_web_hooks.rb
@@ -2,8 +2,6 @@
 
 module WebHooks
   module HasWebHooks
-    extend ActiveSupport::Concern
-
     WEB_HOOK_CACHE_EXPIRY = 1.hour
 
     def any_hook_failed?
diff --git a/app/models/users/group_callout.rb b/app/models/users/group_callout.rb
index 863ebddad998d9..1e2afdc28d516b 100644
--- a/app/models/users/group_callout.rb
+++ b/app/models/users/group_callout.rb
@@ -25,7 +25,7 @@ class GroupCallout < ApplicationRecord
       usage_quota_trial_alert: 14, # EE-only
       preview_usage_quota_free_plan_alert: 15, # EE-only
       enforcement_at_limit_alert: 16, # EE-only
-      web_hook_disabled: 17
+      web_hook_disabled: 17 # EE-only
     }
 
     validates :group, presence: true
diff --git a/ee/app/helpers/ee/users/callouts_helper.rb b/ee/app/helpers/ee/users/callouts_helper.rb
index 1b75805698b40c..a7ebf8ead8faa8 100644
--- a/ee/app/helpers/ee/users/callouts_helper.rb
+++ b/ee/app/helpers/ee/users/callouts_helper.rb
@@ -16,7 +16,6 @@ module CalloutsHelper
       EOA_BRONZE_PLAN_END_DATE = '2022-01-26'
       CL_SUBSCRIPTION_ACTIVATION = 'cloud_licensing_subscription_activation_banner'
       PROFILE_PERSONAL_ACCESS_TOKEN_EXPIRY = 'profile_personal_access_token_expiry'
-      WEB_HOOK_DISABLED = 'web_hook_disabled'
 
       def show_enable_hashed_storage_warning?
         return if hashed_storage_enabled?
@@ -92,6 +91,13 @@ def show_verification_reminder?
 
       private
 
+      override :dismissed_callout?
+      def dismissed_callout?(object, query)
+        return super if object.is_a?(Project)
+
+        current_user.dismissed_callout_for_group?(group: object, **query)
+      end
+
       def eoa_bronze_plan_end_date
         Date.parse(EOA_BRONZE_PLAN_END_DATE)
       end
diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
index a9ffd4b2cdc214..e502360e09ac53 100644
--- a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
+++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
@@ -5,18 +5,10 @@ module WebHooks
     module WebHooksHelper
       def show_group_hook_failed_callout?(group:)
         return false if group_hook_page?
-        return false unless current_user
 
-        return false unless Ability.allowed?(current_user, :read_web_hooks, group)
-
-        # Assumes include of Users::CalloutsHelper
-        return false if web_hook_disabled_dismissed?(group)
-
-        group.fetch_web_hook_failure
+        show_hook_failed_callout?(group)
       end
 
-      private
-
       def group_hook_page?
         current_controller?('groups/hooks') || current_controller?('groups/hook_logs')
       end
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 1830f9d9d0eadd..df4c4fbdef27e6 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -649,8 +649,8 @@ def execute_hooks(data, hooks_scope)
 
     override :any_hook_failed?
     def any_hook_failed?
-      # morally `hooks.disabled.exists?`, but since group hooks include
-      # Unstoppable, we simply return `false`.
+      # morally `hooks.disabled.exists?`, but since the GroupHook model includes
+      # WebHooks::Unstoppable, we simply return `false`.
       false
     end
 
diff --git a/ee/spec/helpers/ee/users/callouts_helper_spec.rb b/ee/spec/helpers/ee/users/callouts_helper_spec.rb
index c22e4642271fda..63ec2758060596 100644
--- a/ee/spec/helpers/ee/users/callouts_helper_spec.rb
+++ b/ee/spec/helpers/ee/users/callouts_helper_spec.rb
@@ -394,7 +394,7 @@
       context 'when the web-hook failure callout has been dismissed', :freeze_time, :clean_gitlab_redis_shared_state do
         before do
           create(factory,
-            feature_name: described_class::WEB_HOOK_DISABLED,
+            feature_name: Users::CalloutsHelper::WEB_HOOK_DISABLED,
             user: user,
             dismissed_at: 1.week.ago,
             container_key => container)
-- 
GitLab


From a985d6232d6df7fc5d2a8fbd55c02159199225a1 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Fri, 24 Feb 2023 14:19:11 +0100
Subject: [PATCH 5/8] Remove replace read_web_hooks policy

---
 app/helpers/web_hooks/web_hooks_helper.rb                       | 2 +-
 app/policies/project_policy.rb                                  | 1 -
 .../shared_contexts/policies/group_policy_shared_context.rb     | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index 7d4d266fc8bd75..d8dd31b3de5d1b 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -13,7 +13,7 @@ def show_project_hook_failed_callout?(project:)
     def show_hook_failed_callout?(object)
       return false unless current_user
 
-      return false unless Ability.allowed?(current_user, :read_web_hooks, object)
+      return false unless Ability.allowed?(current_user, :read_web_hook, object)
 
       # Assumes include of Users::CalloutsHelper
       return false if web_hook_disabled_dismissed?(object)
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 2cbe9ad83cc560..1d635b45207dfd 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -531,7 +531,6 @@ class ProjectPolicy < BasePolicy
     enable :update_runners_registration_token
     enable :admin_project_google_cloud
     enable :admin_secure_files
-    enable :read_web_hooks
     enable :read_upload
     enable :destroy_upload
     enable :admin_incident_management_timeline_event_tag
diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
index 58f26894a473f6..ccfd403bebd25e 100644
--- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb
+++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
@@ -60,7 +60,6 @@
       destroy_upload
       admin_achievement
       award_achievement
-      read_web_hooks
     ]
   end
 
-- 
GitLab


From f3f2793e5ed5c5a8d7a2e95d0fb7f43d81e1dc98 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Tue, 28 Feb 2023 15:31:03 +0100
Subject: [PATCH 6/8] Replace access check with admin_object policies

---
 app/helpers/web_hooks/web_hooks_helper.rb          |  8 +++++++-
 app/policies/group_policy.rb                       |  1 -
 ee/app/helpers/ee/web_hooks/web_hooks_helper.rb    | 13 +++++++++++++
 ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb |  2 +-
 4 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index d8dd31b3de5d1b..95931264be6161 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -13,7 +13,7 @@ def show_project_hook_failed_callout?(project:)
     def show_hook_failed_callout?(object)
       return false unless current_user
 
-      return false unless Ability.allowed?(current_user, :read_web_hook, object)
+      return false unless can_access_web_hooks?(object)
 
       # Assumes include of Users::CalloutsHelper
       return false if web_hook_disabled_dismissed?(object)
@@ -24,6 +24,12 @@ def show_hook_failed_callout?(object)
     def project_hook_page?
       current_controller?('projects/hooks') || current_controller?('projects/hook_logs')
     end
+
+    def can_access_web_hooks?(object)
+      return true if Ability.allowed?(current_user, :admin_project, object)
+
+      false
+    end
   end
 end
 
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index fb4887fd98543b..e6ff5c870c2c42 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -203,7 +203,6 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :read_upload
     enable :destroy_upload
     enable :admin_achievement
-    enable :read_web_hooks
   end
 
   rule { owner }.policy do
diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
index e502360e09ac53..875c810100b1b7 100644
--- a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
+++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
@@ -12,6 +12,19 @@ def show_group_hook_failed_callout?(group:)
       def group_hook_page?
         current_controller?('groups/hooks') || current_controller?('groups/hook_logs')
       end
+
+      private
+
+      def can_access_web_hooks?(object)
+        case object
+        when Group
+          return true if Ability.allowed?(current_user, :admin_group, object)
+        when Project
+          return true if Ability.allowed?(current_user, :admin_project, object)
+        end
+
+        false
+      end
     end
   end
 end
diff --git a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index 3b291df9cededf..81b93372785b86 100644
--- a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -24,7 +24,7 @@
 
   shared_context 'when the user has permission' do
     before do
-      group.add_maintainer(current_user) if current_user
+      group.add_owner(current_user) if current_user
     end
   end
 
-- 
GitLab


From b074748c68b182a110894d72381d25118ed8fc90 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Tue, 28 Feb 2023 17:01:04 +0100
Subject: [PATCH 7/8] Apply review suggestions

---
 app/helpers/web_hooks/web_hooks_helper.rb       | 4 +---
 ee/app/helpers/ee/web_hooks/web_hooks_helper.rb | 9 ++-------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index 95931264be6161..ad792f761f82e1 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -26,9 +26,7 @@ def project_hook_page?
     end
 
     def can_access_web_hooks?(object)
-      return true if Ability.allowed?(current_user, :admin_project, object)
-
-      false
+      Ability.allowed?(current_user, :admin_project, object)
     end
   end
 end
diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
index 875c810100b1b7..39052b2599c90d 100644
--- a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
+++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb
@@ -16,14 +16,9 @@ def group_hook_page?
       private
 
       def can_access_web_hooks?(object)
-        case object
-        when Group
-          return true if Ability.allowed?(current_user, :admin_group, object)
-        when Project
-          return true if Ability.allowed?(current_user, :admin_project, object)
-        end
+        return super if object.is_a?(Project)
 
-        false
+        Ability.allowed?(current_user, :admin_group, object)
       end
     end
   end
-- 
GitLab


From cf421c494ddf845df313ea63d518ab569f2fa370 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Tue, 28 Feb 2023 21:30:10 +0100
Subject: [PATCH 8/8] Fix conflict leftover

---
 app/policies/group_policy.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index e6ff5c870c2c42..ae736865e1772c 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -202,7 +202,6 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :maintainer_access
     enable :read_upload
     enable :destroy_upload
-    enable :admin_achievement
   end
 
   rule { owner }.policy do
-- 
GitLab