Skip to content

Commit

Permalink
Rescue notify api errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rjlynch committed Oct 4, 2024
1 parent e188462 commit 13e9e1f
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 64 deletions.
12 changes: 12 additions & 0 deletions app/forms/mobile_number_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ def save
)

journey_session.save!
rescue NotifySmsMessage::NotifySmsError => e
handle_notify_error(e)
false
end

private
Expand All @@ -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
176 changes: 112 additions & 64 deletions spec/forms/mobile_number_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 13e9e1f

Please sign in to comment.