Automatically spot Open311 co-ord/ID attributes.

6 files changed, 116 insertions, 49 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index 6934c6d79..cfb572d83 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -615,7 +615,7 @@ sub setup_categories_and_bodies : Private {
         unless ( $seen{$contact->category} ) {
             push @category_options, $contact->category;
 
-            my $metas = $contact->get_extra_fields;
+            my $metas = $contact->get_metadata_for_input;
             if (scalar @$metas) {
                 foreach (@$metas) {
                     if (ref $_->{values} eq 'HASH') {
@@ -861,7 +861,7 @@ sub process_report : Private {
 
         my @extra;
         foreach my $contact (@contacts) {
-            my $metas = $contact->get_extra_fields;
+            my $metas = $contact->get_metadata_for_input;
             foreach my $field ( @$metas ) {
                 if ( lc( $field->{required} ) eq 'true' ) {
                     unless ( $c->get_param($field->{code}) ) {
diff --git a/perllib/FixMyStreet/DB/Result/Contact.pm b/perllib/FixMyStreet/DB/Result/Contact.pm
index dab5432c6..58d8e58de 100644
--- a/perllib/FixMyStreet/DB/Result/Contact.pm
+++ b/perllib/FixMyStreet/DB/Result/Contact.pm
@@ -68,4 +68,15 @@ use namespace::clean -except => [ 'meta' ];
 
 with 'FixMyStreet::Roles::Extra';
 
+sub get_metadata_for_input {
+    my $self = shift;
+    my $id_field = $self->id_field;
+    return [ grep { $_->{code} !~ /^(easting|northing|$id_field)$/ } @{$self->get_extra_fields} ];
+}
+
+sub id_field {
+    my $self = shift;
+    return $self->get_extra_metadata('id_field') || 'fixmystreet_id';
+}
+
 1;
diff --git a/perllib/FixMyStreet/SendReport/Open311.pm b/perllib/FixMyStreet/SendReport/Open311.pm
index 9dfd3e0ea..ee40f371a 100644
--- a/perllib/FixMyStreet/SendReport/Open311.pm
+++ b/perllib/FixMyStreet/SendReport/Open311.pm
@@ -12,6 +12,7 @@ use Readonly;
 Readonly::Scalar my $COUNCIL_ID_OXFORDSHIRE => 2237;
 Readonly::Scalar my $COUNCIL_ID_WARWICKSHIRE => 2243;
 Readonly::Scalar my $COUNCIL_ID_GREENWICH => 2493;
+Readonly::Scalar my $COUNCIL_ID_BROMLEY => 2482;
 
 has open311_test_req_used => (
     is => 'rw',
@@ -32,21 +33,11 @@ sub send {
 
         my $extended_desc = 1;
 
-        # To rollback temporary changes made by this function
-        my $revert = 0;
+        my $extra = $row->get_extra_fields();
 
         # Extra bromley fields
-        if ( $row->bodies_str eq '2482' ) {
-
-            $revert = 1;
-
-            my $extra = $row->get_extra_fields();
-            if ( $row->used_map || ( !$row->used_map && !$row->postcode ) ) {
-                push @$extra, { name => 'northing', value => $h->{northing} };
-                push @$extra, { name => 'easting', value => $h->{easting} };
-            }
+        if ( $row->bodies_str eq $COUNCIL_ID_BROMLEY ) {
             push @$extra, { name => 'report_url', value => $h->{url} };
-            push @$extra, { name => 'service_request_id_ext', value => $row->id };
             push @$extra, { name => 'report_title', value => $row->title };
             push @$extra, { name => 'public_anonymity_required', value => $row->anonymous ? 'TRUE' : 'FALSE' };
             push @$extra, { name => 'email_alerts_requested', value => 'FALSE' }; # always false as can never request them
@@ -58,41 +49,47 @@ sub send {
                 my ( $firstname, $lastname ) = ( $row->name =~ /(\w+)\.?\s+(.+)/ );
                 push @$extra, { name => 'last_name', value => $lastname };
             }
-            $row->set_extra_fields( @$extra );
-
             $always_send_latlong = 0;
             $send_notpinpointed = 1;
-            $use_service_as_deviceid = 0;
             $extended_desc = 0;
-        }
-
-        # extra Oxfordshire fields: send nearest street, postcode, northing and easting, and the FMS id
-        if ( $row->bodies_str =~ /\b(?:$COUNCIL_ID_OXFORDSHIRE|$COUNCIL_ID_WARWICKSHIRE)\b/ ) {
-
-            my $extra = $row->get_extra_fields;
+        } elsif ( $row->bodies_str =~ /\b$COUNCIL_ID_OXFORDSHIRE\b/ ) {
+            # Oxfordshire doesn't have category metadata to fill these
+            $extended_desc = 'oxfordshire';
             push @$extra, { name => 'external_id', value => $row->id };
             push @$extra, { name => 'closest_address', value => $h->{closest_address} } if $h->{closest_address};
             if ( $row->used_map || ( !$row->used_map && !$row->postcode ) ) {
                 push @$extra, { name => 'northing', value => $h->{northing} };
                 push @$extra, { name => 'easting', value => $h->{easting} };
             }
-            $row->set_extra_fields( @$extra );
-
-            if ($row->bodies_str =~ /$COUNCIL_ID_OXFORDSHIRE/) {
-                $extended_desc = 'oxfordshire';
-            }
-            elsif ($row->bodies_str =~ /$COUNCIL_ID_WARWICKSHIRE/) {
-                $extended_desc = 'warwickshire';
-            }
+        } elsif ( $row->bodies_str =~ /\b$COUNCIL_ID_WARWICKSHIRE\b/ ) {
+            $extended_desc = 'warwickshire';
+            push @$extra, { name => 'closest_address', value => $h->{closest_address} } if $h->{closest_address};
+        } elsif ( $row->bodies_str == $COUNCIL_ID_GREENWICH ) {
+            # Greenwich doesn't have category metadata to fill this
+            push @$extra, { name => 'external_id', value => $row->id };
         }
 
-        # FIXME: we've already looked this up before
+        # Try and fill in some ones that we've been asked for, but not asked the user for
+
         my $contact = $row->result_source->schema->resultset("Contact")->find( {
             deleted => 0,
             body_id => $body->id,
             category => $row->category
         } );
 
+        my $id_field = $contact->id_field;
+        foreach (@{$contact->get_extra_fields}) {
+            if ($_->{code} eq $id_field) {
+                push @$extra, { name => $id_field, value => $row->id };
+            } elsif ($_->{code} =~ /^(easting|northing)$/) {
+                if ( $row->used_map || ( !$row->used_map && !$row->postcode ) ) {
+                    push @$extra, { name => $_->{code}, value => $h->{$_->{code}} };
+                }
+            }
+        }
+
+        $row->set_extra_fields( @$extra ) if @$extra;
+
         my %open311_params = (
             jurisdiction            => $conf->jurisdiction,
             endpoint                => $conf->endpoint,
@@ -121,19 +118,11 @@ sub send {
         # non-standard Oxfordshire endpoint (because it's just a script, not a full Open311 service)
         if ( $row->bodies_str =~ /$COUNCIL_ID_OXFORDSHIRE/ ) {
             $open311->endpoints( { requests => 'open311_service_request.cgi' } );
-            $revert = 1;
         }
 
         # required to get round issues with CRM constraints
         if ( $row->bodies_str =~ /2218/ ) {
             $row->user->name( $row->user->id . ' ' . $row->user->name );
-            $revert = 1;
-        }
-
-        if ($row->bodies_str =~ /$COUNCIL_ID_GREENWICH/) {
-            # Greenwich endpoint expects external_id as an attribute
-            $row->set_extra_fields( { 'name' => 'external_id', 'value' => $row->id  } );
-            $revert = 1;
         }
 
         my $resp = $open311->send_service_request( $row, $h, $contact->email );
@@ -142,7 +131,7 @@ sub send {
         }
 
         # make sure we don't save user changes from above
-        $row->discard_changes() if $revert;
+        $row->discard_changes();
 
         if ( $resp ) {
             $row->external_id( $resp );
diff --git a/perllib/Open311/PopulateServiceList.pm b/perllib/Open311/PopulateServiceList.pm
index 949e2baa9..c5f17334b 100644
--- a/perllib/Open311/PopulateServiceList.pm
+++ b/perllib/Open311/PopulateServiceList.pm
@@ -227,26 +227,26 @@ sub _add_meta_to_contact {
     # for attributes which we *don't* want to display to the user (e.g. as
     # fields in "category_extras"
 
+    if ($self->_current_body->name eq 'Bromley Council') {
+        $contact->set_extra_metadata( id_field => 'service_request_id_ext');
+    } elsif ($self->_current_body->name eq 'Warwickshire County Council') {
+        $contact->set_extra_metadata( id_field => 'external_id');
+    }
+
     my %override = (
         #2482
         'Bromley Council' => [qw(
-            service_request_id_ext
             requested_datetime
             report_url
             title
             last_name
             email
-            easting
-            northing
             report_title
             public_anonymity_required
             email_alerts_requested
         ) ],
         #2243, 
         'Warwickshire County Council' => [qw(
-            external_id
-            easting
-            northing
             closest_address
         ) ],
     );
diff --git a/t/cobrand/bromley.t b/t/cobrand/bromley.t
index e39bcbe4c..43d936684 100644
--- a/t/cobrand/bromley.t
+++ b/t/cobrand/bromley.t
@@ -2,17 +2,25 @@ use strict;
 use warnings;
 use Test::More;
 
+use CGI::Simple;
 use FixMyStreet::TestMech;
 my $mech = FixMyStreet::TestMech->new;
 
 # Create test data
 my $user = $mech->create_user_ok( 'bromley@example.com' );
 my $body = $mech->create_body_ok( 2482, 'Bromley Council', id => 2482 );
-$mech->create_contact_ok(
+my $contact = $mech->create_contact_ok(
     body_id => $body->id,
     category => 'Other',
     email => 'LIGHT',
 );
+$contact->set_extra_metadata(id_field => 'service_request_id_ext');
+$contact->set_extra_fields(
+    { code => 'easting', datatype => 'number', },
+    { code => 'northing', datatype => 'number', },
+    { code => 'service_request_id_ext', datatype => 'number', },
+);
+$contact->update;
 
 my @reports = $mech->create_problems_for_body( 1, $body->id, 'Test', {
     cobrand => 'bromley',
@@ -45,15 +53,24 @@ subtest 'testing special Open311 behaviour', sub {
     $report->set_extra_fields();
     $report->update;
     $body->update( { send_method => 'Open311', endpoint => 'http://bromley.endpoint.example.com', jurisdiction => 'FMS', api_key => 'test' } );
+    my $test_data;
     FixMyStreet::override_config {
         SEND_REPORTS_ON_STAGING => 1,
+        ALLOWED_COBRANDS => [ 'fixmystreet', 'bromley' ],
     }, sub {
-        FixMyStreet::DB->resultset('Problem')->send_reports();
+        $test_data = FixMyStreet::DB->resultset('Problem')->send_reports();
     };
     $report->discard_changes;
     ok $report->whensent, 'Report marked as sent';
     is $report->send_method_used, 'Open311', 'Report sent via Open311';
     is $report->external_id, 248, 'Report has right external ID';
+
+    my $req = $test_data->{test_req_used};
+    my $c = CGI::Simple->new($req->content);
+    is $c->param('attribute[easting]'), 529025, 'Request had easting';
+    is $c->param('attribute[northing]'), 179716, 'Request had northing';
+    is $c->param('attribute[service_request_id_ext]'), $report->id, 'Request had correct ID';
+    is $c->param('jurisdiction_id'), 'FMS', 'Request had correct jurisdiction';
 };
 
 for my $test (
diff --git a/t/sendreport/open311.t b/t/sendreport/open311.t
new file mode 100644
index 000000000..636faba31
--- /dev/null
+++ b/t/sendreport/open311.t
@@ -0,0 +1,50 @@
+use strict;
+use warnings;
+use Test::More;
+
+use CGI::Simple;
+use FixMyStreet::TestMech;
+my $mech = FixMyStreet::TestMech->new;
+
+my $user = $mech->create_user_ok( 'eh@example.com' );
+my $body = $mech->create_body_ok( 2342, 'East Hertfordshire Council');
+my $contact = $mech->create_contact_ok( body_id => $body->id, category => 'Potholes', email => 'POT' );
+$contact->set_extra_fields(
+    { code => 'easting', datatype => 'number' },
+    { code => 'northing', datatype => 'number' },
+    { code => 'fixmystreet_id', datatype => 'number' },
+);
+$contact->update;
+
+my ($report) = $mech->create_problems_for_body( 1, $body->id, 'Test', {
+    cobrand => 'fixmystreet',
+    category => 'Potholes',
+    user => $user,
+});
+
+subtest 'testing Open311 behaviour', sub {
+    $body->update( { send_method => 'Open311', endpoint => 'http://endpoint.example.com', jurisdiction => 'FMS', api_key => 'test' } );
+    my $test_data;
+    FixMyStreet::override_config {
+        SEND_REPORTS_ON_STAGING => 1,
+        ALLOWED_COBRANDS => [ 'fixmystreet' ],
+    }, sub {
+        $test_data = FixMyStreet::DB->resultset('Problem')->send_reports();
+    };
+    $report->discard_changes;
+    ok $report->whensent, 'Report marked as sent';
+    is $report->send_method_used, 'Open311', 'Report sent via Open311';
+    is $report->external_id, 248, 'Report has right external ID';
+
+    my $req = $test_data->{test_req_used};
+    my $c = CGI::Simple->new($req->content);
+    is $c->param('attribute[easting]'), 529025, 'Request had easting';
+    is $c->param('attribute[northing]'), 179716, 'Request had northing';
+    is $c->param('attribute[fixmystreet_id]'), $report->id, 'Request had correct ID';
+    is $c->param('jurisdiction_id'), 'FMS', 'Request had correct jurisdiction';
+};
+
+# Clean up
+$mech->delete_user($user);
+$mech->delete_body($body);
+done_testing();