Merge a6e7516 into 078fec6

mysociety · Oct 20, 2021 · 96d016d · 96d016d
2 parents 078fec6 + a6e7516
commit 96d016d
Show file tree

Hide file tree

Showing 12 changed files with 286 additions and 229 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@
         linux install #3544
         - Upgrade jQuery. #3017
         - Upgrade Mozilla::CA to handle new root certificates.
+        - Factor alert script to slightly smaller functions. #3615
     - Open311 improvements:
         - Consistent protected field ordering.
         - Move test handling out of core code.

diff --git a/bin/send-alerts b/bin/send-alerts
@@ -17,6 +17,7 @@ BEGIN {
     require "$d/../setenv.pl";
 }
 
+use Getopt::Long::Descriptive;
 use CronFns;
 
 use FixMyStreet;
@@ -25,5 +26,16 @@ use FixMyStreet::Script::Alerts;
 my $site = CronFns::site(FixMyStreet->config('BASE_URL'));
 CronFns::language($site);
 
-FixMyStreet::Script::Alerts::send();
-
+my ($opts, $usage) = describe_options(
+    '%c %o - if no subgroup picked, all will be sent',
+    ['updates|u', 'send report update alerts'],
+    ['local|l', 'send non-radius-based local alerts'],
+    ['radius|r', 'send radius-based local alerts'],
+    ['help|h', "print usage message and exit" ],
+);
+$usage->die if $opts->help;
+
+my $any = $opts->updates || $opts->local || $opts->radius;
+FixMyStreet::Script::Alerts::send_updates() if $opts->updates || !$any;
+FixMyStreet::Script::Alerts::send_other() if $opts->local || !$any;
+FixMyStreet::Script::Alerts::send_local() if $opts->radius || !$any;
diff --git a/perllib/FixMyStreet/Script/Alerts.pm b/perllib/FixMyStreet/Script/Alerts.pm
diff --git a/t/app/controller/admin/triage.t b/t/app/controller/admin/triage.t
@@ -140,7 +140,7 @@ FixMyStreet::override_config {
         $mech->content_lacks('Report triaged from Potholes to Traffic lights');
 
         $mech->clear_emails_ok;
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
         $mech->email_count_is(0);
     };
 };

