Automatically create integration webhooks when missing

Previously we were silently failing if the webhook could not be saved
when the integration was edited, which led to further exceptions later
when the integration was trying to execute the webhook.

The webhook is really only used to associate records in `web_hook_logs`,
so we can create it automatically when needed and don't have to
backfill existing integrations.

We now also raise errors if the webhook cannot be saved, which should
help us diagnose why this is failing in the first place.

Changelog: fixed

app/controllers/projects/service_hook_logs_controller.rb

 # frozen_string_literal: true
 
 class Projects::ServiceHookLogsController < Projects::HookLogsController
+  extend Gitlab::Utils::Override
+
   before_action :integration, only: [:show, :retry]
 
   def retry
@@ -10,11 +12,12 @@ class Projects::ServiceHookLogsController < Projects::HookLogsController
 
   private
 
-  def hook
-    @hook ||= integration.service_hook
-  end
-
   def integration
     @integration ||= @project.find_or_initialize_integration(params[:service_id])
   end
+
+  override :hook
+  def hook
+    @hook ||= integration.service_hook || not_found
+  end
 end

app/models/concerns/integrations/has_web_hook.rb

+# frozen_string_literal: true
+
+module Integrations
+  module HasWebHook
+    extend ActiveSupport::Concern
+
+    included do
+      after_save :update_web_hook!, if: :activated?
+    end
+
+    # Return the URL to be used for the webhook.
+    def hook_url
+      raise NotImplementedError
+    end
+
+    # Return whether the webhook should use SSL verification.
+    def hook_ssl_verification
+      true
+    end
+
+    # Create or update the webhook, raising an exception if it cannot be saved.
+    def update_web_hook!
+      hook = service_hook || build_service_hook
+      hook.url = hook_url if hook.url != hook_url # avoid reencryption
+      hook.enable_ssl_verification = hook_ssl_verification
+      hook.save! if hook.changed?
+      hook
+    end
+
+    # Execute the webhook, creating it if necessary.
+    def execute_web_hook!(*args)
+      update_web_hook!
+      service_hook.execute(*args)
+    end
+  end
+end

app/models/integrations/bamboo.rb

@@ -18,14 +18,8 @@ module Integrations
 
     attr_accessor :response
 
-    after_save :compose_service_hook, if: :activated?
     before_update :reset_password
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.save
-    end
-
     def reset_password
       if bamboo_url_changed? && !password_touched?
         self.password = nil

app/models/integrations/buildkite.rb

@@ -4,7 +4,9 @@ require "addressable/uri"
 
 module Integrations
   class Buildkite < BaseCi
+    include HasWebHook
     include ReactiveService
+    extend Gitlab::Utils::Override
 
     ENDPOINT = "https://buildkite.com"
 
@@ -13,8 +15,6 @@ module Integrations
     validates :project_url, presence: true, public_url: true, if: :activated?
     validates :token, presence: true, if: :activated?
 
-    after_save :compose_service_hook, if: :activated?
-
     def self.supported_events
       %w(push merge_request tag_push)
     end
@@ -35,21 +35,15 @@ module Integrations
       self.properties.delete('enable_ssl_verification') # Remove unused key
     end
 
-    def webhook_url
+    override :hook_url
+    def hook_url
       "#{buildkite_endpoint('webhook')}/deliver/#{webhook_token}"
     end
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.url = webhook_url
-      hook.enable_ssl_verification = true
-      hook.save
-    end
-
     def execute(data)
       return unless supported_events.include?(data[:object_kind])
 
-      service_hook.execute(data)
+      execute_web_hook!(data)
     end
 
     def commit_status(sha, ref)

app/models/integrations/datadog.rb

@@ -2,6 +2,9 @@
 
 module Integrations
   class Datadog < Integration
