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

@@ -28,6 +28,9 @@ def save
     )
 
     journey_session.save!
+  rescue NotifySmsMessage::NotifySmsError => e
+    handle_notify_error(e)
+    false
   end
 
   private
@@ -48,4 +51,12 @@ 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"))
+    else
+      raise error
+    end
+  end
 end

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