Merge branch 'send-reopen-comment-option'

18 files changed, 367 insertions, 193 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3805483db..8b71e3081 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@
     - Bugfixes:
         - Add perl 5.26/5.28 support.
         - Fix subcategory issues when visiting /report/new directly #2276
+    - 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..665f377bc 100755
--- a/bin/send-comments
+++ b/bin/send-comments
@@ -18,167 +18,12 @@ 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;
-
-# 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(
+    verbose => $verbose,
+);
+$updates->send;
diff --git a/perllib/FixMyStreet/Cobrand/Bromley.pm b/perllib/FixMyStreet/Cobrand/Bromley.pm
index a6d58504c..e1021eda9 100644
--- a/perllib/FixMyStreet/Cobrand/Bromley.pm
+++ b/perllib/FixMyStreet/Cobrand/Bromley.pm
@@ -165,5 +165,24 @@ sub open311_config {
     $params->{extended_description} = 0;
 }
 
+sub open311_config_updates {
+    my ($self, $params) = @_;
+    $params->{use_extended_updates} = 1;
+    $params->{endpoints} = {
+        service_request_updates => 'update.xml',
+        update => 'update.xml'
+    };
+}
+
+sub open311_pre_send {
+    my ($self, $row, $open311) = @_;
+
+    my $extra = $row->extra || {};
+    unless ( $extra->{title} ) {
+        $extra->{title} = $row->user->title;
+        $row->extra( $extra );
+    }
+}
+
 1;
 
diff --git a/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm b/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
index 783a565f1..fb074da85 100644
--- a/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Buckinghamshire.pm
@@ -100,6 +100,11 @@ sub open311_post_send {
     $sender->send($row, $h);
 }
 
+sub open311_config_updates {
+    my ($self, $params) = @_;
+    $params->{mark_reopen} = 1;
+}
+
 sub map_type { 'Buckinghamshire' }
 
 sub default_map_zoom { 3 }
diff --git a/perllib/FixMyStreet/Cobrand/Lewisham.pm b/perllib/FixMyStreet/Cobrand/Lewisham.pm
new file mode 100644
index 000000000..325f6e833
--- /dev/null
+++ b/perllib/FixMyStreet/Cobrand/Lewisham.pm
@@ -0,0 +1,15 @@
+package FixMyStreet::Cobrand::Lewisham;
+use base 'FixMyStreet::Cobrand::UK';
+
+use strict;
+use warnings;
+
+sub council_area_id { 2492 }
+
+sub open311_post_update_skip {
+    my ($self) = @_;
+    return 1;
+}
+
+1;
+
diff --git a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
index f03f1b605..4d627c756 100644
--- a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
@@ -171,6 +171,19 @@ sub open311_config {
     $params->{extended_description} = 'oxfordshire';
 }
 