+    include HasWebHook
+    extend Gitlab::Utils::Override
+
     DEFAULT_DOMAIN = 'datadoghq.com'
     URL_TEMPLATE = 'https://webhooks-http-intake.logs.%{datadog_domain}/api/v2/webhook'
     URL_TEMPLATE_API_KEYS = 'https://app.%{datadog_domain}/account/settings#api'
@@ -21,8 +24,6 @@ module Integrations
       validates :api_url, presence: true, unless: -> (obj) { obj.datadog_site.present? }
     end
 
-    after_save :compose_service_hook, if: :activated?
-
     def initialize_properties
       super
 
@@ -98,12 +99,7 @@ module Integrations
       ]
     end
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.url = hook_url
-      hook.save
-    end
-
+    override :hook_url
     def hook_url
       url = api_url.presence || sprintf(URL_TEMPLATE, datadog_domain: datadog_domain)
       url = URI.parse(url)
@@ -127,7 +123,7 @@ module Integrations
       object_kind = 'job' if object_kind == 'build'
       return unless supported_events.include?(object_kind)
 
-      service_hook.execute(data, "#{object_kind} hook")
+      execute_web_hook!(data, "#{object_kind} hook")
     end
 
     def test(data)

app/models/integrations/drone_ci.rb

@@ -2,8 +2,10 @@
 
 module Integrations
   class DroneCi < BaseCi
+    include HasWebHook
     include ReactiveService
     include ServicePushDataValidations
+    extend Gitlab::Utils::Override
 
     prop_accessor :drone_url, :token
     boolean_accessor :enable_ssl_verification
@@ -11,24 +13,16 @@ module Integrations
     validates :drone_url, presence: true, public_url: true, if: :activated?
     validates :token, presence: true, if: :activated?
 
