Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Waste] [Echo] Check event before/after sending. #4896

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions perllib/FixMyStreet/Cobrand/Brent.pm
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,17 @@
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
Expand All @@ -699,6 +710,8 @@
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 714 in perllib/FixMyStreet/Cobrand/Brent.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/Brent.pm#L714

Added line #L714 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 @@ -261,6 +261,8 @@
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 @@ -321,6 +323,8 @@
} 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 327 in perllib/FixMyStreet/Cobrand/Bromley.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/Bromley.pm#L327

Added line #L327 was not covered by tests
}
});
}
Expand Down
37 changes: 37 additions & 0 deletions perllib/FixMyStreet/Roles/CobrandEcho.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 15 additions & 7 deletions perllib/FixMyStreet/Roles/CobrandSLWP.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
});
}
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 @@ -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) = @_;
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 @@ -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) = @_;
Expand Down
7 changes: 7 additions & 0 deletions t/cobrand/brent.t
Original file line number Diff line number Diff line change
Expand Up @@ -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' ,
Expand Down Expand Up @@ -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; }
Expand Down Expand Up @@ -880,6 +884,8 @@ FixMyStreet::override_config {
}
};

$echo = shared_echo_mocks();

FixMyStreet::override_config {
ALLOWED_COBRANDS => [ 'brent', 'tfl' ],
MAPIT_URL => 'http://mapit.uk/',
Expand Down Expand Up @@ -1467,6 +1473,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 @@ -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',
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 @@ -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 },
);
Expand All @@ -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(
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 @@ -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');
});
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