[Open311] Allow save/drop of row extra during send

12 files changed, 137 insertions, 92 deletions

diff --git a/perllib/FixMyStreet/Cobrand/Bexley.pm b/perllib/FixMyStreet/Cobrand/Bexley.pm
index 0586c18c4..74a7b5032 100644
--- a/perllib/FixMyStreet/Cobrand/Bexley.pm
+++ b/perllib/FixMyStreet/Cobrand/Bexley.pm
@@ -83,14 +83,17 @@ sub lookup_site_code_config {
 }
 
 sub open311_config {
-    my ($self, $row, $h, $params, $contact) = @_;
+    my ($self, $row, $h, $params) = @_;
 
     $params->{multi_photos} = 1;
+}
 
-    my $extra = $row->get_extra_fields;
+sub open311_extra_data {
+    my ($self, $row, $h, $extra, $contact) = @_;
 
+    my $open311_only;
     if ($contact->email =~ /^Confirm/) {
-        push @$extra,
+        push @$open311_only,
             { name => 'report_url', description => 'Report URL',
               value => $h->{url} },
             { name => 'title', description => 'Title',
@@ -123,7 +126,7 @@ sub open311_config {
         }
     }
 
-    $row->set_extra_fields(@$extra);
+    return $open311_only;
 }
 
 sub admin_user_domain { 'bexley.gov.uk' }
diff --git a/perllib/FixMyStreet/Cobrand/Bromley.pm b/perllib/FixMyStreet/Cobrand/Bromley.pm
index abe8796b5..73d5f94bf 100644
--- a/perllib/FixMyStreet/Cobrand/Bromley.pm
+++ b/perllib/FixMyStreet/Cobrand/Bromley.pm
@@ -161,7 +161,14 @@ sub title_list {
 sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
-    my $extra = $row->get_extra_fields;
+    $params->{always_send_latlong} = 0;
+    $params->{send_notpinpointed} = 1;
+    $params->{extended_description} = 0;
+}
+
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
+
     my $title = $row->title;
 
     foreach (@$extra) {
@@ -169,9 +176,8 @@ sub open311_config {
         $title .= ' | ID: ' . $_->{value} if $_->{name} eq 'feature_id';
         $title .= ' | PROW ID: ' . $_->{value} if $_->{name} eq 'prow_reference';
     }
-    @$extra = grep { $_->{name} !~ /feature_id|prow_reference/ } @$extra;
 
-    push @$extra,
+    my $open311_only = [
         { name => 'report_url',
           value => $h->{url} },
         { name => 'report_title',
@@ -183,23 +189,20 @@ sub open311_config {
         { name => 'requested_datetime',
           value => DateTime::Format::W3CDTF->format_datetime($row->confirmed->set_nanosecond(0)) },
         { name => 'email',
-          value => $row->user->email };
+          value => $row->user->email }
+    ];
 
     # make sure we have last_name attribute present in row's extra, so
     # it is passed correctly to Bromley as attribute[]
     if (!$row->get_extra_field_value('last_name')) {
         my ( $firstname, $lastname ) = ( $row->name =~ /(\S+)\.?\s+(.+)/ );
-        push @$extra, { name => 'last_name', value => $lastname };
+        push @$open311_only, { name => 'last_name', value => $lastname };
     }
     if (!$row->get_extra_field_value('fms_extra_title') && $row->user->title) {
-        push @$extra, { name => 'fms_extra_title', value => $row->user->title };
+        push @$open311_only, { name => 'fms_extra_title', value => $row->user->title };
     }
 
-    $row->set_extra_fields(@$extra);
-
-    $params->{always_send_latlong} = 0;
-    $params->{send_notpinpointed} = 1;
-    $params->{extended_description} = 0;
+    return ($open311_only, [ 'feature_id', 'prow_reference' ]);
 }
 
 sub open311_config_updates {
diff --git a/perllib/FixMyStreet/Cobrand/CheshireEast.pm b/perllib/FixMyStreet/Cobrand/CheshireEast.pm
index e8cf8f8a3..c5e5107f3 100644
--- a/perllib/FixMyStreet/Cobrand/CheshireEast.pm
+++ b/perllib/FixMyStreet/Cobrand/CheshireEast.pm
@@ -60,15 +60,19 @@ sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
     $params->{multi_photos} = 1;
+}
+
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
-    my $extra = $row->get_extra_fields;
-    push @$extra,
+    my $open311_only = [
         { name => 'report_url',
           value => $h->{url} },
         { name => 'title',
           value => $row->title },
         { name => 'description',
-          value => $row->detail };
+          value => $row->detail },
+    ];
 
     # Reports made via FMS.com or the app probably won't have a site code
     # value because we don't display the adopted highways layer on those
@@ -82,7 +86,7 @@ sub open311_config {
         }
     }
 
