Merge branch '1072-reason-for-reporting'

10 files changed, 221 insertions, 127 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2ba77e48c..cdd389ca9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@
         - Don't require two taps on reports list on touchscreens. #2294
         - Allow moderation to work without JavaScript. #2339
         - More prominent display of "state" on report page #2350
+        - Improved report/update display on contact form. #2351
     - Admin improvements:
         - Allow moderation to potentially change category. #2320
         - Add Mark/View private reports permission #2306
diff --git a/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm b/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
index 1b437b2c5..055e481d9 100644
--- a/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
@@ -358,5 +358,24 @@ sub lookup_site_code_config { {
     }
 } }
 
+sub extra_contact_validation {
+    my $self = shift;
+    my $c = shift;
+
+    # Don't care about dest unless reporting abuse
+    return () unless $c->stash->{problem};
+
+    my %errors;
+
+    $c->stash->{dest} = $c->get_param('dest');
+
+    if (!$c->get_param('dest')) {
+        $errors{dest} = "Please enter a topic of your message";
+    } elsif ( $c->get_param('dest') eq 'council' || $c->get_param('dest') eq 'update' ) {
+        $errors{not_for_us} = 1;
+    }
+
+    return %errors;
+}
 
 1;
diff --git a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm
index b2fc5ba83..fb454f495 100644
--- a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm
+++ b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm
@@ -52,9 +52,6 @@ sub extra_contact_validation {
     my $self = shift;
     my $c = shift;
 
-    # Don't care about dest if reporting abuse
-    return () if $c->stash->{problem};
-
     my %errors;
 
     $c->stash->{dest} = $c->get_param('dest');
diff --git a/t/app/controller/contact.t b/t/app/controller/contact.t
index 254522b92..fe67e89ec 100644
--- a/t/app/controller/contact.t
+++ b/t/app/controller/contact.t
@@ -279,25 +279,16 @@ for my $test (
     };
 }
 
-for my $test (
-    {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-        },
-    },
-    {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            id      => $problem_main->id,
-        },
-    },
+my %common = (
+    em => 'test@example.com',
+    name => 'A name',
+    subject => 'A subject',
+    message => 'A message',
+);
 
