Merge branch 'sms-mssid-error'

10 files changed, 71 insertions, 84 deletions

diff --git a/conf/general.yml-example b/conf/general.yml-example
index 528fc1012..72b5fbaa3 100644
--- a/conf/general.yml-example
+++ b/conf/general.yml-example
@@ -211,6 +211,7 @@ PHONE_COUNTRY: ''
 TWILIO_ACCOUNT_SID: ''
 TWILIO_AUTH_TOKEN: ''
 TWILIO_FROM_PARAMETER: ''
+TWILIO_MESSAGING_SERVICE_SID: ''
 
 # If you want to hide all pages from non-logged-in users, set this to 1.
 LOGIN_REQUIRED: 0
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Phone.pm b/perllib/FixMyStreet/App/Controller/Auth/Phone.pm
index e4ffc2205..8387b9d64 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Phone.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Phone.pm
@@ -23,17 +23,19 @@ Handle the submission of a code sent by text to a mobile number.
 =cut
 
 sub code : Path('') {
-    my ( $self, $c ) = @_;
+    my ( $self, $c, $scope, $success_action ) = @_;
     $c->stash->{template} = 'auth/smsform.html';
+    $scope ||= 'phone_sign_in';
+    $success_action ||= '/auth/process_login';
 
     my $token = $c->stash->{token} = $c->get_param('token');
     my $code = $c->get_param('code') || '';
 
-    my $data = $c->forward('/auth/get_token', [ $token, 'phone_sign_in' ]) || return;
+    my $data = $c->stash->{token_data} = $c->forward('/auth/get_token', [ $token, $scope ]) || return;
 
     $c->stash->{incorrect_code} = 1, return if $data->{code} ne $code;
 
-    $c->detach( '/auth/process_login', [ $data, 'phone' ] );
+    $c->detach( $success_action, [ $data, 'phone' ] );
 }
 
 =head2 sign_in
