[Zurich] Tweaks to redirect

See mysociety/FixMyStreet-Commercial#690

I understand redirection to summary page was introduced at ZWN's request,
to make it easy to
process reports and then quickly move onto the next one.

However, during testing now, Tobias has mentioned this is a) slowing
things down and b) confusing because it's not obvious that the report
has been saved.

I've tried to address (b) by adding the "Aktualisiert!" message when
you are redirected.

Also, for (a) I've removed the redirection from a few cases, and
disabled it for superuser.  If need be, I can remove redirection from
more (or all) cases.

Fix Official answer/Reply to user

 - hide label for status update on state change
 - correct wording on button for closure (single wording)
 - correct wording on label for user reply (either "Official answer" or "Reply to user" as appropriate)
 - Make sure the official response texts are shown for edit/static as appropriate, and test.
 - javascript improvements
 - honour public status update for Extern/Wunsch too
 - don't show public message for Wunsch
 - Ignore all other fields when rotating photos.
   (See mysociety/FixMyStreet-Commercial#718)

5 files changed, 144 insertions, 65 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 5ebeffc11..677aa66d7 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -683,7 +683,15 @@ sub report_edit : Path('report_edit') : Args(1) {
 
     if (my $rotate_photo_param = $self->_get_rotate_photo_param($c)) {
         $self->rotate_photo($c,  @$rotate_photo_param);
-        return 1;
+        if ( $c->cobrand->moniker eq 'zurich' ) {
+            # Clicking the photo rotation buttons should do nothing
+            # except for rotating the photo, so return the user
+            # to the report screen now.
+            $c->res->redirect( $c->uri_for( 'report_edit', $problem->id ) );
+            return;
+        } else {
+            return 1;
+        }
     }
 
     if ( $c->cobrand->moniker eq 'zurich' ) {
diff --git a/perllib/FixMyStreet/Cobrand/Zurich.pm b/perllib/FixMyStreet/Cobrand/Zurich.pm
index 1a9963a60..0e75d71b9 100644
--- a/perllib/FixMyStreet/Cobrand/Zurich.pm
+++ b/perllib/FixMyStreet/Cobrand/Zurich.pm
@@ -123,6 +123,26 @@ sub problem_is_closed {
     return exists $self->zurich_closed_states->{ $problem->state } ? 1 : 0;
 }
 
+sub zurich_public_response_states {
+    my $states = {
+        'fixed - council' => 1,
+        'closed'          => 1, # extern
+        'unable to fix'   => 1, # jurisdiction  unknown
+
+        # e.g. as above, but WITHOUT the following (as they are hidden)
+        # 'hidden'          => 1,
+        # 'investigating'   => 1, # wish
+        # 'partial'         => 1, # not contactable
+    };
+
+    return wantarray ? keys %{ $states } : $states;
+}
+
+sub problem_has_public_response {
+    my ($self, $problem) = @_;
+    return exists $self->zurich_public_response_states->{ $problem->state } ? 1 : 0;
+}
+
 sub problem_as_hashref {
     my $self = shift;
     my $problem = shift;
@@ -570,13 +590,8 @@ sub admin_report_edit {
         my $state = $c->get_param('state') || '';
         my $oldstate = $problem->state;
 
-        my %closure_states = (
-            'closed' => 1,
-            'investigating' => 1,
-            'hidden' => 1,
-            'partial' => 1,
-            'unable to fix' => 1,
-        );
+        my $closure_states = $self->zurich_closed_states;
+        delete $closure_states->{'fixed - council'}; # may not be needed?
 
         my $old_closure_state = $problem->get_extra_metadata('closure_status');
 
@@ -595,7 +610,7 @@ sub admin_report_edit {
             $internal_note_text = "Weitergeleitet von $old_cat an $new_cat";
             $self->update_admin_log($c, $problem, "Changed category from $old_cat to $new_cat");
             $redirect = 1 if $cat->body_id ne $body->id;
-        } elsif ( $closure_states{$state} and
+        } elsif ( $closure_states->{$state} and
                     ( $oldstate ne 'planned' )
                     || (($old_closure_state ||'') ne $state))
         {
@@ -610,7 +625,7 @@ sub admin_report_edit {
             $problem->set_extra_metadata( closure_status => $state );
             $self->set_problem_state($c, $problem, 'planned');
             $state = 'planned';
-            $redirect = 1;
+            $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
 
         } elsif ( my $subdiv = $c->get_param('body_subdivision') ) {
             $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
@@ -628,12 +643,8 @@ sub admin_report_edit {
                 }
 
                 $self->set_problem_state($c, $problem, $state)
-                    unless $closure_states{$state}; # in closure-states case, we'll defer to clause below
-
-                if ($self->problem_is_closed($problem)) {
-                    $problem->set_extra_metadata_if_undefined( closed_overdue => $self->overdue( $problem ) );
-                    $c->stash->{problem_is_closed} = 1;
-                }
+                    unless $closure_states->{$state};
+                    # we'll defer to 'planned' clause below to change the state
             }
         }
 
@@ -641,15 +652,17 @@ sub admin_report_edit {
             # Rueckmeldung ausstehend
             # override $state from the metadata set above
             $state = $problem->get_extra_metadata('closure_status') || '';
-            my $closed = 0;
+            my ($moderated, $closed) = (0, 0);
 
             if ($state eq 'hidden' && $c->req->params->{publish_response} ) {
                 _admin_send_email( $c, 'problem-rejected.txt', $problem );
-                $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
+
                 $self->set_problem_state($c, $problem, $state);
+                $moderated++;
                 $closed++;
             }
             elsif ($state =~/^(closed|investigating)$/) { # Extern | Wish
+                $moderated++;
                 # Nested if instead of `and` because in these cases, we *don't*
                 # want to close unless we have body_external (so we don't want
                 # the final elsif clause below to kick in on publish_response)
@@ -659,7 +672,6 @@ sub admin_report_edit {
                     $problem->external_body( $external );
                 }
                 if ($problem->external_body && $c->req->params->{publish_response}) {
-                    $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
                     $problem->whensent( undef );
                     $self->set_problem_state($c, $problem, $state);
                     my $template = ($state eq 'investigating') ? 'problem-wish.txt' : 'problem-external.txt';
@@ -671,14 +683,20 @@ sub admin_report_edit {
             }
             elsif ($c->req->params->{publish_response}) {
                 # otherwise we only set the state if publish_response is set
+                #
 
                 # if $state wasn't set, then we are simply closing the message as fixed
                 $state ||= 'fixed - council';
                 _admin_send_email( $c, 'problem-closed.txt', $problem );
                 $redirect = 1;
+                $moderated++;
                 $closed++;
             }
 
+            if ($moderated) {
+                $problem->set_extra_metadata_if_undefined( moderated_overdue => $self->overdue( $problem ) );
+            }
+
             if ($closed) {
                 # set to either the closure_status from metadata or 'fixed - council' as above
                 $self->set_problem_state($c, $problem, $state);
@@ -757,7 +775,9 @@ sub admin_report_edit {
         # (this will only happen if no other update_admin_log has already been called)
         $self->update_admin_log($c, $problem);
 
-        if ( $redirect ) {
+        if ( $redirect and $type eq 'dm' ) {
+            # only redirect for DM
+            $c->stash->{status_message} ||= '<p><em>' . _('Updated!') . '</em></p>';
             $c->go('index');
         }
 
diff --git a/t/cobrand/zurich.t b/t/cobrand/zurich.t
index 0a622cb2b..a8a1d973e 100644
--- a/t/cobrand/zurich.t
+++ b/t/cobrand/zurich.t
@@ -782,7 +782,7 @@ subtest "test stats" => sub {
         $mech->get_ok( '/admin/stats' );
         is $mech->res->code, 200, "superuser should be able to see stats page";
 
-        $mech->content_contains('Innerhalb eines Arbeitstages moderiert: 2'); # now including hidden
+        $mech->content_contains('Innerhalb eines Arbeitstages moderiert: 3');
         $mech->content_contains('Innerhalb von f&uuml;nf Arbeitstagen abgeschlossen: 3');
         # my @data = $mech->content =~ /(?:moderiert|abgeschlossen): \d+/g;
         # diag Dumper(\@data); use Data::Dumper;
@@ -854,6 +854,42 @@ subtest 'email images to external partners' => sub {
         };
 };
 
+subtest 'Status update shown as appropriate' => sub {
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'zurich' ],
+    }, sub {
+        # ALL closed states must hide the public_response edit, and public ones
+        # must show the answer in blue.
+        for (['planned', 1, 0],
+             ['fixed - council', 0, 1],
+             ['closed', 0, 1],
+             ['hidden', 0, 0])
+         {
+            my ($state, $update, $public) = @$_;
+            $report->update({ state => $state });
+            $mech->get_ok( '/admin/report_edit/' . $report->id );
+            contains_or_lacks($mech, $update, "<textarea name='status_update'");
+            contains_or_lacks($mech, $public, '<div class="admin-official-answer">');
+
+            if ($public) {
+                $mech->get_ok( '/report/' . $report->id );
+                $mech->content_contains('Antwort</h4>');
+            }
+       }
+    };
+};
+
+# TODO refactor into FixMyStreet::TestMech;
+sub contains_or_lacks {
+    my ($mech, $contains, $text) = @_;
+    if ($contains) {
+        $mech->content_contains($text);
+    }
+    else {
+        $mech->content_lacks($text);
+    }
+}
+
 subtest 'time_spent' => sub {
     FixMyStreet::override_config {
         ALLOWED_COBRANDS => [ 'zurich' ],
diff --git a/templates/web/zurich/admin/report_edit.html b/templates/web/zurich/admin/report_edit.html
index 95aa5d311..802ac466d 100644
--- a/templates/web/zurich/admin/report_edit.html
+++ b/templates/web/zurich/admin/report_edit.html
@@ -25,7 +25,7 @@
 
 <li class="report-edit-action">&raquo; <a href="http://webgis.intra.stzh.ch/AV_Online/Direct.asp?Map=AV&Search=Koord&West=[% problem.local_coords.0 %]&Nord=[% problem.local_coords.1 %]&B=300" target="_blank">Standort in AV-Online anzeigen</a></li>
 
-[% IF problem_is_closed %]
+[% IF c.cobrand.problem_is_closed(problem) %]
     <li><span class="mock-label">[% loc('Details:') %]</span> [% problem.detail | html %]
     [% IF problem.extra.original_detail %]
     <br>[%
@@ -133,12 +133,15 @@
 </script>
 </p>
 
-<p><span class="mock-label">[% loc('State:') %]</span> <select name="state"  id="state">
-    <option value="">--</option>
-  [% FOREACH s IN states %]
-    <option [% 'selected ' IF s.state == pstate %] value="[% s.state %]">[% s.trans %]</option>
-  [% END %]
-</select></p>
+<p> 
+    <span class="mock-label">[% loc('State:') %]</span>
+    <select name="state"  id="state">
+        <option value="">--</option>
+    [% FOREACH s IN states %]
+        <option [% 'selected ' IF s.state == pstate %] value="[% s.state %]">[% s.trans %]</option>
+    [% END %]
+    </select>
+</p>
 
 <script type="text/javascript">
 $(function(){
@@ -157,35 +160,33 @@ $(function(){
         // Show or hide the automatic reply field
         var state = $(this).val();
 
-        // Show or hide the categories
-        if (state === 'confirmed') {
-            $('#assignation__category').show();
-            $('#assignation__subdivision').show();
-        } else {
-            $('#assignation__category').hide();
-            $('#assignation__subdivision').hide();
-            $('#assignation__category select').val('');
-            $('#assignation__subdivision select').val('');
-        }
-
-        if ((state === 'closed') || (state === 'investigating')) {
-            $('#assignation__external').show();
-        } else {
-            $('#assignation__external select').val('');
-            $('#assignation__external').hide();
-        }
-
-        // disable the templates, public_response, publish if we've selected a
-        // different state to the one we started on
+        // show or disable assignation, templates, public_response, publish if
+        // same or different state to the one we started on
         if ((state === '[% pstate %]')) {
             $('input[name=publish_response]').show();
             $('.response_templates_select').show();
-            $('#status_update').show();
+            $('#status_update_container').show();
+
+            if (state === 'confirmed') {
+                $('#assignation__category').show();
+                $('#assignation__subdivision').show();
+            }
+            if ((state === 'closed') || (state === 'investigating')) {
+                $('#assignation__external').show();
+            }
         }
         else {
             $('input[name=publish_response]').hide();
             $('.response_templates_select').hide();
-            $('#status_update').hide();
+            $('#status_update_container').hide();
+
+            $('#assignation__category').hide();
+            $('#assignation__subdivision').hide();
+            $('#assignation__category select').val('');
+            $('#assignation__subdivision select').val('');
+
+            $('#assignation__external select').val('');
+            $('#assignation__external').hide();
         }
 
     }).change();
@@ -256,28 +257,42 @@ $(function(){
     </ul>
 [% END %]
 
-[%# Public response field shown for Ruckmeldung ausstehend/Extern/
-  # Zustandigkeit unbekannt/Wunsch/Nicht kontaktierbar/Unsichtbar %]
-[% SWITCH pstate %]
-    [% CASE ['planned','closed','unable to fix','investigating','partial','hidden'] %]
-        <ul class="no-bullets report-edit-action">
-            <li><label for="status_update">[% loc('Public response:') %]</label>
-            [% INCLUDE 'admin/response_templates_select.html' for='status_update' %]
-            <textarea name='status_update' id='status_update' cols=60 rows=5>
-            [%- problem.extra.public_response || default_public_response | html -%]
-            </textarea>
-            </li>
-        </ul>
+[%# Public response field shown for Ruckmeldung ausstehend states
+  # (e.g. various pstates) %]
+[% IF problem.state == 'planned' %]
+    <ul class="no-bullets report-edit-action">
+        <li id="status_update_container"><label for="status_update">
+        [% SWITCH pstate %]
+            [% CASE ['hidden', 'investigating', 'partial'] %][%# Hidden/Wish/Not contactable %]
+                [% loc('Reply to user:') %]
+            [% CASE DEFAULT %]
+                [% loc('Public response:') %]
+        [% END %]
+        </label>
+        [% INCLUDE 'admin/response_templates_select.html' for='status_update' %]
+        <textarea name='status_update' id='status_update' cols=60 rows=5>
+        [%- problem.extra.public_response || default_public_response | html -%]
+        </textarea>
+        </li>
+    </ul>
 [% END %]
 
 <p align="right" class="report-edit-action">
 [% IF show_publish_response %]
-<input type="submit" name="publish_response" value="[% loc('Publish the response') %]">
+    [%# While we call this 'publish_response', the response will not actually
+      # be "published" for these cases: Wish / Hidden / Not contactable (for these,
+      # only a private email will be sent to the user.  However, in all cases,
+      # this is the end of processing, so we mark this with the same text used
+      # for 'No further updates %]
+<input type="submit" name="publish_response" value="[% loc('No further updates') %]">
 [% END %]
+
+    [%# This button simply saves changes, but does NOT close the report (though
+      # it may trigger other workflow %]
 <input type="submit" name="Submit changes" value="[% loc('Submit changes') %]" >
 </p>
 
-[% IF problem.state == 'fixed - council' %]
+[% IF c.cobrand.problem_has_public_response(problem) %]
     <h2>[% loc('Public response:') %]</h2>
     <div class="admin-official-answer">
         [% problem.extra.public_response | html_para %]
diff --git a/templates/web/zurich/report/updates.html b/templates/web/zurich/report/updates.html
index ea71f2b1c..cb7644694 100644
--- a/templates/web/zurich/report/updates.html
+++ b/templates/web/zurich/report/updates.html
@@ -8,7 +8,7 @@
                     [%# XXX following should honour zurich_closed_states instead? %]
                     [% IF problem.state == 'fixed - council'
                     || ( problem.external_body AND problem.state == 'closed' ) %]
-                     [% add_links( problem.extra.public_response ) | html_para %]
+                    [% add_links( problem.extra.public_response ) | html_para %]
                   [% END %]
                 </div>
             </div>