Skip to content

Commit

Permalink
[Waste] [Echo] Check event before/after sending.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dracos committed Sep 20, 2024
1 parent 73acd35 commit 4c4da0b
Show file tree
Hide file tree
Showing 13 changed files with 158 additions and 15 deletions.
9 changes: 9 additions & 0 deletions perllib/FixMyStreet/Cobrand/Brent.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

Check warning on line 756 in perllib/FixMyStreet/Cobrand/Brent.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/Brent.pm#L756

Added line #L756 was not covered by tests
}
});
}
Expand Down
4 changes: 4 additions & 0 deletions perllib/FixMyStreet/Cobrand/Bromley.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);

Check warning on line 354 in perllib/FixMyStreet/Cobrand/Bromley.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/Bromley.pm#L354

Added line #L354 was not covered by tests
}
});
}
Expand Down
37 changes: 37 additions & 0 deletions perllib/FixMyStreet/Roles/Cobrand/Echo.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 20 additions & 7 deletions perllib/FixMyStreet/Roles/Cobrand/SLWP.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
});
}
Expand Down
4 changes: 2 additions & 2 deletions perllib/FixMyStreet/SendReport/Open311.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions t/app/controller/waste_bromley_bulky.t
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions t/app/controller/waste_kingston_r.t
Original file line number Diff line number Diff line change
Expand Up @@ -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) = @_;
Expand Down
1 change: 1 addition & 0 deletions t/app/controller/waste_sutton_r.t
Original file line number Diff line number Diff line change
Expand Up @@ -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) = @_;
Expand Down
7 changes: 7 additions & 0 deletions t/cobrand/brent.t
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ create_contact({ category => 'Additional collection', email => '[email protected]
{ 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' ,
Expand Down Expand Up @@ -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; }
Expand Down Expand Up @@ -906,6 +910,8 @@ FixMyStreet::override_config {
}
};

$echo = shared_echo_mocks();

FixMyStreet::override_config {
ALLOWED_COBRANDS => [ 'brent', 'tfl' ],
MAPIT_URL => 'http://mapit.uk/',
Expand Down Expand Up @@ -1587,6 +1593,7 @@ sub shared_echo_mocks {
};
});
$e->mock('GetEventsForObject', sub { [] });
$e->mock('GetEvent', sub { {} });
$e->mock('GetTasks', sub { [] });
return $e;
}
Expand Down
3 changes: 3 additions & 0 deletions t/cobrand/bromley.t
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 6 additions & 0 deletions t/cobrand/bromley_waste.t
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
);
Expand All @@ -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(
Expand Down
70 changes: 64 additions & 6 deletions t/cobrand/sutton.t
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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', '<?xml version="1.0" encoding="utf-8"?><errors><error><code></code><description>Duplicate Event! Original eventID: 123</description></error></errors>', 500);
$sender->send($report, {
easting => 1,
Expand All @@ -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', '<?xml version="1.0" encoding="utf-8"?><errors><error><code></code><description>Internal error</description></error></errors>', 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', '<?xml version="1.0" encoding="utf-8"?><errors><error><code></code><description>Internal error</description></error></errors>', 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' });
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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');
});
Expand Down
3 changes: 3 additions & 0 deletions t/sendreport/open311.t
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 4c4da0b

Please sign in to comment.