[Open311] Move send-comments code to package.

5 files changed, 304 insertions, 157 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a0a3d2d5d..bc13c6dac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@
         - Clearer relocation options while you’re reporting a problem #2238
     - Bugfixes:
         - Add perl 5.26/5.28 support.
+    - Internal things:
+        - Move send-comments code to package for testing.
 
 * v2.4.1 (2nd October 2018)
     - New features:
diff --git a/bin/send-comments b/bin/send-comments
index 33da7379e..53afeb045 100755
--- a/bin/send-comments
+++ b/bin/send-comments
@@ -18,167 +18,18 @@ BEGIN {
 }
 
 use CronFns;
-
-use DateTime;
 use FixMyStreet;
-use FixMyStreet::Cobrand;
-use FixMyStreet::DB;
-use Open311;
-
-# send_method config values found in by-area config data, for selecting to appropriate method
-use constant SEND_METHOD_EMAIL      => 'email';
-use constant SEND_METHOD_OPEN311    => 'Open311';
+use Open311::PostServiceRequestUpdates;
 
-use constant COUNCIL_ID_OXFORDSHIRE => 2237;
-use constant COUNCIL_ID_BROMLEY => 2482;
-use constant COUNCIL_ID_LEWISHAM => 2492;
-use constant COUNCIL_ID_BUCKS => 2217;
+my ($verbose, $nomail) = CronFns::options();
 
 # Set up site, language etc.
-my ($verbose, $nomail) = CronFns::options();
 my $base_url = FixMyStreet->config('BASE_URL');
 my $site = '';
 $site = 'fixmystreet.com' if $base_url eq "https://www.fixmystreet.com";
 
