From c3105dcb6f77747dba4d7a06471109b613215e3d Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sat, 17 Sep 2022 17:17:01 +1000 Subject: [PATCH] feat: ignore deleted webhooks when calculating the latest triggered webhook status Fixes: https://github.com/pact-foundation/pact_broker/issues/568 --- lib/pact_broker/integrations/integration.rb | 9 ++- lib/pact_broker/webhooks/repository.rb | 4 +- lib/pact_broker/webhooks/triggered_webhook.rb | 7 +++ .../integrations/integration_spec.rb | 30 ++++++++-- .../webhooks/triggered_webhook_spec.rb | 56 +++++++++++++++++++ 5 files changed, 98 insertions(+), 8 deletions(-) diff --git a/lib/pact_broker/integrations/integration.rb b/lib/pact_broker/integrations/integration.rb index feae365f9..a2a24c2ec 100644 --- a/lib/pact_broker/integrations/integration.rb +++ b/lib/pact_broker/integrations/integration.rb @@ -12,7 +12,14 @@ class Integration < Sequel::Model(Sequel::Model.db[:integrations].select(:id, :c associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id) associate(:many_to_one, :provider, :class => "PactBroker::Domain::Pacticipant", :key => :provider_id, :primary_key => :id) associate(:one_to_one, :latest_verification, :class => "PactBroker::Verifications::LatestVerificationForConsumerAndProvider", key: [:consumer_id, :provider_id], primary_key: [:consumer_id, :provider_id]) - associate(:one_to_many, :latest_triggered_webhooks, :class => "PactBroker::Webhooks::LatestTriggeredWebhook", key: [:consumer_id, :provider_id], primary_key: [:consumer_id, :provider_id]) + + one_to_many(:latest_triggered_webhooks, + :class => PactBroker::Webhooks::TriggeredWebhook, + key: [:consumer_id, :provider_id], + allow_eager: true, + primary_key: [:consumer_id, :provider_id]) do | ds | + ds.latest_triggered_webhooks + end # When viewing the index, every latest_pact in the database will match at least one of the rows, so # it makes sense to load the entire table and match each pact to the appropriate row. diff --git a/lib/pact_broker/webhooks/repository.rb b/lib/pact_broker/webhooks/repository.rb index 540b824c8..0cd520ac1 100644 --- a/lib/pact_broker/webhooks/repository.rb +++ b/lib/pact_broker/webhooks/repository.rb @@ -159,9 +159,9 @@ def find_latest_triggered_webhooks_for_pact pact def find_latest_triggered_webhooks consumer, provider # policy already applied to pact - deliberately_unscoped(LatestTriggeredWebhook) + deliberately_unscoped(TriggeredWebhook) .where(consumer: consumer, provider: provider) - .order(:id) + .latest_triggered_webhooks .all end diff --git a/lib/pact_broker/webhooks/triggered_webhook.rb b/lib/pact_broker/webhooks/triggered_webhook.rb index 2547eaaa3..00ef50d57 100644 --- a/lib/pact_broker/webhooks/triggered_webhook.rb +++ b/lib/pact_broker/webhooks/triggered_webhook.rb @@ -45,6 +45,13 @@ def failed def not_run where(status: STATUS_NOT_RUN) end + + # Return the dataset for the latest triggered webhooks grouped by + # consumer, provider, webhook and event. + # Excludes the deleted webhooks + def latest_triggered_webhooks + exclude(webhook_id: nil).max_group_by(:id, [:consumer_id, :provider_id, :webhook_uuid, :event_name]).order(:id) + end end associate(:one_to_many, :webhook_executions, :class => "PactBroker::Webhooks::Execution", :key => :triggered_webhook_id, :primary_key => :id, :order => :id) diff --git a/spec/lib/pact_broker/integrations/integration_spec.rb b/spec/lib/pact_broker/integrations/integration_spec.rb index 940f7d2f7..030ae2428 100644 --- a/spec/lib/pact_broker/integrations/integration_spec.rb +++ b/spec/lib/pact_broker/integrations/integration_spec.rb @@ -86,14 +86,34 @@ module Integrations .create_provider("Bar") .create_consumer_version .create_pact - .create_global_webhook - .create_triggered_webhook + .create_webhook + .create_triggered_webhook(uuid: "1") + .create_webhook_execution + .create_triggered_webhook(uuid: "2") + .create_webhook_execution + .create_consumer("NotFoo") + .create_provider("NotBar") + .create_consumer_version + .create_pact + .create_webhook + .create_triggered_webhook(uuid: "3") + .create_webhook_execution + .create_triggered_webhook(uuid: "4") .create_webhook_execution end - it "returns a list of triggered webhooks" do - integrations = Integration.eager(:latest_triggered_webhooks).order(Sequel.desc(:id)).all - expect(integrations.first.latest_triggered_webhooks.count).to eq 1 + context "lazy loading" do + it "returns a list of triggered webhooks" do + integrations = Integration.order(Sequel.desc(:id)).all + expect(integrations.first.latest_triggered_webhooks.count).to eq 1 + end + end + + context "eager loading" do + it "returns a list of triggered webhooks" do + integrations = Integration.eager(:latest_triggered_webhooks).order(Sequel.desc(:id)).all + expect(integrations.first.associations[:latest_triggered_webhooks].count).to eq 1 + end end end diff --git a/spec/lib/pact_broker/webhooks/triggered_webhook_spec.rb b/spec/lib/pact_broker/webhooks/triggered_webhook_spec.rb index bf6a2528a..8b7a0ccd6 100644 --- a/spec/lib/pact_broker/webhooks/triggered_webhook_spec.rb +++ b/spec/lib/pact_broker/webhooks/triggered_webhook_spec.rb @@ -35,6 +35,62 @@ module Webhooks its(:number_of_attempts_remaining) { is_expected.to eq 0} end end + + describe "latest_triggered_webhooks" do + before do + td.create_consumer("Foo") + .create_provider("Bar") + .create_webhook(uuid: "1") + .create_consumer_version + .comment("tw 1") + .create_pact + .create_triggered_webhook(status: PactBroker::Webhooks::TriggeredWebhook::STATUS_FAILURE, uuid: "tw1") + .create_webhook_execution + .create_consumer_version + .comment("tw 2") + .create_pact + .create_triggered_webhook(status: PactBroker::Webhooks::TriggeredWebhook::STATUS_SUCCESS, uuid: "tw2") + .create_webhook_execution + .create_webhook(uuid: "2") + .create_consumer_version + .comment("tw 3") + .create_pact + .create_triggered_webhook(status: PactBroker::Webhooks::TriggeredWebhook::STATUS_FAILURE, uuid: "tw3") + .create_webhook_execution + .create_consumer_version + .comment("tw 4") + .create_pact + .create_triggered_webhook(status: PactBroker::Webhooks::TriggeredWebhook::STATUS_SUCCESS, uuid: "tw4") + .create_webhook_execution + end + + subject do + TriggeredWebhook + .where(Sequel[:triggered_webhooks][:consumer_id] => td.and_return(:consumer).id) + .where(Sequel[:triggered_webhooks][:provider_id] => td.and_return(:provider).id) + .latest_triggered_webhooks + .all + end + + it "groups by consumer, provider, webhook and event" do + expect(subject).to contain_exactly( + have_attributes(webhook_uuid: "1", uuid: "tw2"), + have_attributes(webhook_uuid: "2", uuid: "tw4") + ) + end + + context "when one of the webhooks has been deleted" do + before do + PactBroker::Webhooks::Service.delete_by_uuid("2") + end + + it "does not include the triggered webhook for the deleted webhook" do + expect(subject).to contain_exactly( + have_attributes(webhook_uuid: "1", uuid: "tw2") + ) + end + end + end end end end