Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor alerts script #3615

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Refactor alerts script #3615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4cc170
Factor alert script to slightly smaller functions.
dracos Oct 12, 2021
a6e7516
Fix more over-display of "State changed" in alerts
dracos Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 14 additions & 2 deletions bin/send-alerts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ BEGIN {
require "$d/../setenv.pl";
}

use Getopt::Long::Descriptive;
use CronFns;

use FixMyStreet;
Expand All @@ -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;
399 changes: 215 additions & 184 deletions perllib/FixMyStreet/Script/Alerts.pm
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion t/app/controller/admin/triage.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
};
Expand Down
73 changes: 43 additions & 30 deletions t/app/controller/alert_new.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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( {
Expand All @@ -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';

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;

Expand All @@ -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};
Expand Down
2 changes: 1 addition & 1 deletion t/app/controller/report_new_update.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
12 changes: 6 additions & 6 deletions t/app/model/alert_type.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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+/;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion t/cobrand/cheshireeast.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ FixMyStreet::override_config {
subtest 'testing reference numbers shown' => sub {
$mech->get_ok('/report/' . $report->id);
$mech->content_contains('Council ref: ' . $report->id);
FixMyStreet::Script::Alerts::send();
FixMyStreet::Script::Alerts::send_updates();
like $mech->get_text_body_from_email, qr/reference number is @{[$report->id]}/;
};

Expand Down
2 changes: 1 addition & 1 deletion t/cobrand/fixamingata.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion t/cobrand/hackney.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion t/cobrand/isleofwight.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion t/cobrand/oxfordshire.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down