-my $bodies = FixMyStreet::DB->resultset('Body')->search( {
-    send_method => SEND_METHOD_OPEN311,
-    send_comments => 1,
-} );
-
-while ( my $body = $bodies->next ) {
-
-    # XXX Cobrand specific - see also list in Problem->updates_sent_to_body
-    if ($site eq 'fixmystreet.com') {
-        # Lewisham does not yet accept updates
-        next if $body->areas->{+COUNCIL_ID_LEWISHAM};
-    }
-
-    my $use_extended = 0;
-    my $comments = FixMyStreet::DB->resultset('Comment')->search( {
-            'me.whensent'    => undef,
-            'me.external_id' => undef,
-            'me.state'          => 'confirmed',
-            'me.confirmed'      => { '!=' => undef },
-            'problem.whensent'    => { '!=' => undef },
-            'problem.external_id'  => { '!=' => undef },
-            'problem.bodies_str' => { -like => '%' . $body->id . '%' },
-            'problem.send_method_used' => 'Open311',
-        },
-        {
-            join => 'problem',
-            order_by => [ 'confirmed', 'id' ],
-        }
-    );
-
-    if ( $site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
-        $use_extended = 1;
-    }
-
-    my %open311_conf = (
-            endpoint => $body->endpoint,
-            jurisdiction => $body->jurisdiction,
-            api_key => $body->api_key,
-            use_extended_updates => $use_extended,
-    );
-
-    if ( $body->areas->{+COUNCIL_ID_OXFORDSHIRE} ) {
-            $open311_conf{use_customer_reference} = 1,
-    }
-
-    if ( $body->areas->{+COUNCIL_ID_BUCKS} ) {
-            $open311_conf{mark_reopen} = 1;
-    }
-
-    if ( $body->send_extended_statuses ) {
-        $open311_conf{extended_statuses} = 1;
-    }
-
-    my $o = Open311->new( %open311_conf );
-
-    if ( $site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
-        my $endpoints = $o->endpoints;
-        $endpoints->{update} = 'update.xml';
-        $endpoints->{service_request_updates} = 'update.xml';
-        $o->endpoints( $endpoints );
-    }
-
-    while ( my $comment = $comments->next ) {
-        my $cobrand = $body->get_cobrand_handler ||
-                      FixMyStreet::Cobrand->get_class_for_moniker($comment->cobrand)->new();
-
-        # Some cobrands (e.g. Buckinghamshire) don't want to receive updates
-        # from anyone except the original problem reporter.
-        if ($cobrand->call_hook(should_skip_sending_update => $comment)) {
-            unless (defined $comment->get_extra_metadata('cobrand_skipped_sending')) {
-                $comment->set_extra_metadata(cobrand_skipped_sending => 1);
-                $comment->update;
-            }
-            next;
-        }
-
-        # Oxfordshire stores the external id of the problem as a customer reference
-        # in metadata
-        if ($body->areas->{+COUNCIL_ID_OXFORDSHIRE} &&
-            !$comment->problem->get_extra_metadata('customer_reference') ) {
-            next;
-        }
-
-        # TODO actually this should be OK for any devolved endpoint if original Open311->can_be_devolved, presumably
-        if ( 0 ) { # Check can_be_devolved and do this properly if set
-            my $sender = $cobrand->get_body_sender( $body, $comment->problem->category );
-            my $config = $sender->{config};
-            $o = Open311->new(
-                    endpoint => $config->endpoint,
-                    jurisdiction => $config->jurisdiction,
-                    api_key => $config->api_key,
-                    use_extended_updates => 1, # FMB uses extended updates
-            );
-        }
-
-        next if !$verbose && $comment->send_fail_count && retry_timeout($comment);
-
-        if ( $site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
-            my $extra = $comment->extra;
-            if ( !$extra ) {
-                $extra = {};
-            }
-
-            unless ( $extra->{title} ) {
-                $extra->{title} = $comment->user->title;
-                $comment->extra( $extra );
-            }
-        }
-
-        my $id = $o->post_service_request_update( $comment );
-
-        if ( $id ) {
-            $comment->update( {
-                external_id => $id,
-                whensent    => \'current_timestamp',
-            } );
-        } else {
-            $comment->update( {
-                send_fail_count => $comment->send_fail_count + 1,
-                send_fail_timestamp => \'current_timestamp',
-                send_fail_reason => "Failed to post over Open311\n\n" . $o->error,
-            } );
-
-            if ( $verbose && $o->error ) {
-                warn $o->error;
-            }
-        }
-    }
-}
-
-sub retry_timeout {
-    my $row = shift;
-
-    my $tz = FixMyStreet->local_time_zone;
-    my $now = DateTime->now( time_zone => $tz );
-    my $diff = $now - $row->send_fail_timestamp;
-    if ( $diff->in_units( 'minutes' ) < 30 ) {
-        return 1;
-    }
-
-    return 0;
-}
+my $updates = Open311::PostServiceRequestUpdates->new(
+    site => $site,
+    verbose => $verbose,
+);
+$updates->send;
diff --git a/perllib/Open311/PostServiceRequestUpdates.pm b/perllib/Open311/PostServiceRequestUpdates.pm
new file mode 100755
index 000000000..c2b8c2494
--- /dev/null
+++ b/perllib/Open311/PostServiceRequestUpdates.pm
@@ -0,0 +1,174 @@
+package Open311::PostServiceRequestUpdates;
+
+use strict;
+use warnings;
+use v5.14;
+
+use DateTime;
+use Moo;
+use FixMyStreet;
+use FixMyStreet::Cobrand;
+use FixMyStreet::DB;
+use Open311;
+
+use constant SEND_METHOD_OPEN311 => 'Open311';
+
+use constant COUNCIL_ID_OXFORDSHIRE => 2237;
+use constant COUNCIL_ID_BROMLEY => 2482;
+use constant COUNCIL_ID_LEWISHAM => 2492;
+use constant COUNCIL_ID_BUCKS => 2217;
+
+has verbose => ( is => 'ro', default => 0 );
+has site => ( is => 'ro', default => '' );
+
+sub send {
+    my $self = shift;
+
+    my $bodies = FixMyStreet::DB->resultset('Body')->search( {
+        send_method => SEND_METHOD_OPEN311,
+        send_comments => 1,
+    } );
+
+    while ( my $body = $bodies->next ) {
+        # XXX Cobrand specific - see also list in Problem->updates_sent_to_body
+        if ($self->site eq 'fixmystreet.com') {
+            # Lewisham does not yet accept updates
+            next if $body->areas->{+COUNCIL_ID_LEWISHAM};
+        }
+
+        $self->process_body($body);
+    }
+}
+
+sub open311_params {
+    my ($self, $body) = @_;
+
+    my $use_extended = 0;
+    if ( $self->site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
+        $use_extended = 1;
+    }
+
+    my %open311_conf = (
+        endpoint => $body->endpoint,
+        jurisdiction => $body->jurisdiction,
+        api_key => $body->api_key,
+        use_extended_updates => $use_extended,
+    );
+
+    if ( $body->areas->{+COUNCIL_ID_OXFORDSHIRE} ) {
+        $open311_conf{use_customer_reference} = 1;
+    }
+
+    if ( $body->areas->{+COUNCIL_ID_BUCKS} ) {
+        $open311_conf{mark_reopen} = 1;
+    }
+
+    if ( $body->send_extended_statuses ) {
+        $open311_conf{extended_statuses} = 1;
+    }
+
+    return %open311_conf;
+}
+
+sub process_body {
+    my ($self, $body) = @_;
+
+    my $o = Open311->new( $self->open311_params($body) );
+
+    if ( $self->site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
+        my $endpoints = $o->endpoints;
+        $endpoints->{update} = 'update.xml';
+        $endpoints->{service_request_updates} = 'update.xml';
+        $o->endpoints( $endpoints );
+    }
+
+    my $comments = FixMyStreet::DB->resultset('Comment')->search( {
+            'me.whensent' => undef,
+            'me.external_id' => undef,
+            'me.state' => 'confirmed',
+            'me.confirmed' => { '!=' => undef },
+            'problem.whensent' => { '!=' => undef },
+            'problem.external_id' => { '!=' => undef },
+            'problem.bodies_str' => { -like => '%' . $body->id . '%' },
+            'problem.send_method_used' => 'Open311',
+        },
+        {
+            join => 'problem',
+            order_by => [ 'confirmed', 'id' ],
+        }
+    );
+
+    while ( my $comment = $comments->next ) {
+        my $cobrand = $body->get_cobrand_handler ||
+                      FixMyStreet::Cobrand->get_class_for_moniker($comment->cobrand)->new();
+
+        # Some cobrands (e.g. Buckinghamshire) don't want to receive updates
+        # from anyone except the original problem reporter.
+        if ($cobrand->call_hook(should_skip_sending_update => $comment)) {
+            unless (defined $comment->get_extra_metadata('cobrand_skipped_sending')) {
+                $comment->set_extra_metadata(cobrand_skipped_sending => 1);
+                $comment->update;
+            }
+            next;
+        }
+
+        # Oxfordshire stores the external id of the problem as a customer reference
+        # in metadata
+        if ($body->areas->{+COUNCIL_ID_OXFORDSHIRE} &&
+            !$comment->problem->get_extra_metadata('customer_reference') ) {
+            next;
+        }
+
+        next if !$self->verbose && $comment->send_fail_count && retry_timeout($comment);
+
+        $self->process_update($body, $o, $comment);
+    }
+}
+
+sub process_update {
+    my ($self, $body, $o, $comment) = @_;
+
+    if ( $self->site eq 'fixmystreet.com' && $body->areas->{+COUNCIL_ID_BROMLEY} ) {
+        my $extra = $comment->extra;
+        $extra = {} if !$extra;
+
+        unless ( $extra->{title} ) {
+            $extra->{title} = $comment->user->title;
+            $comment->extra( $extra );
+        }
+    }
+
+    my $id = $o->post_service_request_update( $comment );
+
+    if ( $id ) {
+        $comment->update( {
+            external_id => $id,
+            whensent => \'current_timestamp',
+        } );
+    } else {
+        $comment->update( {
+            send_fail_count => $comment->send_fail_count + 1,
+            send_fail_timestamp => \'current_timestamp',
+            send_fail_reason => "Failed to post over Open311\n\n" . $o->error,
+        } );
+
+        if ( $self->verbose && $o->error ) {
+            warn $o->error;
+        }
+    }
+}
+
+sub retry_timeout {
+    my $row = shift;
+
+    my $tz = FixMyStreet->local_time_zone;
+    my $now = DateTime->now( time_zone => $tz );
+    my $diff = $now - $row->send_fail_timestamp;
+    if ( $diff->in_units( 'minutes' ) < 30 ) {
+        return 1;
+    }
+
+    return 0;
+}
+
+1;
diff --git a/t/open311.t b/t/open311.t
index e1ad578d7..ef52eb538 100644
--- a/t/open311.t
+++ b/t/open311.t
@@ -415,10 +415,26 @@ foreach my $test (
         status => 'OPEN',
         extended => 'IN_PROGRESS',
     },