-    after_save :compose_service_hook, if: :activated?
-
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      # If using a service template, project may not be available
-      hook.url = [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join if project
-      hook.enable_ssl_verification = !!enable_ssl_verification
-      hook.save
-    end
-
     def execute(data)
+      return unless project
+
       case data[:object_kind]
       when 'push'
-        service_hook.execute(data) if push_valid?(data)
+        execute_web_hook!(data) if push_valid?(data)
       when 'merge_request'
-        service_hook.execute(data) if merge_request_valid?(data)
+        execute_web_hook!(data) if merge_request_valid?(data)
       when 'tag_push'
-        service_hook.execute(data) if tag_push_valid?(data)
+        execute_web_hook!(data) if tag_push_valid?(data)
       end
     end
 
@@ -105,5 +99,21 @@ module Integrations
         { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" }
       ]
     end
+
+    override :hook_url
+    def hook_url
+      [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join
+    end
+
+    override :hook_ssl_verification
+    def hook_ssl_verification
+      !!enable_ssl_verification
+    end
+
+    override :update_web_hook!
+    def update_web_hook!
+      # If using a service template, project may not be available
+      super if project
+    end
   end
 end

app/models/integrations/jenkins.rb

@@ -2,7 +2,9 @@
 
 module Integrations
   class Jenkins < BaseCi
+    include HasWebHook
     include ActionView::Helpers::UrlHelper
+    extend Gitlab::Utils::Override
 
     prop_accessor :jenkins_url, :project_name, :username, :password
 
@@ -16,8 +18,6 @@ module Integrations
     default_value_for :merge_requests_events, false
     default_value_for :tag_push_events, false
 
-    after_save :compose_service_hook, if: :activated?
-
     def reset_password
       # don't reset the password if a new one is provided
       if (jenkins_url_changed? || username.blank?) && !password_touched?
@@ -25,16 +25,10 @@ module Integrations
       end
     end
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.url = hook_url
-      hook.save
-    end
-
     def execute(data)
       return unless supported_events.include?(data[:object_kind])
 
-      service_hook.execute(data, "#{data[:object_kind]}_hook")
+      execute_web_hook!(data, "#{data[:object_kind]}_hook")
     end
 
     def test(data)
@@ -48,6 +42,7 @@ module Integrations
       { success: true, result: result[:message] }
     end
 
+    override :hook_url
     def hook_url
       url = URI.parse(jenkins_url)
       url.path = File.join(url.path || '/', "project/#{project_name}")

app/models/integrations/packagist.rb

@@ -2,6 +2,9 @@
 
 module Integrations
   class Packagist < Integration
+    include HasWebHook
+    extend Gitlab::Utils::Override
+
     prop_accessor :username, :token, :server
 
     validates :username, presence: true, if: :activated?
@@ -10,8 +13,6 @@ module Integrations
     default_value_for :push_events, true
     default_value_for :tag_push_events, true
 
-    after_save :compose_service_hook, if: :activated?
-
     def title
       'Packagist'
     end
@@ -39,7 +40,7 @@ module Integrations
     def execute(data)
       return unless supported_events.include?(data[:object_kind])
 
-      service_hook.execute(data)
+      execute_web_hook!(data)
     end
 
     def test(data)
@@ -53,12 +54,7 @@ module Integrations
       { success: true, result: result[:message] }
     end
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.url = hook_url
-      hook.save
-    end
-
+    override :hook_url
     def hook_url
       base_url = server.presence || 'https://packagist.org'
       "#{base_url}/api/update-package?username=#{username}&apiToken=#{token}"

app/models/integrations/teamcity.rb

@@ -18,7 +18,6 @@ module Integrations
 
     attr_accessor :response
 
-    after_save :compose_service_hook, if: :activated?
     before_update :reset_password
 
     class << self
@@ -31,11 +30,6 @@ module Integrations
       end
     end
 
-    def compose_service_hook
-      hook = service_hook || build_service_hook
-      hook.save
-    end
-
     def reset_password
       if teamcity_url_changed? && !password_touched?
         self.password = nil

app/workers/project_service_worker.rb

@@ -15,6 +15,6 @@ class ProjectServiceWorker # rubocop:disable Scalability/IdempotentWorker
     integration.execute(data)
   rescue StandardError => error
     integration_class = integration&.class&.name || "Not Found"
-    logger.error class: self.class.name, service_class: integration_class, message: error.message
+    Gitlab::ErrorTracking.log_exception(error, integration_class: integration_class)
   end
 end

spec/controllers/projects/service_hook_logs_controller_spec.rb

@@ -27,6 +27,15 @@ RSpec.describe Projects::ServiceHookLogsController do
     specify do
       expect(response).to be_successful
     end
+
+    it 'renders a 404 if the hook does not exist' do
+      log_params
+      integration.service_hook.destroy!
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
   end
 
   describe 'POST #retry' do
@@ -37,5 +46,14 @@ RSpec.describe Projects::ServiceHookLogsController do
       expect_any_instance_of(described_class).to receive(:set_hook_execution_notice)
       expect(subject).to redirect_to(edit_project_service_path(project, integration))
     end
+
+    it 'renders a 404 if the hook does not exist' do
+      log_params
+      integration.service_hook.destroy!
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
   end
 end

spec/models/integration_spec.rb

@@ -9,11 +9,12 @@ RSpec.describe Integration do
   let_it_be(:project) { create(:project, group: group) }
 
   describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to belong_to :group }
-    it { is_expected.to have_one :service_hook }
-    it { is_expected.to have_one :jira_tracker_data }
-    it { is_expected.to have_one :issue_tracker_data }
+    it { is_expected.to belong_to(:project).inverse_of(:integrations) }
+    it { is_expected.to belong_to(:group).inverse_of(:integrations) }
+    it { is_expected.to have_one(:service_hook).inverse_of(:integration).with_foreign_key(:service_id) }
+    it { is_expected.to have_one(:issue_tracker_data).autosave(true).inverse_of(:integration).with_foreign_key(:service_id).class_name('Integrations::IssueTrackerData') }
+    it { is_expected.to have_one(:jira_tracker_data).autosave(true).inverse_of(:integration).with_foreign_key(:service_id).class_name('Integrations::JiraTrackerData') }
+    it { is_expected.to have_one(:open_project_tracker_data).autosave(true).inverse_of(:integration).with_foreign_key(:service_id).class_name('Integrations::OpenProjectTrackerData') }
   end
 
   describe 'validations' do

spec/models/integrations/asana_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Asana do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'active' do
       before do
@@ -42,7 +37,6 @@ RSpec.describe Integrations::Asana do
       allow(@asana).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         api_key: 'verySecret',
         restrict_to_branch: 'master'
       )

