From 4c4da0b45c3c1b3c9da7d65d23cd3d2aa4e5f988 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 | 9 +++ perllib/FixMyStreet/Cobrand/Bromley.pm | 4 ++ perllib/FixMyStreet/Roles/Cobrand/Echo.pm | 37 ++++++++++++ perllib/FixMyStreet/Roles/Cobrand/SLWP.pm | 27 ++++++--- 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, 158 insertions(+), 15 deletions(-) diff --git a/perllib/FixMyStreet/Cobrand/Brent.pm b/perllib/FixMyStreet/Cobrand/Brent.pm index c300fb56e37..c0860146528 100644 --- a/perllib/FixMyStreet/Cobrand/Brent.pm +++ b/perllib/FixMyStreet/Cobrand/Brent.pm @@ -707,9 +707,16 @@ 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 'SKIP' if $row->category eq 'Request new container' && $row->get_extra_field_value('request_referral'); + return 'SENT' if $self->open311_pre_send_check($row, 'FMS'); } =head2 open311_post_send @@ -745,6 +752,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 879ef8d7df8..167ee100d66 100644 --- a/perllib/FixMyStreet/Cobrand/Bromley.pm +++ b/perllib/FixMyStreet/Cobrand/Bromley.pm @@ -288,6 +288,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 { @@ -348,6 +350,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/Cobrand/Echo.pm b/perllib/FixMyStreet/Roles/Cobrand/Echo.pm index 0b9f1657c2b..9ee8bb397e6 100644 --- a/perllib/FixMyStreet/Roles/Cobrand/Echo.pm +++ b/perllib/FixMyStreet/Roles/Cobrand/Echo.pm @@ -673,6 +673,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/Cobrand/SLWP.pm b/perllib/FixMyStreet/Roles/Cobrand/SLWP.pm index 3520dae816a..73235fe27d9 100644 --- a/perllib/FixMyStreet/Roles/Cobrand/SLWP.pm +++ b/perllib/FixMyStreet/Roles/Cobrand/SLWP.pm @@ -478,6 +478,23 @@ sub open311_extra_data_include { return $open311_only; } +sub echo_reference_prefix { + my $self = shift; + return 'LBS' if $self->moniker eq 'sutton'; + return 'RBK' if $self->moniker eq 'kingston'; + return 'MRT' if $self->moniker eq 'merton'; +} + +=item * 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, $self->echo_reference_prefix); +} + =item * If Echo errors, we try and deal with standard issues - a renewal on an expired subscription, or a duplicate event =cut @@ -503,13 +520,9 @@ 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/) { + $self->open311_post_send_error_check($self->echo_reference_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 0549955a875..b48ec1109f6 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 a25f10e6230..c8204611267 100644 --- a/t/app/controller/waste_kingston_r.t +++ b/t/app/controller/waste_kingston_r.t @@ -485,6 +485,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 c433a7e3622..4645a22bd75 100644 --- a/t/app/controller/waste_sutton_r.t +++ b/t/app/controller/waste_sutton_r.t @@ -454,6 +454,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 f3b93eb03d3..a18a648c611 100644 --- a/t/cobrand/brent.t +++ b/t/cobrand/brent.t @@ -223,6 +223,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' , @@ -698,6 +700,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; } @@ -906,6 +910,8 @@ FixMyStreet::override_config { } }; +$echo = shared_echo_mocks(); + FixMyStreet::override_config { ALLOWED_COBRANDS => [ 'brent', 'tfl' ], MAPIT_URL => 'http://mapit.uk/', @@ -1587,6 +1593,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 b4405060470..fe2b6e69a35 100644 --- a/t/cobrand/bromley.t +++ b/t/cobrand/bromley.t @@ -160,6 +160,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 b3aa0adfa00..50698e63378 100644 --- a/t/cobrand/bromley_waste.t +++ b/t/cobrand/bromley_waste.t @@ -94,6 +94,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 }, ); @@ -111,6 +114,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 c305abf3d03..6efeb5161f6 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; @@ -402,6 +459,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