From e7ffa723f981d421229bd10b53153f04898e78c9 Mon Sep 17 00:00:00 2001 From: Matthew Somerville Date: Tue, 26 Mar 2024 22:49:06 +0000 Subject: [PATCH] [Waste] [Echo] Check event before/after sending. If we get an internal error from Echo when sending, try and look up the event directly to see if it has actually been created. Also do the same before sending as an extra check. --- perllib/FixMyStreet/Cobrand/Brent.pm | 13 +++++ perllib/FixMyStreet/Cobrand/Bromley.pm | 4 ++ perllib/FixMyStreet/Roles/CobrandEcho.pm | 37 ++++++++++++ perllib/FixMyStreet/Roles/CobrandSLWP.pm | 22 ++++--- perllib/FixMyStreet/SendReport/Open311.pm | 4 +- t/app/controller/waste_bromley_bulky.t | 1 + t/app/controller/waste_kingston_r.t | 1 + t/app/controller/waste_sutton_r.t | 1 + t/cobrand/brent.t | 7 +++ t/cobrand/bromley.t | 3 + t/cobrand/bromley_waste.t | 6 ++ t/cobrand/sutton.t | 70 +++++++++++++++++++++-- t/sendreport/open311.t | 3 + 13 files changed, 157 insertions(+), 15 deletions(-) diff --git a/perllib/FixMyStreet/Cobrand/Brent.pm b/perllib/FixMyStreet/Cobrand/Brent.pm index 93334f0f6d2..021155b0d11 100644 --- a/perllib/FixMyStreet/Cobrand/Brent.pm +++ b/perllib/FixMyStreet/Cobrand/Brent.pm @@ -678,6 +678,17 @@ sub open311_extra_data_exclude { return []; } +=head2 open311_pre_send + +We check in Echo to see if something has already been sent there first. + +=cut + +sub open311_pre_send { + my ($self, $row, $open311) = @_; + return 'SENT' if $self->open311_pre_send_check($row, 'FMS'); +} + =head2 open311_post_send Restore the original detail field if it was changed by open311_extra_data_include @@ -699,6 +710,8 @@ sub open311_post_send { if ($error =~ /Selected reservations expired|Invalid reservation reference/) { $self->bulky_refetch_slots($row2); $row->discard_changes; + } elsif ($error =~ /Internal error/) { + $self->open311_post_send_error_check("FMS", $row, $row2, $sender); } }); } diff --git a/perllib/FixMyStreet/Cobrand/Bromley.pm b/perllib/FixMyStreet/Cobrand/Bromley.pm index 03a9ba9c350..cc7febd89fc 100644 --- a/perllib/FixMyStreet/Cobrand/Bromley.pm +++ b/perllib/FixMyStreet/Cobrand/Bromley.pm @@ -261,6 +261,8 @@ sub open311_pre_send { my $text = $row->detail . "\n\nPrivate comments: $private_comments"; $row->detail($text); } + + return 'SENT' if $self->open311_pre_send_check($row, 'FMS'); } sub _include_user_title_in_extra { @@ -321,6 +323,8 @@ sub open311_post_send { } elsif ($error =~ /Selected reservations expired|Invalid reservation reference/) { $self->bulky_refetch_slots($row2); $row->discard_changes; + } elsif ($error =~ /Internal error/) { + $self->open311_post_send_error_check("FMS", $row, $row2, $sender); } }); } diff --git a/perllib/FixMyStreet/Roles/CobrandEcho.pm b/perllib/FixMyStreet/Roles/CobrandEcho.pm index 5ebfd31a834..64541e8e4c7 100644 --- a/perllib/FixMyStreet/Roles/CobrandEcho.pm +++ b/perllib/FixMyStreet/Roles/CobrandEcho.pm @@ -569,6 +569,43 @@ sub admin_templates_external_status_code_hook { return $code; } +sub open311_pre_send_check { + my ($self, $row, $prefix) = @_; + + my $guid = $self->open311_check_existing_event("$prefix-" . $row->id, "ClientReference"); + if ($guid) { + # Event already exists + $row->discard_changes; + $row->update({ external_id => $guid }); + return 1; + } + return 0; +} + +sub open311_post_send_error_check { + my ($self, $prefix, $row, $row2, $sender) = @_; + $self->open311_post_send_check("$prefix-" . $row->id, 'ClientReference', $row, $row2, $sender); +} + +sub open311_post_send_check { + my ($self, $id, $type, $row, $row2, $sender) = @_; + if (my $guid = $self->open311_check_existing_event($id, $type)) { + $row2->external_id($guid); + $sender->success(1); + $row2->update; + $row->discard_changes; + } +} + +sub open311_check_existing_event { + my ($self, $id, $type) = @_; + + my $cfg = $self->feature('echo'); + my $echo = Integrations::Echo->new(%$cfg); + my $event = $echo->GetEvent($id, $type) || {}; + return $event->{Guid}; +} + =item waste_fetch_events Loop through all open waste events to see if there have been any updates diff --git a/perllib/FixMyStreet/Roles/CobrandSLWP.pm b/perllib/FixMyStreet/Roles/CobrandSLWP.pm index 2bd20dadb3b..1f2b5d81395 100644 --- a/perllib/FixMyStreet/Roles/CobrandSLWP.pm +++ b/perllib/FixMyStreet/Roles/CobrandSLWP.pm @@ -95,6 +95,17 @@ sub open311_extra_data_include { return $open311_only; } +=item * We check in Echo to see if something has already been sent there first + +=cut + +sub open311_pre_send { + my ($self, $row, $open311) = @_; + + my $prefix = $self->moniker eq 'sutton' ? 'LBS' : 'RBK'; + return 'SENT' if $self->open311_pre_send_check($row, $prefix); +} + =item * If Echo errors, we try and deal with standard issues - a renewal on an expired subscription, or a duplicate event =cut @@ -120,13 +131,10 @@ sub open311_post_send { $row->discard_changes; } elsif ($error =~ /Duplicate Event! Original eventID: (\d+)/) { my $id = $1; - my $cfg = $self->feature('echo'); - my $echo = Integrations::Echo->new(%$cfg); - my $event = $echo->GetEvent($id, 'Id'); - $row2->external_id($event->{Guid}); - $sender->success(1); - $row2->update; - $row->discard_changes; + $self->open311_post_send_check($id, "Id", $row, $row2, $sender); + } elsif ($error =~ /Internal error/) { + my $prefix = $self->moniker eq 'sutton' ? 'LBS' : 'RBK'; + $self->open311_post_send_error_check($prefix, $row, $row2, $sender); } }); } diff --git a/perllib/FixMyStreet/SendReport/Open311.pm b/perllib/FixMyStreet/SendReport/Open311.pm index f4bdfcea355..83187efe6af 100644 --- a/perllib/FixMyStreet/SendReport/Open311.pm +++ b/perllib/FixMyStreet/SendReport/Open311.pm @@ -77,7 +77,7 @@ sub send { my $open311 = Open311->new( %open311_params ); my $skip = $cobrand->call_hook(open311_pre_send => $row, $open311); - $skip = $skip && $skip eq 'SKIP'; + $skip = $skip && ($skip eq 'SKIP' || $skip eq 'SENT'); my $resp; if (!$skip) { @@ -88,7 +88,7 @@ sub send { $row->discard_changes; if ( $skip || $resp ) { - $row->update({ external_id => $resp }); + $row->update({ external_id => $resp }) if $resp; $self->success( 1 ); } else { $self->error( "Failed to send over Open311\n" ) unless $self->error; diff --git a/t/app/controller/waste_bromley_bulky.t b/t/app/controller/waste_bromley_bulky.t index edb48b98606..e3e889d5384 100644 --- a/t/app/controller/waste_bromley_bulky.t +++ b/t/app/controller/waste_bromley_bulky.t @@ -162,6 +162,7 @@ FixMyStreet::override_config { $echo->mock( 'CancelReservedSlotsForEvent', sub { [] } ); $echo->mock( 'GetTasks', sub { [] } ); $echo->mock( 'GetEventsForObject', sub { [] } ); + $echo->mock( 'GetEvent', sub { {} } ); $echo->mock( 'FindPoints',sub { [ { Description => '2 Example Street, Bromley, BR1 1AF', diff --git a/t/app/controller/waste_kingston_r.t b/t/app/controller/waste_kingston_r.t index 751a70441a7..983f820140e 100644 --- a/t/app/controller/waste_kingston_r.t +++ b/t/app/controller/waste_kingston_r.t @@ -443,6 +443,7 @@ sub shared_echo_mocks { }); $e->mock('GetServiceUnitsForObject', sub { $bin_data }); $e->mock('GetEventsForObject', sub { [] }); + $e->mock('GetEvent', sub { {} }); $e->mock('GetTasks', sub { [] }); $e->mock( 'CancelReservedSlotsForEvent', sub { my (undef, $guid) = @_; diff --git a/t/app/controller/waste_sutton_r.t b/t/app/controller/waste_sutton_r.t index 56c872c49db..41503f93e06 100644 --- a/t/app/controller/waste_sutton_r.t +++ b/t/app/controller/waste_sutton_r.t @@ -436,6 +436,7 @@ sub shared_echo_mocks { }); $e->mock('GetServiceUnitsForObject', sub { $bin_data }); $e->mock('GetEventsForObject', sub { [] }); + $e->mock('GetEvent', sub { {} }); $e->mock('GetTasks', sub { [] }); $e->mock( 'CancelReservedSlotsForEvent', sub { my (undef, $guid) = @_; diff --git a/t/cobrand/brent.t b/t/cobrand/brent.t index 04b0659e322..58ba91e7be9 100644 --- a/t/cobrand/brent.t +++ b/t/cobrand/brent.t @@ -195,6 +195,8 @@ create_contact({ category => 'Additional collection', email => 'general@brent.go { code => 'service_id', required => 1, automated => 'hidden_field' }, ); +my $echo = shared_echo_mocks(); + subtest "title is labelled 'location of problem' in open311 extended description" => sub { my ($problem) = $mech->create_problems_for_body(1, $brent->id, 'title', { category => 'Graffiti' , @@ -672,6 +674,8 @@ FixMyStreet::override_config { $mech->host("brent.fixmystreet.com"); }; +undef $echo; # Otherwise tests below fail + package SOAP::Result; sub result { return $_[0]->{result}; } sub new { my $c = shift; bless { @_ }, $c; } @@ -880,6 +884,8 @@ FixMyStreet::override_config { } }; +$echo = shared_echo_mocks(); + FixMyStreet::override_config { ALLOWED_COBRANDS => [ 'brent', 'tfl' ], MAPIT_URL => 'http://mapit.uk/', @@ -1467,6 +1473,7 @@ sub shared_echo_mocks { }; }); $e->mock('GetEventsForObject', sub { [] }); + $e->mock('GetEvent', sub { {} }); $e->mock('GetTasks', sub { [] }); return $e; } diff --git a/t/cobrand/bromley.t b/t/cobrand/bromley.t index 1ff0237b80b..cdcf9f6a808 100644 --- a/t/cobrand/bromley.t +++ b/t/cobrand/bromley.t @@ -148,6 +148,9 @@ subtest 'Updates from staff with no text but with private comments are sent' => }; }; +my $echo = Test::MockModule->new('Integrations::Echo'); +$echo->mock(GetEvent => sub { {} }); + for my $test ( { desc => 'testing special Open311 behaviour', diff --git a/t/cobrand/bromley_waste.t b/t/cobrand/bromley_waste.t index 49f625d5542..054a66d2e4b 100644 --- a/t/cobrand/bromley_waste.t +++ b/t/cobrand/bromley_waste.t @@ -93,6 +93,9 @@ subtest 'check footer is powered by SocietyWorks' => sub { }; subtest 'test waste duplicate' => sub { + my $echo = Test::MockModule->new('Integrations::Echo'); + $echo->mock(GetEvent => sub { {} }); + my $sender = FixMyStreet::SendReport::Open311->new( bodies => [ $body ], body_config => { $body->id => $body }, ); @@ -110,6 +113,9 @@ subtest 'test waste duplicate' => sub { }; subtest 'test DD taking so long it expires' => sub { + my $echo = Test::MockModule->new('Integrations::Echo'); + $echo->mock(GetEvent => sub { {} }); + my $title = $report->title; $report->update({ title => "Garden Subscription - Renew" }); my $sender = FixMyStreet::SendReport::Open311->new( diff --git a/t/cobrand/sutton.t b/t/cobrand/sutton.t index 4903e1b9f6a..ba8a64d766c 100644 --- a/t/cobrand/sutton.t +++ b/t/cobrand/sutton.t @@ -72,6 +72,9 @@ FixMyStreet::override_config { MAPIT_URL => 'http://mapit.uk/', STAGING_FLAGS => { send_reports => 1 }, }, sub { + my $echo = Test::MockModule->new('Integrations::Echo'); + $echo->mock('GetEvent', sub { {} }); + subtest 'test waste duplicate' => sub { my $sender = FixMyStreet::SendReport::Open311->new( bodies => [ $body ], body_config => { $body->id => $body }, @@ -106,11 +109,10 @@ FixMyStreet::override_config { my $sender = FixMyStreet::SendReport::Open311->new( bodies => [ $body ], body_config => { $body->id => $body }, ); - my $echo = Test::MockModule->new('Integrations::Echo'); - $echo->mock('GetEvent', sub { { - Guid => 'a-guid', - Id => 123, - } } ); + $echo->mock('GetEvent', sub { + return {} if $_[2] eq 'ClientReference'; + return { Guid => 'a-guid', Id => 123 } + }); Open311->_inject_response('/requests.xml', 'Duplicate Event! Original eventID: 123', 500); $sender->send($report, { easting => 1, @@ -121,6 +123,61 @@ FixMyStreet::override_config { is $report->external_id, 'a-guid'; }; + subtest 'test internal error, no event, at the Echo side' => sub { + $report->update({ external_id => undef }); + my $sender = FixMyStreet::SendReport::Open311->new( + bodies => [ $body ], body_config => { $body->id => $body }, + ); + $echo->mock('GetEvent', sub { {} }); + Open311->_inject_response('/requests.xml', 'Internal error', 500); + $sender->send($report, { + easting => 1, + northing => 2, + url => 'http://example.org/', + }); + is $sender->success, 0; + is $report->external_id, undef; + }; + + subtest 'test internal error, event accepted, at the Echo side' => sub { + my $sender = FixMyStreet::SendReport::Open311->new( + bodies => [ $body ], body_config => { $body->id => $body }, + ); + $echo->mock('GetEvent', sub { + my $fn = (caller(2))[3]; + if ($fn =~ /open311_post_send_check/) { + return { Guid => 'c-guid', Id => 123 } + } else { + return {}; + } + }); + Open311->_inject_response('/requests.xml', 'Internal error', 500); + $sender->send($report, { + easting => 1, + northing => 2, + url => 'http://example.org/', + }); + is $sender->success, 1; + is $report->external_id, 'c-guid'; + }; + + subtest 'test event already existing at the Echo side' => sub { + my $sender = FixMyStreet::SendReport::Open311->new( + bodies => [ $body ], body_config => { $body->id => $body }, + ); + $echo->mock('GetEvent', sub { + return { Guid => 'b-guid', Id => 123 }; + }); + $sender->send($report, { + easting => 1, + northing => 2, + url => 'http://example.org/', + }); + is $sender->success, 1; + is $report->external_id, 'b-guid'; + $echo->mock('GetEvent', sub { {} }); + }; + subtest 'correct payment data sent across' => sub { $report->category('Garden Subscription'); $report->update_extra_field({ name => 'PaymentCode', value => 'Code4321' }); @@ -159,7 +216,7 @@ FixMyStreet::override_config { like $body, qr/Dear Sutton Council,\s+A user of FixMyStreet has submitted/; like $body, qr{http://www.example.org/report/$id}; }; - + undef $echo; }; package SOAP::Result; @@ -393,6 +450,7 @@ FixMyStreet::override_config { bodies => [ $body ], body_config => { $body->id => $body }, ); my $echo = Test::MockModule->new('Integrations::Echo'); + $echo->mock('GetEvent', sub { {} }); $echo->mock('CancelReservedSlotsForEvent', sub { is $_[1], $report->get_extra_field_value('GUID'); }); diff --git a/t/sendreport/open311.t b/t/sendreport/open311.t index d50da9835bf..60878b598fe 100644 --- a/t/sendreport/open311.t +++ b/t/sendreport/open311.t @@ -16,6 +16,9 @@ $ukc->mock('lookup_site_code', sub { return "Road ID"; }); +my $echo = Test::MockModule->new('Integrations::Echo'); +$echo->mock(GetEvent => sub { {} }); + package main; sub test_overrides; # defined below