fix: delete related triggered webhooks when webhook is deleted

Trying to keep the triggered webhooks to keep the status on the index page after a webhook had been deleted
just made things too complicated. Also, it was likely that the webhook was deleted because it was failing,
so it just made the failure message hang around.

Author: bethesque

db/migrations/39_add_triggered_webhooks_fk_to_execution.rb

@@ -17,7 +17,7 @@
     # TODO drop_column(:webhook_executions, :provider_id)
 
     # TODO
-    # alter_table(:triggered_webhooks) do
+    # alter_table(:webhook_executions) do
     #  set_column_not_null(:triggered_webhook_id)
     # end
   end

db/migrations/42_delete_orphan_webhook_data.rb

@@ -0,0 +1,20 @@
+require 'securerandom'
+
+Sequel.migration do
+  up do
+    from(:triggered_webhooks).where(webhook_id: nil).each do | triggered_webhook |
+      from(:webhook_executions).where(triggered_webhook_id: triggered_webhook[:id]).delete
+      from(:triggered_webhooks).where(id: triggered_webhook[:id]).delete
+    end
+
+    from(:webhook_executions).where(webhook_id: nil, triggered_webhook_id: nil).delete
+  end
+
+  # TODO
+  # alter_table(:triggered_webhooks) do
+  #  set_column_not_null(:webhook_id)
+  # end
+
+  down do
+  end
+end

lib/pact_broker/webhooks/execution.rb

@@ -27,6 +27,8 @@ def <=> other
       end
     end
 
+    # For a brief time, the code was released with a direct relationship between
+    # webhook and execution. Need to make sure any existing data is handled properly.
     class DeprecatedExecution < Sequel::Model(:webhook_executions)
       associate(:many_to_one, :provider, :class => "PactBroker::Domain::Pacticipant", :key => :provider_id, :primary_key => :id)
       associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id)

lib/pact_broker/webhooks/repository.rb

@@ -104,9 +104,11 @@ def delete_executions_by_pacticipant pacticipants
         Execution.where(id: execution_ids).delete
       end
 
-      def unlink_triggered_webhooks_by_webhook_uuid uuid
-        TriggeredWebhook.where(webhook: Webhook.where(uuid: uuid)).update(webhook_id: nil)
-        DeprecatedExecution.where(webhook_id: Webhook.where(uuid: uuid).select(:id)).update(webhook_id: nil)
+      def delete_triggered_webhooks_by_webhook_uuid uuid
+        triggered_webhook_ids = TriggeredWebhook.where(webhook: Webhook.where(uuid: uuid)).select_for_subquery(:id)
+        Execution.where(triggered_webhook_id: triggered_webhook_ids).delete
+        DeprecatedExecution.where(webhook_id: Webhook.where(uuid: uuid).select_for_subquery(:id)).delete
+        TriggeredWebhook.where(id: triggered_webhook_ids).delete
       end
 
       def delete_triggered_webhooks_by_pact_publication_ids pact_publication_ids

lib/pact_broker/webhooks/service.rb

@@ -40,7 +40,7 @@ def self.update_by_uuid uuid, webhook
       end
 
       def self.delete_by_uuid uuid
-        webhook_repository.unlink_triggered_webhooks_by_webhook_uuid uuid
+        webhook_repository.delete_triggered_webhooks_by_webhook_uuid uuid
         webhook_repository.delete_by_uuid uuid
       end
 

lib/pact_broker/webhooks/webhook.rb

@@ -11,6 +11,10 @@ class Webhook < Sequel::Model
       associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id)
       one_to_many :headers, :class => "PactBroker::Webhooks::WebhookHeader", :reciprocal => :webhook
 
+      dataset_module do
+        include PactBroker::Repositories::Helpers
+      end
+
       def before_destroy
         WebhookHeader.where(webhook_id: id).destroy
       end

spec/lib/pact_broker/webhooks/repository_spec.rb

@@ -354,7 +354,7 @@ module Webhooks
         end
       end
 
-      describe "unlink_triggered_webhooks_by_webhook_uuid" do
+      describe "delete_triggered_webhooks_by_webhook_uuid" do
         let(:td) { TestDataBuilder.new }
 
         before do
@@ -365,22 +365,38 @@ module Webhooks
             .create_webhook
             .create_triggered_webhook
             .create_deprecated_webhook_execution
+            .create_webhook_execution
+            .create_webhook
+            .create_triggered_webhook
+            .create_deprecated_webhook_execution
+            .create_webhook_execution
+        end
+
+        let(:webhook_id) { Webhook.find(uuid: td.webhook.uuid).id }
+        subject { Repository.new.delete_triggered_webhooks_by_webhook_uuid td.webhook.uuid }
+
+        it "deletes the related triggered webhooks" do
+          expect { subject }.to change {
+              TriggeredWebhook.where(id: td.triggered_webhook.id).count
+            }.from(1).to(0)
         end
 
