Split up two username fields.

Rename the not-logging-in username field to username_register. Keep the sign-in field as username because that e.g. overlaps with auth code in two-factor authentication.
mysociety · Sep 24, 2020 · dba30f9 · dba30f9
1 parent 727b115
commit dba30f9
Show file tree

Hide file tree

Showing 22 changed files with 134 additions and 97 deletions.
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -6,7 +6,7 @@ BEGIN { extends 'Catalyst::Controller'; }
 
 use utf8;
 use Encode;
-use List::Util qw(first uniq);
+use List::Util qw(uniq);
 use HTML::Entities;
 use Path::Class;
 use Utils;
@@ -126,8 +126,10 @@ sub report_new_ajax : Path('mobile') : Args(0) {
 
     # Apps are sending email as username
     # Prepare for when they upgrade
-    if (!$c->get_param('username')) {
-        $c->set_param('username', $c->get_param('email'));
+    my $username_field = ( $c->get_param('submit_sign_in') || $c->get_param('password_sign_in') )
+        ? 'username': 'username_register';
+    if (!$c->get_param($username_field)) {
+        $c->set_param($username_field, $c->get_param('email'));
     }
 
     # create the report - loading a partial if available
@@ -844,8 +846,14 @@ sub process_user : Private {
     my %params = map { $_ => $c->get_param($_) }
       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')) || '';
+    if ($c->user_exists) {
+        $params{username} = $c->get_param('username');
+    } elsif ($c->get_param('submit_sign_in') || $c->get_param('password_sign_in')) {
+        $params{username} = $c->get_param('username');
+    } else {
+        $params{username} = $c->get_param('username_register');
+    }
+    $params{username} ||= '';
 
     my $anon_button = $c->cobrand->allow_anonymous_reports eq 'button' && $c->get_param('report_anonymously');
     my $anon_fallback = $c->cobrand->allow_anonymous_reports eq '1' && !$c->user_exists && !$params{username};
@@ -913,8 +921,7 @@ sub process_user : Private {
     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};
-    $report->user( $c->model('DB::User')->find_or_new( { $type => $parsed->{username} } ) )
-        unless $report->user;
+    $report->user( $c->model('DB::User')->find_or_new( { $type => $parsed->{username} } ) );
 
     $c->stash->{phone_may_be_mobile} = $type eq 'phone' && $parsed->{may_be_mobile};
 

diff --git a/perllib/FixMyStreet/App/Controller/Report/Update.pm b/perllib/FixMyStreet/App/Controller/Report/Update.pm
@@ -6,7 +6,6 @@ BEGIN { extends 'Catalyst::Controller'; }
 
 use utf8;
 use Path::Class;
-use List::Util 'first';
 use Utils;
 
 =head1 NAME
@@ -103,8 +102,14 @@ sub process_user : Private {
     my %params = map { $_ => $c->get_param($_) }
       ( 'name', 'password_register', 'fms_extra_title' );
 
-    # Update form includes two username fields: #form_username_register and #form_username_sign_in
-    $params{username} = (first { $_ } $c->get_param_list('username')) || '';
+    if ($c->user_exists) {
+        $params{username} = $c->get_param('username');
+    } elsif ($c->get_param('submit_sign_in') || $c->get_param('password_sign_in')) {
+        $params{username} = $c->get_param('username');
+    } else {
+        $params{username} = $c->get_param('username_register');
+    }
+    $params{username} ||= '';
 
     my $anon_button = $c->cobrand->allow_anonymous_reports eq 'button' && $c->get_param('report_anonymously');
     if ($anon_button) {
@@ -145,8 +150,7 @@ sub process_user : Private {
     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};
-    $update->user( $c->model('DB::User')->find_or_new( { $type => $parsed->{username} } ) )
-        unless $update->user;
+    $update->user( $c->model('DB::User')->find_or_new( { $type => $parsed->{username} } ) );
 
     $c->stash->{phone_may_be_mobile} = $type eq 'phone' && $parsed->{may_be_mobile};
 

diff --git a/t/app/controller/auth_social.t b/t/app/controller/auth_social.t
@@ -241,7 +241,8 @@ for my $state ( 'refused', 'no email', 'existing UID', 'okay' ) {
                 $mech->content_contains('We need your email address, please give it below.');
                 # We don't have an email, so check that we can still submit it,
                 # and the ID carries through the confirmation
-                $fields->{username} = $test->{email};
+                $fields->{username} = $test->{email} if $page eq 'my';
+                $fields->{username_register} = $test->{email} unless $page eq 'my';
                 $fields->{name} = 'Ffion Tester' unless $page eq 'my';
                 $mech->submit_form(with_fields => $fields, $page eq 'my' ? (button => 'sign_in_by_code') : ());
                 $mech->content_contains('Nearly done! Now check your email');
@@ -408,7 +409,8 @@ for my $tw_state ( 'refused', 'existing UID', 'no email' ) {
                 $mech->content_contains('We need your email address, please give it below.');
                 # We don't have an email, so check that we can still submit it,
                 # and the ID carries through the confirmation
-                $fields->{username} = $tw_email;
+                $fields->{username_register} = $tw_email unless $page eq 'my';
+                $fields->{username} = $tw_email if $page eq 'my';
                 $fields->{name} = 'Ffion Tester' unless $page eq 'my';
                 $mech->submit_form(with_fields => $fields, $page eq 'my' ? (button => 'sign_in_by_code') : ());
                 $mech->content_contains('Nearly done! Now check your email');

diff --git a/t/app/controller/contact_enquiry.t b/t/app/controller/contact_enquiry.t
@@ -80,7 +80,7 @@ FixMyStreet::override_config {
         $mech->submit_form_ok( {
             with_fields => {
                 name => 'Test User',
-                username => 'testuser@example.org',
+                username_register => 'testuser@example.org',
                 category => 'Other',
                 detail => 'This is a general enquiry',
             }
@@ -149,7 +149,7 @@ FixMyStreet::override_config {
         $mech->submit_form_ok( {
             with_fields => {
                 name => 'Simon Neil',
-                username => $user->email,
+                username_register => $user->email,
                 category => 'General Enquiry',
                 detail => 'This is a general enquiry',
             }
@@ -212,7 +212,7 @@ FixMyStreet::override_config {
             submit_problem => 1,
             token => $csrf,
             name => 'Test User',
-            username => 'testuser@example.org',
+            username_register => 'testuser@example.org',
             category => 'Other',
             detail => encode_utf8('This is a general enquiry‽'),
             photo1         => [ $sample_jpeg, undef, Content_Type => 'image/jpeg' ],

diff --git a/t/app/controller/report_display.t b/t/app/controller/report_display.t
@@ -109,7 +109,7 @@ subtest "test a good report" => sub {
 
     my %fields = (
         name      => '',
-        username => '',
+        username_register => '',
         update    => '',
         add_alert => 1, # defaults to true
         fixed     => undef

diff --git a/t/app/controller/report_import.t b/t/app/controller/report_import.t
@@ -376,7 +376,7 @@ subtest "Submit a correct entry (with location) to cobrand" => sub {
         photo2         => '',
         photo3         => '',
         phone         => '',
-        username => 'test-ll@example.com',
+        username_register => 'test-ll@example.com',
       },
       "check imported fields are shown"
           or diag Dumper( $mech->visible_form_values ); use Data::Dumper;

diff --git a/t/app/controller/report_new.t b/t/app/controller/report_new.t
@@ -156,7 +156,7 @@ foreach my $test (
                     photo1        => '',
                     name          => 'Joe Bloggs',
                     may_show_name => '1',
-                    username      => 'test-1@example.com',
+                    username_register => 'test-1@example.com',
                     phone         => '07903 123 456',
                     category      => 'Street lighting',
                     password_register => $test->{password} ? 'secret' : '',
@@ -680,7 +680,7 @@ subtest "test report creation for a category that is non public" => sub {
                     title         => 'Test Report',
                     detail        => 'Test report details.',
                     photo1        => '',
-                    username      => $user->email,
+                    username_register => $user->email,
                     name          => 'Joe Bloggs',
                     category      => 'Street lighting',
                 }
@@ -940,7 +940,7 @@ for my $test (
             title             => "Test Report",
             detail            => 'Test report details.',
             photo1            => '',
-            username          => 'firstlast@example.com',
+            username_register => 'firstlast@example.com',
             may_show_name     => '1',
             phone             => '07903 123 456',
             category          => 'Trees',
@@ -1085,7 +1085,7 @@ subtest "test Hart" => sub {
                 $mech->submit_form_ok( { with_fields => { pc => 'GU51 4AE' } }, "submit location" );
                 $mech->follow_link_ok( { text_regex => qr/skip this step/i, }, "follow 'skip this step' link" );
                 my %optional_fields = $test->{confirm} ?  () :
-                    ( username => $test_email, phone => '07903 123 456' );
+                    ( username_register => $test_email, phone => '07903 123 456' );
 
                 # we do this as otherwise test::www::mechanize::catalyst
                 # goes to the value set in ->host above irregardless and
@@ -1279,7 +1279,7 @@ subtest "extra google analytics code displayed on email confirmation problem cre
             title             => "Test Report",
             detail            => 'Test report details.',
             photo1            => '',
-            username          => 'firstlast@example.com',
+            username_register => 'firstlast@example.com',
             name              => 'Test User',
             may_show_name     => '1',
             phone             => '07903 123 456',