+sub open311_config_updates {
+    my ($self, $params) = @_;
+    $params->{use_customer_reference} = 1;
+}
+
+sub should_skip_sending_update {
+    my ($self, $update ) = @_;
+
+    # Oxfordshire stores the external id of the problem as a customer reference
+    # in metadata
+    return 1 if !$update->problem->get_extra_metadata('customer_reference');
+}
+
 sub on_map_default_status { return 'open'; }
 
 sub contact_email {
diff --git a/perllib/FixMyStreet/DB/Result/Comment.pm b/perllib/FixMyStreet/DB/Result/Comment.pm
index b4e42b456..d27c5ea8c 100644
--- a/perllib/FixMyStreet/DB/Result/Comment.pm
+++ b/perllib/FixMyStreet/DB/Result/Comment.pm
@@ -115,6 +115,26 @@ my $stz = sub {
 around created => $stz;
 around confirmed => $stz;
 
+=head2 get_cobrand_logged
+
+Get a cobrand object for the cobrand the update was made on.
+
+e.g. if an update was logged at www.fixmystreet.com, this will be a
+FixMyStreet::Cobrand::FixMyStreet object.
+
+=cut
+
+has get_cobrand_logged => (
+    is => 'ro',
+    lazy => 1,
+    default => sub {
+        my $self = shift;
+        my $cobrand_class = FixMyStreet::Cobrand->get_class_for_moniker( $self->cobrand );
+        return $cobrand_class->new;
+    },
+);
+
+
 # You can replace this text with custom code or comments, and it will be preserved on regeneration
 
 sub check_for_errors {
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 76a6f72fb..2192158d3 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -780,7 +780,7 @@ sub duration_string {
 
 sub local_coords {
     my $self = shift;
-    my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($self->cobrand)->new;
+    my $cobrand = $self->get_cobrand_logged;
     if ($cobrand->moniker eq 'zurich') {
         my ($x, $y) = Geo::Coordinates::CH1903Plus::from_latlon($self->latitude, $self->longitude);
         return ( int($x+0.5), int($y+0.5) );
@@ -896,8 +896,9 @@ sub updates_sent_to_body {
     return unless $self->send_method_used && $self->send_method_used eq 'Open311';
 
     # Some bodies only send updates *to* FMS, they don't receive updates.
-    # NB See also the list in bin/send-comments
-    my $excluded = qr{Lewisham|Oxfordshire};
+    my $cobrand = $self->get_cobrand_logged;
+    my $handler = $cobrand->call_hook(get_body_handler_for_problem => $self);
+    return 0 if $handler && $handler->call_hook('open311_post_update_skip');
 
     my @bodies = values %{ $self->bodies };
     my @updates_sent = grep {
@@ -905,8 +906,7 @@ sub updates_sent_to_body {
         (
             $_->send_method eq 'Open311' ||
             $_->send_method eq 'Noop' # Sending might be temporarily disabled
-        ) &&
-        !($_->name =~ /$excluded/)
+        )
     } @bodies;
     return scalar @updates_sent;
 }
diff --git a/perllib/FixMyStreet/Script/Questionnaires.pm b/perllib/FixMyStreet/Script/Questionnaires.pm
index d89f1bcf8..bcdaa06d6 100644
--- a/perllib/FixMyStreet/Script/Questionnaires.pm
+++ b/perllib/FixMyStreet/Script/Questionnaires.pm
@@ -43,7 +43,7 @@ sub send_questionnaires_period {
 
     while (my $row = $unsent->next) {
 
-        my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($row->cobrand)->new();
+        my $cobrand = $row->get_cobrand_logged;
         $cobrand->set_lang_and_domain($row->lang, 1);
         FixMyStreet::Map::set_map_class($cobrand->map_type);
 
diff --git a/perllib/FixMyStreet/Script/Reports.pm b/perllib/FixMyStreet/Script/Reports.pm
index 87f9fd124..dd8f4e370 100644
--- a/perllib/FixMyStreet/Script/Reports.pm
+++ b/perllib/FixMyStreet/Script/Reports.pm
@@ -44,7 +44,7 @@ sub send(;$) {
     debug_print("starting to loop through unsent problem reports...") if $debug_mode;
     while (my $row = $unsent->next) {
 
-        my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($row->cobrand)->new();
+        my $cobrand = $row->get_cobrand_logged;
         FixMyStreet::DB->schema->cobrand($cobrand);
 
         if ($debug_mode) {
diff --git a/perllib/FixMyStreet/SendReport/Email.pm b/perllib/FixMyStreet/SendReport/Email.pm
index 4307694a1..1b484aefe 100644
--- a/perllib/FixMyStreet/SendReport/Email.pm
+++ b/perllib/FixMyStreet/SendReport/Email.pm
@@ -71,7 +71,7 @@ sub send {
     }
 
     my ($verbose, $nomail) = CronFns::options();
-    my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($row->cobrand)->new();
+    my $cobrand = $row->get_cobrand_logged;
     $cobrand = $cobrand->call_hook(get_body_handler_for_problem => $row) || $cobrand;
 
     my $params = {
diff --git a/perllib/Open311.pm b/perllib/Open311.pm
index 6c6ec59cc..56fe567c5 100644
--- a/perllib/Open311.pm
+++ b/perllib/Open311.pm
@@ -397,7 +397,7 @@ sub _populate_service_request_update_params {
     }
 
     if ( $comment->photo ) {
-        my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($comment->cobrand)->new();
+        my $cobrand = $comment->get_cobrand_logged;
         my $email_base_url = $cobrand->base_url($comment->cobrand_data);
         my $url = $email_base_url . $comment->photos->[0]->{url_full};
         $params->{media_url} = $url;
diff --git a/perllib/Open311/GetServiceRequestUpdates.pm b/perllib/Open311/GetServiceRequestUpdates.pm
index bef1aca67..c6da37790 100644
--- a/perllib/Open311/GetServiceRequestUpdates.pm
+++ b/perllib/Open311/GetServiceRequestUpdates.pm
@@ -18,7 +18,7 @@ Readonly::Scalar my $AREA_ID_BROMLEY     => 2482;
 Readonly::Scalar my $AREA_ID_OXFORDSHIRE => 2237;
 
 sub fetch {
-    my $self = shift;
+    my ($self, $open311) = @_;
 
     my $bodies = $self->schema->resultset('Body')->search(
         {
@@ -31,25 +31,23 @@ sub fetch {
 
     while ( my $body = $bodies->next ) {
 
-        my $o = Open311->new(
-            endpoint          => $body->endpoint,
-            api_key           => $body->api_key,
-            jurisdiction      => $body->jurisdiction,
+        my %open311_conf = (
+            endpoint => $body->endpoint,
+            api_key => $body->api_key,
+            jurisdiction => $body->jurisdiction,
             extended_statuses => $body->send_extended_statuses,
         );
 
-        # custom endpoint URLs because these councils have non-standard paths
-        if ( $body->areas->{$AREA_ID_BROMLEY} ) {
-            my $endpoints = $o->endpoints;
-            $endpoints->{update} = 'update.xml';
-            $endpoints->{service_request_updates} = 'update.xml';
-            $o->endpoints( $endpoints );
-        }
+        my $cobrand = $body->get_cobrand_handler;
+        $cobrand->call_hook(open311_config_updates => \%open311_conf)
+            if $cobrand;
+
+        $open311 //= Open311->new(%open311_conf);
 
         $self->suppress_alerts( $body->suppress_alerts );
         $self->blank_updates_permitted( $body->blank_updates_permitted );
         $self->system_user( $body->comment_user );
-        $self->update_comments( $o, $body );
+        $self->update_comments( $open311, $body );
     }
 }
 
diff --git a/perllib/Open311/PostServiceRequestUpdates.pm b/perllib/Open311/PostServiceRequestUpdates.pm
new file mode 100755
index 000000000..63e149ece
--- /dev/null
+++ b/perllib/Open311/PostServiceRequestUpdates.pm
@@ -0,0 +1,128 @@
+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';
+
+has verbose => ( is => 'ro', default => 0 );
+
+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 ) {
+        my $cobrand = $body->get_cobrand_handler;
+        next if $cobrand && $cobrand->call_hook('open311_post_update_skip');
+        $self->process_body($body);
+    }
+}
+
+sub open311_params {
+    my ($self, $body) = @_;
+
+    my %open311_conf = (
+        endpoint => $body->endpoint,
+        jurisdiction => $body->jurisdiction,
+        api_key => $body->api_key,
+        extended_statuses => $body->send_extended_statuses,
+    );
+
+    my $cobrand = $body->get_cobrand_handler;
+    $cobrand->call_hook(open311_config_updates => \%open311_conf)
+        if $cobrand;
+
+    return %open311_conf;
+}
+
+sub process_body {
+    my ($self, $body) = @_;
+
+    my $o = Open311->new( $self->open311_params($body) );
+
+    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 || $comment->get_cobrand_logged;
+
+        # 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;
+        }
+
+        next if !$self->verbose && $comment->send_fail_count && retry_timeout($comment);
+
+        $self->process_update($body, $o, $comment, $cobrand);
+    }
+}
+
+sub process_update {
+    my ($self, $body, $o, $comment, $cobrand) = @_;
+
+    $cobrand->call_hook(open311_pre_send => $comment, $o);
+
+    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/app/controller/admin.t b/t/app/controller/admin.t
index b69a711c8..a0a93d996 100644
--- a/t/app/controller/admin.t
+++ b/t/app/controller/admin.t
@@ -129,7 +129,7 @@ subtest 'check summary counts' => sub {
 
 subtest "Check admin_base_url" => sub {
     my $rs = FixMyStreet::App->model('DB::Problem');
-    my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($report->cobrand)->new();
+    my $cobrand = $report->get_cobrand_logged;
 
     is ($report->admin_url($cobrand),
         (sprintf 'http://www.example.org/admin/report_edit/%d', $report_id),
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/getservicerequestupdates.t b/t/open311/getservicerequestupdates.t
index 2f7917946..7ebab3b3f 100644
--- a/t/open311/getservicerequestupdates.t
+++ b/t/open311/getservicerequestupdates.t
@@ -21,8 +21,15 @@ my $user = FixMyStreet::DB->resultset('User')->find_or_create(
 
 my %bodies = (
     2237 => FixMyStreet::DB->resultset("Body")->create({ name => 'Oxfordshire' }),
-    2482 => FixMyStreet::DB->resultset("Body")->create({ name=> 'Bromley', id => 2482 }),
-    2651 => FixMyStreet::DB->resultset("Body")->new({ id => 2651 }),
+    2482 => FixMyStreet::DB->resultset("Body")->create({
+        name => 'Bromley',
+        send_method => 'Open311',
+        send_comments => 1,
+        endpoint => 'endpoint',
+        comment_user_id => $user->id,
+        blank_updates_permitted => 1,
+    }),
+    2651 => FixMyStreet::DB->resultset("Body")->new({ id => 123 }),
 );
 $bodies{2237}->body_areas->create({ area_id => 2237 });
 
@@ -141,7 +148,7 @@ my $problem = $problem_rs->new(
         lastupdate   => DateTime->now()->subtract( days => 1 ),
         anonymous    => 1,
         external_id  => time(),
-        bodies_str   => 2482,
+        bodies_str   => $bodies{2482}->id,
     }
 );
 
@@ -421,11 +428,8 @@ for my $test (
         $problem->state( $test->{start_state} );
         $problem->update;
 
-        my $update = Open311::GetServiceRequestUpdates->new(
-            system_user => $user,
-            blank_updates_permitted => 1,
-        );
-        $update->update_comments( $o, $bodies{2482} );
+        my $update = Open311::GetServiceRequestUpdates->new;
+        $update->fetch($o);
 
         is $problem->comments->count, 1, 'comment count';
         $problem->discard_changes;
@@ -580,7 +584,7 @@ my $problem2 = $problem_rs->create(
         lastupdate   => DateTime->now(),
         anonymous    => 1,
         external_id  => $problem->external_id,
-        bodies_str   => 2651,
+        bodies_str   => $bodies{2651}->id,
     }
 );
 
diff --git a/t/open311/post-service-request-updates.t b/t/open311/post-service-request-updates.t
new file mode 100644
index 000000000..5c8211bbf
--- /dev/null
+++ b/t/open311/post-service-request-updates.t
@@ -0,0 +1,107 @@
+#!/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 {
+  FixMyStreet::override_config {
+    ALLOWED_COBRANDS => ['fixmystreet', 'bromley', 'buckinghamshire', 'lewisham', 'oxfordshire'],
+  }, sub {
+    my $result = {
+        endpoint => 'endpoint',
+        jurisdiction => 'home',
+        api_key => 'KEY',
+        extended_statuses => undef,
+    };
+    my %conf = $o->open311_params($bromley);
+    is_deeply \%conf, {
+        %$result,
+        extended_statuses => 1,
+        use_extended_updates => 1,
+        endpoints => { service_request_updates => 'update.xml', update => 'update.xml' },
+    }, '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();