From 1c288ef2dfc1b2e57d6a51c11401e95e8c589bd4 Mon Sep 17 00:00:00 2001 From: Zarino Zappia Date: Fri, 23 Nov 2018 13:37:54 +0000 Subject: Move email input nearer password input on forms. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This moves the email input from `user_loggedout.html` closer to the password inputs in `user_loggedout_{by_email,password}.html`, because we want to emphasise the connection between your login email/username and your password, and, now that only one "Yes I have an account / No I do not have an account" fieldset is displayed at a time, there was no reason to ask for the email/username up front. This, however, now means the form includes two username inputs, so: * `Report/New.pm` and `Report/Update.pm` now pick the first non-empty username param and use that. * `user_loggedout_email.html` now expects a `name` parameter, so that we can give the two username inputs unique ids in the markup. Also: * The "optional" phone and email inputs in user_loggedout_by_email.html are printed *after* the main username input if SMS login is enabled (since one or other of them is unhidden by javascript, based on whether you entered a phone number or and email address into the "username" input, and it would look weird to have an input become unhidden *above* the input you’re currently editing). --- .cypress/cypress/integration/simple_spec.js | 2 +- perllib/FixMyStreet/App/Controller/Report/New.pm | 6 ++++- .../FixMyStreet/App/Controller/Report/Update.pm | 6 ++++- templates/web/base/report/form/user_loggedout.html | 7 ------ .../base/report/form/user_loggedout_by_email.html | 27 ++++++++++++++++------ .../web/base/report/form/user_loggedout_email.html | 4 ++-- .../base/report/form/user_loggedout_password.html | 6 +++++ .../web/base/report/new/oauth_email_form.html | 4 +--- templates/web/base/report/update/form_user.html | 3 +-- .../bromley/report/form/user_loggedout_email.html | 4 ++-- .../zurich/report/new/fill_in_details_form.html | 4 ++-- web/cobrands/fixmystreet/fixmystreet.js | 5 ++-- 12 files changed, 47 insertions(+), 31 deletions(-) diff --git a/.cypress/cypress/integration/simple_spec.js b/.cypress/cypress/integration/simple_spec.js index 01fb77f49..7040681a3 100644 --- a/.cypress/cypress/integration/simple_spec.js +++ b/.cypress/cypress/integration/simple_spec.js @@ -12,8 +12,8 @@ describe('Clicking the map', function() { cy.get('[name=title]').type('Title'); cy.get('[name=detail]').type('Detail'); cy.get('#js-new-report-user-show').click(); - cy.get('[name=username]').type('user@example.org'); cy.get('#js-new-report-show-sign-in').click(); + cy.get('#form_username_sign_in').type('user@example.org'); cy.get('[name=password_sign_in]').type('password'); cy.get('[name=password_sign_in]').parents('form').submit(); cy.get('#map_sidebar').should('contain', 'check and confirm your details'); diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm index 1a1a657a9..ce2fe19f6 100644 --- a/perllib/FixMyStreet/App/Controller/Report/New.pm +++ b/perllib/FixMyStreet/App/Controller/Report/New.pm @@ -6,6 +6,7 @@ BEGIN { extends 'Catalyst::Controller'; } use Encode; use List::MoreUtils qw(uniq); +use List::Util 'first'; use POSIX 'strcoll'; use HTML::Entities; use mySociety::MaPit; @@ -789,7 +790,10 @@ sub process_user : Private { # Extract all the params to a hash to make them easier to work with my %params = map { $_ => $c->get_param($_) } - ( 'username', 'email', 'name', 'phone', 'password_register', 'fms_extra_title' ); + ( 'email', 'name', 'phone', 'password_register', 'fms_extra_title' ); + + # Report form includes two username fields: #form_username_register and #form_username_sign_in + $params{username} = (first { $_ } $c->get_param_list('username')) || ''; if ( $c->cobrand->allow_anonymous_reports ) { my $anon_details = $c->cobrand->anonymous_account; diff --git a/perllib/FixMyStreet/App/Controller/Report/Update.pm b/perllib/FixMyStreet/App/Controller/Report/Update.pm index dc46be61f..cbedf7a01 100644 --- a/perllib/FixMyStreet/App/Controller/Report/Update.pm +++ b/perllib/FixMyStreet/App/Controller/Report/Update.pm @@ -5,6 +5,7 @@ use namespace::autoclean; BEGIN { extends 'Catalyst::Controller'; } use Path::Class; +use List::Util 'first'; use Utils; =head1 NAME @@ -99,7 +100,10 @@ sub process_user : Private { # Extract all the params to a hash to make them easier to work with my %params = map { $_ => $c->get_param($_) } - ( 'username', 'name', 'password_register', 'fms_extra_title' ); + ( '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')) || ''; # Extra block to use 'last' if ( $c->user_exists ) { { diff --git a/templates/web/base/report/form/user_loggedout.html b/templates/web/base/report/form/user_loggedout.html index 8743883ee..c9c3fabad 100644 --- a/templates/web/base/report/form/user_loggedout.html +++ b/templates/web/base/report/form/user_loggedout.html @@ -2,13 +2,6 @@
[% PROCESS 'report/form/private_details.html' %] - -[% IF c.cobrand.social_auth_enabled %] - [% PROCESS 'report/form/user_loggedout_email.html' required=0 %] -[% ELSE %] - [% PROCESS 'report/form/user_loggedout_email.html' required=1 %] -[% END %] -

[% tprintf(loc("Do you have a %s password?", "%s is the site name"), site_name) %]

[% PROCESS 'report/form/user_loggedout_password.html' %] [% PROCESS 'report/form/user_loggedout_by_email.html' %] 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 e115f6d20..4ae3db868 100644 --- a/templates/web/base/report/form/user_loggedout_by_email.html +++ b/templates/web/base/report/form/user_loggedout_by_email.html @@ -8,7 +8,7 @@ [% INCLUDE 'report/_show_name_label.html' %] [% IF type != 'update' %] - [% UNLESS c.cobrand.call_hook('disable_phone_number_entry') %] + [% IF NOT c.cobrand.call_hook('disable_phone_number_entry') AND NOT c.config.SMS_AUTHENTICATION %]
[% IF field_errors.phone %] @@ -17,12 +17,6 @@
[% END %] - [% IF c.config.SMS_AUTHENTICATION %] -
- - -
- [% END %] [% END %] [% IF c.cobrand.moniker == 'fixamingata' %] @@ -31,6 +25,25 @@
[% END %] + [% IF c.cobrand.social_auth_enabled AND NOT email_required %] + [% PROCESS 'report/form/user_loggedout_email.html' name='username_register' required=0 %] + [% ELSE %] + [% PROCESS 'report/form/user_loggedout_email.html' name='username_register' required=1 %] + [% END %] + + [% IF type != 'update' AND c.config.SMS_AUTHENTICATION %] + [% UNLESS c.cobrand.call_hook('disable_phone_number_entry') %] +
+ + +
+ [% END %] +
+ + +
+ [% END %] + [% IF type == 'update' %]
diff --git a/templates/web/base/report/form/user_loggedout_email.html b/templates/web/base/report/form/user_loggedout_email.html index 932bc97a7..c32691eb4 100644 --- a/templates/web/base/report/form/user_loggedout_email.html +++ b/templates/web/base/report/form/user_loggedout_email.html @@ -9,11 +9,11 @@ [% SET username_value = object.user.email %] [% END %] - + [% IF field_errors.username %]

[% field_errors.username %]

[% END %] - diff --git a/templates/web/base/report/form/user_loggedout_password.html b/templates/web/base/report/form/user_loggedout_password.html index 016da88ad..dfd7d0d14 100644 --- a/templates/web/base/report/form/user_loggedout_password.html +++ b/templates/web/base/report/form/user_loggedout_password.html @@ -8,6 +8,12 @@ [% loc('Fill in your details manually.') %]

+ [% IF c.cobrand.social_auth_enabled %] + [% PROCESS 'report/form/user_loggedout_email.html' name='username_sign_in' required=0 %] + [% ELSE %] + [% PROCESS 'report/form/user_loggedout_email.html' name='username_sign_in' required=1 %] + [% END %] + [% IF field_errors.password %]

[% field_errors.password %]

diff --git a/templates/web/base/report/new/oauth_email_form.html b/templates/web/base/report/new/oauth_email_form.html index 394a4b743..5b4622cda 100644 --- a/templates/web/base/report/new/oauth_email_form.html +++ b/templates/web/base/report/new/oauth_email_form.html @@ -11,10 +11,8 @@
- [% PROCESS 'report/form/user_loggedout_email.html' type='report' object=report required=1 %] -
- [% PROCESS 'report/form/user_loggedout_by_email.html' object=report type='report' %] + [% PROCESS 'report/form/user_loggedout_by_email.html' object=report type='report' email_required=1 %]
diff --git a/templates/web/base/report/update/form_user.html b/templates/web/base/report/update/form_user.html index 5d4c67528..47ba18c12 100644 --- a/templates/web/base/report/update/form_user.html +++ b/templates/web/base/report/update/form_user.html @@ -10,9 +10,8 @@ [% IF c.user_exists %] [% PROCESS "report/update/form_user_loggedin.html" %] [% ELSIF oauth_need_email %] - [% PROCESS 'report/form/user_loggedout_email.html' type='update' object=update required=1 %]
- [% PROCESS 'report/form/user_loggedout_by_email.html' object=update type='update' valid_class='validNameU' %] + [% PROCESS 'report/form/user_loggedout_by_email.html' object=update type='update' valid_class='validNameU' email_required=1 %]
[% ELSE %] diff --git a/templates/web/bromley/report/form/user_loggedout_email.html b/templates/web/bromley/report/form/user_loggedout_email.html index fc4f83f9c..78b4a0044 100644 --- a/templates/web/bromley/report/form/user_loggedout_email.html +++ b/templates/web/bromley/report/form/user_loggedout_email.html @@ -8,7 +8,7 @@ [% SET username_value = object.user.email %] [% END %] -