Skip to content

Commit

Permalink
Change the alert specs to use webmock
Browse files Browse the repository at this point in the history
When implementing the unpublishing alert feature, we merged code with
a bug which was passing all the tests.  The bug was that we were
calling a method in gds-api-adapters with incorrect arguments.

This bug passed the tests because the tests do not actually use
gds-api-adapters, they mock those methods and check that the mocked
methods are called appropriately.

This is a problem for two reasons:

1. It let us write code which looked correct and even passed tests,
but which could only have been verified as correct by consulting a
different repository.

2. An update to gds-api-adapters could change the method signature and
our tests would continue to pass on the dependabot branch, even though
merging it would break the service.

The point of testing is to catch bugs.  If a test actively obscures
bugs, it should be fixed.  This commit fixes the issue by using
webmock to check the actual calls we make to email-alert-api, rather
than stubbing the methods in gds-api-adapters.
  • Loading branch information
barrucadu committed Jan 24, 2022
1 parent c7dfb5c commit 8502c9c
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 42 deletions.
23 changes: 15 additions & 8 deletions spec/models/email_alert_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ def with_lock_unless_done
end

let(:logger) { double(:logger, info: nil) }
let(:alert_api) { double(:alert_api, create_content_change: nil) }
let(:email_alert) { EmailAlert.new(document, logger) }
let(:fake_lock_handler) { fake_lock_handler_class.new }

before :each do
allow(Services).to receive(:email_api_client).and_return(alert_api)
allow(LockHandler).to receive(:new).and_return(fake_lock_handler)
end

describe "#trigger" do
it "logs receiving a major change notification for a document" do
stub_create_content_change.to_return(status: 202)

email_alert.trigger

expect(logger).to have_received(:info).with(
Expand All @@ -66,16 +66,18 @@ def with_lock_unless_done
end

it "sends an alert to the Email Alert API" do
stub = stub_create_content_change
.with(body: hash_including(content_id: content_id), headers: { "GOVUK-Request-Id" => govuk_request_id })
.to_return(status: 202)

email_alert.trigger

expect(alert_api).to have_received(:create_content_change).with(
hash_including("content_id" => content_id),
govuk_request_id: govuk_request_id,
)
expect(stub).to have_been_made
end

it "logs if it receives a conflict" do
allow(alert_api).to receive(:create_content_change).and_raise(GdsApi::HTTPConflict.new(409))
stub_create_content_change.to_return(status: 409)

email_alert.trigger

expect(logger).to have_received(:info).with(
Expand All @@ -84,13 +86,18 @@ def with_lock_unless_done
end

it "logs if it receives an unprocessable entity" do
allow(alert_api).to receive(:create_content_change).and_raise(GdsApi::HTTPUnprocessableEntity.new(422))
stub_create_content_change.to_return(status: 422)

email_alert.trigger

expect(logger).to have_received(:info).with(
"email-alert-api returned unprocessable entity for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end

def stub_create_content_change
stub_request(:post, "#{GdsApi::TestHelpers::EmailAlertApi::EMAIL_ALERT_API_ENDPOINT}/content-changes")
end
end

describe "#format_for_email_api" do
Expand Down
91 changes: 57 additions & 34 deletions spec/models/unpublishing_alert_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,59 +69,82 @@ def with_lock_unless_done
},
}
end
let(:alert_api) { double(:alert_api, bulk_unsubscribe: nil, find_subscriber_list: subscriber_list_response) }

before :each do
allow(LockHandler).to receive(:new).and_return(fake_lock_handler)
allow(Services).to receive(:email_api_client).and_return(alert_api)
end

describe "#trigger" do
shared_examples "a request to bulk unsubscribe with appropriate logging" do
it "logs an unsubscription notification" do
it "logs that the list does not exist and does not make a request to bulk-unsubscribe" do
stub_email_alert_api_does_not_have_subscriber_list("content_id" => content_id)

unpublishing_alert.trigger

expect(logger).to have_received(:info).with(
"Received unsubscription notification for #{document['base_path']}, unpublishing_scenario: #{unpublishing_scenario}, full payload: #{document}",
"subscriber list not found for content id #{document['content_id']}",
)
end

it "sends a bulk unsubscribe request to the Email Alert API" do
unpublishing_alert.trigger
context "when the list exists" do
before do
stub_email_alert_api_has_subscriber_list(
"content_id" => content_id,
"slug" => subscriber_list_slug,
)
end

expect(alert_api).to have_received(:bulk_unsubscribe).with(
slug: subscriber_list_slug,
govuk_request_id: govuk_request_id,
body: email_markdown.strip,
sender_message_id: sender_message_id,
)
end
it "logs an unsubscription notification" do
stub_email_alert_api_bulk_unsubscribe(slug: subscriber_list_slug)

it "logs if it recieves a conflict" do
allow(alert_api).to receive(:bulk_unsubscribe).and_raise(GdsApi::HTTPConflict.new(409))
unpublishing_alert.trigger
unpublishing_alert.trigger

expect(logger).to have_received(:info).with(
"email-alert-api returned conflict for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end
expect(logger).to have_received(:info).with(
"Received unsubscription notification for #{document['base_path']}, unpublishing_scenario: #{unpublishing_scenario}, full payload: #{document}",
)
end

it "logs if it recieves an unprocessable entity" do
allow(alert_api).to receive(:bulk_unsubscribe).and_raise(GdsApi::HTTPUnprocessableEntity.new(422))
unpublishing_alert.trigger
it "sends a bulk unsubscribe request to the Email Alert API" do
stub = stub_email_alert_api_bulk_unsubscribe_with_message(
slug: subscriber_list_slug,
govuk_request_id: govuk_request_id,
body: email_markdown.strip,
sender_message_id: sender_message_id,
)

expect(logger).to have_received(:info).with(
"email-alert-api returned unprocessable entity for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end
unpublishing_alert.trigger

it "logs if it recieves not found" do
allow(alert_api).to receive(:bulk_unsubscribe).and_raise(GdsApi::HTTPNotFound.new(404))
unpublishing_alert.trigger
expect(stub).to have_been_made
end

expect(logger).to have_received(:info).with(
"email-alert-api returned not_found for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
it "logs if it receives a conflict" do
stub_email_alert_api_bulk_unsubscribe_conflict(slug: subscriber_list_slug)

unpublishing_alert.trigger

expect(logger).to have_received(:info).with(
"email-alert-api returned conflict for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end

it "logs if it receives an unprocessable entity" do
stub_email_alert_api_bulk_unsubscribe_bad_request(slug: subscriber_list_slug)

unpublishing_alert.trigger

expect(logger).to have_received(:info).with(
"email-alert-api returned unprocessable entity for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end

it "logs if it receives not found" do
stub_email_alert_api_bulk_unsubscribe_not_found(slug: subscriber_list_slug)

unpublishing_alert.trigger

expect(logger).to have_received(:info).with(
"email-alert-api returned not_found for #{document['content_id']}, #{document['base_path']}, #{document['public_updated_at']}",
)
end
end
end

Expand Down

0 comments on commit 8502c9c

Please sign in to comment.