From ba3e30d96f0078388f77c58652241f07dd07105b Mon Sep 17 00:00:00 2001 From: Moray Jones Date: Wed, 21 Aug 2024 12:47:20 +0100 Subject: [PATCH] [Camden] Adds boundary check against neighbours Camden have provided a responsibility layer around boundary areas - we check against this to see who is responsible for a location that may be outside their defined boundary area and set the body responsible accordingly. https://github.com/mysociety/societyworks/issues/4399 --- perllib/FixMyStreet/Cobrand/Brent.pm | 38 +++---- perllib/FixMyStreet/Cobrand/Camden.pm | 109 ++++++++++++--------- perllib/FixMyStreet/Cobrand/FixMyStreet.pm | 5 + perllib/FixMyStreet/Cobrand/Westminster.pm | 2 +- t/cobrand/brent.t | 25 ++--- t/cobrand/camden.t | 101 ++++++++++++++++++- 6 files changed, 192 insertions(+), 88 deletions(-) diff --git a/perllib/FixMyStreet/Cobrand/Brent.pm b/perllib/FixMyStreet/Cobrand/Brent.pm index 7f298345cf0..1b5f0d44323 100644 --- a/perllib/FixMyStreet/Cobrand/Brent.pm +++ b/perllib/FixMyStreet/Cobrand/Brent.pm @@ -221,8 +221,15 @@ sub munge_overlapping_asset_bodies { $_->name ne 'Harrow Borough Council' } values %$bodies; } else { + # Camden wants sole responsibility of Camden agreed areas + if ($cobrand eq 'Camden') { + %$bodies = map { $_->id => $_ } grep { + $_->name eq 'Camden Borough Council' || $_->name eq 'TfL' || $_->name eq 'National Highways' + } values %$bodies; + return; + } # ...someone else's responsibility, take out the ones definitely not responsible - my %cobrands = (Harrow => 'Harrow Borough Council', Camden => 'Camden Borough Council', Barnet => 'Barnet Borough Council'); + my %cobrands = (Harrow => 'Harrow Borough Council', Barnet => 'Barnet Borough Council'); my $selected = $cobrands{$cobrand}; %$bodies = map { $_->id => $_ } grep { $_->name eq $selected || $_->name eq 'Brent Council' || $_->name eq 'TfL' || $_->name eq 'National Highways' @@ -236,7 +243,12 @@ sub munge_overlapping_asset_bodies { $_->name ne 'Brent Council' } values %$bodies; } else { - # ...Brent's responsibility - leave (both) bodies alone + # If it's for Brent shared with Camden, make wholly Brent's responsibility + if (grep { $_->name eq 'Camden Borough Council' } values %$bodies) { + %$bodies = map { $_->id => $_ } grep { + $_->name eq 'Brent Council' || $_->name eq 'TfL' || $_->name eq 'National Highways' + } values %$bodies; + } } } } @@ -254,7 +266,6 @@ sub munge_cobrand_asset_categories { my %bodies = map { $_->body->name => $_->body } @$contacts; my %non_street = ( 'Barnet' => { map { $_ => 1 } @{ $self->_barnet_non_street } }, - 'Camden' => { map { $_ => 1 } @{ $self->_camden_non_street } }, 'Harrow' => { map { $_ => 1 } @{ $self->_harrow_non_street } }, ); @@ -262,14 +273,16 @@ sub munge_cobrand_asset_categories { my $in_area = grep ($self->council_area_id->[0] == $_, keys %{$self->{c}->stash->{all_areas}}); # cobrand will be true if the point is within an area of different responsibility from the norm my $cobrand = $self->check_report_is_on_cobrand_asset || ''; + return unless $cobrand; my $brent_body = $self->body->id; if (!$in_area && $cobrand eq 'Brent') { # Outside the area of Brent, but Brent's responsibility my $area; + # Camden do not mix categories with Brent if (grep {$_->{name} eq 'Camden Borough Council'} values %{$self->{c}->stash->{all_areas}}){ - $area = 'Camden'; + return; } elsif (grep {$_->{name} eq 'Harrow Borough Council'} values %{$self->{c}->stash->{all_areas}}) { $area = 'Harrow'; } elsif (grep {$_->{name} eq 'Barnet Borough Council'} values %{$self->{c}->stash->{all_areas}}) { @@ -283,6 +296,9 @@ sub munge_cobrand_asset_categories { @$contacts = grep { !(!$non_street{$area}{$_->category} && $_->body_id == $other_body->id) } @$contacts if $other_body; } elsif ($in_area && $cobrand ne 'Brent') { + if ($cobrand eq 'Camden') { + return; + } # Inside the area of Brent, but not Brent's responsibility my $other_body = $bodies{$cobrand . " Borough Council"}; # Remove the street contacts of Brent @@ -1666,20 +1682,6 @@ sub _barnet_non_street { ] }; -sub _camden_non_street { - return [ - 'Abandoned Vehicles', - 'Dead animal', - 'Flyposting', - 'Public toilets', - 'Recycling & rubbish (Missed bin)', - 'Dott e-bike / e-scooter', - 'Human Forest e-bike', - 'Lime e-bike / e-scooter', - 'Tier e-bike / e-scooter', - ] -} - sub _harrow_non_street { return [ 'Abandoned vehicles', diff --git a/perllib/FixMyStreet/Cobrand/Camden.pm b/perllib/FixMyStreet/Cobrand/Camden.pm index 6abe4c4c4b3..05cc5c5abd1 100644 --- a/perllib/FixMyStreet/Cobrand/Camden.pm +++ b/perllib/FixMyStreet/Cobrand/Camden.pm @@ -15,7 +15,17 @@ use parent 'FixMyStreet::Cobrand::Whitelabel'; use strict; use warnings; -sub council_area_id { return [2505, 2488]; } +sub council_area_id { + return [ + 2505, # Camden + 2488, # Brent + 2489, # Barnet + 2504, # Westminster + 2507, # Islington + 2509, # Haringey + 2512, # City of London + ]; +} sub council_area { return 'Camden'; } sub council_name { return 'Camden Council'; } sub council_url { return 'camden'; } @@ -146,7 +156,7 @@ sub user_from_oidc { If the location is covered by an area of differing responsibility (e.g. Brent in Camden, or Camden in Brent), return true (either 1 if an area name is -provided, or the name of the area if not). Identical to function in Brent.pm +provided, or the name of the area if not). =cut @@ -159,9 +169,9 @@ sub check_report_is_on_cobrand_asset { my $host = FixMyStreet->config('STAGING_SITE') ? "tilma.staging.mysociety.org" : "tilma.mysociety.org"; my $cfg = { - url => "https://$host/mapserver/brent", + url => "https://$host/mapserver/camden", srsname => "urn:ogc:def:crs:EPSG::27700", - typename => "BrentDiffs", + typename => "AgreementBoundaries", filter => "Geometry$x,$y", outputformat => 'GML3', }; @@ -170,11 +180,11 @@ sub check_report_is_on_cobrand_asset { if ($$features[0]) { if ($council_area) { - if ($$features[0]->{'ms:BrentDiffs'}->{'ms:name'} eq $council_area) { + if ($$features[0]->{'ms:AgreementBoundaries'}->{'ms:RESPBOROUG'} eq $council_area) { return 1; } } else { - return $$features[0]->{'ms:BrentDiffs'}->{'ms:name'}; + return $$features[0]->{'ms:AgreementBoundaries'}->{'ms:RESPBOROUG'}; } } } @@ -182,8 +192,9 @@ sub check_report_is_on_cobrand_asset { =head2 munge_overlapping_asset_bodies Alters the list of available bodies for the location, -depending on calculated responsibility. Here, we remove the -Brent body if we're inside Camden and it's not a Brent area. +depending on calculated responsibility. Here, we remove +any other council bodies if we're inside Camden or on the +Camden boundaries layer. =cut @@ -194,51 +205,38 @@ sub munge_overlapping_asset_bodies { my $in_area = grep ($self->council_area_id->[0] == $_, keys %{$self->{c}->stash->{all_areas}}); # cobrand will be true if the point is within an area of different responsibility from the norm my $cobrand = $self->check_report_is_on_cobrand_asset; - if ($in_area && !$cobrand) { - # Within Camden, and Camden's responsibility - %$bodies = map { $_->id => $_ } grep { - $_->name ne 'Brent Council' + + if ($in_area) { + if ($in_area && !$cobrand) { + # Within Camden, and Camden's responsibility + %$bodies = map { $_->id => $_ } grep { + $_->name ne 'Brent Council' && + $_->name ne 'Barnet Borough Council' && + $_->name ne 'Westminster City Council' && + $_->name ne 'Islington Borough Council' && + $_->name ne 'Haringey Borough Council' && + $_->name ne 'City of London Corporation' + } values %$bodies; + } else { + # ...read from the asset layer whose responsibility it is and keep them and TfL and National Highways + my $selected = &_boundary_councils->{$cobrand}; + %$bodies = map { $_->id => $_ } grep { + $_->name eq $selected || $_->name eq 'TfL' || $_->name eq 'National Highways' } values %$bodies; + } + } else { + # Not in the area of Camden... + if (!$cobrand || $cobrand ne 'LB Camden') { + # ...not Camden's responsibility - remove Camden + %$bodies = map { $_->id => $_ } grep { + $_->name ne 'Camden Borough Council' + } values %$bodies; + } else { + # ...Camden's responsibility - leave bodies alone + } } }; -=head2 munge_cobrand_asset_categories - -If we're in an overlapping area, we want to take the street categories -of one body, and the non-street categories of the other. - -=cut - -sub munge_cobrand_asset_categories { - my ($self, $contacts) = @_; - - # in_area will be true if the point is within the administrative area of Camden - my $in_area = grep ($self->council_area_id->[0] == $_, keys %{$self->{c}->stash->{all_areas}}); - # cobrand will be true if the point is within an area of different responsibility from the norm - my $cobrand = $self->check_report_is_on_cobrand_asset || ''; - - my $brent = FixMyStreet::Cobrand::Brent->new(); - my %non_street = map { $_ => 1 } @{ $brent->_camden_non_street } ; - my $brent_body = $brent->body; - my $camden_body = $self->body; - - if ($in_area && $cobrand eq 'Brent') { - # Within Camden, but Brent's responsibility - # Remove the non-street contacts of Brent - @$contacts = grep { !($_->email !~ /^Symology/ && $_->body_id == $brent_body->id) } @$contacts - if $brent_body; - # Remove the street contacts of Camden - @$contacts = grep { !(!$non_street{$_->category} && $_->body_id == $camden_body->id) } @$contacts; - } elsif (!$in_area && $cobrand eq 'Camden') { - # Outside Camden, but Camden's responsibility - # Remove the street contacts of Brent - @$contacts = grep { !($_->email =~ /^Symology/ && $_->body_id == $brent_body->id) } @$contacts - if $brent_body; - # Remove the non-street contacts of Camden - @$contacts = grep { !($non_street{$_->category} && $_->body_id == $camden_body->id) } @$contacts; - } -} - =head2 dashboard_export_problems_add_columns Has user name and email fields added to their csv export @@ -288,4 +286,17 @@ sub post_report_sent { } } +sub _boundary_councils { + return { + 'LB Brent' => 'Brent Council', + 'LB Barnet' => 'Barnet Borough Council', + 'LB Camden' => 'Camden Borough Council', + 'LB Westminster' => 'Westminster City Council', + 'LB Islington' => 'Islington Borough Council', + 'LB Haringey' => 'Haringey Borough Council', + 'City Of London' => 'City of London Corporation', + 'TfL' => 'Transport for London', + } +} + 1; diff --git a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm index 125302bfebf..2e2e2e75141 100644 --- a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm +++ b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm @@ -211,6 +211,11 @@ sub munge_report_new_bodies { $brent->munge_overlapping_asset_bodies($bodies); } + if ( $bodies{'Camden Borough Council'} ) { + my $camden = FixMyStreet::Cobrand::Camden->new({ c => $self->{c} }); + $camden->munge_overlapping_asset_bodies($bodies); + } + if ( $bodies{'Lewisham Borough Council'} ) { my $bromley = FixMyStreet::Cobrand::Bromley->new({ c => $self->{c} }); $bromley->munge_overlapping_asset_bodies($bodies); diff --git a/perllib/FixMyStreet/Cobrand/Westminster.pm b/perllib/FixMyStreet/Cobrand/Westminster.pm index 9f57709eec8..30d8a1bcf2a 100644 --- a/perllib/FixMyStreet/Cobrand/Westminster.pm +++ b/perllib/FixMyStreet/Cobrand/Westminster.pm @@ -25,7 +25,7 @@ use URI; =cut -sub council_area_id { return 2504; } +sub council_area_id { return [2504, 2505] } # 2505 Camden sub council_area { return 'Westminster'; } sub council_name { return 'Westminster City Council'; } sub council_url { return 'Westminster'; } diff --git a/t/cobrand/brent.t b/t/cobrand/brent.t index 581540e727e..f3b93eb03d3 100644 --- a/t/cobrand/brent.t +++ b/t/cobrand/brent.t @@ -529,12 +529,13 @@ FixMyStreet::override_config { subtest "categories on $host cobrand in Brent on Camden cobrand layer" => sub { $mech->host("$host.fixmystreet.com"); $brent_mock->mock('_fetch_features', sub { [{ 'ms:BrentDiffs' => { 'ms:name' => 'Camden' } } ]}); + $camden_mock->mock('_fetch_features', sub { [ { 'ms:AgreementBoundaries' => { 'ms:RESPBOROUG' => 'LB Camden' } } ] }); $mech->get_ok("/report/new/ajax?longitude=-0.28168&latitude=51.55904"); - is $mech->content_contains("Potholes"), 1, 'Brent category present'; + is $mech->content_lacks("Potholes"), 1, 'Brent category not present'; is $mech->content_lacks("Gully grid missing"), 1, 'Brent Symology category not present'; is $mech->content_contains("Sweeping"), 1, 'TfL category present'; is $mech->content_contains("Fly-tipping"), 1, 'Camden category present'; - is $mech->content_lacks("Dead animal"), 1, 'Camden non-street category not present'; + is $mech->content_contains("Dead animal"), 1, 'Camden non-street category present'; is $mech->content_lacks("Abandoned vehicles"), 1, 'Barnet non-street category not present'; is $mech->content_lacks("Parking"), 1, 'Barnet street category not present'; } @@ -576,13 +577,13 @@ FixMyStreet::override_config { $brent_mock->mock('_fetch_features', sub { [ { 'ms:BrentDiffs' => { 'ms:name' => 'Brent' } } ] }); $camden_mock->mock('_fetch_features', - sub { [ { 'ms:BrentDiffs' => { 'ms:name' => 'Brent' } } ] }); + sub { [ { 'ms:AgreementBoundaries' => { 'ms:RESPBOROUG' => 'LB Brent' } } ] }); $mech->get_ok("/report/new/ajax?longitude=-0.124514&latitude=51.529432"); - is $mech->content_lacks("Potholes"), 1, 'Brent category not present'; + is $mech->content_contains("Potholes"), 1, 'Brent category present'; is $mech->content_contains("Gully grid missing"), 1, 'Brent Symology category present'; is $mech->content_contains("Sweeping"), 1, 'TfL category present'; is $mech->content_lacks("Fly-tipping"), 1, 'Camden street category not present'; - is $mech->content_contains("Dead animal"), 1, 'Camden non-street category present'; + is $mech->content_lacks("Dead animal"), 1, 'Camden non-street category not present'; } }; @@ -617,20 +618,6 @@ FixMyStreet::override_config { is $mech->content_lacks('That location is not covered by Brent Council'), 1, 'Can not make report in Camden off asset'; }; - subtest "can access Brent from Camden on Camden asset layer" => sub { - $mech->host("camden.fixmystreet.com"); - $camden_mock->mock('_fetch_features', sub { [{ 'ms:BrentDiffs' => { 'ms:name' => 'Camden' } }] }); - $mech->get_ok("/report/new?longitude=-0.28168&latitude=51.55904"); - is $mech->content_lacks('That location is not covered by Camden Council'), 1, "Can make a report on Camden asset"; - }; - - subtest "can not access Brent from Camden not on asset layer" => sub { - $mech->host("camden.fixmystreet.com"); - $camden_mock->mock('_fetch_features', sub { [] }); - $mech->get_ok("/report/new?longitude=-0.28168&latitude=51.55904"); - is $mech->content_contains('That location is not covered by Camden Council'), 1, "Can make a report on Camden asset"; - }; - for my $test ( { council => 'Brent', diff --git a/t/cobrand/camden.t b/t/cobrand/camden.t index 46400f939f0..abbb9d9b442 100644 --- a/t/cobrand/camden.t +++ b/t/cobrand/camden.t @@ -14,9 +14,10 @@ my $tilma = t::Mock::Tilma->new; LWP::Protocol::PSGI->register($tilma->to_psgi_app, host => 'tilma.mysociety.org'); use constant CAMDEN_MAPIT_ID => 2505; +use constant BARNET_MAPIT_ID => 2489; my $comment_user = $mech->create_user_ok('camden@example.net'); -my $camden = $mech->create_body_ok(CAMDEN_MAPIT_ID, 'Camden Council', { +my $camden = $mech->create_body_ok(CAMDEN_MAPIT_ID, 'Camden Borough Council', { comment_user => $comment_user, }, { cobrand => 'camden' @@ -151,4 +152,102 @@ FixMyStreet::override_config { }; }; + my $barnet = $mech->create_body_ok(BARNET_MAPIT_ID, 'Barnet Borough Council'); + my $tfl = FixMyStreet::DB->resultset('Body')->search({ name => 'TfL'})->first; + $mech->create_contact_ok(body_id => $tfl->id, category => 'Bus stops', email => 'tfl@example.org'); + + FixMyStreet::DB->resultset('BodyArea')->find_or_create({ area_id => BARNET_MAPIT_ID, body_id => $tfl->id }); # TfL covers Barnet, already set to cover Camden + FixMyStreet::DB->resultset('BodyArea')->find_or_create({ area_id => BARNET_MAPIT_ID, body_id => $camden->id }); # Camden covers Barnet + FixMyStreet::DB->resultset('BodyArea')->find_or_create({ area_id => CAMDEN_MAPIT_ID, body_id => $barnet->id }); # Barnet covers Camden + + $mech->create_contact_ok( + body_id => $barnet->id, + category => 'Flytipping', + email => 'barnetflytipping@example.org', + send_method => 'Email' + ); + +for my $test ( + { + description => 'reporting in Camden area, not on boundary asset', + result => 'shows only Camden and TfL categories', + asset_returned => undef, + location => '/report/new/ajax?latitude=51.529432&longitude=-0.124514', + categories => ['Abandoned yellow bike', 'Bus stops', 'Potholes', 'River Piers', 'River Piers - Cleaning', 'River Piers Damage doors and glass'], + }, + { + description => 'reporting in Camden area, on boundary asset labelled Camden', + result => 'shows only Camden and TfL categories', + asset_returned => 'LB Camden', + location => '/report/new/ajax?latitude=51.529432&longitude=-0.124514', + categories => ['Abandoned yellow bike', 'Bus stops', 'Potholes', 'River Piers', 'River Piers - Cleaning', 'River Piers Damage doors and glass'], + }, + { + description => 'reporting in Camden area, on boundary asset labelled Barnet', + result => 'shows only Barnet and TfL categories', + asset_returned => 'LB Barnet', + location => '/report/new/ajax?latitude=51.529432&longitude=-0.124514', + categories => [ 'Bus stops', 'Flytipping', 'River Piers', 'River Piers - Cleaning', 'River Piers Damage doors and glass'], + }, + { + description => 'reporting in Barnet area, not on boundary asset', + result => 'not Camden\'s responsibility', + asset_returned => undef, + location => '/report/new/ajax?latitude=51.558568&longitude=-0.207702', + barnet_categories => [ 'Bus stops', 'Flytipping', 'River Piers', 'River Piers - Cleaning', 'River Piers Damage doors and glass'] + }, + { + description => 'reporting in Barnet area, on asset labelled Barnet', + result => 'shows only Barnet and TfL categories', + asset_returned => ['LB Barnet'], + location => '/report/new/ajax?latitude=51.558568&longitude=-0.207702', + categories => [ 'Bus stops', 'Flytipping', 'River Piers', 'River Piers - Cleaning', 'River Piers Damage doors and glass'], + }) +{ + FixMyStreet::override_config { + ALLOWED_COBRANDS => [ 'tfl', 'camden', 'fixmystreet' ], + MAPIT_URL => 'http://mapit.uk/', + }, sub { + my $camden_mock = Test::MockModule->new('FixMyStreet::Cobrand::Camden'); + for my $host ('fixmystreet.com', 'camden.fixmystreet.com') { + $mech->host($host); + if ($host =~ /camden/ && $test->{categories} ) { + @{$test->{categories}} = grep { $_ !~ /River Piers/ } @{$test->{categories}}; + } + subtest $test->{description} => sub { + $camden_mock->mock('check_report_is_on_cobrand_asset', sub { $test->{asset_returned} }); + my $json = $mech->get_ok_json($test->{location}); + if ($test->{categories}) { + is_deeply [sort keys %{$json->{by_category}}], $test->{categories}, $host . ': ' . $test->{result}; + } else { + if ($host =~ /camden/) { + $mech->content_contains('That location is not covered by Camden Council', 'camden.fixmystreet.com: can\'t make Barnet report'); + } else { + is_deeply [sort keys %{$json->{by_category}}], $test->{barnet_categories}, 'fixmystreet.com: making report in Barnet'; + } + } + }; + } + }; +}; + +FixMyStreet::override_config { + ALLOWED_COBRANDS => [ 'camden', 'tfl' ], + MAPIT_URL => 'http://mapit.uk/', +}, sub { + subtest 'Make a report to Barnet from Camden' => sub { + $mech->host('camden.fixmystreet.com'); + my $camden_mock = Test::MockModule->new('FixMyStreet::Cobrand::Camden'); + $camden_mock->mock('check_report_is_on_cobrand_asset', sub { 'LB Barnet' }); + $mech->get_ok('/report/new?latitude=51.529432&longitude=-0.124514'); + $mech->submit_form_ok({ with_fields => { + title => 'Report for Barnet', + detail => 'Test report details.', + category => 'Flytipping', + name => 'Gavin Stacey', + } }, "submit report"); + $mech->content_like(qr/passed this report on to.*Barnet Borough Council<\/b>/s); + } +}; + done_testing;