-    $row->set_extra_fields(@$extra);
+    return $open311_only;
 }
 
 # TODO These values may not be accurate
diff --git a/perllib/FixMyStreet/Cobrand/Greenwich.pm b/perllib/FixMyStreet/Cobrand/Greenwich.pm
index bf172dbc7..be260d0c0 100644
--- a/perllib/FixMyStreet/Cobrand/Greenwich.pm
+++ b/perllib/FixMyStreet/Cobrand/Greenwich.pm
@@ -44,13 +44,13 @@ sub reports_per_page { return 20; }
 
 sub admin_user_domain { 'royalgreenwich.gov.uk' }
 
-sub open311_config {
-    my ($self, $row, $h, $params) = @_;
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
-    my $extra = $row->get_extra_fields;
     # Greenwich doesn't have category metadata to fill this
-    push @$extra, { name => 'external_id', value => $row->id };
-    $row->set_extra_fields( @$extra );
+    return [
+        { name => 'external_id', value => $row->id },
+    ];
 }
 
 sub open311_contact_meta_override {
diff --git a/perllib/FixMyStreet/Cobrand/Northamptonshire.pm b/perllib/FixMyStreet/Cobrand/Northamptonshire.pm
index 0bc44a1f3..4f819d37e 100644
--- a/perllib/FixMyStreet/Cobrand/Northamptonshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Northamptonshire.pm
@@ -88,12 +88,13 @@ sub map_type { 'Northamptonshire' }
 sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
-    my $extra = $row->get_extra_fields;
+    $params->{multi_photos} = 1;
+}
 
-    # remove the emergency category which is informational only
-    @$extra = grep { $_->{name} ne 'emergency' } @$extra;
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
-    push @$extra,
+    return ([
         { name => 'report_url',
           value => $h->{url} },
         { name => 'title',
@@ -101,11 +102,10 @@ sub open311_config {
         { name => 'description',
           value => $row->detail },
         { name => 'category',
-          value => $row->category };
-
-    $row->set_extra_fields(@$extra);
-
-    $params->{multi_photos} = 1;
+          value => $row->category },
+    ], [
+        'emergency'
+    ]);
 }
 
 sub open311_get_update_munging {
diff --git a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
index db6d120ed..12714185d 100644
--- a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
@@ -118,20 +118,21 @@ sub state_groups_inspect {
 sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
-    my $extra = $row->get_extra_fields;
-    push @$extra, { name => 'external_id', value => $row->id };
-    push @$extra, { name => 'northing', value => $h->{northing} };
-    push @$extra, { name => 'easting', value => $h->{easting} };
-
-    if ($h->{closest_address}) {
-        push @$extra, { name => 'closest_address', value => "$h->{closest_address}" }
-    }
-    $row->set_extra_fields( @$extra );
-
     $params->{multi_photos} = 1;
     $params->{extended_description} = 'oxfordshire';
 }
 
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
+
+    return [
+        { name => 'external_id', value => $row->id },
+        { name => 'northing', value => $h->{northing} },
+        { name => 'easting', value => $h->{easting} },
+        $h->{closest_address} ? { name => 'closest_address', value => "$h->{closest_address}" } : (),
+    ];
+}
+
 sub open311_config_updates {
     my ($self, $params) = @_;
     $params->{use_customer_reference} = 1;
diff --git a/perllib/FixMyStreet/Cobrand/Rutland.pm b/perllib/FixMyStreet/Cobrand/Rutland.pm
index 70ddef1b0..63a20d893 100644
--- a/perllib/FixMyStreet/Cobrand/Rutland.pm
+++ b/perllib/FixMyStreet/Cobrand/Rutland.pm
@@ -26,17 +26,18 @@ sub report_validation {
 sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
-    my $extra = $row->get_extra_fields;
-    push @$extra, { name => 'external_id', value => $row->id };
-    push @$extra, { name => 'title', value => $row->title };
-    push @$extra, { name => 'description', value => $row->detail };
+    $params->{multi_photos} = 1;
+}
 
-    if ($h->{closest_address}) {
-        push @$extra, { name => 'closest_address', value => "$h->{closest_address}" }
-    }
-    $row->set_extra_fields( @$extra );
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
-    $params->{multi_photos} = 1;
+    return [
+        { name => 'external_id', value => $row->id },
+        { name => 'title', value => $row->title },
+        { name => 'description', value => $row->detail },
+        $h->{closest_address} ? { name => 'closest_address', value => "$h->{closest_address}" } : (),
+    ];
 }
 
 sub disambiguate_location {
diff --git a/perllib/FixMyStreet/Cobrand/Westminster.pm b/perllib/FixMyStreet/Cobrand/Westminster.pm
index 7a3d7bbcd..d991ab165 100644
--- a/perllib/FixMyStreet/Cobrand/Westminster.pm
+++ b/perllib/FixMyStreet/Cobrand/Westminster.pm
@@ -76,8 +76,10 @@ sub open311_config {
     my $id = $row->user->get_extra_metadata('westminster_account_id');
     # Westminster require 0 as the account ID if there's no MyWestminster ID.
     $h->{account_id} = $id || '0';
+}
 
-    my $extra = $row->get_extra_fields;
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
     # Reports made via the app probably won't have a USRN because we don't
     # display the road layer. Instead we'll look up the closest asset from the
@@ -97,8 +99,6 @@ sub open311_config {
             push @$extra, { name => 'UPRN', value => $ref };
         }
     }
