Merge branch 'zurich-problem-status-stats'

7 files changed, 174 insertions, 29 deletions

diff --git a/bin/update-schema b/bin/update-schema
index 92868e1b6..ef33b82a7 100755
--- a/bin/update-schema
+++ b/bin/update-schema
@@ -85,6 +85,7 @@ print "Nothing to do\n" if $nothing;
 # By querying the database schema, we can see where we're currently at
 # (assuming schema change files are never half-applied, which should be the case)
 sub get_db_version {
+    return '0030' if ! constraint_exists('admin_log_action_check');
     return '0029' if column_exists('body', 'deleted');
     return '0028' if table_exists('body');
     return '0027' if column_exists('problem', 'subcategory');
@@ -134,3 +135,10 @@ sub column_like {
     my ( $table, $where, $column, $contents ) = @_;
     return dbh()->selectrow_array("select count(*) from $table WHERE $where AND $column LIKE ?", {}, "%$contents%");
 }
+
+# Returns true if a check constraint on a table exists
+sub constraint_exists {
+    my ( $constraint ) = @_;
+    return dbh()->selectrow_array('select count(*) from pg_constraint where conname = ?', {}, $constraint);
+}
+
diff --git a/bin/zurich-overdue-alert b/bin/zurich-overdue-alert
index 4a507c481..500c6b016 100755
--- a/bin/zurich-overdue-alert
+++ b/bin/zurich-overdue-alert
@@ -23,9 +23,9 @@ exit if FixMyStreet::Cobrand::Zurich::is_public_holiday($now) or FixMyStreet::Co
 my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker('zurich')->new();
 my %bodies = map { $_->id => $_ } FixMyStreet::App->model("DB::Body")->all;
 
-loop_through( 'alert-moderation-overdue.txt', 0, 1, [ 'unconfirmed', 'confirmed' ] );
+loop_through( 'alert-moderation-overdue.txt', 0, 1, [ 'unconfirmed' ] );
 loop_through( 'alert-overdue.txt', 1, 6, 'in progress' );
-loop_through( 'alert-overdue.txt', 0, 6, 'planned' );
+loop_through( 'alert-overdue.txt', 0, 6, ['confirmed', 'planned'] );
 
 sub loop_through {
     my ( $template, $include_parent, $days, $states ) = @_;
diff --git a/db/schema.sql b/db/schema.sql
index 5c54e2d88..125e680c6 100644
--- a/db/schema.sql
+++ b/db/schema.sql
@@ -448,9 +448,6 @@ create table admin_log (
       or object_type = 'user'
     ),
     object_id integer not null,
-    action text not null check (
-        action = 'edit'
-        or action = 'state_change' 
-        or action = 'resend'),
+    action text not null,
     whenedited timestamp not null default ms_current_timestamp()
 ); 
diff --git a/db/schema_0030-drop-action-log-check-constraint.sql b/db/schema_0030-drop-action-log-check-constraint.sql
new file mode 100644
index 000000000..dfc5387f3
--- /dev/null
+++ b/db/schema_0030-drop-action-log-check-constraint.sql
@@ -0,0 +1,6 @@
+BEGIN;
+
+ALTER TABLE admin_log 
+    DROP CONSTRAINT admin_log_action_check;
+
+COMMIT;
diff --git a/perllib/FixMyStreet/Cobrand/Default.pm b/perllib/FixMyStreet/Cobrand/Default.pm
index 38efb7a35..a39a98135 100644
--- a/perllib/FixMyStreet/Cobrand/Default.pm
+++ b/perllib/FixMyStreet/Cobrand/Default.pm
@@ -295,8 +295,11 @@ to null/0.
 sub uri {
     my ( $self, $uri ) = @_;
 
-    (my $map_class = $FixMyStreet::Map::map_class) =~ s/^FixMyStreet::Map:://;
-    return $uri unless $map_class =~ /OSM|FMS/;
+    {
+        no warnings 'once';
+        (my $map_class = $FixMyStreet::Map::map_class) =~ s/^FixMyStreet::Map:://;
+        return $uri unless $map_class =~ /OSM|FMS/;
+    }
 
     $uri->query_param( zoom => 3 )
       if $uri->query_param('lat') && !$uri->query_param('zoom');
diff --git a/perllib/FixMyStreet/Cobrand/Zurich.pm b/perllib/FixMyStreet/Cobrand/Zurich.pm
index e15170721..690fff104 100644
--- a/perllib/FixMyStreet/Cobrand/Zurich.pm
+++ b/perllib/FixMyStreet/Cobrand/Zurich.pm
@@ -38,6 +38,7 @@ The entries will be something like this (but with different ids).
     Users:
          id |      email       | from_body
         ----+------------------+-----------
+          1 | super@example.org|         1
           2 | dm1@example.org  |         2
           3 | sdm1@example.org |         3
 
@@ -215,19 +216,35 @@ sub overdue {
     my $w = $problem->created;
     return 0 unless $w;
 
-    if ( $problem->state eq 'unconfirmed' || $problem->state eq 'confirmed' ) {
+    # call with previous state
+    if ( $problem->state eq 'unconfirmed' ) {
         # One working day
         $w = add_days( $w, 1 );
         return $w < DateTime->now() ? 1 : 0;
-    } elsif ( $problem->state eq 'in progress' || $problem->state eq 'planned' ) {
+    } elsif ( $problem->state eq 'confirmed' || $problem->state eq 'in progress' || $problem->state eq 'planned' ) {
+        # States which affect the subdiv_overdue statistic.  TODO: this may no longer be required
         # Six working days from creation
         $w = add_days( $w, 6 );
         return $w < DateTime->now() ? 1 : 0;
+
+    # call with new state
+    } elsif ( $problem->state eq 'fixed - council' || $problem->state eq 'closed' || $problem->state eq 'hidden' ) {
+        # States which affect the closed_overdue statistic
+        # Five working days from moderation (so 6 from creation)
+        $w = add_days( $w, 6 );
+        return $w < DateTime->now() ? 1 : 0;
     } else {
         return 0;
     }
 }
 
+sub set_problem_state {
+    my ($self, $c, $problem, $new_state) = @_;
+    return if $new_state eq $problem->state;
+    $problem->state( $new_state );
+    $c->forward( 'log_edit', [ $problem->id, 'problem', "state change to $new_state" ] );
+}
+
 sub email_indent { ''; }
 
 # Specific administrative displays
@@ -444,23 +461,36 @@ sub admin_report_edit {
             $internal_note_text = "Weitergeleitet von $old_cat an $new_cat";
             $redirect = 1 if $cat->body_id ne $body->id;
         } elsif ( my $subdiv = $c->req->params->{body_subdivision} ) {
-            $extra->{moderated_overdue} = $self->overdue( $problem );
-            $problem->state( 'in progress' );
+            $extra->{moderated_overdue} //= $self->overdue( $problem );
+            $self->set_problem_state($c, $problem, 'in progress');
             $problem->external_body( undef );
             $problem->bodies_str( $subdiv );
             $problem->whensent( undef );
             $redirect = 1;
         } elsif ( my $external = $c->req->params->{body_external} ) {
-            $extra->{moderated_overdue} = $self->overdue( $problem );
-            $problem->state( 'closed' );
+            $extra->{moderated_overdue} //= $self->overdue( $problem );
+            $self->set_problem_state($c, $problem, 'closed');
+            $extra->{closed_overdue} //= $self->overdue( $problem );
             $problem->external_body( $external );
             $problem->whensent( undef );
             _admin_send_email( $c, 'problem-external.txt', $problem );
             $redirect = 1;
         } else {
-            $problem->state( $c->req->params->{state} ) if $c->req->params->{state};
-            if ( $problem->state eq 'hidden' && $c->req->params->{send_rejected_email} ) {
-                _admin_send_email( $c, 'problem-rejected.txt', $problem );
+            if (my $state = $c->req->params->{state}) {
+
+                if ($problem->state eq 'unconfirmed' and $state ne 'unconfirmed') {
+                    # only set this for the first state change
+                    $extra->{moderated_overdue} //= $self->overdue( $problem );
+                }
+
+                $self->set_problem_state($c, $problem, $state);
+
+                if ($state eq 'fixed - council' || $state eq 'closed' || $state eq 'hidden') {
+                    $extra->{closed_overdue} //= $self->overdue( $problem );
+                }
+                if ( $state eq 'hidden' && $c->req->params->{send_rejected_email} ) {
+                    _admin_send_email( $c, 'problem-rejected.txt', $problem );
+                }
             }
         }
 
@@ -475,7 +505,9 @@ sub admin_report_edit {
             $extra->{public_response} = $update;
             $problem->extra( $extra );
             if ($c->req->params->{publish_response}) {
-                $problem->state( 'fixed - council' );
+                $self->set_problem_state($c, $problem, 'fixed - council');
+                $extra->{closed_overdue} = $self->overdue( $problem );
+                $problem->extra( { %$extra } );
                 _admin_send_email( $c, 'problem-closed.txt', $problem );
             }
         }
@@ -523,7 +555,7 @@ sub admin_report_edit {
             $c->forward('check_token');
 
             $problem->bodies_str( $body->parent->id );
-            $problem->state( 'confirmed' );
+            $self->set_problem_state($c, $problem, 'confirmed');
             $problem->update;
             # log here
             $c->res->redirect( '/admin/summary' );
@@ -562,7 +594,7 @@ sub admin_report_edit {
                 $problem->extra( $extra );
                 $problem->bodies_str( $body->parent->id );
                 $problem->whensent( undef );
-                $problem->state( 'planned' );
+                $self->set_problem_state($c, $problem, 'planned');
                 $problem->update;
                 $c->res->redirect( '/admin/summary' );
             }
@@ -646,7 +678,7 @@ sub admin_stats {
 
     my %date_params;
     my $ym = $c->req->params->{ym};
-    my ($m, $y) = $ym =~ /^(\d+)\.(\d+)$/;
+    my ($m, $y) = $ym ? ($ym =~ /^(\d+)\.(\d+)$/) : ();
     $c->stash->{ym} = $ym;
     if ($y && $m) {
         $c->stash->{start_date} = DateTime->new( year => $y, month => $m, day => 1 );
@@ -706,8 +738,10 @@ sub admin_stats {
     my $closed = $c->model('DB::Problem')->search( { state => 'closed', %date_params } )->count;
     # Reports moderated within 1 day
     my $moderated = $c->model('DB::Problem')->search( { extra => { like => '%moderated_overdue,I1:0%' }, %params } )->count;
-    # Reports solved within 5 days
+    # Reports solved within 5 days (sent back from subdiv)
     my $subdiv_dealtwith = $c->model('DB::Problem')->search( { extra => { like => '%subdiv_overdue,I1:0%' }, %params } )->count;
+    # Reports solved within 5 days (marked as 'fixed - council', 'closed', or 'hidden'
+    my $fixed_in_time = $c->model('DB::Problem')->search( { extra => { like => '%closed_overdue,I1:0%' }, %params } )->count;
     # Reports per category
     my $per_category = $c->model('DB::Problem')->search( \%params, {
         select   => [ 'category', { count => 'id' } ],
@@ -738,7 +772,7 @@ sub admin_stats {
         reports_spam => $hidden,
         reports_assigned => $closed,
         reports_moderated => $moderated,
-        reports_dealtwith => $subdiv_dealtwith,
+        reports_dealtwith => $fixed_in_time,
         reports_category_changed => $changed,
         pictures_taken => $pictures_taken,
         pictures_published => $pictures_published,
diff --git a/t/cobrand/zurich.t b/t/cobrand/zurich.t
index edc99d758..a03099aac 100644
--- a/t/cobrand/zurich.t
+++ b/t/cobrand/zurich.t
@@ -26,10 +26,17 @@ sub send_reports_for_zurich {
         FixMyStreet::App->model('DB::Problem')->send_reports('zurich');
     };
 }
-
 use FixMyStreet::TestMech;
 my $mech = FixMyStreet::TestMech->new;
 
+sub cleanup {
+    $mech->delete_problems_for_body( 2 );
+    $mech->delete_user( 'dm1@example.org' );
+    $mech->delete_user( 'sdm1@example.org' );
+}
+
+cleanup();
+
 # Front page test
 ok $mech->host("zurich.example.com"), "change host to Zurich";
 FixMyStreet::override_config {
@@ -140,6 +147,7 @@ subtest "changing of categories" => sub {
     $report->update({category => $original_category });
 };
 
+ok ( ! exists ${$report->extra}{moderated_overdue}, 'Report currently unmoderated' );
 
 FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'zurich' ],
@@ -154,6 +162,48 @@ $mech->content_contains('Test Test');
 $mech->content_lacks('photo/' . $report->id . '.jpeg');
 $mech->email_count_is(0);
 
+$report->discard_changes;
+is ( $report->extra->{moderated_overdue}, 0, 'Report now marked moderated' );
+
+# Set state back to 10 days ago so that report is overdue
+my $created = $report->created;
+$report->update({
+    state   => 'unconfirmed',
+    created => $created->subtract(days => 10),
+});
+
+FixMyStreet::override_config {
+    ALLOWED_COBRANDS => [ 'zurich' ],
+}, sub {
+    $mech->get_ok( '/admin/report_edit/' . $report->id );
+    $mech->submit_form_ok( { with_fields => { state => 'confirmed' } } );
+    $mech->get_ok( '/report/' . $report->id );
+};
+$report->discard_changes;
+is ( $report->extra->{moderated_overdue}, 0, 'Report still not overdue (subsequent time)' );
+
+$report->update({ created => $created }); # reset
+        
+# delete the {moderated_overdue} so that changing status will now reset moderation
+{
+    my $extra = $report->extra;
+    delete $extra->{moderated_overdue};
+    $report->update({
+        extra   => { %$extra },
+        state   => 'unconfirmed',
+    });
+}
+
+FixMyStreet::override_config {
+    ALLOWED_COBRANDS => [ 'zurich' ],
+}, sub {
+    $mech->get_ok( '/admin/report_edit/' . $report->id );
+    $mech->submit_form_ok( { with_fields => { state => 'confirmed' } } );
+    $mech->get_ok( '/report/' . $report->id );
+};
+$report->discard_changes;
+is ( $report->extra->{moderated_overdue}, 1, 'Report marked as moderated_overdue' );
+
 FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'zurich' ],
 }, sub {
@@ -286,11 +336,11 @@ $mech->clear_emails_ok;
 });
 $report = $reports[0];
 
-$mech->get_ok( '/admin/report_edit/' . $report->id );
-$mech->submit_form_ok( { with_fields => { state => 'planned' } } );
 FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'zurich' ],
 }, sub {
+    $mech->get_ok( '/admin/report_edit/' . $report->id );
+    $mech->submit_form_ok( { with_fields => { state => 'planned' } } );
     $mech->get_ok( '/report/' . $report->id );
 };
 $mech->content_contains('In Bearbeitung');
@@ -362,6 +412,29 @@ like $email->body, qr/test\@example.com/, 'body does contain email address';
 $mech->clear_emails_ok;
 $mech->log_out_ok;
 
+subtest "only superuser can see stats" => sub {
+    $user = $mech->log_in_ok( 'super@example.org' );
+    # a user from body $zurich is a superuser, as $zurich has no parent id!
+    $user->update({ from_body => $zurich->id }); 
+
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'zurich' ],
+    }, sub {
+        $mech->get( '/admin/stats' );
+    };
+    is $mech->res->code, 200, "superuser should be able to see stats page";
+    $mech->log_out_ok;
+
+    $user = $mech->log_in_ok( 'dm1@example.org' );
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'zurich' ],
+    }, sub {
+        $mech->get( '/admin/stats' );
+    };
+    is $mech->res->code, 404, "only superuser should be able to see stats page";
+    $mech->log_out_ok;
+};
+
 subtest "only superuser can edit bodies" => sub {
     $user = $mech->log_in_ok( 'dm1@example.org' );
     FixMyStreet::override_config {
@@ -466,9 +539,33 @@ subtest "hidden report email are only sent when requested" => sub {
     };
 };
 
-$mech->delete_problems_for_body( 2 );
-$mech->delete_user( 'dm1@example.org' );
-$mech->delete_user( 'sdm1@example.org' );
+subtest "test stats" => sub {
+    $user = $mech->log_in_ok( 'super@example.org' );
+
+    FixMyStreet::override_config {
+        ALLOWED_COBRANDS => [ 'zurich' ],
+    }, sub {
+        $mech->get( '/admin/stats' );
+    };
+    is $mech->res->code, 200, "superuser should be able to see stats page";
+
+    $mech->content_contains('Innerhalb eines Arbeitstages moderiert: 1');
+    $mech->content_contains('Innerhalb von f&uuml;nf Arbeitstagen abgeschlossen: 1');
+
+    $mech->log_out_ok;
+};
+
+subtest "test admin_log" => sub {
+    diag $report->id;
+    my @entries = FixMyStreet::App->model('DB::AdminLog')->search({
+        object_type => 'problem',
+        object_id   => $report->id,
+    });
+    is scalar @entries, 4, 'State changes logged'; 
+    is $entries[-1]->action, 'state change to hidden', 'State change logged as expected';
+};
+
+cleanup();
 
 ok $mech->host("www.fixmystreet.com"), "change host back";