From 13e9e1f2a479f47ca788a99f517cd5d27ba5eaa1 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 4 Oct 2024 16:04:35 +0100 Subject: [PATCH] Rescue notify api errors Notify uses the https://pypi.org/project/phonenumbers/ library to validate phone numbers. There's some scenarios where our regex permits a phone number that libphonenumber doesn't allow, causing notify to return an error when sending to that number. We've gone for rescuing this api error and adding a validation error to the the form. We may at some point, especially if we move message sending into a background job, want to investigate using a gem like https://github.com/daddyz/phonelib that will provide stricter phone number validations. --- app/forms/mobile_number_form.rb | 12 ++ spec/forms/mobile_number_form_spec.rb | 176 ++++++++++++++++---------- 2 files changed, 124 insertions(+), 64 deletions(-) diff --git a/app/forms/mobile_number_form.rb b/app/forms/mobile_number_form.rb index 13197e0e8c..276d3e52ec 100644 --- a/app/forms/mobile_number_form.rb +++ b/app/forms/mobile_number_form.rb @@ -28,6 +28,9 @@ def save ) journey_session.save! + rescue NotifySmsMessage::NotifySmsError => e + handle_notify_error(e) + false end private @@ -48,4 +51,13 @@ def send_sms_message def mobile_number_changed? mobile_number != answers.mobile_number end + + def handle_notify_error(error) + if error.message.include?("ValidationError: phone_number Number is not valid") + errors.add(:mobile_number, i18n_errors_path("invalid")) + false + else + raise error + end + end end diff --git a/spec/forms/mobile_number_form_spec.rb b/spec/forms/mobile_number_form_spec.rb index 7060d4faff..2f68a8aac1 100644 --- a/spec/forms/mobile_number_form_spec.rb +++ b/spec/forms/mobile_number_form_spec.rb @@ -68,89 +68,137 @@ end end - before do - allow(OneTimePassword::Generator).to receive(:new).and_return( - instance_double(OneTimePassword::Generator, code: "111111") - ) - - allow(NotifySmsMessage).to receive(:new).and_return(notify_double) - - form.save - end + context "when notify response is successful" do + before do + allow(OneTimePassword::Generator).to receive(:new).and_return( + instance_double(OneTimePassword::Generator, code: "111111") + ) - let(:notify_double) do - instance_double(NotifySmsMessage, deliver!: notify_response) - end + allow(NotifySmsMessage).to receive(:new).and_return(notify_double) - let(:mobile_number) { "07123456789" } - - context "when notify is successful" do - let(:notify_response) do - Notifications::Client::ResponseNotification.new( - { - id: "123", - reference: "456", - content: "content", - template: "template", - uri: "uri" - } - ) + form.save end - it "stores the mobile number" do - expect(journey_session.reload.answers.mobile_number).to eq(mobile_number) + let(:notify_double) do + instance_double(NotifySmsMessage, deliver!: notify_response) end - it "resets dependent attributes" do - expect(journey_session.reload.answers.mobile_verified).to be_nil - end + let(:mobile_number) { "07123456789" } - it "sends a text message" do - expect(NotifySmsMessage).to have_received(:new).with( - phone_number: mobile_number, - template_id: NotifySmsMessage::OTP_PROMPT_TEMPLATE_ID, - personalisation: { - otp: "111111" - } - ) + context "when notify is successful" do + let(:notify_response) do + Notifications::Client::ResponseNotification.new( + { + id: "123", + reference: "456", + content: "content", + template: "template", + uri: "uri" + } + ) + end + + it "stores the mobile number" do + expect(journey_session.reload.answers.mobile_number).to eq(mobile_number) + end + + it "resets dependent attributes" do + expect(journey_session.reload.answers.mobile_verified).to be_nil + end + + it "sends a text message" do + expect(NotifySmsMessage).to have_received(:new).with( + phone_number: mobile_number, + template_id: NotifySmsMessage::OTP_PROMPT_TEMPLATE_ID, + personalisation: { + otp: "111111" + } + ) + + expect(notify_double).to have_received(:deliver!) + end - expect(notify_double).to have_received(:deliver!) + it "sets sent_one_time_password_at to the current time" do + expect(journey_session.reload.answers.sent_one_time_password_at).to( + eq(DateTime.new(2024, 1, 1, 12, 0, 0)) + ) + end end - it "sets sent_one_time_password_at to the current time" do - expect(journey_session.reload.answers.sent_one_time_password_at).to( - eq(DateTime.new(2024, 1, 1, 12, 0, 0)) - ) + context "when notify is unsuccessful" do + # Not sure how this could be nil rather than a bad response but that's + # what the existing code checks for + let(:notify_response) { nil } + + it "stores the mobile number" do + expect(journey_session.reload.answers.mobile_number).to eq(mobile_number) + end + + it "resets dependent attributes" do + expect(journey_session.reload.answers.mobile_verified).to be_nil + end + + it "sends a text message" do + expect(NotifySmsMessage).to have_received(:new).with( + phone_number: mobile_number, + template_id: NotifySmsMessage::OTP_PROMPT_TEMPLATE_ID, + personalisation: { + otp: "111111" + } + ) + + expect(notify_double).to have_received(:deliver!) + end + + it "sets sent_one_time_password_at to nil" do + expect(journey_session.reload.answers.sent_one_time_password_at).to be_nil + end end end - context "when notify is unsuccessful" do - # Not sure how this could be nil rather than a bad response but that's - # what the existing code checks for - let(:notify_response) { nil } + context "when notify response is not successful" do + context "when the error is an invalid phone number" do + let(:mobile_number) { "07123456789" } - it "stores the mobile number" do - expect(journey_session.reload.answers.mobile_number).to eq(mobile_number) - end + before do + notify_double = instance_double(NotifySmsMessage) - it "resets dependent attributes" do - expect(journey_session.reload.answers.mobile_verified).to be_nil - end + allow(notify_double).to receive(:deliver!).and_raise( + NotifySmsMessage::NotifySmsError, + "ValidationError: phone_number Number is not valid – double check the phone number you entered" + ) - it "sends a text message" do - expect(NotifySmsMessage).to have_received(:new).with( - phone_number: mobile_number, - template_id: NotifySmsMessage::OTP_PROMPT_TEMPLATE_ID, - personalisation: { - otp: "111111" - } - ) + allow(NotifySmsMessage).to receive(:new).and_return(notify_double) + end - expect(notify_double).to have_received(:deliver!) + it "adds a validation error" do + expect(form.save).to eq false + expect(form.errors[:mobile_number]).to include( + "Enter a mobile number, like 07700 900 982 or +44 7700 900 982" + ) + journey_session.reload + expect(journey_session.answers.mobile_number).to be_nil + expect(journey_session.answers.sent_one_time_password_at).to be_nil + end end - it "sets sent_one_time_password_at to nil" do - expect(journey_session.reload.answers.sent_one_time_password_at).to be_nil + context "when some other error" do + let(:mobile_number) { "07123456789" } + + before do + notify_double = instance_double(NotifySmsMessage) + + allow(notify_double).to receive(:deliver!).and_raise( + NotifySmsMessage::NotifySmsError, + "Something went wrong with the SMS service. Please try again later." + ) + + allow(NotifySmsMessage).to receive(:new).and_return(notify_double) + end + + it "raises the error" do + expect { form.save }.to raise_error(NotifySmsMessage::NotifySmsError) + end end end end