Fix missing URLs in alert emails

If a staff user changed a problem’s state and an empty update was generated,
it was possible for alert emails to be sent with missing URLs because the
empty string in the update text was falsy and Alert.pm took the wrong path.

This fixes the problem by changing the test to defined() and includes a
regression test.

2 files changed, 81 insertions, 2 deletions

diff --git a/perllib/FixMyStreet/Script/Alerts.pm b/perllib/FixMyStreet/Script/Alerts.pm
index aefe13318..c001cc311 100644
--- a/perllib/FixMyStreet/Script/Alerts.pm
+++ b/perllib/FixMyStreet/Script/Alerts.pm
@@ -102,7 +102,7 @@ sub send() {
 
             my $url = $cobrand->base_url_for_report($row);
             # this is currently only for new_updates
-            if ($row->{item_text}) {
+            if (defined($row->{item_text})) {
                 if ( $cobrand->moniker ne 'zurich' && $row->{alert_user_id} == $row->{user_id} ) {
                     # This is an alert to the same user who made the report - make this a login link
                     # Don't bother with Zurich which has no accounts
diff --git a/t/app/controller/alert_new.t b/t/app/controller/alert_new.t
index ddf881f4d..97a19b3b8 100644
--- a/t/app/controller/alert_new.t
+++ b/t/app/controller/alert_new.t
@@ -288,7 +288,7 @@ for my $test (
     };
 }
 
-$mech->create_body_ok(2226, 'Gloucestershire County Council');
+my $gloucester = $mech->create_body_ok(2226, 'Gloucestershire County Council');
 $mech->create_body_ok(2326, 'Cheltenham Borough Council');
 
 subtest "Test two-tier council alerts" => sub {
@@ -475,6 +475,85 @@ subtest "Test normal alert signups and that alerts are sent" => sub {
     $mech->delete_user($user2);
 };
 
+subtest "Test alerts are correct 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 );
+    my $dt = DateTime->now(time_zone => 'Europe/London')->add(days => 2);
+
+    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( {
+        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       => $user2,
+    } )->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";
+
+    $mech->clear_emails_ok;
+    FixMyStreet::override_config {
+        MAPIT_URL => 'http://mapit.uk/',
+    }, sub {
+        FixMyStreet::Script::Alerts::send();
+    };
+
+    $mech->email_count_is(1);
+    my $email = $mech->get_email;
+    my $body = $mech->get_text_body_from_email($email);
+    like $body, qr/The following updates have been left on this report:/, 'email is about updates to existing report';
+    like $body, qr/Staff User/, 'Update comes from correct user';
+
+    my @urls = $mech->get_link_from_email($email, 1);
+    is $urls[0], "http://www.example.org/report/" . $report_id, "Correct report URL in email";
+
+    $mech->delete_user($user1);
+    $mech->delete_user($user2);
+    $mech->delete_user($user3);
+};
+
 subtest "Test signature template is used from cobrand" => sub {
     $mech->delete_user( 'reporter@example.com' );
     $mech->delete_user( 'alerts@example.com' );