From 1c5528d99197790fee67818e49a89c368683f2dc Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Tue, 16 Jan 2024 10:27:06 +0000 Subject: [PATCH] Add and use PollingAlertCheckWorker - AlertCheckWorker likely to cause false alarms due to the callbacks being 429'd, so add a backup where if we have no delivered emails, we actively poll for them until we find a delivered one. --- app/services/check_notify_email_service.rb | 35 ++++++++ app/workers/polling_alert_check_worker.rb | 33 +++++++ config/sidekiq.yml | 4 +- spec/support/notify_request_helpers.rb | 25 ++++++ .../polling_alert_check_worker_spec.rb | 88 +++++++++++++++++++ 5 files changed, 183 insertions(+), 2 deletions(-) create mode 100644 app/services/check_notify_email_service.rb create mode 100644 app/workers/polling_alert_check_worker.rb create mode 100644 spec/workers/polling_alert_check_worker_spec.rb diff --git a/app/services/check_notify_email_service.rb b/app/services/check_notify_email_service.rb new file mode 100644 index 000000000..dec729ede --- /dev/null +++ b/app/services/check_notify_email_service.rb @@ -0,0 +1,35 @@ +class CheckNotifyEmailService + def initialize(status) + @status = status + @client = Notifications::Client.new(Rails.application.credentials.notify_api_key) + end + + def present?(reference) + results = @client.get_notifications( + template_type: "email", + status: @status, + reference:, + ) + + results.collection.count.positive? + rescue Notifications::Client::RequestError, Net::OpenTimeout, Net::ReadTimeout, SocketError => e + Rails.logger.debug("Unable to contact Notify to determine status of email reference #{reference}: #{e}") + end + +private + + attr_reader :email, :client + + def undeliverable_failure?(error) + return false unless error.is_a?(Notifications::Client::BadRequestError) + + return true if error.message.end_with?("Not a valid email address") + + # This is a stop-gap fix for an issue where this system can produce emails + # that are so long they exceed Notify's limit of 2MB. As emails of this + # size will never succeed we mark them as failures. Ideally this will + # eventually be resolved by some product thinking that only allows the + # system to create emails within a reasonable length. + error.message.start_with?("BadRequestError: Your message is too long") + end +end diff --git a/app/workers/polling_alert_check_worker.rb b/app/workers/polling_alert_check_worker.rb new file mode 100644 index 000000000..94591b1d1 --- /dev/null +++ b/app/workers/polling_alert_check_worker.rb @@ -0,0 +1,33 @@ +class PollingAlertCheckWorker < ApplicationWorker + include SearchAlertList + + def perform(document_type) + content_items = get_alert_content_items(document_type:) + delivered = 0 + content_items.each do |ci| + if any_emails_delivered_for?(ci[:content_id], ci[:valid_from], document_type) + delivered += 1 + else + Rails.logger.warn("Couldn't find any delivered emails for #{document_type.titleize} records with content id #{ci[:content_id]} (at #{ci[:url]})") + end + end + + Rails.logger.info("Checking #{document_type.titleize} records: #{delivered} out of #{content_items.count} alerts have been delivered to at least one recipient") + + Rails.cache.write("current_#{document_type}s", content_items.count, expires_in: 15.minutes) + Rails.cache.write("delivered_#{document_type}s", delivered, expires_in: 15.minutes) + end + + def any_emails_delivered_for?(content_id, valid_from, document_type) + return true if Email.where("notify_status = 'delivered' AND content_id = ? AND created_at > ?", content_id, valid_from).exists? + + Rails.logger.info("First pass couldn't find any delivered emails for #{document_type.titleize} records with content id #{content_id}, reverting to polling") + + service = CheckNotifyEmailService.new("delivered") + Email.sent.where("content_id = ? AND created_at > ? AND notify_status IS NULL", content_id, valid_from).find_each do |email| + return true if service.present?(email.id) + end + + false + end +end diff --git a/config/sidekiq.yml b/config/sidekiq.yml index 3f3f6e890..80a0b431e 100644 --- a/config/sidekiq.yml +++ b/config/sidekiq.yml @@ -41,9 +41,9 @@ class: RecoverLostJobsWorker check_medical_safety_alerts: every: '15m' - class: AlertCheckWorker + class: PollingAlertCheckWorker args: ["medical_safety_alert"] check_travel_advice_alerts: every: '15m' - class: AlertCheckWorker + class: PollingAlertCheckWorker args: ["travel_advice"] diff --git a/spec/support/notify_request_helpers.rb b/spec/support/notify_request_helpers.rb index 8eca3801e..a062ce1b3 100644 --- a/spec/support/notify_request_helpers.rb +++ b/spec/support/notify_request_helpers.rb @@ -4,6 +4,31 @@ def stub_notify stub_request(:post, /notifications\.service\.gov\.uk/).to_return(body:) end + def stub_notify_email_delivery_status_delivered(reference) + stub_request(:get, "https://api.notifications.service.gov.uk/v2/notifications?reference=#{reference}&status=delivered&template_type=email").to_return( + body: { + notifications: [ + { + "id": "740e5834-3a29-46b4-9a6f-16142fde533a", + "reference": reference, + "type": "email", + "status": "delivered", + }, + ], + "links": {}, + }.to_json, + ) + end + + def stub_notify_email_delivery_status_delivered_empty(reference) + stub_request(:get, "https://api.notifications.service.gov.uk/v2/notifications?reference=#{reference}&status=delivered&template_type=email").to_return( + body: { + notifications: [], + "links": {}, + }.to_json, + ) + end + def expect_an_email_was_sent(subject: /.*/, address: "test@test.com") request_data = nil diff --git a/spec/workers/polling_alert_check_worker_spec.rb b/spec/workers/polling_alert_check_worker_spec.rb new file mode 100644 index 000000000..aa5f0196d --- /dev/null +++ b/spec/workers/polling_alert_check_worker_spec.rb @@ -0,0 +1,88 @@ +RSpec.describe PollingAlertCheckWorker do + include SearchAlertListHelpers + include NotifyRequestHelpers + + describe "#perform", caching: true do + def perform + described_class.new.perform("medical_safety_alert") + end + + context "there are no alerts older than an hour" do + before { stub_medical_safety_alert_feed(content_id: SecureRandom.uuid, age: 30.minutes) } + + it "should put 0/0 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 0, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 0, expires_in: 15.minutes) + perform + end + end + + context "there are no alerts younger than 2 days" do + before { stub_medical_safety_alert_feed(content_id: SecureRandom.uuid, age: 3.days) } + + it "should put 0/0 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 0, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 0, expires_in: 15.minutes) + perform + end + end + + context "there is a valid alert with delivered emails" do + before do + content_id = SecureRandom.uuid + stub_medical_safety_alert_feed(content_id:, age: 2.hours) + create(:email, content_id:, notify_status: "delivered", status: "sent") + end + + it "should put 1/1 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 1, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 1, expires_in: 15.minutes) + perform + end + end + + context "there is a valid alert with undelivered emails" do + before do + content_id = SecureRandom.uuid + stub_medical_safety_alert_feed(content_id:, age: 2.hours) + email = create(:email, content_id:, notify_status: nil, status: "sent") + stub_notify_email_delivery_status_delivered_empty(email.id) + end + + it "should put 1/0 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 1, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 0, expires_in: 15.minutes) + perform + end + end + + context "there is a valid alert with apparently undelivered emails but they actually were delivered" do + before do + content_id = SecureRandom.uuid + stub_medical_safety_alert_feed(content_id:, age: 2.hours) + email = create(:email, content_id:, notify_status: nil, status: "sent") + stub_notify_email_delivery_status_delivered(email.id) + end + + it "should put 1/0 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 1, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 1, expires_in: 15.minutes) + perform + end + end + + context "there is a valid alert with delivered emails that are too old to be valid" do + before do + content_id = SecureRandom.uuid + stub_medical_safety_alert_feed(content_id:, age: 2.hours) + create(:email, content_id:, notify_status: "delivered", created_at: Time.zone.now - 3.hours, status: "sent") + end + + it "should put 1/0 in the cache" do + expect(Rails.cache).to receive(:write).with("current_medical_safety_alerts", 1, expires_in: 15.minutes) + expect(Rails.cache).to receive(:write).with("delivered_medical_safety_alerts", 0, expires_in: 15.minutes) + perform + end + end + end +end