From 19f75bf9284d637d314724cae6117bbb71c21499 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 18 Sep 2025 16:02:56 +0100 Subject: [PATCH] Handle invalid phone errors from GOV.UK Notify This ensures that we handle the `InvalidPhoneError: Not a UK mobile number` error from GOV.UK Notify by recording the response as a permanent failure (which is accurate since retrying is not going to succeed). This also means the job finishes and doesn't get put back on the retry queue, and also allows the nurses to see useful feedback. Sentry-Issue: 6235087330 --- .../app_parent_summary_component.rb | 32 +++++----- app/jobs/sms_delivery_job.rb | 13 ++-- app/models/notify_log_entry.rb | 3 +- spec/jobs/sms_delivery_job_spec.rb | 59 +++++++++++++++++++ 4 files changed, 86 insertions(+), 21 deletions(-) diff --git a/app/components/app_parent_summary_component.rb b/app/components/app_parent_summary_component.rb index 515bbb53f2..e185791857 100644 --- a/app/components/app_parent_summary_component.rb +++ b/app/components/app_parent_summary_component.rb @@ -108,20 +108,19 @@ def call delegate :govuk_summary_list, to: :helpers + EMAIL_FAILURE_TEXTS = { + "permanent_failure" => "Email address does not exist", + "temporary_failure" => "Inbox not accepting messages right now" + }.freeze + def email_address delivery_status = @parent.email_delivery_status elements = [ tag.p(@parent.email, class: "nhsuk-body nhsuk-u-margin-0"), - if delivery_status == "permanent_failure" - render AppStatusComponent.new( - text: "Email address does not exist", - colour: "red", - small: true - ) - elsif delivery_status == "temporary_failure" + if (failure_text = EMAIL_FAILURE_TEXTS[delivery_status]) render AppStatusComponent.new( - text: "Inbox not accepting messages right now", + text: failure_text, colour: "red", small: true ) @@ -131,20 +130,21 @@ def email_address safe_join(elements) end + PHONE_FAILURE_TEXTS = { + "not_uk_mobile_number_failure" => + "Phone number is a landline not accepting text messages", + "permanent_failure" => "Phone number does not exist", + "temporary_failure" => "Inbox not accepting messages right now" + }.freeze + def phone_number delivery_status = @parent.sms_delivery_status elements = [ tag.p(@parent.phone, class: "nhsuk-body nhsuk-u-margin-0"), - if delivery_status == "permanent_failure" - render AppStatusComponent.new( - text: "Phone number does not exist", - colour: "red", - small: true - ) - elsif delivery_status == "temporary_failure" + if (failure_text = PHONE_FAILURE_TEXTS[delivery_status]) render AppStatusComponent.new( - text: "Inbox not accepting messages right now", + text: failure_text, colour: "red", small: true ) diff --git a/app/jobs/sms_delivery_job.rb b/app/jobs/sms_delivery_job.rb index b5f76141c0..e7577c6993 100644 --- a/app/jobs/sms_delivery_job.rb +++ b/app/jobs/sms_delivery_job.rb @@ -3,6 +3,8 @@ class SMSDeliveryJob < NotifyDeliveryJob include NotifyThrottlingConcern + INVALID_UK_MOBILE_NUMBER_ERROR = "InvalidPhoneError: Not a UK mobile number" + def perform( template_name, consent: nil, @@ -43,30 +45,33 @@ def perform( template_id: } - delivery_id = + delivery_id, delivery_status = if self.class.send_via_notify? begin - self.class.client.send_sms(**args).id + [self.class.client.send_sms(**args).id, "sending"] rescue Notifications::Client::BadRequestError => e if !Rails.env.production? && e.message.include?(TEAM_ONLY_API_KEY_MESSAGE) # Prevent retries and job failures. Sentry.capture_exception(e) + elsif e.message == INVALID_UK_MOBILE_NUMBER_ERROR + [nil, "not_uk_mobile_number_failure"] else raise end end elsif self.class.send_via_test? self.class.deliveries << args - SecureRandom.uuid + [SecureRandom.uuid, "delivered"] else Rails.logger.info "Sending SMS to #{phone_number} with template #{template_id}" - nil + [nil, "delivered"] end NotifyLogEntry.create!( consent_form: personalisation.consent_form, delivery_id:, + delivery_status:, parent: personalisation.parent, patient: personalisation.patient, programme_ids: personalisation.programmes.map(&:id), diff --git a/app/models/notify_log_entry.rb b/app/models/notify_log_entry.rb index 87c03bf66f..d305e88c8e 100644 --- a/app/models/notify_log_entry.rb +++ b/app/models/notify_log_entry.rb @@ -49,7 +49,8 @@ class NotifyLogEntry < ApplicationRecord delivered: 1, permanent_failure: 2, temporary_failure: 3, - technical_failure: 4 + technical_failure: 4, + not_uk_mobile_number_failure: 5 } validates :recipient, presence: true diff --git a/spec/jobs/sms_delivery_job_spec.rb b/spec/jobs/sms_delivery_job_spec.rb index 1ea0222ca0..b59608262e 100644 --- a/spec/jobs/sms_delivery_job_spec.rb +++ b/spec/jobs/sms_delivery_job_spec.rb @@ -97,6 +97,36 @@ end end + context "when the parent phone number is invalid" do + before do + allow(notifications_client).to receive(:send_sms).and_raise( + Notifications::Client::BadRequestError.new( + OpenStruct.new( + code: 400, + body: "InvalidPhoneError: Not a UK mobile number" + ) + ) + ) + end + + it "creates a log entry for the failure" do + expect { perform_now }.to change(NotifyLogEntry, :count).by(1) + + notify_log_entry = NotifyLogEntry.last + expect(notify_log_entry).to be_sms + expect(notify_log_entry).to be_not_uk_mobile_number_failure + expect(notify_log_entry.delivery_id).to be_nil + expect(notify_log_entry.recipient).to eq("01234 567890") + expect(notify_log_entry.template_id).to eq( + GOVUK_NOTIFY_SMS_TEMPLATES[template_name] + ) + expect(notify_log_entry.parent).to eq(parent) + expect(notify_log_entry.patient).to eq(patient) + expect(notify_log_entry.programme_ids).to eq(programmes.map(&:id)) + expect(notify_log_entry.sent_by).to eq(sent_by) + end + end + context "with a consent form" do let(:consent_form) do create(:consent_form, session:, parent_phone: "01234567890") @@ -137,6 +167,35 @@ perform_now end end + + context "when the parent phone number is invalid" do + before do + allow(notifications_client).to receive(:send_sms).and_raise( + Notifications::Client::BadRequestError.new( + OpenStruct.new( + code: 400, + body: "InvalidPhoneError: Not a UK mobile number" + ) + ) + ) + end + + it "creates a log entry for the failure" do + expect { perform_now }.to change(NotifyLogEntry, :count).by(1) + + notify_log_entry = NotifyLogEntry.last + expect(notify_log_entry).to be_sms + expect(notify_log_entry).to be_not_uk_mobile_number_failure + expect(notify_log_entry.delivery_id).to be_nil + expect(notify_log_entry.recipient).to eq("01234 567890") + expect(notify_log_entry.template_id).to eq( + GOVUK_NOTIFY_SMS_TEMPLATES[template_name] + ) + expect(notify_log_entry.consent_form).to eq(consent_form) + expect(notify_log_entry.programme_ids).to eq(programmes.map(&:id)) + expect(notify_log_entry.sent_by).to eq(sent_by) + end + end end end