Merge branch 'FD4750-prevent-out-of-order-updates' into commercial-st…

…aging
mysociety · Dec 19, 2024 · 5fc7b6e · 5fc7b6e
2 parents fcd80d4 + c10627f
commit 5fc7b6e
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 0 deletions.
diff --git a/perllib/Open311/PostServiceRequestUpdates.pm b/perllib/Open311/PostServiceRequestUpdates.pm
@@ -182,6 +182,30 @@ sub process_update {
         return;
     }
 
+    # Comments are ordered randomly.
+    # Some cobrands/APIs do not handle ordering by age their end (e.g.
+    # Northumberland + Alloy) so we skip comment for now if an older unsent
+    # one exists for the problem. Otherwise an older update may overwrite a
+    # newer one in Alloy etc.
+    my $formatter = FixMyStreet::DB->schema->storage->datetime_parser;
+    my $unsent_comment_for_problem
+        = $problem->comments->search(
+            {
+                state => 'confirmed',
+                send_state => 'unprocessed',
+                confirmed => { '<' =>
+                        $formatter->format_datetime( $comment->confirmed ) },
+                id => { '!=' => $comment->id },
+            },
+            { rows => 1 },
+        )->single;
+
+    if ($unsent_comment_for_problem) {
+        $self->log( $comment,
+            'Skipping for now because of older unprocessed update' );
+        return;
+    }
+
     # Some cobrands (e.g. Buckinghamshire) don't want to receive updates
     # from anyone except the original problem reporter.
     if (my $skip = $cobrand->call_hook(should_skip_sending_update => $comment)) {

diff --git a/t/script/send-daemon.t b/t/script/send-daemon.t
@@ -91,4 +91,62 @@ subtest 'Normal update sending works' => sub {
     is $c->external_id, 456, 'Correct external ID';
 };
 
+subtest 'Multiple updates on same problem should send in order of confirmation' => sub {
+    my $mock = Test::MockModule->new('Open311');
+    $mock->mock('post_service_request_update', sub { 456 });
+
+    my ($p3) = $mech->create_problems_for_body(
+        1,
+        $body->id,
+        'Title',
+        {   category         => 'Graffiti',
+            whensent         => '\NOW()',
+            send_state       => 'sent',
+            external_id      => 999,
+            send_method_used => 'Open311',
+        }
+    );
+
+    my $c1_p3
+        = $mech->create_comment_for_problem( $p3, $p3->user, $p3->user->name,
+        'An update 1', 'f', 'confirmed', 'confirmed',
+        { confirmed => '2024-12-11 15:30:00' } );
+    my $c2_p3
+        = $mech->create_comment_for_problem( $p3, $p3->user, $p3->user->name,
+        'An update 2', 'f', 'confirmed', 'confirmed',
+        { confirmed => '2024-12-11 15:31:00' } );
+    my $c3_p3
+        = $mech->create_comment_for_problem( $p3, $p3->user, $p3->user->name,
+        'An update 3', 'f', 'confirmed', 'confirmed',
+        { confirmed => '2024-12-11 15:32:00' } );
+
+    my $countdown = 20; # Ran test 100 times and no failure, so seems a solid number
+    my %pending = map { $_->id => $_ } ( $c1_p3, $c2_p3, $c3_p3 );
+    my @sent;
+    while ( $countdown && _check_updates( \%pending, \@sent ) ) {
+        FixMyStreet::Script::SendDaemon::look_for_update($opts);
+        $countdown--;
+    }
+
+    is_deeply \@sent, [ $c1_p3->id, $c2_p3->id, $c3_p3->id ],
+        'comments sent in order';
+};
+
+sub _check_updates {
+    my ( $pending, $sent ) = @_;
+
+    my $unsent = 0;
+    for ( values %$pending ) {
+        $_->discard_changes;
+        if ( $_->send_state eq 'unprocessed' ) {
+            $unsent++;
+        } elsif ( $_->send_state eq 'sent' ) {
+            delete $pending->{ $_->id };
+            push @$sent, $_->id;
+        }
+    }
+
+    return $unsent;
+}
+
 done_testing;