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 c300fb56e3..c086014652 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 879ef8d7df..167ee100d6 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 0b9f1657c2..9ee8bb397e 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 3520dae816..73235fe27d 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 f4bdfcea35..83187efe6a 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 0549955a87..b48ec1109f 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 a25f10e623..c820461126 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 c433a7e362..4645a22bd7 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 f3b93eb03d..a18a648c61 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 b440506047..fe2b6e69a3 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 b3aa0adfa0..50698e6337 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 c305abf3d0..6efeb5161f 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 d50da9835b..60878b598f 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