spec/models/integrations/assembla_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 RSpec.describe Integrations::Assembla do
   include StubRequests
 
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project, :repository) }
@@ -19,7 +14,6 @@ RSpec.describe Integrations::Assembla do
       allow(@assembla_integration).to receive_messages(
         project_id: project.id,
         project: project,
-        service_hook: true,
         token: 'verySecret',
         subdomain: 'project_name'
       )

spec/models/integrations/bamboo_spec.rb

@@ -22,11 +22,6 @@ RSpec.describe Integrations::Bamboo, :use_clean_rails_memory_store_caching do
     )
   end
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when active' do
       before do

spec/models/integrations/base_chat_notification_spec.rb

@@ -28,7 +28,6 @@ RSpec.describe Integrations::BaseChatNotification do
       allow(chat_integration).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         webhook: webhook_url
       )
 

spec/models/integrations/bugzilla_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Bugzilla do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/buildkite_spec.rb

@@ -12,16 +12,14 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
     described_class.create!(
       project: project,
       properties: {
-        service_hook: true,
         project_url: 'https://buildkite.com/organization-name/example-pipeline',
         token: 'secret-sauce-webhook-token:secret-sauce-status-token'
       }
     )
   end
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
+  it_behaves_like Integrations::HasWebHook do
+    let(:hook_url) { 'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token' }
   end
 
   describe 'Validations' do
@@ -66,9 +64,9 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
         .to change { integration.service_hook.enable_ssl_verification }.from(false).to(true)
     end
 
-    describe '#webhook_url' do
+    describe '#hook_url' do
       it 'returns the webhook url' do
