Don't ask for email on alert signup if logged in.

The “Get updates” flow on a report page, if logged in, was showing
an input label but no input field (because one is not needed), but
then on submission asking for your email address. Add missing name
on submit button to fix this.

3 files changed, 31 insertions, 181 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 085fcecb1..c7d5d4bb6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,10 +15,11 @@
         - Delete cache photos upon photo moderation. #2374
         - Remove any use of `my $x if $foo`. #2377
         - Fix saving of inspect form data offline.
-        - Add CSRF and time to contact form.
-        - Make sure admin metadata dropdown index numbers are updated too.
-        - Fix issue with Open311 codes starting with ‘_’.
-        - Add parameter to URL when “Show older” clicked.
+        - Add CSRF and time to contact form. #2388
+        - Make sure admin metadata dropdown index numbers are updated too. #2369
+        - Fix issue with Open311 codes starting with ‘_’. #2391
+        - Add parameter to URL when “Show older” clicked. #2397
+        - Don't ask for email on alert signup if logged in. #2402
     - Development improvements:
         - Make front page cache time configurable.
         - Better working of /fakemapit/ under https.
diff --git a/t/app/controller/alert_new.t b/t/app/controller/alert_new.t
index 49efc4efa..f816c5317 100644
--- a/t/app/controller/alert_new.t
+++ b/t/app/controller/alert_new.t
@@ -326,11 +326,7 @@ subtest "Test two-tier council alerts" => sub {
 };
 
 subtest "Test normal alert signups and that alerts are sent" => sub {
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
-
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User' );
-
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User' );
 
     for my $alert (
@@ -375,64 +371,29 @@ subtest "Test normal alert signups and that alerts are sent" => sub {
     my $dt_parser = FixMyStreet::App->model('DB')->schema->storage->datetime_parser;
 
     my $report_time = '2011-03-01 12:00:00';
-    my $report = FixMyStreet::App->model('DB::Problem')->find_or_create( {
+    my ($report) = $mech->create_problems_for_body(1, 1, 'Testing', {
+        dt => $dt,
+        user => $user1,
         postcode           => 'EH1 1BB',
-        bodies_str         => '1',
         areas              => ',11808,135007,14419,134935,2651,20728,',
         category           => 'Street lighting',
-        title              => 'Testing',
-        detail             => 'Testing Detail',
-        used_map           => 1,
-        name               => $user1->name,
-        anonymous          => 0,
         state              => 'fixed - user',
-        confirmed          => $dt_parser->format_datetime($dt),
         lastupdate         => $dt_parser->format_datetime($dt),
         whensent           => $dt_parser->format_datetime($dt->clone->add( minutes => 5 )),
-        lang               => 'en-gb',
-        service            => '',
-        cobrand            => 'default',
-        cobrand_data       => '',
-        send_questionnaire => 1,
         latitude           => '55.951963',
         longitude          => '-3.189944',
-        user_id            => $user1->id,
-    } );
+    });
     my $report_id = $report->id;
     ok $report, "created test report - $report_id";
 
-    my $alert = FixMyStreet::App->model('DB::Alert')->create( {
-        parameter  => $report_id,
-        alert_type => 'new_updates',
-        user       => $user1,
-    } )->confirm;
-    ok $alert, 'created alert for reporter';
-
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report_id,
-        user_id    => $user2->id,
-        name       => 'Other User',
-        mark_fixed => 'false',
-        text       => 'This is some update text',
-        state      => 'confirmed',
-        confirmed  => $dt->clone->add( hours => 7 ),
-        anonymous  => 'f',
-    } );
-    my $update_id = $update->id;
-    ok $update, "created test update - $update_id";
-
-    $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report_id,
-        user_id    => $user2->id,
-        name       => 'Anonymous User',
-        mark_fixed => 'true',
-        text       => 'This is some more update text',
-        state      => 'confirmed',
-        confirmed  => $dt->clone->add( hours => 8 ),
-        anonymous  => 't',
-    } );
-    $update_id = $update->id;
-    ok $update, "created test update - $update_id";
+    subtest 'check signing up for alerts via report page' => sub {
+        $mech->log_in_ok($user1->email);
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form_ok({ button => 'alert', with_fields => { type => 'updates' } });
+    };
+
+    $mech->create_comment_for_problem($report, $user2, 'Other User', 'This is some update text', 'f', 'confirmed', undef, { confirmed  => $dt->clone->add( hours => 7 ) });
+    $mech->create_comment_for_problem($report, $user2, 'Anonymous User', 'This is some more update text', 't', 'confirmed', 'fixed - user', { confirmed  => $dt->clone->add( hours => 8 ) });
 
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
@@ -477,9 +438,6 @@ subtest "Test normal alert signups and that alerts are sent" => sub {
 };
 
 subtest "Test alerts are not sent for no-text updates" => sub {
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
-
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User' );
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User' );
     my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
@@ -555,31 +513,8 @@ subtest "Test alerts are not sent for no-text updates" => sub {
     } )->confirm;
     ok $alert, 'created alert for other user';
 
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id    => $report_id,
-        user_id       => $user3->id,
-        name          => 'Staff User',
-        mark_fixed    => 'false',
-        text          => '',
-        state         => 'confirmed',
-        confirmed     => $dt->clone->add( hours => 9 ),
-        anonymous     => 'f',
-    } );
-    my $update_id = $update->id;
-    ok $update, "created test update from staff user - $update_id";
-
-    my $update2 = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id    => $report2_id,
-        user_id       => $user3->id,
-        name          => 'Staff User',
-        mark_fixed    => 'false',
-        text          => 'This is a normal update',
-        state         => 'confirmed',
-        confirmed     => $dt->clone->add( hours => 9 ),
-        anonymous     => 'f',
-    } );
-    my $update2_id = $update2->id;
-    ok $update2, "created test update from staff user - $update2_id";
+    $mech->create_comment_for_problem($report, $user3, 'Staff User', '', 'f', 'confirmed', undef, { confirmed  => $dt->clone->add( hours => 9 ) });
+    $mech->create_comment_for_problem($report2, $user3, 'Staff User', 'This is a normal update', 'f', 'confirmed', undef, { confirmed  => $dt->clone->add( hours => 9 ) });
 
     $mech->clear_emails_ok;
     FixMyStreet::override_config {
@@ -596,9 +531,6 @@ subtest "Test alerts are not sent for no-text updates" => sub {
 };
 
 subtest "Test no marked as confirmed added to alerts" => sub {
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
-
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User' );
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User' );
     my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
@@ -640,19 +572,7 @@ subtest "Test no marked as confirmed added to alerts" => sub {
     } )->confirm;
     ok $alert, 'created alert for other user';
 
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id    => $report_id,
-        user_id       => $user3->id,
-        name          => 'Staff User',
-        mark_fixed    => 'false',
-        text          => 'this is update',
-        state         => 'confirmed',
-        problem_state => 'confirmed',
-        confirmed     => $dt->clone->add( hours => 9 ),
-        anonymous     => 'f',
-    } );
-    my $update_id = $update->id;
-    ok $update, "created test update from staff user - $update_id";
+    $mech->create_comment_for_problem($report, $user3, 'Staff User', 'this is update', 'f', 'confirmed', 'confirmed', { confirmed  => $dt->clone->add( hours => 9 ) });
 
     $mech->clear_emails_ok;
     FixMyStreet::override_config {
@@ -694,9 +614,6 @@ for my $test (
     },
 ) {
     subtest $test->{desc} => sub {
-        $mech->delete_user( 'reporter@example.com' );
-        $mech->delete_user( 'alerts@example.com' );
-
         my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User' );
         my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User' );
         my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
@@ -738,19 +655,7 @@ for my $test (
         } )->confirm;
         ok $alert, 'created alert for other user';
 
-        my $update = FixMyStreet::App->model('DB::Comment')->create( {
-            problem_id    => $report_id,
-            user_id       => $user3->id,
-            name          => 'Staff User',
-            mark_fixed    => 'false',
-            text          => $test->{update_text},
-            problem_state => $test->{problem_state},
-            state         => 'confirmed',
-            confirmed     => $dt->clone->add( hours => 9 ),
-            anonymous     => 'f',
-        } );
-        my $update_id = $update->id;
-        ok $update, "created test update from staff user - $update_id";
+        $mech->create_comment_for_problem($report, $user3, 'Staff User', $test->{update_text}, 'f', 'confirmed', $test->{problem_state}, { confirmed  => $dt->clone->add( hours => 9 ) });
 
         $mech->clear_emails_ok;
         FixMyStreet::override_config {
@@ -777,11 +682,7 @@ for my $test (
 }
 
 subtest "Test signature template is used from cobrand" => sub {
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
-
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User' );
-
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User' );
 
     my $dt = DateTime->now(time_zone => 'Europe/London')->add(days => 2);
@@ -824,19 +725,7 @@ subtest "Test signature template is used from cobrand" => sub {
     my $ret = $alert->confirm;
     ok $ret, 'created alert for reporter';
 
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report_id,
-        user_id    => $user2->id,
-        name       => 'Other User',
-        mark_fixed => 'false',
-        text       => 'This is some update text',
-        state      => 'confirmed',
-        confirmed  => $dt->clone->add( hours => 7 ),
-        anonymous  => 'f',
-    } );
-    my $update_id = $update->id;
-    ok $update, "created test update - $update_id";
-
+    $mech->create_comment_for_problem($report, $user2, 'Other User', 'This is some update text', 'f', 'confirmed', undef, { confirmed  => $dt->clone->add( hours => 7 ) });
 
     $mech->clear_emails_ok;
     FixMyStreet::override_config {
@@ -850,18 +739,7 @@ subtest "Test signature template is used from cobrand" => sub {
     like $email, qr/All the best/, 'default signature used';
     unlike $email, qr/twitter.com/, 'nothing from fixmystreet signature';
 
-    $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report_id,
-        user_id    => $user2->id,
-        name       => 'Anonymous User',
-        mark_fixed => 'true',
-        text       => 'This is some more update text',
-        state      => 'confirmed',
-        confirmed  => $dt->clone->add( hours => 8 ),
-        anonymous  => 't',
-    } );
-    $update_id = $update->id;
-    ok $update, "created test update - $update_id";
+    $mech->create_comment_for_problem($report, $user2, 'Anonymous User', 'This is some more update text', 't', 'confirmed', 'fixed - user', { confirmed  => $dt->clone->add( hours => 8 ) });
 
     $alert->cobrand('fixmystreet');
     $alert->update;
@@ -916,11 +794,7 @@ for my $test (
     },
 ) {
     subtest $test->{desc} => sub {
-        $mech->delete_user( 'reporter@example.com' );
-        $mech->delete_user( 'alerts@example.com' );
-
         my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User');
-
         my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User');
 
         my $dt = DateTime->now->add( minutes => -30 );
@@ -984,13 +858,8 @@ for my $test (
 }
 
 subtest 'check new updates alerts for non public reports only go to report owner' => sub {
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
-
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User');
-
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User');
-
     my $user3 = $mech->create_user_ok('updates@example.com', name => 'Update User');
 
     my $dt = DateTime->now->add( minutes => -30 );
@@ -1023,16 +892,7 @@ subtest 'check new updates alerts for non public reports only go to report owner
         non_public         => 1,
     } );
 
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report->id,
-        user_id    => $user3->id,
-        name       => 'Anonymous User',
-        mark_fixed => 'false',
-        text       => 'This is some more update text',
-        state      => 'confirmed',
-        confirmed  => $r_dt->clone->add( minutes => 8 ),
-        anonymous  => 't',
-    } );
+    $mech->create_comment_for_problem($report, $user3, 'Anonymous User', 'This is some more update text', 't', 'confirmed', undef, { confirmed  => $r_dt->clone->add( minutes => 8 ) });
 
     my $alert_user1 = FixMyStreet::App->model('DB::Alert')->create( {
             user       => $user1,
@@ -1072,18 +932,14 @@ subtest 'check new updates alerts for non public reports only go to report owner
     $mech->delete_user( $user3 );
 };
 
-subtest 'check setting inlude dates in new updates cobrand option' => sub {
+subtest 'check setting include dates in new updates cobrand option' => sub {
     my $include_date_in_alert_override= Sub::Override->new(
         "FixMyStreet::Cobrand::Default::include_time_in_update_alerts",
         sub { return 1; }
     );
-    $mech->delete_user( 'reporter@example.com' );
-    $mech->delete_user( 'alerts@example.com' );
 
     my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User');
-
     my $user2 = $mech->create_user_ok('alerts@example.com', name => 'Alert User');
-
     my $user3 = $mech->create_user_ok('updates@example.com', name => 'Update User');
 
     my $dt = DateTime->now->add( minutes => -30 );
@@ -1115,16 +971,7 @@ subtest 'check setting inlude dates in new updates cobrand option' => sub {
         user_id            => $user2->id,
     } );
 
-    my $update = FixMyStreet::App->model('DB::Comment')->create( {
-        problem_id => $report->id,
-        user_id    => $user3->id,
-        name       => 'Anonymous User',
-        mark_fixed => 'false',
-        text       => 'This is some more update text',
-        state      => 'confirmed',
-        confirmed  => $r_dt->clone->add( minutes => 8 ),
-        anonymous  => 't',
-    } );
+    my $update = $mech->create_comment_for_problem($report, $user3, 'Anonymous User', 'This is some more update text', 't', 'confirmed', undef, { confirmed  => $r_dt->clone->add( minutes => 8 ) });
 
     my $alert_user1 = FixMyStreet::App->model('DB::Alert')->create( {
             user       => $user1,
diff --git a/templates/web/base/report/display_tools.html b/templates/web/base/report/display_tools.html
index be788a50d..4ba8c8b2c 100644
--- a/templates/web/base/report/display_tools.html
+++ b/templates/web/base/report/display_tools.html
@@ -43,13 +43,15 @@
         </a>
         [% loc('Receive email when updates are left on this problem.' ) %]</p>
         <fieldset>
+      [% IF c.user_exists %]
+        <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
+      [% ELSE %]
         <label for="alert_rznvy">[% loc('Your email') %]</label>
         <div class="form-txt-submit-box">
-          [% IF NOT c.user_exists %]
             <input type="email" class="form-control" name="rznvy" id="alert_rznvy" value="[% email | html %]" size="30">
-          [% END %]
-            <input class="green-btn" type="submit" value="[% loc('Subscribe') %]">
+            <input class="green-btn" type="submit" name="alert" value="[% loc('Subscribe') %]">
         </div>
+      [% END %]
         <input type="hidden" name="token" value="[% csrf_token %]">
         <input type="hidden" name="id" value="[% problem.id %]">
         <input type="hidden" name="type" value="updates">