+for my $test (
+    { fields => \%common },
+    { fields => { %common, id => $problem_main->id } },
   )
 {
     subtest 'check email sent correctly' => sub {
@@ -336,39 +327,22 @@ for my $test (
 
 for my $test (
     {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => undef,
-        },
+        fields => { %common, dest => undef },
         page_errors =>
           [ 'There were problems with your report. Please see below.',
             'Please enter who your message is for',
+            'You can only contact the team behind FixMyStreet using our contact form', # The JS-hidden one
         ]
     },
     {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => 'council',
-        },
+        fields => { %common, dest => 'council' },
         page_errors =>
           [ 'There were problems with your report. Please see below.',
             'You can only contact the team behind FixMyStreet using our contact form',
         ]
     },
     {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => 'update',
-        },
+        fields => { %common, dest => 'update' },
         page_errors =>
           [ 'There were problems with your report. Please see below.',
             'You can only contact the team behind FixMyStreet using our contact form',
@@ -391,44 +365,21 @@ for my $test (
             $test->{fields}->{'extra.phone'} = '';
             is_deeply $mech->visible_form_values, $test->{fields}, 'form values';
 
+            # Ugh, but checking div not hidden; text always shown and hidden with CSS
             if ( $test->{fields}->{dest} and $test->{fields}->{dest} eq 'update' ) {
-                $mech->content_contains( 'www.writetothem.com', 'includes link to WTT if trying to update report' );
+                $mech->content_contains('<div class="form-error__box form-error--update">');
             } elsif ( $test->{fields}->{dest} and $test->{fields}->{dest} eq 'council' ) {
-                $mech->content_lacks( 'www.writetothem.com', 'does not include link to WTT if trying to contact council' );
-                $mech->content_contains( 'should find contact details', 'mentions checking council website for contact details' );
+                # Ugh, but checking div not hidden
+                $mech->content_contains('<div class="form-error__box form-error--council">');
             }
         }
     };
 }
 
 for my $test (
-    {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => 'help',
-        },
-    },
-    {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => 'feedback',
-        },
-    },
-    {
-        fields => {
-            em      => 'test@example.com',
-            name    => 'A name',
-            subject => 'A subject',
-            message => 'A message',
-            dest    => 'from_council',
-        },
-    },
+    { fields => { %common, dest => 'help' } },
+    { fields => { %common, dest => 'feedback' } },
+    { fields => { %common, dest => 'from_council' } },
   )
 {
     subtest 'check email sent correctly with dest field set to us' => sub {
@@ -446,11 +397,53 @@ for my $test (
 
 for my $test (
     {
+        fields => { %common, dest => undef },
+        page_errors =>
+          [ 'There were problems with your report. Please see below.',
+            'Please enter a topic of your message',
+            'You can only use this form to report inappropriate content', # The JS-hidden one
+        ]
+    },
+    {
+        fields => { %common, dest => 'council' },
+        page_errors =>
+          [ 'There were problems with your report. Please see below.',
+            'You can only use this form to report inappropriate content',
+        ]
+    },
+    {
+        fields => { %common, dest => 'update' },
+        page_errors =>
+          [ 'There were problems with your report. Please see below.',
+            'You can only use this form to report inappropriate content',
+        ]
+    },
+  )
+{
+    subtest 'check Bucks submit page incorrect destination handling' => sub {
+        FixMyStreet::override_config {
+            ALLOWED_COBRANDS => [ 'buckinghamshire' ],
+        }, sub {
+            $mech->get_ok( '/contact?id=' . $problem_main->id, 'can visit for abuse report' );
+            $mech->submit_form_ok( { with_fields => $test->{fields} } );
+            is_deeply $mech->page_errors, $test->{page_errors}, 'page errors';
+
+            $test->{fields}->{'extra.phone'} = '';
+            is_deeply $mech->visible_form_values, $test->{fields}, 'form values';
+
+            if ( $test->{fields}->{dest} and $test->{fields}->{dest} eq 'update' ) {
+                $mech->content_contains('please leave an update');
+            } elsif ( $test->{fields}->{dest} and $test->{fields}->{dest} eq 'council' ) {
+                $mech->content_contains('should find other contact details');
+            }
+        }
+    };
+}
+
+for my $test (
+    {
         fields => {
-            em          => 'test@example.com',
-            name        => 'A name',
-            subject     => 'A subject',
-            message     => 'A message',
+            %common,
             dest        => 'from_council',
             success_url => '/faq',
         },
@@ -458,10 +451,7 @@ for my $test (
     },
     {
         fields => {
-            em          => 'test@example.com',
-            name        => 'A name',
-            subject     => 'A subject',
-            message     => 'A message',
+            %common,
             dest        => 'from_council',
             success_url => 'http://www.example.com',
         },
diff --git a/templates/web/base/contact/index.html b/templates/web/base/contact/index.html
index 326c26ce8..f71c36fb1 100644
--- a/templates/web/base/contact/index.html
+++ b/templates/web/base/contact/index.html
@@ -1,3 +1,6 @@
+[% extra_js = [
+    version('/js/contact.js')
+] -%]
 [% INCLUDE 'header.html',
     title = loc('Contact Us')
     robots = 'noindex,nofollow'
@@ -23,16 +26,16 @@
             </p>
 
             <blockquote>
-                <p>
+                <cite>
                     [% IF update.anonymous %]
                         [% tprintf( loc('Update below added anonymously at %s'), prettify_dt( update.confirmed ) ) %]
                     [% ELSE %]
                         [% tprintf( loc('Update below added by %s at %s'), update.name, prettify_dt( update.confirmed ) ) | html %]
                     [% END %]
-                </p>
+                </cite>
 
                 <p>
-                    [% update.text | html %]
+                    [%~ update.text | html ~%]
                 </p>
 
             </blockquote>
@@ -50,16 +53,16 @@
             <blockquote>
                 <h2>[% problem.title_safe | html %]</h2>
 
-                <p>
+                <cite>
                     [% IF problem.anonymous %]
                         [% tprintf( loc('Reported anonymously at %s'), prettify_dt( problem.confirmed ) ) %]
                     [% ELSE %]
                         [% tprintf( loc('Reported by %s at %s'), problem.user.name, prettify_dt( problem.confirmed ) ) | html %]
                     [% END %]
-                </p>
+                </cite>
 
                 <p>
-                    [% problem.detail | html %]
+                    [%~ problem.detail | html ~%]
                 </p>
 
             </blockquote>
@@ -71,6 +74,8 @@
 
         [% END %]
 
+        [% INCLUDE 'contact/who.html' %]
+
         <label for="form_name">[% loc('Your name') %]</label>
         [% IF field_errors.name %]
            <div class="form-error">[% field_errors.name %]</div>
@@ -95,8 +100,6 @@
         [% END %]
         <input type="text" class="form-control required" name="subject" id="form_subject" value="[% subject | html %]" size="30">
 
-        [% INCLUDE 'contact/who.html' %]
-
         <label for="form_message">[% loc('Message') %]</label>
         [% IF field_errors.message %]
            <div class="form-error">[% field_errors.message %]</div>
diff --git a/templates/web/bathnes/contact/index.html b/templates/web/bathnes/contact/index.html
index d9947cbec..c6bca0350 100644
--- a/templates/web/bathnes/contact/index.html
+++ b/templates/web/bathnes/contact/index.html
@@ -25,16 +25,16 @@
             </p>
 
             <blockquote>
-                <p>
+                <cite>
                     [% IF update.anonymous %]
                         [% tprintf( loc('Update below added anonymously at %s'), prettify_dt( update.confirmed ) ) %]
                     [% ELSE %]
                         [% tprintf( loc('Update below added by %s at %s'), update.name, prettify_dt( update.confirmed ) ) | html %]
                     [% END %]
-                </p>
+                </cite>
 
                 <p>
-                    [% update.text | html %]
+                    [%~ update.text | html ~%]
                 </p>
 
             </blockquote>
@@ -54,16 +54,16 @@
             <blockquote>
                 <h2>[% problem.title_safe | html %]</h2>
 
-                <p>
+                <cite>
                     [% IF problem.anonymous %]
                         [% tprintf( loc('Reported anonymously at %s'), prettify_dt( problem.confirmed ) ) %]
                     [% ELSE %]
                         [% tprintf( loc('Reported by %s at %s'), problem.user.name, prettify_dt( problem.confirmed ) ) | html %]
                     [% END %]
-                </p>
+                </cite>
 
                 <p>
-                    [% problem.detail | html %]
+                    [%~ problem.detail | html ~%]
                 </p>
 
             </blockquote>
diff --git a/templates/web/buckinghamshire/contact/who.html b/templates/web/buckinghamshire/contact/who.html
new file mode 100644
index 000000000..e3b045521
--- /dev/null
+++ b/templates/web/buckinghamshire/contact/who.html
@@ -0,0 +1,48 @@
+[% IF problem %]
+<h4>Topic:</h4>
+
+[% IF field_errors.dest %]
+<div class="form-error">[% field_errors.dest %]</div>
+[% END %]
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_rules" type="radio" value="rules" class="required"[% IF dest AND dest == 'rules' %] checked[% END %]>
+    <label class="inline" for="dest_rules">This [% update ? 'update' : 'report' %] is abusive, contains personal information, or similar</label>
+</div>
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_update" type="radio" value="update" class="required"[% IF dest AND dest == 'update' %] checked[% END %]>
+    <label class="inline" for="dest_update">I wish to report it has not been fixed yet</label>
+</div>
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_council" type="radio" value="council" class="required"[% IF dest AND dest == 'council' %] checked[% END %]>
+    <label class="inline" for="dest_council">I wish to make a new report</label>
+</div>
+
+<div id="dest-error"[% IF NOT field_errors.not_for_us %] class="hidden"[% END %]>
+  <div class="form-error">You can only use this form to report inappropriate content</div>
+
+  <div class="form-error__box form-error--council[% IF dest != 'council' %] hidden[% END %]">
+    <p>
+    <strong>If you want to report a street problem</strong>, return to the
+    <a href="/">homepage</a> and enter your postcode. You can then make a report.
+    We'll send it to your council, and publish it on the site.
+    </p>
+    <p>
+    <strong>If your problem is not a street issue</strong>, or is <strong>not
+    suitable for publication on the site</strong>, then this isn't the
+    right place for it. You should find other contact details on the
+    <a href="https://www.buckscc.gov.uk/">council's website</a>.
+    </p>
+  </div>
+
+  <div class="form-error__box form-error--update[% IF dest != 'update' %] hidden[% END %]">
+    <p>
+    <strong>If you'd like to update a report</strong>, please leave an update
+    on the <a href="/report/[% problem.id %]">report’s page</a>, underneath the report details.
+    </p>
+  </div>
+</div>
+
+[% END %]
diff --git a/templates/web/fixmystreet.com/contact/who.html b/templates/web/fixmystreet.com/contact/who.html
index e16809e48..996ceeca7 100644
--- a/templates/web/fixmystreet.com/contact/who.html
+++ b/templates/web/fixmystreet.com/contact/who.html
@@ -1,12 +1,53 @@
-[% IF NOT problem %]
 <h4>Topic:</h4>
 
 [% IF field_errors.dest %]
 <div class="form-error">[% field_errors.dest %]</div>
-[% ELSIF field_errors.not_for_us %]
-    <div class="form-error">You can only contact the team behind FixMyStreet using our contact form</div>
+[% END %]
+
+[% IF problem %]
+<div class="checkbox-group">
+    <input name="dest" id="dest_rules" type="radio" value="rules" class="required"[% IF dest AND dest == 'rules' %] checked[% END %]>
+    <label class="inline" for="dest_rules">This [% update ? 'update' : 'report' %] breaks the <a href="/about/house-rules">House Rules</a></label>
+</div>
+<div class="checkbox-group">
+    <input name="dest" id="dest_update" type="radio" value="update" class="required"[% IF dest AND dest == 'update' %] checked[% END %]>
+    <label class="inline" for="dest_update">This report has not been fixed</label>
+</div>
+<div class="checkbox-group">
+    <input name="dest" id="dest_council" type="radio" value="council" class="required"[% IF dest AND dest == 'council' %] checked[% END %]>
+    <label class="inline" for="dest_council">I want to make a new report about a street problem</label>
+</div>
+[% ELSE %]
+<div class="checkbox-group">
+    <input name="dest" id="dest_help" type="radio" value="help" class="required"[% IF dest AND dest == 'help' %] checked[% END %]>
+    <label class="inline" for="dest_help">I need help using the site</label>
+</div>
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_feedback" type="radio" value="feedback" class="required"[% IF dest AND dest == 'feedback' %] checked[% END %]>
+    <label class="inline" for="dest_feedback">I have feedback about the site</label>
+</div>
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_from_council" type="radio" value="from_council" class="required"[% IF dest AND dest == 'from_council' %] checked[% END %]>
+    <label class="inline" for="dest_from_council">I am from a council and I have a question for the FixMyStreet team</label>
+</div>
+
+<div class="checkbox-group">
+    <input name="dest" id="dest_council" type="radio" value="council" class="required"[% IF dest AND dest == 'council' %] checked[% END %]>
+    <label class="inline" for="dest_council">I want to report a street problem</label>
+</div>
 
-    [% IF dest == 'council' %]
+<div class="checkbox-group">
+    <input name="dest" id="dest_update" type="radio" value="update"[% IF dest AND dest == 'update' %] checked[% END %]>
+    <label class="inline" for="dest_update">My street problem hasn't been fixed</label>
+</div>
+[% END %]
+
+<div id="dest-error"[% IF NOT field_errors.not_for_us %] class="hidden"[% END %]>
+  <div class="form-error">You can only contact the team behind FixMyStreet using our contact form</div>
+
+  <div class="form-error__box form-error--council[% IF dest != 'council' %] hidden[% END %]">
     <p>
     We’re not the council: we just run this website which helps you report issues
     to them.
@@ -24,7 +65,9 @@
     right place for it. You should find contact details on your council's own
     website.
     </p>
-    [% ELSIF dest == 'update' %]
+  </div>
+
+  <div class="form-error__box form-error--update[% IF dest != 'update' %] hidden[% END %]">
     <p>
     FixMyStreet is great for reporting problems, but we don't fix them - your
     council oversees that.
@@ -46,32 +89,5 @@
     you could try contacting your local councillor, using another useful mySociety site:
     <a href="https://www.writetothem.com/?utm_source=fixmystreet.com&amp;utm_campaign=contact_workflow_links&amp;utm_medium=link&amp;utm_content=contact+not_fixed">WriteToThem</a>.
     </p>
-
-    [% END %]
-[% END %]
-
-<div class="checkbox-group">
-    <input name="dest" id="dest_help" type="radio" value="help" class="required"[% IF dest AND dest == 'help' %] checked[% END %]>
-    <label class="inline" for="dest_help">I need help using the site</label>
-</div>
-
-<div class="checkbox-group">
-    <input name="dest" id="dest_feedback" type="radio" value="feedback" class="required"[% IF dest AND dest == 'feedback' %] checked[% END %]>
-    <label class="inline" for="dest_feedback">I have feedback about the site</label>
+  </div>
 </div>
-
-<div class="checkbox-group">
-    <input name="dest" id="dest_from_council" type="radio" value="from_council" class="required"[% IF dest AND dest == 'from_council' %] checked[% END %]>
-    <label class="inline" for="dest_from_council">I am from a council and I have a question for the FixMyStreet team</label>
-</div>
-
-<div class="checkbox-group">
-    <input name="dest" id="dest_council" type="radio" value="council" class="required"[% IF dest AND dest == 'council' %] checked[% END %]>
-    <label class="inline" for="dest_council">I want to report a street problem</label>
-</div>
-
-<div class="checkbox-group">
-    <input name="dest" id="dest_update" type="radio" value="update"[% IF dest AND dest == 'update' %] checked[% END %]>
-    <label class="inline" for="dest_update">My street problem hasn't been fixed</label>
-</div>
-[% END %]
diff --git a/web/cobrands/sass/_base.scss b/web/cobrands/sass/_base.scss
index 15b42cc3e..a2da14280 100644
--- a/web/cobrands/sass/_base.scss
+++ b/web/cobrands/sass/_base.scss
@@ -510,6 +510,12 @@ textarea.form-error {
   @include border-radius(0 0.25em 0.25em 0.25em);
 }
 
+.form-error__box {
+    border: solid 2px #ff0000;
+    padding: 0.5em;
+    margin-bottom: 0.5em;
+}
+
 ul.error {
   background:#ff0000;
   color:#fff;
diff --git a/web/js/contact.js b/web/js/contact.js
new file mode 100644
index 000000000..9529ede16
--- /dev/null
+++ b/web/js/contact.js
@@ -0,0 +1,14 @@
+$('[name=dest]').change(function() {
+    var err = $('.form-error--' + this.value),
+        inputs = $(this).closest('form').find('input[type=text], input[type=submit]');
+    $('.form-error__box').addClass('hidden');
+    if (err.length) {
+        $('#dest-error').removeClass('hidden');
+        $('#dest-error .form-error').show(); // might have been hidden by normal validate
+        inputs.prop('disabled', true);
+        $('.form-error--' + this.value).removeClass('hidden');
+    } else {
+        $('#dest-error').addClass('hidden');
+        inputs.prop('disabled', false);
+    }
+});