-
-    $row->set_extra_fields(@$extra);
 }
 
 sub lookup_site_code_config {
diff --git a/perllib/FixMyStreet/Roles/ConfirmOpen311.pm b/perllib/FixMyStreet/Roles/ConfirmOpen311.pm
index b9e424d4f..0845105f1 100644
--- a/perllib/FixMyStreet/Roles/ConfirmOpen311.pm
+++ b/perllib/FixMyStreet/Roles/ConfirmOpen311.pm
@@ -11,15 +11,19 @@ sub open311_config {
     my ($self, $row, $h, $params) = @_;
 
     $params->{multi_photos} = 1;
+}
+
+sub open311_extra_data {
+    my ($self, $row, $h, $extra) = @_;
 
-    my $extra = $row->get_extra_fields;
-    push @$extra,
+    my $open311_only = [
         { name => 'report_url',
           value => $h->{url} },
         { name => 'title',
           value => $row->title },
         { name => 'description',
-          value => $row->detail };
+          value => $row->detail },
+    ];
 
     # Reports made via FMS.com or the app probably won't have a USRN
     # value because we don't display the adopted highways layer on those
@@ -33,7 +37,7 @@ sub open311_config {
         }
     }
 
-    $row->set_extra_fields(@$extra);
+    return $open311_only;
 }
 
 1;
