Rewrite open311-update-reports to share code.

Make GetUpdates and GetServiceRequestUpdates share a common base;
spot all visible states.

10 files changed, 384 insertions, 465 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e77fcfa42..b5e42de4e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -32,6 +32,7 @@
         - Add cobrand hook to specify custom domain for VERP emails.
     - Open311 improvements:
         - Use devolved data on update sending.
+        - Rewrite open311-update-reports to share code and improve functionality.
     - UK:
         - Add option for recaptcha. #3050
 
diff --git a/bin/open311-update-reports b/bin/open311-update-reports
index 2d384b813..b9b4d594d 100755
--- a/bin/open311-update-reports
+++ b/bin/open311-update-reports
@@ -8,6 +8,7 @@
 
 use strict;
 use warnings;
+use v5.14;
 
 BEGIN {
     use File::Basename qw(dirname);
@@ -16,21 +17,21 @@ BEGIN {
     require "$d/../setenv.pl";
 }
 
+use Getopt::Long::Descriptive;
 use Open311::GetUpdates;
-use FixMyStreet;
-use FixMyStreet::DB;
 
-# FIXME - make this configurable and/or better
-my $system_user = FixMyStreet::DB->resultset('User')->find_or_create(
-    {
-        email => FixMyStreet->config('CONTACT_EMAIL'),
-        name => 'System User',
-    }
+my ($opts, $usage) = describe_options(
+    '%c %o',
+    ['body|b:s', 'body name to only fetch this body' ],
+    ['verbose|v', 'more verbose output'],
+    ['help|h', "print usage message and exit" ],
 );
+$usage->die if $opts->help;
 
-my $body_list = FixMyStreet::DB->resultset('Body');
+my %params = (
+    verbose => $opts->verbose,
+    body => $opts->body,
+);
 
-my $update = Open311::GetUpdates->new(
-    body_list => $body_list,
-    system_user  => $system_user
-)->get_updates;
+my $updates = Open311::GetUpdates->new(%params);
+$updates->fetch;
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 42983c9ad..0653de32b 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -838,87 +838,6 @@ sub local_coords {
     }
 }
 
-=head2 update_from_open311_service_request
-
-    $p->update_from_open311_service_request( $request, $body, $system_user );
-
-Updates the problem based on information in the passed in open311 request
-(standard, not the extension that uses GetServiceRequestUpdates) . If the
-request has an older update time than the problem's lastupdate time then
-nothing happens.
-
-Otherwise a comment will be created if there is status update text in the
-open311 request. If the open311 request has a state of closed then the problem
-will be marked as fixed.
-
-NB: a comment will always be created if the problem is being marked as fixed.
-
-Fixed problems will not be re-opened by this method.
-
-=cut
-
-sub update_from_open311_service_request {
-    my ( $self, $request, $body, $system_user ) = @_;
-
-    my ( $updated, $status_notes );
-
-    if ( ! ref $request->{updated_datetime} ) {
-        $updated = $request->{updated_datetime};
-    }
-
-    if ( ! ref $request->{status_notes} ) {
-        $status_notes = $request->{status_notes};
-    }
-
-    my $update = $self->new_related(comments => {
-        state => 'confirmed',
-        created => $updated || \'current_timestamp',
-        confirmed => \'current_timestamp',
-        text => $status_notes,
-        mark_open => 0,
-        mark_fixed => 0,
-        user => $system_user,
-        anonymous => 0,
-        name => $body->name,
-    });
-
-    my $w3c = DateTime::Format::W3CDTF->new;
-    my $req_time = $w3c->parse_datetime( $request->{updated_datetime} );
-
-    # set a timezone here as the $req_time will have one and if we don't
-    # use a timezone then the date comparisons are invalid.
-    # of course if local timezone is not the one that went into the data
-    # base then we're also in trouble
-    my $lastupdate = $self->lastupdate;
-    $lastupdate->set_time_zone( FixMyStreet->local_time_zone );
-
-    # update from open311 is older so skip
-    if ( $req_time < $lastupdate ) {
-        return 0;
-    }
-
-    if ( $request->{status} eq 'closed' ) {
-        if ( $self->state ne 'fixed' ) {
-            $self->state('fixed');
-            $update->mark_fixed(1);
-
-            if ( !$status_notes ) {
-                # FIXME - better text here
-                $status_notes = _('Closed by council');
-            }
-        }
-    }
-
-    if ( $status_notes ) {
-        $update->text( $status_notes );
-        $self->lastupdate( $req_time );
-        $self->update;
-        $update->insert;
-    }
-
-    return 1;
-}
-
 sub update_send_failed {
     my $self = shift;
     my $msg  = shift;
diff --git a/perllib/Open311.pm b/perllib/Open311.pm
index 00e4bae9b..41cd0f1e0 100644
--- a/perllib/Open311.pm
+++ b/perllib/Open311.pm
@@ -14,6 +14,7 @@ use FixMyStreet::Cobrand;
 use FixMyStreet::DB;
 use Utils;
 use Path::Tiny 'path';
+use FixMyStreet::App::Model::PhotoSet;
 
 has jurisdiction => ( is => 'ro', isa => Str );;
 has api_key => ( is => 'ro', isa => Str );
@@ -277,7 +278,8 @@ sub get_service_requests {
     };
 
     my $service_request_xml = $self->_get( $self->endpoints->{requests}, $params || undef );
-    return $self->_get_xml_object( $service_request_xml );
+    my $requests = $self->_get_xml_object( $service_request_xml );
+    return $requests->{request};
 }
 
 sub get_service_request_id_from_token {
diff --git a/perllib/Open311/GetServiceRequestUpdates.pm b/perllib/Open311/GetServiceRequestUpdates.pm
index 9fa81ac9e..b4f3f4430 100644
--- a/perllib/Open311/GetServiceRequestUpdates.pm
+++ b/perllib/Open311/GetServiceRequestUpdates.pm
@@ -1,92 +1,17 @@
 package Open311::GetServiceRequestUpdates;
 
 use Moo;
-use Open311;
-use Parallel::ForkManager;
-use FixMyStreet::DB;
-use FixMyStreet::App::Model::PhotoSet;
+extends 'Open311::UpdatesBase';
+
 use DateTime::Format::W3CDTF;
 
-has system_user => ( is => 'rw' );
+has '+send_comments_flag' => ( default => 1 );
 has start_date => ( is => 'ro', default => sub { undef } );
 has end_date => ( is => 'ro', default => sub { undef } );
-has body => ( is => 'ro', default => sub { undef } );
-has suppress_alerts => ( is => 'rw', default => 0 );
-has verbose => ( is => 'ro', default => 0 );
-has schema => ( is =>'ro', lazy => 1, default => sub { FixMyStreet::DB->schema->connect } );
-has blank_updates_permitted => ( is => 'rw', default => 0 );
-
-has current_body => ( is => 'rw' );
-has current_open311 => ( is => 'rw' );
 
 Readonly::Scalar my $AREA_ID_BROMLEY     => 2482;
 Readonly::Scalar my $AREA_ID_OXFORDSHIRE => 2237;
 
-sub fetch {
-    my ($self, $open311) = @_;
-
-    my $bodies = $self->schema->resultset('Body')->search(
-        {
-            send_method     => 'Open311',
-            send_comments   => 1,
-            comment_user_id => { '!=', undef },
-            endpoint        => { '!=', '' },
-        }
-    );
-
-    if ( $self->body ) {
-        $bodies = $bodies->search( { name => $self->body } );
-    }
-
-    my $procs_min = FixMyStreet->config('FETCH_COMMENTS_PROCESSES_MIN') || 0;
-    my $procs_max = FixMyStreet->config('FETCH_COMMENTS_PROCESSES_MAX');
-    my $procs_timeout = FixMyStreet->config('FETCH_COMMENTS_PROCESS_TIMEOUT');
-
-    my $pm = Parallel::ForkManager->new(FixMyStreet->test_mode ? 0 : $procs_min);
-
-    if ($procs_max && $procs_timeout) {
-        my %workers;
-        $pm->run_on_wait(sub {
-            while (my ($pid, $started_at) = each %workers) {
-                next unless time() - $started_at > $procs_timeout;
-                next if $pm->max_procs == $procs_max;
-                $pm->set_max_procs($pm->max_procs + 1);
-                delete $workers{$pid}; # Only want to increase once per long-running thing
-            }
-        }, 1);
-        $pm->run_on_start(sub { my $pid = shift; $workers{$pid} = time(); });
-        $pm->run_on_finish(sub { my $pid = shift; delete $workers{$pid}; });
-    }
-
-    while ( my $body = $bodies->next ) {
-        $pm->start and next;
-
-        $self->current_body( $body );
-
-        my %open311_conf = (
-            endpoint => $body->endpoint,
-            api_key => $body->api_key,
-            jurisdiction => $body->jurisdiction,
-            extended_statuses => $body->send_extended_statuses,
-        );
-
-        my $cobrand = $body->get_cobrand_handler;
-        $cobrand->call_hook(open311_config_updates => \%open311_conf)
-            if $cobrand;
-
-        $self->current_open311( $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->process_body();
-
-        $pm->finish;
-    }
-
-    $pm->wait_all_children;
-}
-
 sub parse_dates {
     my $self = shift;
     my $body = $self->current_body;
@@ -141,201 +66,12 @@ sub process_body {
     return 1;
 }
 
-sub check_date {
-    my ($self, $request, @args) = @_;
-
-    my $comment_time = eval {
-        DateTime::Format::W3CDTF->parse_datetime( $request->{updated_datetime} || "" )
-            ->set_time_zone(FixMyStreet->local_time_zone);
-    };
-    return if $@;
-    my $updated = DateTime::Format::W3CDTF->format_datetime($comment_time->clone->set_time_zone('UTC'));
-    return if @args && ($updated lt $args[0] || $updated gt $args[1]);
-    $request->{comment_time} = $comment_time;
-    return 1;
-}
-
-sub find_problem {
-    my ($self, $request, @args) = @_;
-
-    $self->check_date($request, @args) or return;
-
-    my $body = $self->current_body;
-    my $request_id = $request->{service_request_id};
-
-    # If there's no request id then we can't work out
-    # what problem it belongs to so just skip
-    return unless $request_id || $request->{fixmystreet_id};
-
-    my $problem;
-    my $criteria = {
-        external_id => $request_id,
-    };
-
-    # in some cases we only have the FMS id and not the request id so use that
-    if ( $request->{fixmystreet_id} ) {
-        unless ( $request->{fixmystreet_id} =~ /^\d+$/ ) {
-            warn "skipping bad fixmystreet id in updates for " . $body->name . ": [" . $request->{fixmystreet_id} . "], external id is $request_id\n";
-            return;
-        }
-
-        $criteria = {
-            id => $request->{fixmystreet_id},
-        };
-    }
-
-    $problem = $self->schema->resultset('Problem')->to_body($body)->search( $criteria );
+sub _find_problem {
+    my ($self, $criteria) = @_;
+    my $problem = $self->schema->resultset('Problem')
+        ->to_body($self->current_body)
+        ->search( $criteria );
     return $problem->first;
 }
 
-sub process_update {
-    my ($self, $request, $p) = @_;
-    my $open311 = $self->current_open311;
-    my $body = $self->current_body;
-
-    my $state = $open311->map_state( $request->{status} );
-    my $old_state = $p->state;
-    my $external_status_code = $request->{external_status_code} || '';
-    my $customer_reference = $request->{customer_reference} || '';
-    my $old_external_status_code = $p->get_extra_metadata('external_status_code') || '';
-    my $comment = $self->schema->resultset('Comment')->new(
-        {
-            problem => $p,
-            user => $self->system_user,
-            external_id => $request->{update_id},
-            text => $self->comment_text_for_request(
-                $request, $p, $state, $old_state,
-                $external_status_code, $old_external_status_code
-            ),
-            mark_fixed => 0,
-            mark_open => 0,
-            anonymous => 0,
-            name => $self->system_user->name,
-            confirmed => $request->{comment_time},
-            created => $request->{comment_time},
-            state => 'confirmed',
-        }
-    );
-
-    # Some Open311 services, e.g. Confirm via open311-adapter, provide
-    # a more fine-grained status code that we use within FMS for
-    # response templates.
-    if ( $external_status_code ) {
-        $comment->set_extra_metadata(external_status_code => $external_status_code);
-        $p->set_extra_metadata(external_status_code => $external_status_code);
-    } else {
-        $p->set_extra_metadata(external_status_code => '');
-    }
-
-    # if the customer reference to display in the report metadata is
-    # not the same as the external_id
-    if ( $customer_reference ) {
-        $p->set_extra_metadata( customer_reference => $customer_reference );
-    }
-
-    $open311->add_media($request->{media_url}, $comment)
-        if $request->{media_url};
-
-    # don't update state unless it's an allowed state
-    if ( FixMyStreet::DB::Result::Problem->visible_states()->{$state} &&
-        # For Oxfordshire, don't allow changes back to Open from other open states
-        !( $body->areas->{$AREA_ID_OXFORDSHIRE} && $state eq 'confirmed' && $p->is_open ) &&
-        # Don't let it change between the (same in the front end) fixed states
-        !( $p->is_fixed && FixMyStreet::DB::Result::Problem->fixed_states()->{$state} ) ) {
-
-        $comment->problem_state($state);
-
-        # we only want to update the problem state if that makes sense. We never want to unhide a problem.
-        # If the update is older than the last update then we also do not want to update the state. This
-        # is largely to avoid the situation where we miss some updates, make more updates and then catch
-        # the updates when we fetch the last 24 hours of updates. The exception to this is the first
-        # comment. This is to catch automated updates which happen faster than we get the external_id
-        # back from the endpoint and hence have an created time before the lastupdate.
-        if ( $p->is_visible && $p->state ne $state &&
-            ( $comment->created >= $p->lastupdate || $p->comments->count == 0 ) ) {
-            $p->state($state);
-        }
-    }
-
-    # If nothing to show (no text, photo, or state change), don't show this update
-    $comment->state('hidden') unless $comment->text || $comment->photo
-        || ($comment->problem_state && $state ne $old_state);
-
-    my $cobrand = $body->get_cobrand_handler;
-    $cobrand->call_hook(open311_get_update_munging => $comment)
-        if $cobrand;
-
-    # As comment->created has been looked at above, its time zone has been shifted
-    # to TIME_ZONE (if set). We therefore need to set it back to local before
-    # insertion. We also then need a clone, otherwise the setting of lastupdate
-    # will *also* reshift comment->created's time zone to TIME_ZONE.
-    my $created = $comment->created->set_time_zone(FixMyStreet->local_time_zone);
-    $p->lastupdate($created->clone);
-    $p->update;
-    $comment->insert();
-
-    if ( $self->suppress_alerts ) {
-        my @alerts = $self->schema->resultset('Alert')->search( {
-            alert_type => 'new_updates',
-            parameter  => $p->id,
-            confirmed  => 1,
-            user_id    => $p->user->id,
-        } );
-
-        for my $alert (@alerts) {
-            my $alerts_sent = $self->schema->resultset('AlertSent')->find_or_create( {
-                alert_id  => $alert->id,
-                parameter => $comment->id,
-            } );
-        }
-    }
-
-    return $comment;
-}
-
-sub comment_text_for_request {
-    my ($self, $request, $problem, $state, $old_state,
-        $ext_code, $old_ext_code) = @_;
-
-    # Response templates are only triggered if the state/external status has changed.
-    # And treat any fixed state as fixed.
-    my $state_changed = $state ne $old_state
-        && !( $problem->is_fixed && FixMyStreet::DB::Result::Problem->fixed_states()->{$state} );
-    my $ext_code_changed = $ext_code ne $old_ext_code;
-    my $template;
-    if ($state_changed || $ext_code_changed) {
-        my $order;
-        my $state_params = {
-            'me.state' => $state
-        };
-        if ($ext_code) {
-            $state_params->{'me.external_status_code'} = $ext_code;
-            # make sure that empty string/nulls come last.
-            $order = { order_by => \"me.external_status_code DESC NULLS LAST" };
-        };
-
-        if (my $t = $problem->response_templates->search({
-            auto_response => 1,
-            -or => $state_params,
-        }, $order )->first) {
-            $template = $t->text;
-        }
-    }
-
-    my $desc = $request->{description} || '';
-    if ($desc && (!$template || $template !~ /\{\{description}}/)) {
-        return $desc;
-    }
-
-    if ($template) {
-        $template =~ s/\{\{description}}/$desc/;
-        return $template;
-    }
-
-    return "" if $self->blank_updates_permitted;
-
-    print STDERR "Couldn't determine update text for $request->{update_id} (report " . $problem->id . ")\n";
-    return "";
-}
-
 1;
diff --git a/perllib/Open311/GetServiceRequests.pm b/perllib/Open311/GetServiceRequests.pm
index e5fd6438e..2545f6f29 100644
--- a/perllib/Open311/GetServiceRequests.pm
+++ b/perllib/Open311/GetServiceRequests.pm
@@ -4,7 +4,6 @@ use Moo;
 use Open311;
 use FixMyStreet::DB;
 use FixMyStreet::MapIt;
-use FixMyStreet::App::Model::PhotoSet;
 use DateTime::Format::W3CDTF;
 
 has system_user => ( is => 'rw' );
@@ -85,7 +84,7 @@ sub create_problems {
         ->active
         ->search( { body_id => $body->id } );
 
-    for my $request (@{$requests->{request}}) {
+    for my $request (@$requests) {
         # no point importing if we can't put it on the map
         unless ($request->{service_request_id} && $request->{lat} && $request->{long}) {
             warn "Not creating request '$request->{description}' for @{[$body->name]} as missing one of id, lat or long"
diff --git a/perllib/Open311/GetUpdates.pm b/perllib/Open311/GetUpdates.pm
index f62acf4a8..352f2f218 100644
--- a/perllib/Open311/GetUpdates.pm
+++ b/perllib/Open311/GetUpdates.pm
@@ -1,67 +1,53 @@
 package Open311::GetUpdates;
 
 use Moo;
-use Open311;
-use FixMyStreet::Cobrand;
-
-has body_list => ( is => 'ro' );
-has system_user => ( is => 'ro' );
+extends 'Open311::UpdatesBase';
 
-sub get_updates {
-    my $self = shift;
+use Open311;
 
-    while ( my $body = $self->body_list->next ) {
-        my $open311 = Open311->new(
-            endpoint     => $body->endpoint,
-            jurisdiction => $body->jurisdiction,
-            api_key      => $body->api_key
-        );
+has '+send_comments_flag' => ( default => 0 );
+has ext_to_int_map => ( is => 'rw' );
 
-        my $reports = $body->result_source->schema->resultset('Problem')->to_body($body)->search(
-            {
-                state => { 'IN', [qw/confirmed fixed/] },
-                -and => [
-                    external_id => { '!=', undef },
-                    external_id => { '!=', '' },
-                ],
-            }
-        );
+has report_criteria => ( is => 'ro', default => sub { {
+        state => [ FixMyStreet::DB::Result::Problem->visible_states() ],
+        external_id => { '!=', '' },
+    } } );
 
-        my @report_ids = ();
-        while ( my $report = $reports->next ) {
-            push @report_ids, $report->external_id;
-        }
+sub process_body {
+    my ($self) = @_;
 
-        next unless @report_ids;
+    my $reports = $self->schema->resultset('Problem')
+        ->to_body($self->current_body)
+        ->search($self->report_criteria);
 
-        $self->update_reports( \@report_ids, $open311, $body );
-    }
+    my @reports = $reports->all;
+    $self->update_reports(\@reports);
 }
 
 sub update_reports {
-    my ( $self, $report_ids, $open311, $body ) = @_;
+    my ( $self, $reports ) = @_;
+    return unless @$reports;
 
-    my $service_requests = $open311->get_service_requests( { report_ids => $report_ids } );
-    my $requests = $service_requests->{request};
+    my $requests = $self->current_open311->get_service_requests( {
+        report_ids => [ map { $_->external_id } @$reports ],
+    } );
 
+    $self->ext_to_int_map({ map { $_->external_id => $_ } @$reports });
     for my $request (@$requests) {
-        # if there's no updated date then we can't
-        # tell if it's newer than what we have so we should skip it
-        next unless $request->{updated_datetime};
-
-        my $request_id = $request->{service_request_id};
+        $request->{description} = $request->{status_notes};
 
-        my $problem = $body->result_source->schema->resultset('Problem')
-          ->search( { external_id => $request_id, } );
+        my $p = $self->find_problem($request) or next;
+        next if $request->{comment_time} < $p->lastupdate;
+        # But what if update at our end later than update their end...
 
-        if (my $p = $problem->first) {
-            my $cobrand = FixMyStreet::Cobrand->get_class_for_moniker($p->cobrand)->new();
-            $cobrand->set_lang_and_domain($p->lang, 1, FixMyStreet->path_to('locale')->stringify );
-            $p->update_from_open311_service_request( $request, $body, $self->system_user );
-        }
+        $self->process_update($request, $p);
     }
+}
 
-    return 1;
+sub _find_problem {
+    my ($self, $criteria) = @_;
+    my $problem = $self->ext_to_int_map->{$criteria->{external_id}};
+    return $problem;
 }
 
 1;
diff --git a/perllib/Open311/UpdatesBase.pm b/perllib/Open311/UpdatesBase.pm
new file mode 100644
index 000000000..c6ad3277a
--- /dev/null
+++ b/perllib/Open311/UpdatesBase.pm
@@ -0,0 +1,281 @@
+package Open311::UpdatesBase;
+
+use Moo;
+use Open311;
+use Parallel::ForkManager;
+use FixMyStreet::DB;
+
+has send_comments_flag => ( is => 'ro' );
+
+has system_user => ( is => 'rw' );
+has body => ( is => 'ro', default => sub { undef } );
+has verbose => ( is => 'ro', default => 0 );
+has schema => ( is =>'ro', lazy => 1, default => sub { FixMyStreet::DB->schema->connect } );
+has suppress_alerts => ( is => 'rw', default => 0 );
+has blank_updates_permitted => ( is => 'rw', default => 0 );
+
+has current_body => ( is => 'rw' );
+has current_open311 => ( is => 'rw' );
+
+Readonly::Scalar my $AREA_ID_OXFORDSHIRE => 2237;
+
+sub fetch {
+    my ($self, $open311) = @_;
+
+    my $bodies = $self->schema->resultset('Body')->search(
+        {
+            send_method     => 'Open311',
+            send_comments   => $self->send_comments_flag,
+            comment_user_id => { '!=', undef },
+            endpoint        => { '!=', '' },
+        }
+    );
+
+    if ( $self->body ) {
+        $bodies = $bodies->search( { name => $self->body } );
+    }
+
+    my $procs_min = FixMyStreet->config('FETCH_COMMENTS_PROCESSES_MIN') || 0;
+    my $procs_max = FixMyStreet->config('FETCH_COMMENTS_PROCESSES_MAX');
+    my $procs_timeout = FixMyStreet->config('FETCH_COMMENTS_PROCESS_TIMEOUT');
+
+    my $pm = Parallel::ForkManager->new(FixMyStreet->test_mode ? 0 : $procs_min);
+
+    if ($procs_max && $procs_timeout) {
+        my %workers;
+        $pm->run_on_wait(sub {
+            while (my ($pid, $started_at) = each %workers) {
+                next unless time() - $started_at > $procs_timeout;
+                next if $pm->max_procs == $procs_max;
+                $pm->set_max_procs($pm->max_procs + 1);
+                delete $workers{$pid}; # Only want to increase once per long-running thing
+            }
+        }, 1);
+        $pm->run_on_start(sub { my $pid = shift; $workers{$pid} = time(); });
+        $pm->run_on_finish(sub { my $pid = shift; delete $workers{$pid}; });
+    }
+
+    while ( my $body = $bodies->next ) {
+        $pm->start and next;
+
+        $self->current_body( $body );
+
+        my %open311_conf = (
+            endpoint => $body->endpoint,
+            api_key => $body->api_key,
+            jurisdiction => $body->jurisdiction,
+            extended_statuses => $body->send_extended_statuses,
+        );
+
+        my $cobrand = $body->get_cobrand_handler;
+        $cobrand->call_hook(open311_config_updates => \%open311_conf)
+            if $cobrand;
+
+        $self->current_open311( $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->process_body();
+
+        $pm->finish;
+    }
+
+    $pm->wait_all_children;
+}
+
+sub check_date {
+    my ($self, $request, @args) = @_;
+
+    my $comment_time = eval {
+        DateTime::Format::W3CDTF->parse_datetime( $request->{updated_datetime} || "" )
+            ->set_time_zone(FixMyStreet->local_time_zone);
+    };
+    return if $@;
+    my $updated = DateTime::Format::W3CDTF->format_datetime($comment_time->clone->set_time_zone('UTC'));
+    return if @args && ($updated lt $args[0] || $updated gt $args[1]);
+    $request->{comment_time} = $comment_time;
+    return 1;
+}
+
+sub find_problem {
+    my ($self, $request, @args) = @_;
+
+    $self->check_date($request, @args) or return;
+
+    my $request_id = $request->{service_request_id};
+
+    # If there's no request id then we can't work out
+    # what problem it belongs to so just skip
+    return unless $request_id || $request->{fixmystreet_id};
+
+    my $criteria = {
+        external_id => $request_id,
+    };
+
+    # in some cases we only have the FMS id and not the request id so use that
+    if ( $request->{fixmystreet_id} ) {
+        unless ( $request->{fixmystreet_id} =~ /^\d+$/ ) {
+            warn "skipping bad fixmystreet id in updates for " . $self->current_body->name . ": [" . $request->{fixmystreet_id} . "], external id is $request_id\n";
+            return;
+        }
+
+        $criteria = {
+            id => $request->{fixmystreet_id},
+        };
+    }
+
+    return $self->_find_problem($criteria);
+}
+
+sub process_update {
+    my ($self, $request, $p) = @_;
+    my $open311 = $self->current_open311;
+    my $body = $self->current_body;
+
+    my $state = $open311->map_state( $request->{status} );
+    my $old_state = $p->state;
+    my $external_status_code = $request->{external_status_code} || '';
+    my $customer_reference = $request->{customer_reference} || '';
+    my $old_external_status_code = $p->get_extra_metadata('external_status_code') || '';
+    my $comment = $self->schema->resultset('Comment')->new(
+        {
+            problem => $p,
+            user => $self->system_user,
+            external_id => $request->{update_id},
+            text => $self->comment_text_for_request(
+                $request, $p, $state, $old_state,
+                $external_status_code, $old_external_status_code
+            ),
+            mark_fixed => 0,
+            mark_open => 0,
+            anonymous => 0,
+            name => $self->system_user->name,
+            confirmed => $request->{comment_time},
+            created => $request->{comment_time},
+            state => 'confirmed',
+        }
+    );
+
+    # Some Open311 services, e.g. Confirm via open311-adapter, provide
+    # a more fine-grained status code that we use within FMS for
+    # response templates.
+    if ( $external_status_code ) {
+        $comment->set_extra_metadata(external_status_code => $external_status_code);
+        $p->set_extra_metadata(external_status_code => $external_status_code);
+    } else {
+        $p->set_extra_metadata(external_status_code => '');
+    }
+
+    # if the customer reference to display in the report metadata is
+    # not the same as the external_id
+    if ( $customer_reference ) {
+        $p->set_extra_metadata( customer_reference => $customer_reference );
+    }
+
+    $open311->add_media($request->{media_url}, $comment)
+        if $request->{media_url};
+
+    # don't update state unless it's an allowed state
+    if ( FixMyStreet::DB::Result::Problem->visible_states()->{$state} &&
+        # For Oxfordshire, don't allow changes back to Open from other open states
+        !( $body->areas->{$AREA_ID_OXFORDSHIRE} && $state eq 'confirmed' && $p->is_open ) &&
+        # Don't let it change between the (same in the front end) fixed states
+        !( $p->is_fixed && FixMyStreet::DB::Result::Problem->fixed_states()->{$state} ) ) {
+
+        $comment->problem_state($state);
+
+        # we only want to update the problem state if that makes sense. We never want to unhide a problem.
+        # If the update is older than the last update then we also do not want to update the state. This
+        # is largely to avoid the situation where we miss some updates, make more updates and then catch
+        # the updates when we fetch the last 24 hours of updates. The exception to this is the first
+        # comment. This is to catch automated updates which happen faster than we get the external_id
+        # back from the endpoint and hence have an created time before the lastupdate.
+        if ( $p->is_visible && $p->state ne $state &&
+            ( $comment->created >= $p->lastupdate || $p->comments->count == 0 ) ) {
+            $p->state($state);
+        }
+    }
+
+    # If nothing to show (no text, photo, or state change), don't show this update
+    $comment->state('hidden') unless $comment->text || $comment->photo
+        || ($comment->problem_state && $state ne $old_state);
+
+    my $cobrand = $body->get_cobrand_handler;
+    $cobrand->call_hook(open311_get_update_munging => $comment)
+        if $cobrand;
+
+    # As comment->created has been looked at above, its time zone has been shifted
+    # to TIME_ZONE (if set). We therefore need to set it back to local before
+    # insertion. We also then need a clone, otherwise the setting of lastupdate
+    # will *also* reshift comment->created's time zone to TIME_ZONE.
+    my $created = $comment->created->set_time_zone(FixMyStreet->local_time_zone);
+    $p->lastupdate($created->clone);
+    $p->update;
+    $comment->insert();
+
+    if ( $self->suppress_alerts ) {
+        my @alerts = $self->schema->resultset('Alert')->search( {
+            alert_type => 'new_updates',
+            parameter  => $p->id,
+            confirmed  => 1,
+            user_id    => $p->user->id,
+        } );
+
+        for my $alert (@alerts) {
+            my $alerts_sent = $self->schema->resultset('AlertSent')->find_or_create( {
+                alert_id  => $alert->id,
+                parameter => $comment->id,
+            } );
+        }
+    }
+
+    return $comment;
+}
+
+sub comment_text_for_request {
+    my ($self, $request, $problem, $state, $old_state,
+        $ext_code, $old_ext_code) = @_;
+
+    # Response templates are only triggered if the state/external status has changed.
+    # And treat any fixed state as fixed.
+    my $state_changed = $state ne $old_state
+        && !( $problem->is_fixed && FixMyStreet::DB::Result::Problem->fixed_states()->{$state} );
+    my $ext_code_changed = $ext_code ne $old_ext_code;
+    my $template;
+    if ($state_changed || $ext_code_changed) {
+        my $order;
+        my $state_params = {
+            'me.state' => $state
+        };
+        if ($ext_code) {
+            $state_params->{'me.external_status_code'} = $ext_code;
+            # make sure that empty string/nulls come last.
+            $order = { order_by => \"me.external_status_code DESC NULLS LAST" };
+        };
+
+        if (my $t = $problem->response_templates->search({
+            auto_response => 1,
+            -or => $state_params,
+        }, $order )->first) {
+            $template = $t->text;
+        }
+    }
+
+    my $desc = $request->{description} || '';
+    if ($desc && (!$template || $template !~ /\{\{description}}/)) {
+        return $desc;
+    }
+
+    if ($template) {
+        $template =~ s/\{\{description}}/$desc/;
+        return $template;
+    }
+
+    return "" if $self->blank_updates_permitted;
+
+    print STDERR "Couldn't determine update text for $request->{update_id} (report " . $problem->id . ")\n";
+    return "";
+}
+
+1;
diff --git a/t/app/model/problem.t b/t/app/model/problem.t
index 661a8827f..be949054b 100644
--- a/t/app/model/problem.t
+++ b/t/app/model/problem.t
@@ -2,6 +2,7 @@ use FixMyStreet::TestMech;
 use FixMyStreet;
 use FixMyStreet::DB;
 use FixMyStreet::Script::Reports;
+use Open311::GetUpdates;
 use Sub::Override;
 
 my $problem_rs = FixMyStreet::DB->resultset('Problem');
@@ -133,6 +134,7 @@ $problem->anonymous(1);
 $problem->insert;
 
 my $tz_local = DateTime::TimeZone->new( name => 'local' );
+my $comment_time = DateTime->now->set_time_zone( $tz_local );
 
 my $body = FixMyStreet::DB->resultset('Body')->new({
     name => 'Edinburgh City Council'
@@ -140,52 +142,32 @@ my $body = FixMyStreet::DB->resultset('Body')->new({
 
 for my $test (
     {
-        desc => 'request older than problem ignored',
-        lastupdate => '',
+        desc => 'request after problem created',
         request => {
-            updated_datetime => DateTime::Format::W3CDTF->new()->format_datetime( DateTime->now()->set_time_zone( $tz_local )->subtract( days => 2 ) ),
-        },
-        created => 0,
-    },
-    {
-        desc => 'request newer than problem created',
-        lastupdate => '',
-        request => {
-            updated_datetime => DateTime::Format::W3CDTF->new()->format_datetime( DateTime->now()->set_time_zone( $tz_local ) ),
             status => 'open',
-            status_notes => 'this is an update from the council',
+            comment_time => $comment_time,
+            description => 'this is an update from the council',
         },
-        created => 1,
         state => 'confirmed',
-        mark_fixed => 0,
-        mark_open => 0,
     },
     {
         desc => 'update with state of closed fixes problem',
-        lastupdate => '',
         request => {
-            updated_datetime => DateTime::Format::W3CDTF->new()->format_datetime( DateTime->now()->set_time_zone( $tz_local ) ),
+            comment_time => $comment_time,
             status => 'closed',
-            status_notes => 'the council have fixed this',
+            description => 'the council have fixed this',
         },
-        created => 1,
-        state => 'fixed',
-        mark_fixed => 1,
-        mark_open => 0,
+        state => 'fixed - council',
     },
     {
-        desc => 'update with state of open leaves problem as fixed',
-        lastupdate => '',
+        desc => 'update with state of open reopens problem',
         request => {
-            updated_datetime => DateTime::Format::W3CDTF->new()->format_datetime( DateTime->now()->set_time_zone( $tz_local ) ),
+            comment_time => $comment_time,
             status => 'open',
-            status_notes => 'the council do not think this is fixed',
+            description => 'the council do not think this is fixed',
         },
-        created => 1,
-        start_state => 'fixed',
-        state => 'fixed',
-        mark_fixed => 0,
-        mark_open => 0,
+        start_state => 'fixed - council',
+        state => 'confirmed',
     },
 ) {
     subtest $test->{desc} => sub {
@@ -197,23 +179,20 @@ for my $test (
         $problem->update;
         my $w3c = DateTime::Format::W3CDTF->new();
 
-        my $ret = $problem->update_from_open311_service_request( $test->{request}, $body, $user );
-        is $ret, $test->{created}, 'return value';
-
-        return unless $test->{created};
+        my $o = Open311->new( jurisdiction => 'mysociety', endpoint => 'http://example.com', test_mode => 1 );
+        my $updates = Open311::GetUpdates->new(
+            current_open311 => $o,
+            current_body => $body,
+            system_user => $user,
+        );
+        my $update = $updates->process_update($test->{request}, $problem);
 
         $problem->discard_changes;
-        is $problem->lastupdate, $w3c->parse_datetime($test->{request}->{updated_datetime}), 'lastupdate time';
-
-        my $update = $problem->comments->first;
+        is $problem->lastupdate, $test->{request}->{comment_time}, 'lastupdate time';
 
         ok $update, 'updated created';
-
         is $problem->state, $test->{state}, 'problem state';
-
-        is $update->text, $test->{request}->{status_notes}, 'update text';
-        is $update->mark_open, $test->{mark_open}, 'update mark_open flag';
-        is $update->mark_fixed, $test->{mark_fixed}, 'update mark_fixed flag';
+        is $update->text, $test->{request}->{description}, 'update text';
     };
 }
 
diff --git a/t/open311/getupdates.t b/t/open311/getupdates.t
index 351f17f6f..c1e51d380 100644
--- a/t/open311/getupdates.t
+++ b/t/open311/getupdates.t
@@ -18,9 +18,6 @@ my $body = FixMyStreet::DB->resultset('Body')->new( {
     name => 'Test Body',
 } );
 
-my $updates = Open311::GetUpdates->new( system_user => $user );
-ok $updates, 'created object';
-
 my $requests_xml = qq{<?xml version="1.0" encoding="utf-8"?>
 <service_requests>
 <request>
@@ -101,7 +98,12 @@ for my $test (
 
         my $o = Open311->new( jurisdiction => 'mysociety', endpoint => 'http://example.com', test_mode => 1, test_get_returns => { 'requests.xml' => $local_requests_xml } );
 
-        ok $updates->update_reports( [ 638344 ], $o, $body ), 'Updated reports';
+        my $updates = Open311::GetUpdates->new(
+            system_user => $user,
+            current_open311 => $o,
+            current_body => $body,
+        );
+        $updates->update_reports( [ $problem ] );
         my @parts = uri_split($o->test_uri_used);
         is $parts[2], '/requests.xml', 'path matches';
         my @qs = sort split '&', $parts[3];
@@ -179,7 +181,13 @@ subtest 'update with two requests' => sub {
 
     my $o = Open311->new( jurisdiction => 'mysociety', endpoint => 'http://example.com', test_mode => 1, test_get_returns => { 'requests.xml' => $local_requests_xml } );
 
-    ok $updates->update_reports( [ 638344,638345 ], $o, $body ), 'Updated reports';
+    my $updates = Open311::GetUpdates->new(
+        system_user => $user,
+        current_open311 => $o,
+        current_body => $body,
+    );
+
+    $updates->update_reports( [ $problem, $problem2 ] );
     my @parts = uri_split($o->test_uri_used);
     is $parts[2], '/requests.xml', 'path matches';
     my @qs = sort split '&', $parts[3];
@@ -227,7 +235,7 @@ my $problem3 = $problem_rs->create( {
     external_id  => 638346,
 } );
 
-subtest 'test translation of auto-added comment from old-style Open311 update' => sub {
+subtest 'test auto-added comment from old-style Open311 update' => sub {
     my $dt = sprintf( '<updated_datetime>%s</updated_datetime>', DateTime->now );
     $requests_xml =~ s/UPDATED_DATETIME/$dt/;
 
@@ -236,7 +244,13 @@ subtest 'test translation of auto-added comment from old-style Open311 update' =
     FixMyStreet::override_config {
         ALLOWED_COBRANDS => [ 'fixamingata' ],
     }, sub {
-        ok $updates->update_reports( [ 638346 ], $o, $body ), 'Updated reports';
+        my $updates = Open311::GetUpdates->new(
+            system_user => $user,
+            current_open311 => $o,
+            current_body => $body,
+            blank_updates_permitted => 1,
+        );
+        $updates->update_reports( [ $problem3 ] );
     };
     my @parts = uri_split($o->test_uri_used);
     is $parts[2], '/requests.xml', 'path matches';
@@ -244,7 +258,8 @@ subtest 'test translation of auto-added comment from old-style Open311 update' =
     is_deeply(\@qs, [ 'jurisdiction_id=mysociety', 'service_request_id=638346' ], 'query string matches');
 
     is $problem3->comments->count, 1, 'added a comment';
-    is $problem3->comments->first->text, "Stängd av kommunen", 'correct comment text';
+    is $problem3->comments->first->problem_state, 'fixed - council';
+    is $problem3->comments->first->text, '', 'correct comment text';
 };
 
 END {