[UK] Remove requirement for fixed body IDs.

Historically in UK cobrands, bodies have had IDs the same as the MapIt
area ID they cover. This can be confusing (if you are setting up a dev
environment, say) and should not be necessary. This commit removes the
requirement entirely, by switching any ID checks to either the name of
the body, or the actual area it covers.

One note: the body name in the test has to match so that we do not get
two bodies both covering 2237 created. This will not be necessary when
the tests are compartmentalized in the next commit.

23 files changed, 70 insertions, 63 deletions

diff --git a/perllib/FixMyStreet/App/Controller/JSON.pm b/perllib/FixMyStreet/App/Controller/JSON.pm
index d3cd33546..762e3c115 100644
--- a/perllib/FixMyStreet/App/Controller/JSON.pm
+++ b/perllib/FixMyStreet/App/Controller/JSON.pm
@@ -105,10 +105,9 @@ sub problems : Local {
     foreach my $problem (@problems) {
         $problem->name( '' ) if $problem->anonymous == 1;
         $problem->service( 'Web interface' ) if $problem->service eq '';
-        my $bodies = $problem->bodies;
-        if (keys %$bodies) {
-             my @body_names = map { $_->name } values %$bodies;
-             $problem->bodies_str( join(' and ', @body_names) );
+        my $body_names = $problem->body_names;
+        if (@$body_names) {
+             $problem->bodies_str( join(' and ', @$body_names) );
         }
     }
 
diff --git a/perllib/FixMyStreet/App/Controller/Open311.pm b/perllib/FixMyStreet/App/Controller/Open311.pm
index bc08593de..6829e01ae 100644
--- a/perllib/FixMyStreet/App/Controller/Open311.pm
+++ b/perllib/FixMyStreet/App/Controller/Open311.pm
@@ -258,8 +258,8 @@ sub output_requests : Private {
         }
         else {
             # FIXME Not according to Open311 v2
-            my @body_names = map { $_->name } values %{$problem->bodies};
-            $request->{agency_responsible} = {'recipient' => [ @body_names ] };
+            my $body_names = $problem->body_names;
+            $request->{agency_responsible} = {'recipient' => $body_names };
         }
 
         if ( !$problem->anonymous ) {
diff --git a/perllib/FixMyStreet/App/Controller/Report.pm b/perllib/FixMyStreet/App/Controller/Report.pm
index 368bbcf27..b0a98c9d0 100644
--- a/perllib/FixMyStreet/App/Controller/Report.pm
+++ b/perllib/FixMyStreet/App/Controller/Report.pm
@@ -203,7 +203,8 @@ sub format_problem_for_display : Private {
         $c->stash->{add_alert} = 1;
     }
 
-    $c->stash->{extra_name_info} = $problem->bodies_str && $problem->bodies_str eq '2482' ? 1 : 0;
+    my $first_body = (values %{$problem->bodies})[0];
+    $c->stash->{extra_name_info} = $first_body && $first_body->name =~ /Bromley/ ? 1 : 0;
 
     $c->forward('generate_map_tags');
 
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index bd7c5fa8d..823f4d08f 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -934,7 +934,7 @@ sub set_report_extras : Private {
         }
     }
 
-    $c->cobrand->process_open311_extras( $c, @$contacts[0]->body_id, \@extra )
+    $c->cobrand->process_open311_extras( $c, @$contacts[0]->body, \@extra )
         if ( scalar @$contacts );
 
     if ( @extra ) {
diff --git a/perllib/FixMyStreet/App/Controller/Report/Update.pm b/perllib/FixMyStreet/App/Controller/Report/Update.pm
index 261a49ec1..59171f97b 100644
--- a/perllib/FixMyStreet/App/Controller/Report/Update.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/Update.pm
@@ -307,7 +307,8 @@ sub process_update : Private {
     my @extra; # Next function fills this, but we don't need it here.
     # This is just so that the error checking for these extra fields runs.
     # TODO Use extra here as it is used on reports.
-    $c->cobrand->process_open311_extras( $c, $update->problem->bodies_str, \@extra );
+    my $body = (values %{$update->problem->bodies})[0];
+    $c->cobrand->process_open311_extras( $c, $body, \@extra );
 
     if ( $c->get_param('fms_extra_title') ) {
         my %extras = ();
diff --git a/perllib/FixMyStreet/Cobrand.pm b/perllib/FixMyStreet/Cobrand.pm
index 4b9f2bd0b..a0a076f67 100644
--- a/perllib/FixMyStreet/Cobrand.pm
+++ b/perllib/FixMyStreet/Cobrand.pm
@@ -158,8 +158,8 @@ sub body_handler {
 
     foreach my $avail ( $class->available_cobrand_classes ) {
         my $cobrand = $class->get_class_for_moniker($avail->{moniker})->new({});
-        next unless $cobrand->can('council_id');
-        return $cobrand if $areas->{$cobrand->council_id};
+        next unless $cobrand->can('council_area_id');
+        return $cobrand if $areas->{$cobrand->council_area_id};
     }
 }
 
diff --git a/perllib/FixMyStreet/Cobrand/Angus.pm b/perllib/FixMyStreet/Cobrand/Angus.pm
index 0361c2d11..51a3da56a 100644
--- a/perllib/FixMyStreet/Cobrand/Angus.pm
+++ b/perllib/FixMyStreet/Cobrand/Angus.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2550; }
+sub council_area_id { return 2550; }
 sub council_area { return 'Angus'; }
 sub council_name { return 'Angus Council'; }
 sub council_url { return 'angus'; }
@@ -70,13 +70,17 @@ sub temp_update_contacts {
 
     my $contact_rs = $self->{c}->model('DB::Contact');
 
+    my $body = FixMyStreet::DB->resultset('Body')->search({
+        'body_areas.area_id' => $self->council_area_id,
+    }, { join => 'body_areas' })->first;
+
     my $_update = sub {
         my ($category, $field, $category_details) = @_;
         # NB: we're accepting just 1 field, but supply as array [ $field ]
 
         my $contact = $contact_rs->find_or_create(
             {
-                body_id => $self->council_id,
+                body => $body,
                 category => $category,
                 %{ $category_details || {} },
             },
diff --git a/perllib/FixMyStreet/Cobrand/Borsetshire.pm b/perllib/FixMyStreet/Cobrand/Borsetshire.pm
index 887de57ed..89869ff1b 100644
--- a/perllib/FixMyStreet/Cobrand/Borsetshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Borsetshire.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::Whitelabel';
 use strict;
 use warnings;
 
-sub council_id { return 2608; }
+sub council_area_id { return 2608; }
 sub council_area { return 'Borsetshire'; }
 sub council_name { return 'Borsetshire County Council'; }
 sub council_url { return 'demo'; }
diff --git a/perllib/FixMyStreet/Cobrand/Bristol.pm b/perllib/FixMyStreet/Cobrand/Bristol.pm
index fa7f98666..2876ff782 100644
--- a/perllib/FixMyStreet/Cobrand/Bristol.pm
+++ b/perllib/FixMyStreet/Cobrand/Bristol.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2561; }
+sub council_area_id { return 2561; }
 sub council_area { return 'Bristol'; }
 sub council_name { return 'Bristol County Council'; }
 sub council_url { return 'bristol'; }
diff --git a/perllib/FixMyStreet/Cobrand/Bromley.pm b/perllib/FixMyStreet/Cobrand/Bromley.pm
index 169175947..e7d5e186a 100644
--- a/perllib/FixMyStreet/Cobrand/Bromley.pm
+++ b/perllib/FixMyStreet/Cobrand/Bromley.pm
@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use DateTime::Format::W3CDTF;
 
-sub council_id { return 2482; }
+sub council_area_id { return 2482; }
 sub council_area { return 'Bromley'; }
 sub council_name { return 'Bromley Council'; }
 sub council_url { return 'bromley'; }
diff --git a/perllib/FixMyStreet/Cobrand/EastHerts.pm b/perllib/FixMyStreet/Cobrand/EastHerts.pm
index ea5ed7f55..0e60c6b08 100644
--- a/perllib/FixMyStreet/Cobrand/EastHerts.pm
+++ b/perllib/FixMyStreet/Cobrand/EastHerts.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2342; }
+sub council_area_id { return 2342; }
 sub council_area { return 'East Hertfordshire'; }
 sub council_name { return 'East Hertfordshire District Council'; }
 sub council_url { return 'eastherts'; }
@@ -51,4 +51,4 @@ sub contact_email {
     return join( '@', 'enquiries', 'eastherts.gov.uk' );
 }
 
-1;
\ No newline at end of file
+1;
diff --git a/perllib/FixMyStreet/Cobrand/Greenwich.pm b/perllib/FixMyStreet/Cobrand/Greenwich.pm
index 700a12782..ce4fae381 100644
--- a/perllib/FixMyStreet/Cobrand/Greenwich.pm
+++ b/perllib/FixMyStreet/Cobrand/Greenwich.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2493; }
+sub council_area_id { return 2493; }
 sub council_area { return 'Greenwich'; }
 sub council_name { return 'Royal Borough of Greenwich'; }
 sub council_url { return 'greenwich'; }
diff --git a/perllib/FixMyStreet/Cobrand/Hart.pm b/perllib/FixMyStreet/Cobrand/Hart.pm
index 42c4a636e..3ff2a2a19 100644
--- a/perllib/FixMyStreet/Cobrand/Hart.pm
+++ b/perllib/FixMyStreet/Cobrand/Hart.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2333; } # http://mapit.mysociety.org/area/2333.html
+sub council_area_id { return 2333; } # http://mapit.mysociety.org/area/2333.html
 sub council_area { return 'Hart'; }
 sub council_name { return 'Hart Council'; }
 sub council_url { return 'hart'; }
diff --git a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
index 76d7ee31f..a061ff46c 100644
--- a/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Oxfordshire.pm
@@ -4,7 +4,7 @@ use base 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2237; }
+sub council_area_id { return 2237; }
 sub council_area { return 'Oxfordshire'; }
 sub council_name { return 'Oxfordshire County Council'; }
 sub council_url { return 'oxfordshire'; }
diff --git a/perllib/FixMyStreet/Cobrand/Stevenage.pm b/perllib/FixMyStreet/Cobrand/Stevenage.pm
index 2c305d326..28734b14b 100644
--- a/perllib/FixMyStreet/Cobrand/Stevenage.pm
+++ b/perllib/FixMyStreet/Cobrand/Stevenage.pm
@@ -4,7 +4,7 @@ use parent 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2347; }
+sub council_area_id { return 2347; }
 sub council_area { return 'Stevenage'; }
 sub council_name { return 'Stevenage Council'; }
 sub council_url { return 'stevenage'; }
diff --git a/perllib/FixMyStreet/Cobrand/UK.pm b/perllib/FixMyStreet/Cobrand/UK.pm
index 1a5682dad..e067405ee 100644
--- a/perllib/FixMyStreet/Cobrand/UK.pm
+++ b/perllib/FixMyStreet/Cobrand/UK.pm
@@ -32,12 +32,11 @@ sub disambiguate_location {
 sub process_open311_extras {
     my $self    = shift;
     my $ctx     = shift;
-    my $body_id = shift;
+    my $body = shift;
     my $extra   = shift;
     my $fields  = shift || [];
 
-    # XXX Hardcoded body ID matching mapit area ID
-    if ( $body_id eq '2482' ) {
+    if ( $body && $body->name =~ /Bromley/ ) {
         my @fields = ( 'fms_extra_title', @$fields );
         for my $field ( @fields ) {
             my $value = $ctx->get_param($field);
@@ -328,15 +327,11 @@ sub report_check_for_errors {
         );
     }
 
-    # XXX Hardcoded body ID matching mapit area ID
     if ( $report->bodies_str && $report->detail ) {
         # Custom character limit:
-        # Bromley Council
-        if ( $report->bodies_str eq '2482' && length($report->detail) > 1750 ) {
+        if ( $report->to_body_named('Bromley') && length($report->detail) > 1750 ) {
             $errors{detail} = sprintf( _('Reports are limited to %s characters in length. Please shorten your report'), 1750 );
-        }
-        # Oxfordshire
-        if ( $report->bodies_str eq '2237' && length($report->detail) > 1700 ) {
+        } elsif ( $report->to_body_named('Oxfordshire') && length($report->detail) > 1700 ) {
             $errors{detail} = sprintf( _('Reports are limited to %s characters in length. Please shorten your report'), 1700 );
         }
     }
diff --git a/perllib/FixMyStreet/Cobrand/UKCouncils.pm b/perllib/FixMyStreet/Cobrand/UKCouncils.pm
index e0b6b5298..0c044fb86 100644
--- a/perllib/FixMyStreet/Cobrand/UKCouncils.pm
+++ b/perllib/FixMyStreet/Cobrand/UKCouncils.pm
@@ -1,7 +1,5 @@
 package FixMyStreet::Cobrand::UKCouncils;
-use base 'FixMyStreet::Cobrand::UK';
-
-# XXX Things using this cobrand base assume that a body ID === MapIt area ID
+use parent 'FixMyStreet::Cobrand::UK';
 
 use strict;
 use warnings;
@@ -40,16 +38,25 @@ sub restriction {
     return { cobrand => shift->moniker };
 }
 
+# UK cobrands assume that each MapIt area ID maps both ways with one body
+sub body {
+    my $self = shift;
+    my $body = FixMyStreet::DB->resultset('Body')->search({
+        'body_areas.area_id' => $self->council_area_id
+    }, { join => 'body_areas' })->first;
+    return $body;
+}
+
 sub problems_restriction {
     my ($self, $rs) = @_;
     return $rs if FixMyStreet->staging_flag('skip_checks');
-    return $rs->to_body($self->council_id);
+    return $rs->to_body($self->body);
 }
 
 sub updates_restriction {
     my ($self, $rs) = @_;
     return $rs if FixMyStreet->staging_flag('skip_checks');
-    return $rs->to_body($self->council_id);
+    return $rs->to_body($self->body);
 }
 
 sub users_restriction {
@@ -75,7 +82,7 @@ sub users_restriction {
     )->as_query;
 
     my $or_query = [
-        from_body => $self->council_id,
+        from_body => $self->body->id,
         'me.id' => [ { -in => $problem_user_ids }, { -in => $update_user_ids } ],
     ];
     if ($self->can('admin_user_domain')) {
@@ -108,7 +115,7 @@ sub area_check {
     return 1 if FixMyStreet->staging_flag('skip_checks');
 
     my $councils = $params->{all_areas};
-    my $council_match = defined $councils->{$self->council_id};
+    my $council_match = defined $councils->{$self->council_area_id};
     if ($council_match) {
         return 1;
     }
@@ -164,7 +171,7 @@ sub owns_problem {
         @bodies = values %{$report->bodies};
     }
     my %areas = map { %{$_->areas} } @bodies;
-    return $areas{$self->council_id} ? 1 : undef;
+    return $areas{$self->council_area_id} ? 1 : undef;
 }
 
 # If the council is two-tier then show pins for the other council as grey
@@ -192,7 +199,7 @@ sub admin_allow_user {
     my ( $self, $user ) = @_;
     return 1 if $user->is_superuser;
     return undef unless defined $user->from_body;
-    return $user->from_body->id == $self->council_id;
+    return $user->from_body->areas->{$self->council_area_id};
 }
 
 sub available_permissions {
diff --git a/perllib/FixMyStreet/Cobrand/Warwickshire.pm b/perllib/FixMyStreet/Cobrand/Warwickshire.pm
index e52188311..5fa967c62 100644
--- a/perllib/FixMyStreet/Cobrand/Warwickshire.pm
+++ b/perllib/FixMyStreet/Cobrand/Warwickshire.pm
@@ -4,7 +4,7 @@ use base 'FixMyStreet::Cobrand::UKCouncils';
 use strict;
 use warnings;
 
-sub council_id { return 2243; }
+sub council_area_id { return 2243; }
 sub council_area { return 'Warwickshire'; }
 sub council_name { return 'Warwickshire County Council'; }
 sub council_url { return 'warwickshire'; }
diff --git a/perllib/FixMyStreet/Cobrand/WestBerkshire.pm b/perllib/FixMyStreet/Cobrand/WestBerkshire.pm
index 7e98187bb..e13d701a6 100644
--- a/perllib/FixMyStreet/Cobrand/WestBerkshire.pm
+++ b/perllib/FixMyStreet/Cobrand/WestBerkshire.pm
@@ -4,7 +4,7 @@ use base 'FixMyStreet::Cobrand::UK';
 use strict;
 use warnings;
 
-sub council_id { 2619 }
+sub council_area_id { 2619 }
 
 # non standard west berks end points
 sub open311_pre_send {
diff --git a/perllib/FixMyStreet/DB/Factories.pm b/perllib/FixMyStreet/DB/Factories.pm
index e877ffd9f..c3060507e 100644
--- a/perllib/FixMyStreet/DB/Factories.pm
+++ b/perllib/FixMyStreet/DB/Factories.pm
@@ -44,8 +44,6 @@ __PACKAGE__->fields({
 
 #######################
 
-# This currently creates special 'UK' bodies, with ID == MapIt area_id.
-# We should try and end this, it is just confusing.
 package FixMyStreet::DB::Factory::Body;
 
 use parent -norequire, "FixMyStreet::DB::Factory::Base";
@@ -56,10 +54,6 @@ __PACKAGE__->resultset(FixMyStreet::DB->resultset("Body"));
 __PACKAGE__->exclude(['area_id', 'categories']);
 
 __PACKAGE__->fields({
-    id => __PACKAGE__->callback(sub {
-        my $area_id = shift->get('area_id');
-        $area_id;
-    }),
     name => __PACKAGE__->callback(sub {
         my $area_id = shift->get('area_id');
         my $area = mySociety::MaPit::call('area', $area_id);
diff --git a/perllib/FixMyStreet/DB/Result/Comment.pm b/perllib/FixMyStreet/DB/Result/Comment.pm
index d59245c15..1218d2f76 100644
--- a/perllib/FixMyStreet/DB/Result/Comment.pm
+++ b/perllib/FixMyStreet/DB/Result/Comment.pm
@@ -128,9 +128,10 @@ sub check_for_errors {
       unless $self->text =~ m/\S/;
 
     # Bromley Council custom character limit
-    if ( $self->text && $self->problem && $self->problem->bodies_str
-        && $self->problem->bodies_str eq '2482' && length($self->text) > 1750 ) {
-        $errors{update} = sprintf( _('Updates are limited to %s characters in length. Please shorten your update'), 1750 );
+    if ( $self->text && $self->problem && $self->problem->bodies_str) {
+        if ($self->problem->to_body_named('Bromley') && length($self->text) > 1750) {
+            $errors{update} = sprintf( _('Updates are limited to %s characters in length. Please shorten your update'), 1750 );
+        }
     }
 
     return \%errors;
@@ -281,10 +282,7 @@ sub meta_line {
             $update_state = _( 'marked as an internal referral' )
         }
 
-        if ($c->cobrand->moniker eq 'bromley' || (
-                $self->problem->bodies_str &&
-                $self->problem->bodies_str eq '2482'
-        )) {
+        if ($c->cobrand->moniker eq 'bromley' || $self->problem->to_body_named('Bromley')) {
             if ($state eq 'not responsible') {
                 $update_state = 'marked as third party responsibility'
             }
diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm
index 6703a5102..f62ed6d47 100644
--- a/perllib/FixMyStreet/DB/Result/Problem.pm
+++ b/perllib/FixMyStreet/DB/Result/Problem.pm
@@ -510,6 +510,19 @@ sub bodies($) {
     return { map { $_->id => $_ } @bodies };
 }
 
+sub body_names($) {
+    my $self = shift;
+    my $bodies = $self->bodies;
+    my @names = map { $_->name } values %$bodies;
+    return \@names;
+}
+
+sub to_body_named($$) {
+    my ($self, $re) = @_;
+    my $names = join(',,', @{$self->body_names});
+    $names =~ /$re/;
+}
+
 =head2 url
 
 Returns a URL for this problem report.
@@ -774,7 +787,7 @@ sub defect_types {
 #     Note:   this only makes sense when called on a problem that has been sent!
 sub can_display_external_id {
     my $self = shift;
-    if ($self->external_id && $self->send_method_used && $self->bodies_str =~ /(2237|2550)/) {
+    if ($self->external_id && $self->send_method_used && $self->to_body_named('Oxfordshire|Angus')) {
         return 1;
     }
     return 0;
diff --git a/perllib/FixMyStreet/TestMech.pm b/perllib/FixMyStreet/TestMech.pm
index 094baab15..a698261cc 100644
--- a/perllib/FixMyStreet/TestMech.pm
+++ b/perllib/FixMyStreet/TestMech.pm
@@ -677,15 +677,10 @@ sub create_contact_ok {
 
 sub create_body_ok {
     my $self = shift;
-    my ( $area_id, $name, %extra ) = @_;
+    my ( $area_id, $name ) = @_;
 
     my $body = FixMyStreet::DB->resultset('Body');
-    my $params = { name => $name };
-    if ($extra{id}) {
-        $body = $body->update_or_create({ %$params, id => $extra{id} }, { key => 'primary' });
-    } else {
-        $body = $body->find_or_create($params);
-    }
+    $body = $body->find_or_create({ name => $name });
     ok $body, "found/created body $name";
 
     $body->body_areas->delete;