-        expect(integration.webhook_url).to eq(
+        expect(integration.hook_url).to eq(
           'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token'
         )
       end

spec/models/integrations/campfire_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 RSpec.describe Integrations::Campfire do
   include StubRequests
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do
@@ -37,7 +32,6 @@ RSpec.describe Integrations::Campfire do
       allow(@campfire_integration).to receive_messages(
         project_id: project.id,
         project: project,
-        service_hook: true,
         token: 'verySecret',
         subdomain: 'project-name',
         room: 'test-room'

spec/models/integrations/confluence_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Confluence do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     before do
       subject.active = active

spec/models/integrations/custom_issue_tracker_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::CustomIssueTracker do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/datadog_spec.rb

@@ -38,9 +38,9 @@ RSpec.describe Integrations::Datadog do
   let(:pipeline_data) { Gitlab::DataBuilder::Pipeline.build(pipeline) }
   let(:build_data) { Gitlab::DataBuilder::Build.build(build) }
 
-  describe 'associations' do
-    it { is_expected.to belong_to(:project) }
-    it { is_expected.to have_one(:service_hook) }
+  it_behaves_like Integrations::HasWebHook do
+    let(:integration) { instance }
+    let(:hook_url) { "#{described_class::URL_TEMPLATE % { datadog_domain: dd_site }}?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" }
   end
 
   describe 'validations' do

spec/models/integrations/discord_spec.rb

@@ -35,7 +35,6 @@ RSpec.describe Integrations::Discord do
       allow(subject).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         webhook: webhook_url
       )
 

spec/models/integrations/drone_ci_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do
   include ReactiveCachingHelpers
 
-  describe 'associations' do
-    it { is_expected.to belong_to(:project) }
-    it { is_expected.to have_one(:service_hook) }
-  end
-
   describe 'validations' do
     context 'active' do
       before do
@@ -32,7 +27,15 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do
   end
 
   shared_context :drone_ci_integration do
-    let(:drone)      { described_class.new }
+    subject(:drone) do
+      described_class.new(
+        project: project,
+        active: true,
+        drone_url: drone_url,
+        token: token
+      )
+    end
+
     let(:project)    { create(:project, :repository, name: 'project') }
     let(:path)       { project.full_path }
     let(:drone_url)  { 'http://drone.example.com' }
@@ -45,16 +48,6 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do
     let(:build_page) { "#{drone_url}/gitlab/#{path}/redirect/commits/#{sha}?branch=#{branch}" }
     let(:commit_status_path) { "#{drone_url}/gitlab/#{path}/commits/#{sha}?branch=#{branch}&access_token=#{token}" }
 
-    before do
-      allow(drone).to receive_messages(
-        project_id: project.id,
-        project: project,
-        active: true,
-        drone_url: drone_url,
-        token: token
-      )
-    end
-
     def stub_request(status: 200, body: nil)
       body ||= %q({"status":"success"})
 
@@ -66,6 +59,20 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do
     end
   end
 
+  it_behaves_like Integrations::HasWebHook do
+    include_context :drone_ci_integration
+
+    let(:integration) { drone }
+    let(:hook_url) { "#{drone_url}/hook?owner=#{project.namespace.full_path}&name=#{project.path}&access_token=#{token}" }
+
+    it 'does not create a hook if project is not present' do
+      integration.project = nil
+      integration.instance = true
+
+      expect { integration.save! }.not_to change(ServiceHook, :count)
+    end
+  end
+
   describe "integration page/path methods" do
     include_context :drone_ci_integration
 
@@ -137,10 +144,17 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do
       Gitlab::DataBuilder::Push.build_sample(project, user)
     end
 
-    it do
-      service_hook = double
-      expect(service_hook).to receive(:execute)
-      expect(drone).to receive(:service_hook).and_return(service_hook)
+    it 'executes the webhook' do
+      expect(drone).to receive(:execute_web_hook!).with(push_sample_data)
+
+      drone.execute(push_sample_data)
+    end
+
+    it 'does not try to execute the webhook if the integration is not in a project' do
+      drone.project = nil
+      drone.instance = true
+
+      expect(drone).not_to receive(:execute_web_hook!)
 
       drone.execute(push_sample_data)
     end

spec/models/integrations/ewm_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Ewm do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/external_wiki_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::ExternalWiki do
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/flowdock_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Flowdock do
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do
@@ -38,7 +33,6 @@ RSpec.describe Integrations::Flowdock do
       allow(flowdock_integration).to receive_messages(
         project_id: project.id,
         project: project,
-        service_hook: true,
         token: 'verySecret'
       )
       WebMock.stub_request(:post, api_url)

spec/models/integrations/irker_spec.rb

@@ -5,11 +5,6 @@ require 'socket'
 require 'json'
 
 RSpec.describe Integrations::Irker do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do
@@ -46,7 +41,6 @@ RSpec.describe Integrations::Irker do
         active: true,
         project: project,
         project_id: project.id,
-        service_hook: true,
         server_host: @irker_server.addr[2],
         server_port: @irker_server.addr[1],
         default_irc_uri: 'irc://chat.freenode.net/',

spec/models/integrations/jenkins_spec.rb

@@ -24,9 +24,9 @@ RSpec.describe Integrations::Jenkins do
 
   let(:jenkins_authorization) { "Basic " + ::Base64.strict_encode64(jenkins_username + ':' + jenkins_password) }
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
+  it_behaves_like Integrations::HasWebHook do
+    let(:integration) { described_class.new(jenkins_params) }
+    let(:hook_url) { "http://#{ERB::Util.url_encode jenkins_username}:#{ERB::Util.url_encode jenkins_password}@jenkins.example.com/project/my_project" }
   end
 
   describe 'username validation' do

spec/models/integrations/jira_spec.rb

@@ -109,11 +109,6 @@ RSpec.describe Integrations::Jira do
     end
   end
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe '.reference_pattern' do
     using RSpec::Parameterized::TableSyntax
 

spec/models/integrations/microsoft_teams_spec.rb

@@ -6,11 +6,6 @@ RSpec.describe Integrations::MicrosoftTeams do
   let(:chat_integration) { described_class.new }
   let(:webhook_url) { 'https://example.gitlab.com/' }
 
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do
@@ -45,7 +40,6 @@ RSpec.describe Integrations::MicrosoftTeams do
       allow(chat_integration).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         webhook: webhook_url
       )
 