-        subject { Repository.new.unlink_triggered_webhooks_by_webhook_uuid td.webhook.uuid }
+        it "does not delete the unrelated triggered webhooks" do
+          expect { subject }.to_not change {
+              TriggeredWebhook.exclude(id: td.triggered_webhook.id).count
+            }
+        end
 
-        it "sets the webhook id to nil" do
-          webhook_id = Webhook.find(uuid: td.webhook.uuid).id
+        it "deletes the related deprecated webhook executions" do
           expect { subject }.to change {
-              TriggeredWebhook.find(id: td.triggered_webhook.id).webhook_id
-            }.from(webhook_id).to(nil)
+              DeprecatedExecution.count
+            }.by(-2)
         end
 
-        it "sets the webhook id to nil for the deprecated webhook execution field" do
-          webhook_id = Webhook.find(uuid: td.webhook.uuid).id
+        it "deletes the related webhook executions" do
           expect { subject }.to change {
-              DeprecatedExecution.find(id: td.webhook_execution.id).webhook_id
-            }.from(webhook_id).to(nil)
+              Execution.count
+            }.by(-2)
         end
       end
 

spec/migrations/42_delete_ophan_webhook_data_spec.rb

@@ -0,0 +1,98 @@
+describe 'creating triggered webhooks from webhook executions (migrate 36-41)', migration: true do
+  before do
+    PactBroker::Database.migrate(41)
+  end
+
+  let(:before_now) { DateTime.new(2016, 1, 1) }
+  let(:now) { DateTime.new(2018, 2, 2) }
+  let!(:consumer) { create(:pacticipants, {name: 'Consumer', created_at: now, updated_at: now}) }
+  let!(:provider) { create(:pacticipants, {name: 'Provider', created_at: now, updated_at: now}) }
+  let!(:consumer_version) { create(:versions, {number: '1.2.3', order: 1, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) }
+  let!(:pact_version) { create(:pact_versions, {content: {some: 'json'}.to_json, sha: '1234', consumer_id: consumer[:id], provider_id: provider[:id], created_at: now}) }
+  let!(:pact_publication) do
+    create(:pact_publications, {
+      consumer_version_id: consumer_version[:id],
+      provider_id: provider[:id],
+      revision_number: 1,
+      pact_version_id: pact_version[:id],
+      created_at: (now - 1)
+    })
+  end
+  let!(:webhook) do
+    create(:webhooks, {
+      uuid: '1234',
+      method: 'GET',
+      url: 'http://www.example.org',
+      consumer_id: consumer[:id],
+      provider_id: provider[:id],
+      is_json_request_body: false,
+      created_at: now
+    })
+  end
+  let!(:triggered_webhook) do
+    create(:triggered_webhooks, {
+      trigger_uuid: '12345',
+      trigger_type: 'publication',
+      pact_publication_id: pact_publication[:id],
+      webhook_id: webhook[:id],
+      webhook_uuid: webhook[:uuid],
+      consumer_id: consumer[:id],
+      provider_id: provider[:id],
+      status: 'success',
+      created_at: now,
+      updated_at: now
+    })
+  end
+  let!(:webhook_execution) do
+    create(:webhook_executions, {
+      triggered_webhook_id: triggered_webhook[:id],
+      success: true,
+      logs: 'logs',
+      created_at: now
+    })
+  end
+
+  let!(:orphan_triggered_webhook) do
+    create(:triggered_webhooks, {
+      trigger_uuid: '12345',
+      trigger_type: 'publication',
+      pact_publication_id: pact_publication[:id],
+      webhook_id: nil,
+      webhook_uuid: webhook[:uuid],
+      consumer_id: consumer[:id],
+      provider_id: provider[:id],
+      status: 'success',
+      created_at: now,
+      updated_at: now
+    })
+  end
+
+  let!(:orphan_webhook_execution) do
+    create(:webhook_executions, {
+      triggered_webhook_id: orphan_triggered_webhook[:id],
+      success: true,
+      logs: 'logs',
+      created_at: now
+    })
+  end
+
+  let!(:deprecated_orphan_webhook_execution) do
+    create(:webhook_executions, {
+      triggered_webhook_id: nil,
+      webhook_id: nil,
+      success: true,
+      logs: 'logs',
+      created_at: now
+    })
+  end
+
+  subject { PactBroker::Database.migrate(42) }
+
+  it "deletes the orphan triggered webhooks" do
+    expect { subject }.to change { database[:triggered_webhooks].count }.by(-1)
+  end
+
+  it "deletes the orphan webhook executions" do
+    expect { subject }.to change { database[:webhook_executions].count }.by(-2)
+  end
+end