@@ -90,6 +92,12 @@ sub send_token : Private {
     my ( $self, $c, $token_data, $token_scope, $to ) = @_;
 
     my $result = FixMyStreet::SMS->send_token($token_data, $token_scope, $to);
+    if ($result->{error}) {
+        $c->log->debug("Failure sending text containing code *$result->{random}*");
+        $c->stash->{sms_error} = $result->{error};
+        $c->stash->{username_error} = 'sms_failed';
+        return;
+    }
     $c->stash->{token} = $result->{token};
     $c->log->debug("Sending text containing code *$result->{random}*");
     $c->stash->{template} = 'auth/smsform.html';
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index fa3967bf3..5f36443c0 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -1114,18 +1114,8 @@ sub send_problem_confirm_text : Private {
 sub confirm_by_text : Path('text') {
     my ( $self, $c ) = @_;
 
-    my $token = $c->stash->{token} = $c->get_param('token');
-    my $code = $c->get_param('code') || '';
-
-    my $data = $c->stash->{token_data} = $c->forward('/auth/get_token', [ $token, 'problem' ]) || return;
-    if ($data->{code} ne $code) {
-        $c->stash->{template} = 'auth/smsform.html';
-        $c->stash->{submit_url} = '/report/new/text';
-        $c->stash->{incorrect_code} = 1;
-        return;
-    }
-
-    $c->detach('process_confirmation');
+    $c->stash->{submit_url} = '/report/new/text';
+    $c->forward('/auth/phone/code', [ 'problem', '/report/new/process_confirmation' ]);
 }
 
 sub process_confirmation : Private {
diff --git a/perllib/FixMyStreet/App/Controller/Report/Update.pm b/perllib/FixMyStreet/App/Controller/Report/Update.pm
index 66724f2d1..c28039808 100644
--- a/perllib/FixMyStreet/App/Controller/Report/Update.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/Update.pm
@@ -521,18 +521,8 @@ sub send_confirmation_text : Private {
 sub confirm_by_text : Path('text') {
     my ( $self, $c ) = @_;
 
-    my $token = $c->stash->{token} = $c->get_param('token');
-    my $code = $c->get_param('code') || '';
-
-    my $data = $c->stash->{token_data} = $c->forward('/auth/get_token', [ $token, 'comment' ]) || return;
-    if ($data->{code} ne $code) {
-        $c->stash->{template} = 'auth/smsform.html';
-        $c->stash->{submit_url} = '/report/update/text';
-        $c->stash->{incorrect_code} = 1;
-        return;
-    }
-
-    $c->detach('process_confirmation');
+    $c->stash->{submit_url} = '/report/update/text';
+    $c->forward('/auth/phone/code', [ 'comment', '/report/update/process_confirmation' ]);
 }
 
 sub process_confirmation : Private {
diff --git a/perllib/FixMyStreet/SMS.pm b/perllib/FixMyStreet/SMS.pm
index c71cceadc..874108706 100644
--- a/perllib/FixMyStreet/SMS.pm
+++ b/perllib/FixMyStreet/SMS.pm
@@ -3,7 +3,7 @@ package FixMyStreet::SMS;
 use strict;
 use warnings;
 
-# use JSON::MaybeXS;
+use JSON::MaybeXS;
 use Moo;
 use Number::Phone::Lib;
 use WWW::Twilio::API;
@@ -28,6 +28,11 @@ has from => (
     default => sub { FixMyStreet->config('TWILIO_FROM_PARAMETER') },
 );
 
+has messaging_service => (
+    is => 'lazy',
+    default => sub { FixMyStreet->config('TWILIO_MESSAGING_SERVICE_SID') },
+);
+
 sub send_token {
     my ($class, $token_data, $token_scope, $to) = @_;
 
@@ -44,24 +49,23 @@ sub send_token {
     return {
         random => $random,
         token => $token_obj->token,
-        result => $result,
+        %$result,
     };
 }
 
 sub send {
     my ($self, %params) = @_;
     my $output = $self->twilio->POST('Messages.json', 
-        From => $self->from,
+        $self->from ? (From => $self->from) : (),
+        $self->messaging_service ? (MessagingServiceSid => $self->messaging_service) : (),
         To => $params{to},
         Body => $params{body},
     );
-    # At present, we do nothing and assume sent okay.
-    # TODO add error checking
-    # my $data = decode_json($output->{content});
-    # if ($output->{code} != 200) {
-    #     return { error => "$data->{message} ($data->{code})" };
-    # }
-    # return { success => $data->{sid} };
+    my $data = decode_json($output->{content});
+    if ($output->{code} >= 400) {
+        return { error => "$data->{message} ($data->{code})" };
+    }
+    return { success => $data->{sid} };
 }
 
 =head2 parse_username
diff --git a/t/Mock/Twilio.pm b/t/Mock/Twilio.pm
index eaad30b76..125daa55f 100644
--- a/t/Mock/Twilio.pm
+++ b/t/Mock/Twilio.pm
@@ -20,6 +20,10 @@ sub dispatch_request {
 
     sub (POST + /2010-04-01/Accounts/*/Messages.json + %*) {
         my ($self, $sid, $data) = @_;
+        if ($data->{To} eq '+18165550101') {
+            return [ 400, [ 'Content-Type' => 'application/json' ],
+                [ '{"code":"21408", "message": "Unable to send"}' ] ];
+        }
         push @{$self->texts}, $data;
         return [ 200, [ 'Content-Type' => 'application/json' ], [ '{}' ] ];
     },
diff --git a/t/app/controller/auth_phone.t b/t/app/controller/auth_phone.t
index 435ea7552..dea1c3493 100644
--- a/t/app/controller/auth_phone.t
+++ b/t/app/controller/auth_phone.t
@@ -7,12 +7,13 @@ LWP::Protocol::PSGI->register($twilio->to_psgi_app, host => 'api.twilio.com');
 
 my $mech = FixMyStreet::TestMech->new;
 
-subtest 'Log in with invalid number, fail' => sub {
-    FixMyStreet::override_config {
-        SMS_AUTHENTICATION => 1,
-        PHONE_COUNTRY => 'GB',
-        TWILIO_ACCOUNT_SID => 'AC123',
-    }, sub {
+FixMyStreet::override_config {
+    SMS_AUTHENTICATION => 1,
+    PHONE_COUNTRY => 'GB',
+    TWILIO_ACCOUNT_SID => 'AC123',
+}, sub {
+
+    subtest 'Log in with invalid number, fail' => sub {
         $mech->get_ok('/auth');
         $mech->submit_form_ok({
             form_name => 'general_auth',
@@ -21,14 +22,8 @@ subtest 'Log in with invalid number, fail' => sub {
         }, "sign in using bad number");
         $mech->content_contains('Please check your phone number is correct');
     };
-};
 
-subtest 'Log in using landline, fail' => sub {
-    FixMyStreet::override_config {
-        SMS_AUTHENTICATION => 1,
-        PHONE_COUNTRY => 'GB',
-        TWILIO_ACCOUNT_SID => 'AC123',
-    }, sub {
+    subtest 'Log in using landline, fail' => sub {
         $mech->get_ok('/auth');
         $mech->submit_form_ok({
             form_name => 'general_auth',
@@ -37,14 +32,18 @@ subtest 'Log in using landline, fail' => sub {
         }, "sign in using landline");
         $mech->content_contains('Please enter a mobile number');
     };
-};
 
-subtest 'Log in using mobile, by text' => sub {
-    FixMyStreet::override_config {
-        SMS_AUTHENTICATION => 1,
-        PHONE_COUNTRY => 'GB',
-        TWILIO_ACCOUNT_SID => 'AC123',
-    }, sub {
+    subtest 'Log in using number that fails at Twilio' => sub {
+        $mech->get_ok('/auth');
+        $mech->submit_form_ok({
+            form_name => 'general_auth',
+            fields => { username => '+18165550101' },
+            button => 'sign_in_by_code',
+        }, "sign in using failing number");
+        $mech->content_contains('Sending a confirmation text failed');
+    };
+
+    subtest 'Log in using mobile, by text' => sub {
         $mech->submit_form_ok({
             form_name => 'general_auth',
             fields => { username => '+18165550100', password_register => 'secret' },
@@ -67,12 +66,8 @@ subtest 'Log in using mobile, by text' => sub {
         $mech->logged_in_ok;
         $mech->log_out_ok;
     };
-};
 
-subtest 'Log in using mobile, by password' => sub {
-    FixMyStreet::override_config {
-        SMS_AUTHENTICATION => 1,
-    }, sub {
+    subtest 'Log in using mobile, by password' => sub {
         $mech->get_ok('/auth');
         $mech->submit_form_ok({
             form_name => 'general_auth',
@@ -89,6 +84,7 @@ subtest 'Log in using mobile, by password' => sub {
         is $mech->uri->path, '/my', "redirected to the 'my' section of site";
         $mech->logged_in_ok;
     };
+
 };
 
 done_testing();
diff --git a/templates/web/base/auth/_username_error.html b/templates/web/base/auth/_username_error.html
new file mode 100644
index 000000000..c0ddc135a
--- /dev/null
+++ b/templates/web/base/auth/_username_error.html
@@ -0,0 +1,13 @@
+[% IF username_error;
+    # other keys include fqdn, mxcheck if you'd like to write a custom error message
+    errors = {
+        nonmobile = loc('Please enter a mobile number'),
+        sms_failed = tprintf(loc('Sending a confirmation text failed: "%s"'), sms_error),
+        missing_phone = loc('Please enter your phone number'),
+        other_phone = loc('Please check your phone number is correct'),
+        missing_email = loc('Please enter your email'),
+        other_email = loc('Please check your email address is correct')
+    };
+    default = "other_$default";
+    errors.$username_error || errors.$default;
+END ~%]
diff --git a/templates/web/base/auth/change_phone.html b/templates/web/base/auth/change_phone.html
index 4b181f6c5..27a2f63dd 100644
--- a/templates/web/base/auth/change_phone.html
+++ b/templates/web/base/auth/change_phone.html
@@ -25,14 +25,8 @@ END
     <input type="hidden" name="token" value="[% csrf_token %]">
 
     <fieldset>
-    [% IF username_error;
-        errors = {
-            nonmobile = loc('Please enter a mobile number'),
-            missing_phone = loc('Please enter your phone number'),
-            other_phone = loc('Please check your phone number is correct')
-        };
-        loc_username_error = errors.$username_error || errors.other_phone;
-    %]
+    [% loc_username_error = INCLUDE 'auth/_username_error.html' default='phone' %]
+    [% IF loc_username_error  %]
     <div class="form-error">[% loc_username_error %]</div>
     [% END %]
 
diff --git a/templates/web/base/auth/general.html b/templates/web/base/auth/general.html
index e64230b41..d630dd415 100644
--- a/templates/web/base/auth/general.html
+++ b/templates/web/base/auth/general.html
@@ -36,20 +36,7 @@
       <div id="js-social-email-hide">
 [% END %]
 
-            [% IF username_error;
-
-                # other keys include fqdn, mxcheck if you'd like to write a custom error message
-
-                errors = {
-                    nonmobile = loc('Please enter a mobile number'),
-                    missing_email = loc('Please enter your email'),
-                    other_email = loc('Please check your email address is correct')
-                    missing_phone = loc('Please enter your phone number'),
-                    other_phone = loc('Please check your phone number is correct')
-                };
-
-                loc_username_error = errors.$username_error || errors.other_email;
-            END %]
+        [% loc_username_error = INCLUDE 'auth/_username_error.html' default='email' %]
 
 [% IF c.config.SMS_AUTHENTICATION %]
     [% SET username_label = loc('Your email or mobile') %]