@@ -142,7 +136,6 @@ RSpec.describe Integrations::MicrosoftTeams do
       allow(chat_integration).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         webhook: webhook_url
       )
 
@@ -224,7 +217,6 @@ RSpec.describe Integrations::MicrosoftTeams do
     before do
       allow(chat_integration).to receive_messages(
         project: project,
-        service_hook: true,
         webhook: webhook_url
       )
     end

spec/models/integrations/open_project_spec.rb

@@ -27,9 +27,4 @@ RSpec.describe Integrations::OpenProject do
       it { is_expected.not_to validate_presence_of(:project_identifier_code) }
     end
   end
-
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
 end

spec/models/integrations/packagist_spec.rb

@@ -24,9 +24,9 @@ RSpec.describe Integrations::Packagist do
   let(:packagist_server) { 'https://packagist.example.com' }
   let(:project) { create(:project) }
 
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
+  it_behaves_like Integrations::HasWebHook do
+    let(:integration) { described_class.new(packagist_params) }
+    let(:hook_url) { "#{packagist_server}/api/update-package?username=#{packagist_username}&apiToken=#{packagist_token}" }
   end
 
   describe '#execute' do

spec/models/integrations/pivotaltracker_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 RSpec.describe Integrations::Pivotaltracker do
   include StubRequests
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/prometheus_spec.rb

@@ -12,10 +12,6 @@ RSpec.describe Integrations::Prometheus, :use_clean_rails_memory_store_caching,
 
   let(:integration) { project.prometheus_integration }
 
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-  end
-
   context 'redirects' do
     it 'does not follow redirects' do
       redirect_to = 'https://redirected.example.com'

spec/models/integrations/pushover_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 RSpec.describe Integrations::Pushover do
   include StubRequests
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do
@@ -51,7 +46,6 @@ RSpec.describe Integrations::Pushover do
       allow(pushover).to receive_messages(
         project: project,
         project_id: project.id,
-        service_hook: true,
         api_key: api_key,
         user_key: user_key,
         device: device,

spec/models/integrations/redmine_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Redmine do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     # if redmine is set in setting the urls are set to defaults
     # therefore the validation passes as the values are not nil

spec/models/integrations/teamcity_spec.rb

@@ -22,11 +22,6 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do
     )
   end
 
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/models/integrations/youtrack_spec.rb

@@ -3,11 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Youtrack do
-  describe 'Associations' do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
   describe 'Validations' do
     context 'when integration is active' do
       before do

spec/support/shared_contexts/features/integrations/integrations_shared_context.rb

@@ -18,6 +18,8 @@ Integration.available_integration_names.each do |service|
           hash.merge!(k => 'https://example.atlassian.net/wiki')
         elsif service == 'datadog' && k == :datadog_site
           hash.merge!(k => 'datadoghq.com')
+        elsif service == 'packagist' && k == :server
+          hash.merge!(k => 'https://packagist.example.com')
         elsif k =~ /^(.*_url|url|webhook)/
           hash.merge!(k => "http://example.com")
         elsif service_klass.method_defined?("#{k}?")

spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples Integrations::HasWebHook do
+  include AfterNextHelpers
+
+  describe 'callbacks' do
+    it 'calls #update_web_hook! when enabled' do
+      expect(integration).to receive(:update_web_hook!)
+
+      integration.active = true
+      integration.save!
+    end
+
+    it 'does not call #update_web_hook! when disabled' do
+      expect(integration).not_to receive(:update_web_hook!)
+
+      integration.active = false
+      integration.save!
+    end
+
+    it 'does not call #update_web_hook! when validation fails' do
+      expect(integration).not_to receive(:update_web_hook!)
+
+      integration.active = true
+      integration.project = nil
+      expect(integration.save).to be(false)
+    end
+  end
+
+  describe '#hook_url' do
+    it 'returns a string' do
+      expect(integration.hook_url).to be_a(String)
+    end
+  end
+
+  describe '#hook_ssl_verification' do
+    it 'returns a boolean' do
+      expect(integration.hook_ssl_verification).to be_in([true, false])
+    end
+  end
+
+  describe '#update_web_hook!' do
+    def call
+      integration.update_web_hook!
+    end
+
+    it 'creates or updates a service hook' do
+      expect { call }.to change(ServiceHook, :count).by(1)
+      expect(integration.service_hook.url).to eq(hook_url)
+
+      integration.service_hook.update!(url: 'http://other.com')
+
+      expect { call }.to change { integration.service_hook.reload.url }.from('http://other.com').to(hook_url)
+    end
+
+    it 'raises an error if the service hook could not be saved' do
+      call
+      integration.service_hook.integration = nil
+
+      expect { call }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it 'does not attempt to save the service hook if there are no changes' do
+      call
+
+      expect(integration.service_hook).not_to receive(:save!)
+
+      call
+    end
+  end
+
+  describe '#execute_web_hook!' do
+    let(:args) { ['foo', [1, 2, 3]] }
+
+    def call
+      integration.execute_web_hook!(*args)
+    end
+
+    it 'creates the webhook if necessary and executes it' do
+      expect_next(ServiceHook).to receive(:execute).with(*args)
+      expect { call }.to change(ServiceHook, :count).by(1)
+
+      expect(integration.service_hook).to receive(:execute).with(*args)
+      expect { call }.not_to change(ServiceHook, :count)
+    end
+
+    it 'raises an error if the service hook could not be saved' do
+      expect_next(ServiceHook).to receive(:execute).with(*args)
+
+      call
+      integration.service_hook.integration = nil
+
+      expect(integration.service_hook).not_to receive(:execute)
+      expect { call }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+  end
+end

spec/workers/project_service_worker_spec.rb

@@ -3,22 +3,24 @@ require 'spec_helper'
 
 RSpec.describe ProjectServiceWorker, '#perform' do
   let(:worker) { described_class.new }
-  let(:service) { Integrations::Jira.new }
+  let(:integration) { Integrations::Jira.new }
 
   before do
-    allow(Integration).to receive(:find).and_return(service)
+    allow(Integration).to receive(:find).and_return(integration)
   end
 
-  it 'executes service with given data' do
+  it 'executes integration with given data' do
     data = { test: 'test' }
-    expect(service).to receive(:execute).with(data)
+    expect(integration).to receive(:execute).with(data)
 
     worker.perform(1, data)
   end
 
   it 'logs error messages' do
-    allow(service).to receive(:execute).and_raise(StandardError, 'invalid URL')
-    expect(Sidekiq.logger).to receive(:error).with({ class: described_class.name, service_class: service.class.name, message: "invalid URL" })
+    error = StandardError.new('invalid URL')
+    allow(integration).to receive(:execute).and_raise(error)
+
+    expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, integration_class: 'Integrations::Jira')
 
     worker.perform(1, {})
   end