+    {
+        desc => 'comment that marks problem open sends OPEN if not mark_reopen',
+        state => 'confirmed',
+        status => 'OPEN',
+        extended => 'OPEN',
+        mark_open => 1,
+    },
+    {
+        desc => 'comment that marks problem open sends REOPEN if mark_reopen',
+        state => 'confirmed',
+        status => 'OPEN',
+        extended => 'REOPEN',
+        mark_open => 1,
+        mark_reopen => 1,
+    },
 ) {
     subtest $test->{desc} => sub {
         $comment->problem_state( $test->{state} );
         $comment->problem->state( $test->{state} );
+        $comment->mark_open(1) if $test->{mark_open};
 
         my $results = make_update_req( $comment, '<?xml version="1.0" encoding="utf-8"?><service_request_updates><request_update><update_id>248</update_id></request_update></service_request_updates>' );
 
@@ -426,7 +442,9 @@ foreach my $test (
         is $c->param('status'), $test->{status}, 'correct status';
 
         if ( $test->{extended} ) {
-            my $results = make_update_req( $comment, '<?xml version="1.0" encoding="utf-8"?><service_request_updates><request_update><update_id>248</update_id></request_update></service_request_updates>', { extended_statuses => 1 } );
+            my $params = { extended_statuses => 1 };
+            $params->{mark_reopen} = 1 if $test->{mark_reopen};
+            my $results = make_update_req( $comment, '<?xml version="1.0" encoding="utf-8"?><service_request_updates><request_update><update_id>248</update_id></request_update></service_request_updates>', $params );
             my $c = CGI::Simple->new( $results->{ req }->content );
             is $c->param('status'), $test->{extended}, 'correct extended status';
         }
diff --git a/t/open311/post-service-request-updates.t b/t/open311/post-service-request-updates.t
new file mode 100644
index 000000000..95013b951
--- /dev/null
+++ b/t/open311/post-service-request-updates.t
@@ -0,0 +1,102 @@
+#!/usr/bin/env perl
+
+use FixMyStreet::TestMech;
+
+my $mech = FixMyStreet::TestMech->new;
+
+use_ok( 'Open311::PostServiceRequestUpdates' );
+
+my $o = Open311::PostServiceRequestUpdates->new( site => 'fixmystreet.com' );
+
+my $params = {
+    send_method => 'Open311',
+    send_comments => 1,
+    api_key => 'KEY',
+    endpoint => 'endpoint',
+    jurisdiction => 'home',
+};
+my $bromley = $mech->create_body_ok(2482, 'Bromley', { %$params, send_extended_statuses => 1 });
+my $oxon = $mech->create_body_ok(2237, 'Oxfordshire', $params);
+my $bucks = $mech->create_body_ok(2217, 'Buckinghamshire', $params);
+my $lewisham = $mech->create_body_ok(2492, 'Lewisham', $params);
+
+subtest 'Check Open311 params' => sub {
+    my $result = {
+        endpoint => 'endpoint',
+        jurisdiction => 'home',
+        api_key => 'KEY',
+        use_extended_updates => 0,
+    };
+    my %conf = $o->open311_params($bromley);
+    is_deeply \%conf, {
+        %$result,
+        extended_statuses => 1,
+        use_extended_updates => 1,
+    }, 'Bromley params match';
+    %conf = $o->open311_params($oxon);
+    is_deeply \%conf, {
+        %$result,
+        use_customer_reference => 1
+    }, 'Oxfordshire params match';
+    %conf = $o->open311_params($bucks);
+    is_deeply \%conf, {
+        %$result,
+        mark_reopen => 1,
+    }, 'Bucks params match';
+    %conf = $o->open311_params($lewisham);
+    is_deeply \%conf, $result, 'Lewisham params match';
+};
+
+my $other_user = $mech->create_user_ok('test2@example.com', title => 'MRS');
+
+sub c {
+    my ($p, $user) = @_;
+    my $c = $mech->create_comment_for_problem($p, $user || $p->user, 'Name', 'Update text', 'f', 'confirmed', 'confirmed', { confirmed => \'current_timestamp' });
+    return $c;
+}
+
+sub p_and_c {
+    my ($body, $user) = @_;
+
+    my $prob_params = { send_method_used => 'Open311', whensent => \'current_timestamp', external_id => 1 };
+    my ($p) = $mech->create_problems_for_body(1, $body->id, 'Title', $prob_params);
+    my $c = c($p, $user);
+    return ($p, $c);
+}
+
+my ($p1, $c1) = p_and_c($bromley, $other_user);
+my ($p2, $c2) = p_and_c($oxon);
+my ($p3, $c3a) = p_and_c($bucks);
+my $c3b = c($p3, $other_user);
+my ($p4, $c4) = p_and_c($lewisham);
+
+subtest 'Send comments' => sub {
+  FixMyStreet::override_config {
+    ALLOWED_COBRANDS => ['fixmystreet', 'bromley', 'buckinghamshire', 'lewisham', 'oxfordshire'],
+  }, sub {
+    $o->send;
+    $c3a->discard_changes;
+    is $c3a->extra, undef, 'Bucks update by owner was sent';
+    $c3b->discard_changes;
+    is $c3b->extra->{cobrand_skipped_sending}, 1, 'Bucks update by other was not';
+    $c1->discard_changes;
+    is $c1->extra->{title}, "MRS", 'Title set on Bromley update';
+    $c2->discard_changes;
+    is $c2->send_fail_count, 0, 'Oxfordshire update skipped entirely';
+  };
+};
+
+subtest 'Oxfordshire gets an ID' => sub {
+  FixMyStreet::override_config {
+    ALLOWED_COBRANDS => ['fixmystreet', 'bromley', 'buckinghamshire', 'lewisham', 'oxfordshire'],
+  }, sub {
+    $p2->set_extra_metadata(customer_reference => 'ABC');
+    $p2->update;
+    $o->send;
+    $c2->discard_changes;
+    is $c2->send_fail_count, 1, 'Oxfordshire update tried to send, failed';
+  };
+};
+
+
+done_testing();