aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--CHANGELOG.md2
-rw-r--r--perllib/FixMyStreet/App/Controller/Admin.pm45
-rw-r--r--perllib/FixMyStreet/TestMech.pm8
-rw-r--r--perllib/Open311/GetServiceRequestUpdates.pm23
-rw-r--r--t/app/controller/admin.t112
-rw-r--r--t/open311/getservicerequestupdates.t30
-rw-r--r--templates/web/base/admin/template_edit.html39
-rw-r--r--templates/web/base/admin/templates.html24
-rw-r--r--templates/web/zurich/admin/templates.html28
9 files changed, 277 insertions, 34 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 67f2da48d..bfe8c04fc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -51,6 +51,8 @@
- More JavaScript-enhanced `<select multiple>` elements #1589
- Council dashboard CSV export now has token based authentication #1911
- Consolidate various admin summary statistics page. #1919.
+ - 'Auto-response' flag on response templates is honoured for fetched
+ Open311 updates. #1924
- UK:
- Use SVG logo, inlined on front page. #1887
- Inline critical CSS on front page.
diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index b485ea2dc..e21b0cc2f 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -1102,19 +1102,52 @@ sub template_edit : Path('templates') : Args(2) {
} } @live_contacts;
$c->stash->{contacts} = \@all_contacts;
- if ($c->req->method eq 'POST') {
+ # bare block to use 'last' if form is invalid.
+ if ($c->req->method eq 'POST') { {
if ($c->get_param('delete_template') && $c->get_param('delete_template') eq _("Delete template")) {
$template->contact_response_templates->delete_all;
$template->delete;
} else {
+ my @live_contact_ids = map { $_->id } @live_contacts;
+ my @new_contact_ids = grep { $c->get_param("contacts[$_]") } @live_contact_ids;
+ my %new_contacts = map { $_ => 1 } @new_contact_ids;
+ for my $contact (@all_contacts) {
+ $contact->{active} = $new_contacts{$contact->{id}};
+ }
+
$template->title( $c->get_param('title') );
$template->text( $c->get_param('text') );
$template->state( $c->get_param('state') );
- $template->auto_response( $c->get_param('auto_response') ? 1 : 0 );
- $template->update_or_insert;
- my @live_contact_ids = map { $_->id } @live_contacts;
- my @new_contact_ids = grep { $c->get_param("contacts[$_]") } @live_contact_ids;
+ $template->auto_response( $c->get_param('auto_response') && $template->state ? 1 : 0 );
+ if ($template->auto_response) {
+ my @check_contact_ids = @new_contact_ids;
+ # If the new template has not specific categories (i.e. it
+ # applies to all categories) then we need to check each of those
+ # category ids for existing auto-response templates.
+ if (!scalar @check_contact_ids) {
+ @check_contact_ids = @live_contact_ids;
+ }
+ my $query = {
+ 'auto_response' => 1,
+ 'contact.id' => [ @check_contact_ids, undef ],
+ 'me.state' => $template->state,
+ };
+ if ($template->in_storage) {
+ $query->{'me.id'} = { '!=', $template->id };
+ }
+ if ($c->stash->{body}->response_templates->search($query, {
+ join => { 'contact_response_templates' => 'contact' },
+ })->count) {
+ $c->stash->{errors} = {
+ auto_response => _("There is already an auto-response template for this category/state.")
+ };
+ }
+ }
+
+ last if $c->stash->{errors};
+
+ $template->update_or_insert;
$template->contact_response_templates->search({
contact_id => { '!=' => \@new_contact_ids },
})->delete;
@@ -1126,7 +1159,7 @@ sub template_edit : Path('templates') : Args(2) {
}
$c->res->redirect( $c->uri_for( 'templates', $c->stash->{body}->id ) );
- }
+ } }
$c->stash->{response_template} = $template;
diff --git a/perllib/FixMyStreet/TestMech.pm b/perllib/FixMyStreet/TestMech.pm
index dbfc94286..ac2ac023d 100644
--- a/perllib/FixMyStreet/TestMech.pm
+++ b/perllib/FixMyStreet/TestMech.pm
@@ -630,6 +630,14 @@ sub delete_defect_type {
$defect_type->delete;
}
+sub delete_response_template {
+ my $mech = shift;
+ my $response_template = shift;
+
+ $response_template->contact_response_templates->delete_all;
+ $response_template->delete;
+}
+
sub create_contact_ok {
my $self = shift;
my %contact_params = (
diff --git a/perllib/Open311/GetServiceRequestUpdates.pm b/perllib/Open311/GetServiceRequestUpdates.pm
index db2a452da..2620b176a 100644
--- a/perllib/Open311/GetServiceRequestUpdates.pm
+++ b/perllib/Open311/GetServiceRequestUpdates.pm
@@ -103,16 +103,17 @@ sub update_comments {
$problem = $self->schema->resultset('Problem')->to_body($body)->search( $criteria );
if (my $p = $problem->first) {
- next unless defined $request->{update_id} && defined $request->{description};
+ next unless defined $request->{update_id};
my $c = $p->comments->search( { external_id => $request->{update_id} } );
if ( !$c->first ) {
+ my $state = $self->map_state( $request->{status} );
my $comment = $self->schema->resultset('Comment')->new(
{
problem => $p,
user => $self->system_user,
external_id => $request->{update_id},
- text => $request->{description},
+ text => $self->comment_text_for_request($request, $p, $state),
mark_fixed => 0,
mark_open => 0,
anonymous => 0,
@@ -138,8 +139,6 @@ sub update_comments {
# do not change the status of the problem as it's
# tricky to determine the right thing to do.
if ( $comment->created > $p->lastupdate ) {
- my $state = $self->map_state( $request->{status} );
-
# don't update state unless it's an allowed state and it's
# actually changing the state of the problem
if ( FixMyStreet::DB::Result::Problem->visible_states()->{$state} && $p->state ne $state &&
@@ -180,6 +179,22 @@ sub update_comments {
return 1;
}
+sub comment_text_for_request {
+ my ($self, $request, $problem, $state) = @_;
+
+ return $request->{description} if $request->{description};
+
+ if (my $template = $problem->response_templates->search({
+ auto_response => 1,
+ 'me.state' => $state
+ })->first) {
+ return $template->text;
+ }
+
+ print STDERR "Couldn't determine update text for $request->{update_id} (report " . $problem->id . ")\n";
+ return "";
+}
+
sub map_state {
my $self = shift;
my $incoming_state = shift;
diff --git a/t/app/controller/admin.t b/t/app/controller/admin.t
index 0be54dbc5..179557976 100644
--- a/t/app/controller/admin.t
+++ b/t/app/controller/admin.t
@@ -1659,6 +1659,108 @@ subtest "response templates are included on page" => sub {
};
};
+subtest "auto-response templates that duplicate a single category can't be added" => sub {
+ $mech->delete_response_template($_) for $oxfordshire->response_templates;
+ my $template = $oxfordshire->response_templates->create({
+ title => "Report fixed - potholes",
+ text => "Thank you for your report. This problem has been fixed.",
+ auto_response => 1,
+ state => 'fixed - council',
+ });
+ $template->contact_response_templates->find_or_create({
+ contact_id => $oxfordshirecontact->id,
+ });
+ is $oxfordshire->response_templates->count, 1, "Initial response template was created";
+
+
+ $mech->log_in_ok( $superuser->email );
+ $mech->get_ok( "/admin/templates/" . $oxfordshire->id . "/new" );
+
+ # This response template has the same category & state as an existing one
+ # so won't be allowed.
+ my $fields = {
+ title => "Report marked fixed - potholes",
+ text => "Thank you for your report. This pothole has been fixed.",
+ auto_response => 'on',
+ state => 'fixed - council',
+ "contacts[".$oxfordshirecontact->id."]" => 1,
+ };
+ $mech->submit_form_ok( { with_fields => $fields } );
+ is $mech->uri->path, '/admin/templates/' . $oxfordshire->id . '/new', 'not redirected';
+ $mech->content_contains( 'Please correct the errors below' );
+ $mech->content_contains( 'There is already an auto-response template for this category/state.' );
+
+ is $oxfordshire->response_templates->count, 1, "Duplicate response template wasn't added";
+};
+
+subtest "auto-response templates that duplicate all categories can't be added" => sub {
+ $mech->delete_response_template($_) for $oxfordshire->response_templates;
+ $oxfordshire->response_templates->create({
+ title => "Report investigating - all cats",
+ text => "Thank you for your report. This problem has been fixed.",
+ auto_response => 1,
+ state => 'fixed - council',
+ });
+ is $oxfordshire->response_templates->count, 1, "Initial response template was created";
+
+
+ $mech->log_in_ok( $superuser->email );
+ $mech->get_ok( "/admin/templates/" . $oxfordshire->id . "/new" );
+
+ # There's already a response template for all categories and this state, so
+ # this new template won't be allowed.
+ my $fields = {
+ title => "Report investigating - single cat",
+ text => "Thank you for your report. This problem has been fixed.",
+ auto_response => 'on',
+ state => 'fixed - council',
+ "contacts[".$oxfordshirecontact->id."]" => 1,
+ };
+ $mech->submit_form_ok( { with_fields => $fields } );
+ is $mech->uri->path, '/admin/templates/' . $oxfordshire->id . '/new', 'not redirected';
+ $mech->content_contains( 'Please correct the errors below' );
+ $mech->content_contains( 'There is already an auto-response template for this category/state.' );
+
+
+ is $oxfordshire->response_templates->count, 1, "Duplicate response template wasn't added";
+};
+
+subtest "all-category auto-response templates that duplicate a single category can't be added" => sub {
+ $mech->delete_response_template($_) for $oxfordshire->response_templates;
+ my $template = $oxfordshire->response_templates->create({
+ title => "Report fixed - potholes",
+ text => "Thank you for your report. This problem has been fixed.",
+ auto_response => 1,
+ state => 'fixed - council',
+ });
+ $template->contact_response_templates->find_or_create({
+ contact_id => $oxfordshirecontact->id,
+ });
+ is $oxfordshire->response_templates->count, 1, "Initial response template was created";
+
+
+ $mech->log_in_ok( $superuser->email );
+ $mech->get_ok( "/admin/templates/" . $oxfordshire->id . "/new" );
+
+ # This response template is implicitly for all categories, but there's
+ # already a template for a specific category in this state, so it won't be
+ # allowed.
+ my $fields = {
+ title => "Report marked fixed - all cats",
+ text => "Thank you for your report. This problem has been fixed.",
+ auto_response => 'on',
+ state => 'fixed - council',
+ };
+ $mech->submit_form_ok( { with_fields => $fields } );
+ is $mech->uri->path, '/admin/templates/' . $oxfordshire->id . '/new', 'not redirected';
+ $mech->content_contains( 'Please correct the errors below' );
+ $mech->content_contains( 'There is already an auto-response template for this category/state.' );
+
+ is $oxfordshire->response_templates->count, 1, "Duplicate response template wasn't added";
+};
+
+
+
$mech->log_in_ok( $superuser->email );
subtest "response priorities can be added" => sub {
@@ -1674,8 +1776,8 @@ subtest "response priorities can be added" => sub {
};
$mech->submit_form_ok( { with_fields => $fields } );
- is $oxfordshire->response_priorities->count, 1, "Response template was added to body";
- is $oxfordshirecontact->response_priorities->count, 1, "Response template was added to contact";
+ is $oxfordshire->response_priorities->count, 1, "Response priority was added to body";
+ is $oxfordshirecontact->response_priorities->count, 1, "Response priority was added to contact";
};
subtest "response priorities can set to default" => sub {
@@ -1693,7 +1795,7 @@ subtest "response priorities can set to default" => sub {
$mech->submit_form_ok( { with_fields => $fields } );
is $oxfordshire->response_priorities->count, 1, "Still one response priority";
- is $oxfordshirecontact->response_priorities->count, 1, "Still one response template";
+ is $oxfordshirecontact->response_priorities->count, 1, "Still one response priority";
ok $oxfordshire->response_priorities->first->is_default, "Response priority set to default";
};
@@ -1710,8 +1812,8 @@ subtest "response priorities are limited by body" => sub {
name => "Bromley Cat 0",
} );
- is $bromley->response_priorities->count, 1, "Response template was added to Bromley";
- is $oxfordshire->response_priorities->count, 1, "Response template wasn't added to Oxfordshire";
+ is $bromley->response_priorities->count, 1, "Response priority was added to Bromley";
+ is $oxfordshire->response_priorities->count, 1, "Response priority wasn't added to Oxfordshire";
$mech->get_ok( "/admin/responsepriorities/" . $oxfordshire->id );
$mech->content_lacks( $bromleypriority->name );
diff --git a/t/open311/getservicerequestupdates.t b/t/open311/getservicerequestupdates.t
index 9a8db4374..da427e505 100644
--- a/t/open311/getservicerequestupdates.t
+++ b/t/open311/getservicerequestupdates.t
@@ -20,11 +20,18 @@ my $user = FixMyStreet::DB->resultset('User')->find_or_create(
my %bodies = (
2237 => FixMyStreet::DB->resultset("Body")->create({ name => 'Oxfordshire' }),
- 2482 => FixMyStreet::DB->resultset("Body")->new({ id => 2482 }),
+ 2482 => FixMyStreet::DB->resultset("Body")->create({ name=> 'Bromley', id => 2482 }),
2651 => FixMyStreet::DB->resultset("Body")->new({ id => 2651 }),
);
$bodies{2237}->body_areas->create({ area_id => 2237 });
+my $response_template = $bodies{2482}->response_templates->create({
+ title => "investigating template",
+ text => "We are investigating this report.",
+ auto_response => 1,
+ state => "investigating"
+});
+
my $requests_xml = qq{<?xml version="1.0" encoding="utf-8"?>
<service_requests_updates>
<request_update>
@@ -157,6 +164,10 @@ for my $test (
end_state => 'confirmed',
},
+ # NB because we have an auto-response ResponseTemplate set up for
+ # the 'investigating' state, this test is also testing that the
+ # response template isn't used if the update XML has a non-empty
+ # <description>.
{
desc => 'investigating status changes problem status',
description => 'This is a note',
@@ -334,6 +345,18 @@ for my $test (
end_state => 'fixed - council',
},
{
+ desc => 'empty description triggers auto-response template',
+ description => 'We are investigating this report.',
+ xml_description => '',
+ external_id => 638344,
+ start_state => 'fixed - council',
+ comment_status => 'INVESTIGATING',
+ mark_fixed => 0,
+ mark_open => 0,
+ problem_state => 'investigating',
+ end_state => 'investigating',
+ },
+ {
desc => 'open status does not re-open hidden report',
description => 'This is a note',
external_id => 638344,
@@ -346,7 +369,7 @@ for my $test (
},
) {
subtest $test->{desc} => sub {
- my $local_requests_xml = setup_xml($problem->external_id, $problem->id, $test->{comment_status});
+ my $local_requests_xml = setup_xml($problem->external_id, $problem->id, $test->{comment_status}, $test->{xml_description});
my $o = Open311->new( jurisdiction => 'mysociety', endpoint => 'http://example.com', test_mode => 1, test_get_returns => { 'servicerequestupdates.xml' => $local_requests_xml } );
$problem->lastupdate( DateTime->now()->subtract( days => 1 ) );
@@ -762,13 +785,14 @@ foreach my $test ( {
done_testing();
sub setup_xml {
- my ($id, $id_ext, $status) = @_;
+ my ($id, $id_ext, $status, $description) = @_;
my $xml = $requests_xml;
my $updated_datetime = sprintf( '<updated_datetime>%s</updated_datetime>', $dt );
$xml =~ s/UPDATED_DATETIME/$updated_datetime/;
$xml =~ s#<service_request_id>\d+</service_request_id>#<service_request_id>$id</service_request_id>#;
$xml =~ s#<service_request_id_ext>\d+</service_request_id_ext>#<service_request_id_ext>$id_ext</service_request_id_ext>#;
$xml =~ s#<status>\w+</status>#<status>$status</status># if $status;
+ $xml =~ s#<description>.+</description>#<description>$description</description># if defined $description;
return $xml;
diff --git a/templates/web/base/admin/template_edit.html b/templates/web/base/admin/template_edit.html
index 76de70dcc..3e436dbf9 100644
--- a/templates/web/base/admin/template_edit.html
+++ b/templates/web/base/admin/template_edit.html
@@ -9,20 +9,30 @@
accept-charset="utf-8"
class="validate">
+ [% IF errors %]
+ <p class="error">[% loc('Please correct the errors below') %]</p>
+ [% END %]
+
+
+ <div class="admin-hint">
+ <p>
+ [% loc('This is a <strong>private</strong> name for this template so you can identify it when updating reports or editing in the admin.') %]
+ </p>
+ </div>
<p>
<strong>[% loc('Title:') %] </strong>
<input type="text" name="title" class="required form-control" size="30" value="[% rt.title| html %]">
</p>
+
+ <div class="admin-hint">
+ <p>
+ [% loc('This is the <strong>public</strong> text that will be shown on the site.') %]
+ </p>
+ </div>
<p>
<strong>[% loc('Text:') %] </strong>
<textarea class="form-control" name="text" class="required">[% rt.text |html %]</textarea>
</p>
- <p>
- <label>
- <strong>[% loc('Auto-response:') %]</strong>
- <input type="checkbox" name="auto_response" [% 'checked' IF rt.auto_response %] />
- </label>
- </p>
<div class="admin-hint">
<p>
@@ -41,13 +51,28 @@
[% INCLUDE 'admin/state_groups_select.html' current_state=rt.state include_empty=1 %]
</p>
+ [% IF errors.auto_response %]
+ <div class="form-error">[% errors.auto_response %]</div>
+ [% END %]
+ <div class="admin-hint">
+ <p>
+ [% loc('If ticked, this template will be used for Open311 updates that put problems in this state.') %]
+ </p>
+ </div>
+ <p>
+ <label>
+ <strong>[% loc('Auto-response:') %]</strong>
+ <input type="checkbox" name="auto_response" [% 'checked' IF rt.auto_response %] />
+ </label>
+ </p>
+
<p>
<input type="hidden" name="token" value="[% csrf_token %]" >
<input type="submit" class="btn" name="Edit templates" value="[% rt.id ? loc('Save changes') : loc('Create template') %]" >
</p>
[% IF rt.id %]
<p>
- <input class="delete btn-danger" type="submit" name="delete_template" value="[% loc('Delete template') %]">
+ <input class="delete btn-danger" type="submit" name="delete_template" value="[% loc('Delete template') %]" data-confirm="[% loc('Are you sure?') %]">
</p>
[% END %]
</form>
diff --git a/templates/web/base/admin/templates.html b/templates/web/base/admin/templates.html
index f9dda7a4c..444f2734d 100644
--- a/templates/web/base/admin/templates.html
+++ b/templates/web/base/admin/templates.html
@@ -1,24 +1,30 @@
[% INCLUDE 'admin/header.html' title=tprintf(loc('Response Templates for %s'), body.name) -%]
-[% IF c.cobrand.moniker == 'zurich' %]
- <h2> [% tprintf(loc('Response Templates for %s'), body.name) %] </h2>
-[% END %]
-
<table>
<thead>
<tr>
<th> [% loc('Title') %] </th>
- <th> [% loc('Text') %] </th>
- <th> [% loc('Created') %] </th>
+ <th> [% loc('Categories') %] </th>
+ <th> [% loc('State') %] </th>
+ <th> [% loc('Auto Response') %] </th>
<th> &nbsp; </th>
</tr>
</thead>
<tbody>
[% FOR t IN response_templates %]
<tr>
- <td> [% t.title %] </td>
- <td> [% t.text %] </td>
- <td> [% t.created %] </td>
+ <td> [% t.title | html %] </td>
+ <td>
+ [% UNLESS t.contacts.size %]
+ <em>[% loc('All categories') %]</em>
+ [% ELSE %]
+ [% FOR contact IN t.contacts %]
+ [% contact.category_display %][% ',' UNLESS loop.last %]
+ [% END %]
+ [% END %]
+ </td>
+ <td> [% t.state | html %] </td>
+ <td> [% IF t.auto_response %]X[% END %] </td>
<td> <a href="[% c.uri_for('templates', body.id, t.id) %]" class="btn">[% loc('Edit') %]</a> </td>
</tr>
[% END %]
diff --git a/templates/web/zurich/admin/templates.html b/templates/web/zurich/admin/templates.html
new file mode 100644
index 000000000..2db9e2e34
--- /dev/null
+++ b/templates/web/zurich/admin/templates.html
@@ -0,0 +1,28 @@
+[% INCLUDE 'admin/header.html' title=tprintf(loc('Response Templates for %s'), body.name) -%]
+
+<h2> [% tprintf(loc('Response Templates for %s'), body.name) %] </h2>
+
+<table>
+ <thead>
+ <tr>
+ <th> [% loc('Title') %] </th>
+ <th> [% loc('Text') %] </th>
+ <th> [% loc('Created') %] </th>
+ <th> &nbsp; </th>
+ </tr>
+ </thead>
+ <tbody>
+[% FOR t IN response_templates %]
+ <tr>
+ <td> [% t.title %] </td>
+ <td> [% t.text %] </td>
+ <td> [% t.created %] </td>
+ <td> <a href="[% c.uri_for('templates', body.id, t.id) %]" class="btn">[% loc('Edit') %]</a> </td>
+ </tr>
+[% END %]
+ </tbody>
+</table>
+
+<a href="[% c.uri_for('templates', body.id, 'new') %]" class="btn">[% loc('New template') %]</a>
+
+[% INCLUDE 'admin/footer.html' %]