Enable HTML in update alert emails.

8 files changed, 113 insertions, 19 deletions

diff --git a/docs/_includes/admin-tasks-content.md b/docs/_includes/admin-tasks-content.md
index aa1225cfc..0fdc259e9 100644
--- a/docs/_includes/admin-tasks-content.md
+++ b/docs/_includes/admin-tasks-content.md
@@ -829,7 +829,11 @@ above, ‘Creating a template’. Additionally you can delete the template from
 #### HTML content in templates
 
 HTML tags are permitted in response templates, which makes it possible to include
-images or rich text formatting in the updates which are added to reports.
+hyperlinks or rich text formatting in the updates which are added to reports.
+
+Be aware that response templates are emailed to users as well as being shown on
+the site, so it's best to keep any HTML formatting quite light-touch due to the
+quirks of email clients' rendering of HTML message.
 
 Refer to the section ["HTML Content in notices"](#html-content-in-notices) above for details of
 what tags and attributes are allowed.
diff --git a/perllib/FixMyStreet/App/View/Web.pm b/perllib/FixMyStreet/App/View/Web.pm
index 5e38fc797..41444fdd4 100644
--- a/perllib/FixMyStreet/App/View/Web.pm
+++ b/perllib/FixMyStreet/App/View/Web.pm
@@ -126,29 +126,34 @@ sub staff_html_markup_factory {
 
     return sub {
         my $text = shift;
-        unless ($staff) {
-            return FixMyStreet::Template::html_paragraph(add_links($text));
-        }
+        return _staff_html_markup($text, $staff);
+    }
+}
 
-        $text = FixMyStreet::Template::sanitize($text);
+sub _staff_html_markup {
+    my ( $text, $staff ) = @_;
+    unless ($staff) {
+        return FixMyStreet::Template::html_paragraph(add_links($text));
+    }
 
-        # Apply Markdown-style italics
-        $text =~ s{\*(\S.*?\S)\*}{<i>$1</i>};
+    $text = FixMyStreet::Template::sanitize($text);
 
-        # Mark safe so add_links doesn't escape everything.
-        $text = FixMyStreet::Template::SafeString->new($text);
+    # Apply Markdown-style italics
+    $text =~ s{\*(\S.*?\S)\*}{<i>$1</i>};
 
-        $text = add_links($text);
+    # Mark safe so add_links doesn't escape everything.
+    $text = FixMyStreet::Template::SafeString->new($text);
 
-        # If the update already has block-level elements then don't wrap
-        # individual lines in <p> elements, as we assume the user knows what
-        # they're doing.
-        unless ($text =~ /<(p|ol|ul)>/) {
-            $text = FixMyStreet::Template::html_paragraph($text);
-        }
+    $text = add_links($text);
 
-        return $text;
+    # If the update already has block-level elements then don't wrap
+    # individual lines in <p> elements, as we assume the user knows what
+    # they're doing.
+    unless ($text =~ /<(p|ol|ul)>/) {
+        $text = FixMyStreet::Template::html_paragraph($text);
     }
+
+    return $text;
 }
 
 =head2 escape_js
diff --git a/perllib/FixMyStreet/Script/Alerts.pm b/perllib/FixMyStreet/Script/Alerts.pm
index 03373a8cc..fa90ede48 100644
--- a/perllib/FixMyStreet/Script/Alerts.pm
+++ b/perllib/FixMyStreet/Script/Alerts.pm
@@ -41,6 +41,7 @@ sub send() {
                    $item_table.photo as item_photo,
                    $item_table.problem_state as item_problem_state,
                    $item_table.cobrand as item_cobrand,
+                   $item_table.extra as item_extra,
                    $head_table.*
             from alert, $item_table, $head_table
                 where alert.parameter::integer = $head_table.id
diff --git a/perllib/FixMyStreet/Template.pm b/perllib/FixMyStreet/Template.pm
index ac9e743ff..7d6798415 100644
--- a/perllib/FixMyStreet/Template.pm
+++ b/perllib/FixMyStreet/Template.pm
@@ -11,6 +11,9 @@ use FixMyStreet::Template::SafeString;
 use FixMyStreet::Template::Context;
 use FixMyStreet::Template::Stash;
 
+use RABX;
+use IO::String;
+
 my %FILTERS;
 my %SUBS;
 
@@ -157,4 +160,24 @@ sub sanitize {
     return $text;
 }
 
+
+=head2 email_sanitize_html
+
+Intended for use in the _email_comment_list.html template to allow HTML
+in updates from staff/superusers.
+
+=cut
+
+sub email_sanitize_html : Fn('email_sanitize_html') {
+    my $update = shift;
+
+    my $text = $update->{item_text};
+    my $extra = $update->{item_extra};
+    $extra = $extra ? RABX::wire_rd(new IO::String($extra)) : {};
+
+    my $staff = $extra->{is_superuser} || $extra->{is_body_user};
+
+    return FixMyStreet::App::View::Web::_staff_html_markup($text, $staff);
+}
+
 1;
diff --git a/perllib/FixMyStreet/TestMech.pm b/perllib/FixMyStreet/TestMech.pm
index 277eca2b1..f6854fc98 100644
--- a/perllib/FixMyStreet/TestMech.pm
+++ b/perllib/FixMyStreet/TestMech.pm
@@ -276,6 +276,24 @@ sub get_text_body_from_email {
     return $body;
 }
 
+sub get_html_body_from_email {
+    my ($mech, $email, $obj) = @_;
+    unless ($email) {
+        $email = $mech->get_email;
+        $mech->clear_emails_ok;
+    }
+
+    my $body;
+    $email->walk_parts(sub {
+        my $part = shift;
+        return if $part->subparts;
+        return if $part->content_type !~ m{text/html};
+        $body = $obj ? $part : $part->body_str;
+        ok $body, "Found HTML body";
+    });
+    return $body;
+}
+
 sub get_link_from_email {
     my ($mech, $email, $multiple, $mismatch) = @_;
     unless ($email) {
diff --git a/t/app/controller/alert_new.t b/t/app/controller/alert_new.t
index d968b56b1..16455c7c4 100644
--- a/t/app/controller/alert_new.t
+++ b/t/app/controller/alert_new.t
@@ -866,4 +866,47 @@ subtest 'check setting include dates in new updates cobrand option' => sub {
     $include_date_in_alert_override->restore();
 };
 
+subtest 'check staff updates can include sanitized HTML' => sub {
+    my $user1 = $mech->create_user_ok('reporter@example.com', name => 'Reporter User');
+    my $user2 = $mech->create_user_ok('staff@example.com', name => 'Staff User', from_body => $body);
+    my $user3 = $mech->create_user_ok('updater@example.com', name => 'Another User');
+
+    my $dt = DateTime->now->add( minutes => -30 );
+    my $r_dt = $dt->clone->add( minutes => 20 );
+
+    my ($report) = $mech->create_problems_for_body(1, $body->id, 'Testing', {
+        user => $user1,
+    });
+
+    my $update1 = $mech->create_comment_for_problem($report, $user2, 'Staff User', 'This is some update text with <strong>HTML</strong> and *italics*. <script>not allowed</script>', 't', 'confirmed', undef, { confirmed  => $r_dt->clone->add( minutes => 8 ) });
+    $update1->set_extra_metadata(is_body_user => $user2->from_body->id);
+    $update1->update;
+
+    $mech->create_comment_for_problem($report, $user3, 'Updater User', 'Public users <i>cannot</i> use HTML. <script>not allowed</script>', 't', 'confirmed', undef, { confirmed  => $r_dt->clone->add( minutes => 9 ) });
+
+    my $alert_user1 = FixMyStreet::DB->resultset('Alert')->create( {
+            user       => $user1,
+            alert_type => 'new_updates',
+            parameter  => $report->id,
+            confirmed  => 1,
+            whensubscribed => $dt,
+    } );
+    ok $alert_user1, "alert created";
+
+    FixMyStreet::DB->resultset('AlertType')->email_alerts();
+    my $email = $mech->get_email;
+    my $plain = $mech->get_text_body_from_email($email);
+    like $plain, qr/This is some update text with <strong>HTML<\/strong> and \*italics\*\./, 'plain text part contains exactly what was entered';
+    like $plain, qr/Public users <i>cannot<\/i> use HTML\./, 'plain text part contains exactly what was entered';
+
+    my $html = $mech->get_html_body_from_email($email);
+    like $html, qr{This is some update text with <strong>HTML</strong> and <i>italics</i>\.}, 'HTML part contains HTML tags';
+    unlike $html, qr/<script>/, 'HTML part contains no script tags';
+
+    $mech->delete_user( $user1 );
+    $mech->delete_user( $user2 );
+    $mech->delete_user( $user3 );
+};
+
+
 done_testing();
diff --git a/templates/email/default/_email_comment_list.html b/templates/email/default/_email_comment_list.html
index 346efadfb..fe6cd5c4c 100644
--- a/templates/email/default/_email_comment_list.html
+++ b/templates/email/default/_email_comment_list.html
@@ -5,7 +5,7 @@
         <img style="[% list_item_photo_style %]" src="[% inline_image(update.get_first_image_fp) %]" alt="">
       </a>
     [%~ END %]
-      [% update.item_text | html_para | replace('<p>', '<p style="' _ list_item_p_style _ '">') %]
+      [% email_sanitize_html(update) | html_para | replace('<p>', '<p style="' _ list_item_p_style _ '">') %]
       <p style="[% list_item_date_style %]">
         [%~ update.item_name | html IF update.item_name AND NOT update.item_anonymous -%]
         [% '(' _ cobrand.prettify_dt(update.confirmed) _ ') ' IF cobrand.include_time_in_update_alerts -%]
diff --git a/templates/email/fixamingata/_email_comment_list.html b/templates/email/fixamingata/_email_comment_list.html
index 346efadfb..fe6cd5c4c 100644
--- a/templates/email/fixamingata/_email_comment_list.html
+++ b/templates/email/fixamingata/_email_comment_list.html
@@ -5,7 +5,7 @@
         <img style="[% list_item_photo_style %]" src="[% inline_image(update.get_first_image_fp) %]" alt="">
       </a>
     [%~ END %]
-      [% update.item_text | html_para | replace('<p>', '<p style="' _ list_item_p_style _ '">') %]
+      [% email_sanitize_html(update) | html_para | replace('<p>', '<p style="' _ list_item_p_style _ '">') %]
       <p style="[% list_item_date_style %]">
         [%~ update.item_name | html IF update.item_name AND NOT update.item_anonymous -%]
         [% '(' _ cobrand.prettify_dt(update.confirmed) _ ') ' IF cobrand.include_time_in_update_alerts -%]