diff --git a/t/app/controller/alert_new.t b/t/app/controller/alert_new.t
@@ -453,7 +453,7 @@ subtest "Test normal alert signups and that alerts are sent" => sub {
 
     my $dt = DateTime->now()->add(days => 2);
 
-    my ($report) = $mech->create_problems_for_body(1, 1, 'Testing', {
+    my ($report) = $mech->create_problems_for_body(1, $body->id, 'Testing', {
         dt => $dt,
         user => $user1,
         postcode           => 'EH1 1BB',
@@ -480,7 +480,9 @@ subtest "Test normal alert signups and that alerts are sent" => sub {
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
+        FixMyStreet::Script::Alerts::send_other();
+        FixMyStreet::Script::Alerts::send_local();
     };
     # TODO Note the below will fail if the db has an existing alert that matches
     $mech->email_count_is(3);
@@ -529,7 +531,7 @@ subtest "Test alerts are not sent for no-text updates" => sub {
     my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
     my $dt = DateTime->now()->add(days => 2);
 
-    my ($report, $report2) = $mech->create_problems_for_body(2, 1, 'Testing', {
+    my ($report, $report2) = $mech->create_problems_for_body(2, $body->id, 'Testing', {
         user => $user1,
     });
     my $report_id = $report->id;
@@ -559,7 +561,7 @@ subtest "Test alerts are not sent for no-text updates" => sub {
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     $mech->email_count_is(1);
@@ -577,39 +579,48 @@ subtest "Test no marked as confirmed added to alerts" => sub {
     my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
     my $dt = DateTime->now()->add(days => 2);
 
-    my ($report) = $mech->create_problems_for_body(1, 1, 'Testing', {
+    my ($report1, $report2) = $mech->create_problems_for_body(2, $body->id, 'Testing', {
         user => $user1,
         state => 'investigating',
     });
-    my $report_id = $report->id;
-    ok $report, "created test report - $report_id";
+    my $report_id = $report1->id;
+    ok $report1, "created test reports - $report_id";
 
-    my $alert = FixMyStreet::DB->resultset('Alert')->create( {
+    my $alert1 = FixMyStreet::DB->resultset('Alert')->create( {
         parameter  => $report_id,
         alert_type => 'new_updates',
         user       => $user2,
     } )->confirm;
-    ok $alert, 'created alert for other user';
+    my $alert2 = FixMyStreet::DB->resultset('Alert')->create( {
+        parameter  => $report2->id,
+        alert_type => 'new_updates',
+        user       => $user2,
+    } )->confirm;
 
-    $mech->create_comment_for_problem($report, $user3, 'Staff User', 'this is update', 'f', 'confirmed', 'confirmed', { confirmed  => $dt->clone->add( hours => 9 ) });
-    $mech->create_comment_for_problem($report, $user3, 'Staff User', 'this is another update', 'f', 'confirmed', 'investigating', { confirmed  => $dt->clone->add( hours => 10 ) });
-    $mech->create_comment_for_problem($report, $user3, 'Staff User', 'this is a third update, same state', 'f', 'confirmed', 'investigating', { confirmed  => $dt->clone->add( hours => 11 ) });
+    $mech->create_comment_for_problem($report1, $user3, 'Staff User', 'this is update', 'f', 'confirmed', 'confirmed', { confirmed  => $dt->clone->add( hours => 9 ) });
+    $mech->create_comment_for_problem($report1, $user3, 'Staff User', 'this is another update', 'f', 'confirmed', 'investigating', { confirmed  => $dt->clone->add( hours => 10 ) });
+    $mech->create_comment_for_problem($report1, $user3, 'Staff User', 'this is a third update, same state', 'f', 'confirmed', 'investigating', { confirmed  => $dt->clone->add( hours => 11 ) });
+    $mech->create_comment_for_problem($report2, $user3, 'Staff User', 'this is update', 'f', 'confirmed', 'confirmed', { confirmed  => $dt->clone->add( hours => 9 ) });
 
     $mech->clear_emails_ok;
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
-    $mech->email_count_is(1);
-    my $email = $mech->get_email;
-    my $body = $mech->get_text_body_from_email($email);
+    $mech->email_count_is(2);
+    my @email = $mech->get_email;
+    my $body = $mech->get_text_body_from_email($email[0]);
     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';
     unlike $body, qr/State changed to: Open/s, 'no marked as confirmed text';
     like $body, qr/State changed to: Investigating/, 'mention of state change';
     unlike $body, qr/State changed to: Investigating.*State changed to: Investigating/s, 'only one mention of state change';
+    $body = $mech->get_text_body_from_email($email[1]);
+    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';
+    unlike $body, qr/State changed to: Open/s, 'no marked as confirmed text';
 
     $mech->delete_user($user1);
     $mech->delete_user($user2);
@@ -642,7 +653,7 @@ for my $test (
         my $user3 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $gloucester );
         my $dt = DateTime->now()->add(days => 2);
 
-        my ($report) = $mech->create_problems_for_body(1, 1, 'Testing', {
+        my ($report) = $mech->create_problems_for_body(1, $body->id, 'Testing', {
             user => $user1,
         });
         my $report_id = $report->id;
@@ -661,7 +672,7 @@ for my $test (
         FixMyStreet::override_config {
             MAPIT_URL => 'http://mapit.uk/',
         }, sub {
-            FixMyStreet::Script::Alerts::send();
+            FixMyStreet::Script::Alerts::send_updates();
         };
 
         $mech->email_count_is(1);
@@ -709,7 +720,7 @@ subtest "Test signature template is used from cobrand" => sub {
         MAPIT_URL => 'http://mapit.uk/',
         ALLOWED_COBRANDS => 'fixmystreet',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     my $email = $mech->get_text_body_from_email;
@@ -726,7 +737,7 @@ subtest "Test signature template is used from cobrand" => sub {
         MAPIT_URL => 'http://mapit.uk/',
         ALLOWED_COBRANDS => 'fixmystreet',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     $email = $mech->get_text_body_from_email;
@@ -799,15 +810,17 @@ for my $test (
         FixMyStreet::override_config {
             MAPIT_URL => 'http://mapit.uk/',
         }, sub {
-            FixMyStreet::Script::Alerts::send();
+            FixMyStreet::Script::Alerts::send_other();
+            FixMyStreet::Script::Alerts::send_local();
         };
         $mech->email_count_is(0);
 
         $report->update( { non_public => 0 } );
         FixMyStreet::override_config {
             MAPIT_URL => 'http://mapit.uk/',
         }, sub {
-            FixMyStreet::Script::Alerts::send();
+            FixMyStreet::Script::Alerts::send_other();
+            FixMyStreet::Script::Alerts::send_local();
         };
         my $email = $mech->get_text_body_from_email;
         like $email, qr/Alert\s+test\s+for\s+non\s+public\s+reports/, 'alert contains public report';
@@ -842,7 +855,7 @@ subtest 'check new updates alerts for non public reports only go to report owner
     ok $alert_user1, "alert created";
 
     $mech->clear_emails_ok;
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     $mech->email_count_is(0);
 
     my $alert_user2 = FixMyStreet::DB->resultset('Alert')->create( {
@@ -854,13 +867,13 @@ subtest 'check new updates alerts for non public reports only go to report owner
     } );
     ok $alert_user2, "alert created";
 
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     my $email = $mech->get_text_body_from_email;
     like $email, qr/This is some more update text/, 'alert contains update text';
 
     $mech->clear_emails_ok;
     $report->update( { non_public => 0 } );
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     $email = $mech->get_text_body_from_email;
     like $email, qr/This is some more update text/, 'alert contains update text';
 
@@ -899,7 +912,7 @@ subtest 'check setting include dates in new updates cobrand option' => sub {
 
 
     $mech->clear_emails_ok;
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
 
     my $date_in_alert = Utils::prettify_dt( $update->confirmed );
     my $email = $mech->get_text_body_from_email;
@@ -939,7 +952,7 @@ subtest 'check staff updates can include sanitized HTML' => sub {
     } );
     ok $alert_user1, "alert created";
 
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     my $email = $mech->get_email;
     $mech->clear_emails_ok;
     my $plain = $mech->get_text_body_from_email($email);
@@ -997,15 +1010,15 @@ FixMyStreet::override_config {
     ok $alert_user1, "alert created";
 
     # Check they get a normal email alert by default
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     $mech->email_count_is(1);
     is @{$twilio->texts}, 0;
     $mech->clear_emails_ok;
 
     # Check they don't get any update when set to none
     FixMyStreet::DB->resultset('AlertSent')->delete;
     $user1->update({ extra => { update_notify => 'none' } });
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     $mech->email_count_is(0);
     is @{$twilio->texts}, 0;
 
@@ -1016,7 +1029,7 @@ FixMyStreet::override_config {
     ) {
         FixMyStreet::DB->resultset('AlertSent')->delete;
         $user1->update($_);
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
         $mech->email_count_is(0);
         is @{$twilio->texts}, 1, 'got a text';
         my $text = $twilio->texts->[0]->{Body};

diff --git a/t/app/controller/report_new_update.t b/t/app/controller/report_new_update.t
@@ -45,7 +45,7 @@ subtest "test report creation with initial auto-update" => sub {
     is $comment->external_id, 'auto-internal';
     is $comment->name, 'Glos Council';
 
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
     my $email = $mech->get_email;
 };
 

diff --git a/t/app/model/alert_type.t b/t/app/model/alert_type.t
@@ -133,7 +133,7 @@ for my $test (
         $report->state( $test->{state} );
         $report->update;
 
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
 
         $mech->email_count_is( 2 );
         my @emails = $mech->get_email;
@@ -184,7 +184,7 @@ subtest "correct text for title after URL" => sub {
     FixMyStreet::override_config {
         MAPIT_URL => 'http://mapit.uk/',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_other();
     };
 
     (my $title = $report->title) =~ s/ /\\s+/;
@@ -320,7 +320,7 @@ foreach my $test (
         FixMyStreet::override_config {
             MAPIT_URL => 'http://mapit.uk/',
         }, sub {
-            FixMyStreet::Script::Alerts::send();
+            FixMyStreet::Script::Alerts::send_other();
         };
 
         my $body = $mech->get_text_body_from_email;
@@ -432,7 +432,7 @@ subtest "check alerts from cobrand send main site url for alerts for different c
         BASE_URL => 'https://national.example.org',
         MAPIT_URL => 'http://mapit.uk/',
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_other();
 
         my $body = $mech->get_text_body_from_email;
 
@@ -468,7 +468,7 @@ subtest "check local alerts from cobrand send main site url for alerts for diffe
         }
     )->delete;
 
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_local();
 
     my $body = $mech->get_text_body_from_email;
 
@@ -495,7 +495,7 @@ subtest "correct i18n-ed summary for state of closed" => sub {
     FixMyStreet::override_config {
         ALLOWED_COBRANDS => [ 'fixamingata' ],
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     my $body = $mech->get_text_body_from_email;

diff --git a/t/cobrand/cheshireeast.t b/t/cobrand/cheshireeast.t
@@ -123,7 +123,7 @@ FixMyStreet::override_config {
     subtest 'testing reference numbers shown' => sub {
         $mech->get_ok('/report/' . $report->id);
         $mech->content_contains('Council ref:&nbsp;' . $report->id);
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
         like $mech->get_text_body_from_email, qr/reference number is @{[$report->id]}/;
     };
 

diff --git a/t/cobrand/fixamingata.t b/t/cobrand/fixamingata.t
@@ -80,7 +80,7 @@ my $alert = FixMyStreet::DB->resultset('Alert')->find_or_create({
 FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'fixamingata' ],
 }, sub {
-    FixMyStreet::Script::Alerts::send();
+    FixMyStreet::Script::Alerts::send_updates();
 };
 
 $email = $mech->get_email;

diff --git a/t/cobrand/hackney.t b/t/cobrand/hackney.t
@@ -156,7 +156,7 @@ subtest "sends branded alert emails" => sub {
             govuk_notify => { hackney => { key => 'test-0123456789abcdefghijklmnopqrstuvwxyz-key-goes-here' } },
         },
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     my $id = $p->id;

diff --git a/t/cobrand/isleofwight.t b/t/cobrand/isleofwight.t
@@ -402,7 +402,7 @@ subtest "sends branded alert emails" => sub {
         MAPIT_URL => 'http://mapit.uk/',
         ALLOWED_COBRANDS => ['isleofwight','fixmystreet'],
     }, sub {
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
     };
 
     $mech->email_count_is(1);

diff --git a/t/cobrand/oxfordshire.t b/t/cobrand/oxfordshire.t
@@ -203,7 +203,7 @@ FixMyStreet::override_config {
         $mech->get_ok('/report/' . $problem->id);
         $mech->content_contains('Investigation complete');
 
-        FixMyStreet::Script::Alerts::send();
+        FixMyStreet::Script::Alerts::send_updates();
         $mech->email_count_is(1);
         my $email = $mech->get_email;
         my $body = $mech->get_text_body_from_email($email);