If text auth on, ask which method they wish to use

5 files changed, 106 insertions, 44 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e9c5605a2..3249b0f9c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@
         - Add aerial maps option to Bing and OSM maps.
         - Select matches for both filter category and group. #3110
         - Add an extra zoom level to most map types. #3130
+        - Improve new report form when using phone verification.
     - Changes:
         - Mark user as active when sent an email alert.
     - Bugfixes:
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index d6314dacf..8f3261853 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -844,7 +844,7 @@ sub process_user : Private {
 
     # Extract all the params to a hash to make them easier to work with
     my %params = map { $_ => $c->get_param($_) }
-      ( 'email', 'name', 'phone', 'password_register', 'fms_extra_title' );
+      qw( email name phone password_register fms_extra_title update_method );
 
     # Report form includes two username fields: #form_username_register and #form_username_sign_in
     $params{username} = (first { $_ } $c->get_param_list('username')) || '';
@@ -898,6 +898,20 @@ sub process_user : Private {
         $params{username} = $params{phone};
     }
 
+    # Code to deal with SMS being switched on and so the user being asked to
+    # pick a method and no username field
+    if (!$params{username} && !$params{update_method}) {
+        $c->stash->{field_errors}->{update_method} = _('Please pick your update preference');
+    }
+    if (!$params{username} && $params{update_method}) {
+        if ($params{update_method} eq 'phone') {
+            $params{username} = $params{phone};
+        } else {
+            $params{username} = $params{email};
+        }
+        $c->stash->{update_method} = $params{update_method};
+    }
+
     my $parsed = FixMyStreet::SMS->parse_username($params{username});
     my $type = $parsed->{type} || 'email';
     $type = 'email' unless FixMyStreet->config('SMS_AUTHENTICATION') || $c->stash->{contributing_as_another_user};
@@ -1235,6 +1249,7 @@ sub check_for_errors : Private {
     # if using social login then we don't care about other errors
     $c->stash->{is_social_user} = $c->get_param('social_sign_in') ? 1 : 0;
     if ( $c->stash->{is_social_user} ) {
+        delete $field_errors{update_method};
         delete $field_errors{name};
         delete $field_errors{username};
     }
diff --git a/t/app/controller/report_new_text.t b/t/app/controller/report_new_text.t
index 852cdac76..9256a00de 100644
--- a/t/app/controller/report_new_text.t
+++ b/t/app/controller/report_new_text.t
@@ -15,17 +15,43 @@ $mech->create_contact_ok( body_id => $body->id, category => 'Street lighting', e
 $mech->create_contact_ok( body_id => $body->id, category => 'Trees', email => 'trees@example.com' );
 
 # test that phone number validation works okay
+my %defaults = (
+    title => 'Title', detail => 'Detail', name => 'Bob Jones',
+    category => 'Street lighting', may_show_name => 1,
+    photo1 => '', photo2 => '', photo3 => '',
+    password_register => '', password_sign_in => '',
+);
 foreach my $test (
     {
+        msg => 'missing update method',
+        pc => 'EH1 1BB',
+        fields => {
+            update_method => undef, phone => '', email => '',
+            %defaults,
+        },
+        changes => {
+            username => '',
+        },
+        errors => [ 'Please enter your email', 'Please pick your update preference' ],
+    },
+    {
+        msg => 'email method',
+        pc => 'EH1 1BB',
+        fields => {
+            update_method => 'email', phone => '', email => 'bademail',
+            %defaults,
+        },
+        changes => {
+            username => 'bademail',
+        },
+        errors => [ 'Please enter a valid email' ],
+    },
+    {
         msg => 'invalid number',
         pc => 'EH1 1BB',
         fields => {
-            username => '0121 4960000000', email => '', phone => '',
-            title => 'Title', detail => 'Detail', name => 'Bob Jones',
-            category => 'Street lighting',
-            may_show_name => '1',
-            photo1 => '', photo2 => '', photo3 => '',
-            password_register => '', password_sign_in => '',
+            update_method => 'phone', phone => '0121 4960000000', email => '',
+            %defaults,
         },
         changes => {
             username => '01214960000000',
@@ -37,12 +63,8 @@ foreach my $test (
         msg => 'landline number',
         pc => 'EH1 1BB',
         fields => {
-            username => '0121 4960000', email => '', phone => '',
-            title => 'Title', detail => 'Detail', name => 'Bob Jones',
-            category => 'Street lighting',
-            may_show_name => '1',
-            photo1 => '', photo2 => '', photo3 => '',
-            password_register => '', password_sign_in => '',
+            update_method => 'phone', phone => '0121 4960000', email => '',
+            %defaults,
         },
         changes => {
             username => '0121 496 0000',
@@ -142,7 +164,8 @@ foreach my $test (
                     title => 'Test Report', detail => 'Test report details.',
                     photo1 => '',
                     name => 'Joe Bloggs', may_show_name => '1',
-                    username => $test_phone,
+                    update_method => 'phone',
+                    phone => $test_phone,
                     category => 'Street lighting',
                     password_register => $test->{password} ? 'secret' : '',
                 }
diff --git a/templates/web/base/report/form/user_loggedout_by_email.html b/templates/web/base/report/form/user_loggedout_by_email.html
index 459428059..2e41deaea 100644
--- a/templates/web/base/report/form/user_loggedout_by_email.html
+++ b/templates/web/base/report/form/user_loggedout_by_email.html
@@ -7,15 +7,13 @@
     [% INCLUDE 'report/form/user_name.html' %]
     [% INCLUDE 'report/_show_name_label.html' %]
 
-  [% IF type != 'update' %]
-    [% IF NOT c.cobrand.call_hook('disable_phone_number_entry') AND NOT c.config.SMS_AUTHENTICATION %]
-        <div id="js-hide-if-username-phone">
-            <label for="form_phone">[% loc('Phone number (optional)') %]</label>
-            [% IF field_errors.phone %]
-              <p class='form-error'>[% field_errors.phone %]</p>
-            [% END %]
-            <input class="form-control" type="text" value="[% phone | html %]" name="phone" id="form_phone">
-        </div>
+  [% IF type != 'update' AND NOT c.config.SMS_AUTHENTICATION %]
+    [% UNLESS c.cobrand.call_hook('disable_phone_number_entry') %]
+        <label for="form_phone">[% loc('Phone number (optional)') %]</label>
+        [% IF field_errors.phone %]
+          <p class='form-error'>[% field_errors.phone %]</p>
+        [% END %]
+        <input class="form-control" type="text" value="[% phone | html %]" name="phone" id="form_phone">
     [% END %]
   [% END %]
 
@@ -25,19 +23,44 @@
     </div>
   [% END %]
 
-    [% PROCESS 'report/form/user_loggedout_email.html' name='username_register' %]
+  [% IF type != 'update' AND c.config.SMS_AUTHENTICATION AND NOT c.cobrand.call_hook('disable_phone_number_entry') %]
 
-  [% IF type != 'update' AND c.config.SMS_AUTHENTICATION %]
-    [% UNLESS c.cobrand.call_hook('disable_phone_number_entry') %]
-        <div id="js-hide-if-username-phone" class="hidden-js">
-            <label for="form_phone">[% loc('Phone number (optional)') %]</label>
-            <input class="form-control" type="text" value="[% phone | html %]" name="phone" id="form_phone">
-        </div>
+  <fieldset>
+      <legend>How would you like to receive updates?</legend>
+
+    [% IF field_errors.update_method %]
+        <p class='form-error'>[% field_errors.update_method %]</p>
     [% END %]
-    <div id="js-hide-if-username-email" class="hidden-js">
-        <label for="form_email">[% loc('Email address (optional)') %]</label>
-        <input class="form-control" type="text" value="[% email | html %]" name="email" id="form_email">
-    </div>
+    <p class="segmented-control segmented-control--radio">
+        <input required type="radio" name="update_method" id="update_method_email"
+             data-show="#js-optional-phone" data-hide="#js-optional-email"
+            value="email"[% ' checked' IF update_method == 'email' %]>
+        <label class="btn" for="update_method_email">[% loc('Email') %]</label>
+        <input type="radio" name="update_method" id="update_method_phone"
+             data-show="#js-optional-email" data-hide="#js-optional-phone"
+            value="phone"[% ' checked' IF update_method == 'phone' %]>
+        <label class="btn" for="update_method_phone">[% loc('Phone') %]</label>
+    </p>
+  </fieldset>
+
+    [% IF field_errors.username_register %]
+        <p class='form-error'>[% field_errors.username_register %]</p>
+    [% END %]
+
+    <label for="form_email">[% loc('Your email') %]<span class="hidden-js" id="js-optional-email"> [% loc('(optional)') %]</span></label>
+    [% IF field_errors.email %]
+        <p class='form-error'>[% field_errors.email %]</p>
+    [% END %]
+    <input type="email" name="email" id="form_email" value="[% email %]" class="form-control">
+
+    <label for="form_phone">[% loc('Phone number') %]<span class="hidden-js" id="js-optional-phone"> [% loc('(optional)') %]</span></label>
+    [% IF field_errors.phone %]
+        <p class='form-error'>[% field_errors.phone %]</p>
+    [% END %]
+    <input class="form-control" type="text" value="[% phone %]" name="phone" id="form_phone">
+
+  [% ELSE %]
+    [% PROCESS 'report/form/user_loggedout_email.html' name='username_register' %]
   [% END %]
 
   [% IF type == 'update' %]
diff --git a/web/cobrands/fixmystreet/fixmystreet.js b/web/cobrands/fixmystreet/fixmystreet.js
index 5485ff7c7..09f4f49a3 100644
--- a/web/cobrands/fixmystreet/fixmystreet.js
+++ b/web/cobrands/fixmystreet/fixmystreet.js
@@ -1094,16 +1094,16 @@ $.extend(fixmystreet.set_up, {
     });
   },
 
-  reporting_hide_phone_email: function() {
-    $('#form_username_register').on('keyup change', function() {
-        var username = $(this).val();
-        if (/^[^a-z]+$/i.test(username)) {
-            $('#js-hide-if-username-phone').hide();
-            $('#js-hide-if-username-email').show();
-        } else {
-            $('#js-hide-if-username-phone').show();
-            $('#js-hide-if-username-email').hide();
-        }
+  reporting_required_phone_email: function() {
+    var fem = $('#form_email');
+    var fph = $('#form_phone');
+    $('#update_method_email').on('change', function() {
+      fem.prop('required', true);
+      fph.prop('required', false);
+    });
+    $('#update_method_phone').on('change', function() {
+      fem.prop('required', false);
+      fph.prop('required', true);
     });
   },