Fix issue sending alerts around no-text update.

If there was a normal email alert, and then an alert for an update with
no text (so no email should be sent), it would still try to send the
blank email, and then die because no e.g. $data{cobrand} set.

This moves the skip-blank-update check higher up, above any email
sending code.

2 files changed, 66 insertions, 14 deletions

diff --git a/perllib/FixMyStreet/Script/Alerts.pm b/perllib/FixMyStreet/Script/Alerts.pm
index 3f26b749b..4b5641f9e 100644
--- a/perllib/FixMyStreet/Script/Alerts.pm
+++ b/perllib/FixMyStreet/Script/Alerts.pm
@@ -88,6 +88,24 @@ sub send() {
                 alert_id  => $row->{alert_id},
                 parameter => $row->{item_id},
             } );
+
+            # this is currently only for new_updates
+            if (defined($row->{item_text})) {
+                # this might throw up the odd false positive but only in cases where the
+                # state has changed and there was already update text
+                if ($row->{item_problem_state} &&
+                    !( $last_problem_state eq '' && $row->{item_problem_state} eq 'confirmed' ) &&
+                    $last_problem_state ne $row->{item_problem_state}
+                ) {
+                    my $state = FixMyStreet::DB->resultset("State")->display($row->{item_problem_state}, 1, $cobrand);
+
+                    my $update = _('State changed to:') . ' ' . $state;
+                    $row->{item_text} = $row->{item_text} ? $row->{item_text} . "\n\n" . $update :
+                                                            $update;
+                }
+                next unless $row->{item_text};
+            }
+
             if ($last_alert_id && $last_alert_id != $row->{alert_id}) {
                 $last_problem_state = '';
                 _send_aggregated_alert_email(%data);
@@ -106,19 +124,6 @@ sub send() {
             my $url = $cobrand->base_url_for_report($row);
             # this is currently only for new_updates
             if (defined($row->{item_text})) {
-                # this might throw up the odd false positive but only in cases where the
-                # state has changed and there was already update text
-                if ($row->{item_problem_state} &&
-                    !( $last_problem_state eq '' && $row->{item_problem_state} eq 'confirmed' ) &&
-                    $last_problem_state ne $row->{item_problem_state}
-                ) {
-                    my $state = FixMyStreet::DB->resultset("State")->display($row->{item_problem_state}, 1, $cobrand);
-
-                    my $update = _('State changed to:') . ' ' . $state;
-                    $row->{item_text} = $row->{item_text} ? $row->{item_text} . "\n\n" . $update :
-                                                            $update;
-                }
-                next unless $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 9f2e1f53a..4e8fd1b29 100644
--- a/t/app/controller/alert_new.t
+++ b/t/app/controller/alert_new.t
@@ -513,6 +513,40 @@ subtest "Test alerts are not sent for no-text updates" => sub {
     my $report_id = $report->id;
     ok $report, "created test report - $report_id";
 
+    my $report2 = FixMyStreet::App->model('DB::Problem')->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 $report2_id = $report2->id;
+    ok $report2, "created test report - $report2_id";
+
+    # Must be first
+    my $alert2 = FixMyStreet::App->model('DB::Alert')->create( {
+        parameter  => $report2_id,
+        alert_type => 'new_updates',
+        user       => $user2,
+    } )->confirm;
+    ok $alert2, 'created alert for other user';
+
     my $alert = FixMyStreet::App->model('DB::Alert')->create( {
         parameter  => $report_id,
         alert_type => 'new_updates',
@@ -533,6 +567,19 @@ subtest "Test alerts are not sent for no-text updates" => sub {
     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->clear_emails_ok;
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
@@ -540,7 +587,7 @@ subtest "Test alerts are not sent for no-text updates" => sub {
         FixMyStreet::Script::Alerts::send();
     };
 
-    $mech->email_count_is(0);
+    $mech->email_count_is(1);
 
     $mech->delete_user($user1);
     $mech->delete_user($user2);