diff --git a/perllib/FixMyStreet/Script/Reports.pm b/perllib/FixMyStreet/Script/Reports.pm
index aa46a5c43..7c469f6ac 100644
--- a/perllib/FixMyStreet/Script/Reports.pm
+++ b/perllib/FixMyStreet/Script/Reports.pm
@@ -197,13 +197,12 @@ sub send(;$) {
         # Multiply results together, so one success counts as a success.
         my $result = -1;
 
-        my @methods;
         for my $sender ( keys %reporters ) {
             debug_print("sending using " . $sender, $row->id) if $debug_mode;
             $sender = $reporters{$sender};
             my $res = $sender->send( $row, \%h );
             $result *= $res;
-            push @methods, $sender if !$res;
+            $row->add_send_method($sender) if !$res;
             if ( $sender->unconfirmed_counts) {
                 foreach my $e (keys %{ $sender->unconfirmed_counts } ) {
                     foreach my $c (keys %{ $sender->unconfirmed_counts->{$e} }) {
@@ -216,12 +215,6 @@ sub send(;$) {
                 if FixMyStreet->test_mode && $sender->can('open311_test_req_used');
         }
 
-        # Add the send methods now because e.g. Open311
-        # send() calls $row->discard_changes
-        foreach (@methods) {
-            $row->add_send_method($_);
-        }
-
         unless ($result) {
             $row->update( {
                 whensent => \'current_timestamp',
diff --git a/perllib/FixMyStreet/SendReport/Open311.pm b/perllib/FixMyStreet/SendReport/Open311.pm
index c4532e3b3..e8e840ef5 100644
--- a/perllib/FixMyStreet/SendReport/Open311.pm
+++ b/perllib/FixMyStreet/SendReport/Open311.pm
@@ -37,10 +37,18 @@ sub send {
         my $contact = $self->fetch_category($body, $row) or next;
 
         my $cobrand = $body->get_cobrand_handler || $row->get_cobrand_logged;
-        $cobrand->call_hook(open311_config => $row, $h, \%open311_params, $contact);
+        $cobrand->call_hook(open311_config => $row, $h, \%open311_params);
 
         # Try and fill in some ones that we've been asked for, but not asked the user for
         my $extra = $row->get_extra_fields();
+        my ($include, $exclude) = $cobrand->call_hook(open311_extra_data => $row, $h, $extra, $contact);
+
+        my $original_extra = [ @$extra ];
+        push @$extra, @$include if $include;
+        if ($exclude) {
+            $exclude = join('|', @$exclude);
+            @$extra = grep { $_->{name} !~ /$exclude/ } @$extra;
+        }
 
         my $id_field = $contact->id_field;
         foreach (@{$contact->get_extra_fields}) {
@@ -81,8 +89,8 @@ sub send {
             $self->open311_test_req_used($open311->test_req_used);
         }
 
-        # make sure we don't save user changes from above
-        $row->discard_changes();
+        # make sure we don't save extra changes from above
+        $row->set_extra_fields( @$original_extra );
 
         if ( $resp ) {
             $row->external_id( $resp );
diff --git a/t/app/sendreport/open311.t b/t/app/sendreport/open311.t
index 3e2e7b3cb..4e3c179c2 100644
--- a/t/app/sendreport/open311.t
+++ b/t/app/sendreport/open311.t
@@ -1,5 +1,6 @@
 use FixMyStreet::Test;
 
+use Test::MockModule;
 use Test::Deep;
 
 use Open311;
@@ -8,6 +9,13 @@ use FixMyStreet::DB;
 
 use Data::Dumper;
 
+my $ukc = Test::MockModule->new('FixMyStreet::Cobrand::UKCouncils');
+$ukc->mock('lookup_site_code', sub {
+    my ($self, $row, $buffer) = @_;
+    is $row->latitude, 100, 'Correct latitude';
+    return "Road ID";
+});
+
 package main;
 sub test_overrides; # defined below
 
@@ -32,7 +40,7 @@ my %standard_open311_parameters = (
 test_overrides oxfordshire =>
     {
         body_name => 'Oxfordshire',
-        body_id   => 2237,
+        area_id   => 2237,
         row_data  => {
             postcode => 'OX1 1AA',
         },
@@ -44,7 +52,6 @@ test_overrides oxfordshire =>
     },
     superhashof({
         handler => isa('FixMyStreet::Cobrand::Oxfordshire'),
-        discard_changes => 1,
         'open311' => noclass(superhashof({
             %standard_open311_parameters,
             'extended_description' => 'oxfordshire',
@@ -60,7 +67,6 @@ test_overrides oxfordshire =>
 my $bromley_check =
     superhashof({
         handler => isa('FixMyStreet::Cobrand::Bromley'),
-        discard_changes => 1,
         'open311' => noclass(superhashof({
             %standard_open311_parameters,
             'send_notpinpointed' => 1,
@@ -83,7 +89,7 @@ my $bromley_check =
 test_overrides bromley =>
     {
         body_name => 'Bromley',
-        body_id   => 2482,
+        area_id   => 2482,
         row_data  => {
             postcode => 'BR1 1AA',
             extra => [ { name => 'last_name', value => 'Bloggs' } ],
@@ -92,14 +98,16 @@ test_overrides bromley =>
             northing => 100,
             easting => 100,
             url => 'http://example.com/1234',
+            prow_reference => 'ABC',
         },
     },
-    $bromley_check;
+    $bromley_check,
+    [ { name => 'last_name', value => 'Bloggs' } ];
 
 test_overrides fixmystreet =>
     {
         body_name => 'Bromley',
-        body_id   => 2482,
+        area_id   => 2482,
         row_data  => {
             postcode => 'BR1 1AA',
             # NB: we don't pass last_name here, as main cobrand doesn't know to do this!
@@ -115,7 +123,7 @@ test_overrides fixmystreet =>
 test_overrides greenwich =>
     {
         body_name => 'Greenwich',
-        body_id   => 2493,
+        area_id   => 2493,
     },
     superhashof({
         handler => isa('FixMyStreet::Cobrand::Greenwich'),
@@ -130,7 +138,7 @@ test_overrides greenwich =>
 test_overrides fixmystreet =>
     {
         body_name => 'West Berkshire',
-        body_id   => 2619,
+        area_id   => 2619,
         row_data  => {
             postcode => 'RG1 1AA',
         },
@@ -146,20 +154,45 @@ test_overrides fixmystreet =>
         })),
     });
 
+test_overrides fixmystreet =>
+    {
+        body_name => 'Cheshire East',
+        area_id   => 21069,
+        row_data  => {
+            postcode => 'CW11 1HZ',
+        },
+        extra => {
+            url => 'http://example.com/1234',
+        },
+    },
+    superhashof({
+        handler => isa('FixMyStreet::Cobrand::CheshireEast'),
+        'open311' => noclass(superhashof({
+            %standard_open311_parameters,
+        })),
+        problem_extra => bag(
+            { name => 'report_url' => value => 'http://example.com/1234' },
+            { name => 'title', value => 'Problem' },
+            { name => 'description', value => 'A big problem' },
+            { name => 'site_code', value => 'Road ID' },
+        ),
+    }),
+    [ { name => 'site_code', value => 'Road ID' } ];
+
 sub test_overrides {
     # NB: Open311 and ::SendReport::Open311 are mocked below in BEGIN { ... }
-    my ($cobrand, $input, $expected_data) = @_;
+    my ($cobrand, $input, $expected_data, $end_extra) = @_;
     subtest "$cobrand ($input->{body_name}) overrides" => sub {
 
         FixMyStreet::override_config {
-            ALLOWED_COBRANDS => ['fixmystreet', 'oxfordshire', 'bromley', 'westberkshire', 'greenwich'],
+            ALLOWED_COBRANDS => ['fixmystreet', 'oxfordshire', 'bromley', 'westberkshire', 'greenwich', 'cheshireeast'],
         }, sub {
             my $db = FixMyStreet::DB->schema;
             #$db->txn_begin;
 
-            my $params = { id => $input->{body_id}, name => $input->{body_name} };
+            my $params = { name => $input->{body_name} };
             my $body = $db->resultset('Body')->find_or_create($params);
-            $body->body_areas->find_or_create({ area_id => $input->{body_id} });
+            $body->body_areas->find_or_create({ area_id => $input->{area_id} });
             ok $body, "found/created body " . $input->{body_name};
             $body->update({ can_be_devolved => 1 });
 
@@ -172,7 +205,7 @@ sub test_overrides {
                 whenedited => DateTime->now,
                 jurisdiction => '1234',
                 api_key => 'SEEKRIT',
-                body_id => $input->{body_id},
+                body_id => $body->id,
             );
             $contact->update({ send_method => 'Open311', endpoint => 'http://example.com/open311' });
 
@@ -190,8 +223,8 @@ sub test_overrides {
                 name => 'Fred Bloggs',
                 anonymous => 1,
                 state => 'unconfirmed',
-                bodies_str => $input->{body_id},
-                areas => (sprintf ',%d,', $input->{body_id}),
+                bodies_str => $body->id,
+                areas => (sprintf ',%d,', $input->{area_id}),
                 category => 'ZZ',
                 cobrand => $cobrand,
                 user => $user,
@@ -206,6 +239,8 @@ sub test_overrides {
             $sr->add_body($body, $contact);
             $sr->send( $row, $input->{extra} || {} );
 
+            my $new_extra = $row->get_extra_fields;
+            cmp_deeply $new_extra, $end_extra || [];
             cmp_deeply (Open311->_get_test_data, $expected_data, 'Data as expected')
                 or diag Dumper( Open311->_get_test_data );
 
@@ -241,13 +276,6 @@ BEGIN {
     sub _get_test_data { return +{ %data } }
     sub _reset_test_data { %data = () }
 
-    package FixMyStreet::DB::Result::Problem;
-    use Class::Method::Modifiers; # is marked as immutable by Moose
-    sub discard_changes {
-        $data{discard_changes}++;
-        # no need to actually discard, as we're in transaction anyway
-    };
-
     package FixMyStreet::DB::Result::Body;
     use Class::Method::Modifiers; # is marked as immutable by Moose
     around get_cobrand_handler => sub {