diff options
author | Dave Arter <davea@mysociety.org> | 2019-12-10 13:40:44 +0000 |
---|---|---|
committer | Dave Arter <davea@mysociety.org> | 2019-12-12 09:44:39 +0000 |
commit | b835f2db5823ba59bc4c843124b0debfdd5e9247 (patch) | |
tree | 73f0ca485ee7099e557857213934b5faa6874f8e | |
parent | f9c5f6d8162dc073cb3ec066460343df9ba7c8ba (diff) |
[TfL] Server-side red route lookup for new report categories
This commit checks the RedRoutes WFS layer on tilma to determine if the
point at which a new report is being made is on a TfL red route.
The returned categories are then adjusted accordingly:
- If on a red route, all TfL categories as well as borough categories
specific to street cleaning are returned.
- If not on a red route, all borough categories as well as TfL
categories that don't require a red route are returned.
- This category tweaking doesn't happen on the TfL cobrand, as the JS
handles it by signposting users to fixmystreet.com for borough
reports.
Doing the lookup server side means the app always shows the right
categories to the user and prevents them e.g. sending a borough
flytipping report to TfL.
Fixes https://github.com/mysociety/fixmystreet-commercial/issues/1716
-rw-r--r-- | perllib/FixMyStreet/Cobrand/FixMyStreet.pm | 11 | ||||
-rw-r--r-- | perllib/FixMyStreet/Cobrand/TfL.pm | 92 | ||||
-rw-r--r-- | perllib/FixMyStreet/Cobrand/UKCouncils.pm | 30 | ||||
-rw-r--r-- | perllib/FixMyStreet/Cobrand/Westminster.pm | 4 | ||||
-rw-r--r-- | t/Mock/MapIt.pm | 1 | ||||
-rw-r--r-- | t/Mock/Tilma.pm | 39 | ||||
-rw-r--r-- | t/app/controller/admin/templates.t | 2 | ||||
-rw-r--r-- | t/cobrand/hounslow.t | 10 | ||||
-rw-r--r-- | t/cobrand/tfl.t | 130 | ||||
-rw-r--r-- | web/cobrands/tfl/assets.js | 73 |
10 files changed, 299 insertions, 93 deletions
diff --git a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm index a6161b570..6e0a0e2a5 100644 --- a/perllib/FixMyStreet/Cobrand/FixMyStreet.pm +++ b/perllib/FixMyStreet/Cobrand/FixMyStreet.pm @@ -86,11 +86,7 @@ sub munge_reports_categories_list { sub munge_report_new_category_list { my ($self, $options, $contacts, $extras) = @_; - # No TfL Traffic Lights category in Hounslow my %bodies = map { $_->body->name => $_->body } @$contacts; - if ( $bodies{'Hounslow Borough Council'} ) { - @$options = grep { ($_->{category} || $_->category) !~ /^Traffic lights$/i } @$options; - } if ( $bodies{'Isle of Wight Council'} ) { my $user = $self->{c}->user; @@ -105,6 +101,13 @@ sub munge_report_new_category_list { my $seen = { map { $_->category => 1 } @$contacts }; @$options = grep { my $c = ($_->{category} || $_->category); $c =~ 'Pick a category' || $seen->{ $c } } @$options; } + + if ( $bodies{'TfL'} ) { + # Presented categories vary if we're on/off a red route + my $tfl = FixMyStreet::Cobrand->get_class_for_moniker( 'tfl' )->new({ c => $self->{c} }); + $tfl->munge_red_route_categories($options, $contacts); + } + } sub munge_load_and_group_problems { diff --git a/perllib/FixMyStreet/Cobrand/TfL.pm b/perllib/FixMyStreet/Cobrand/TfL.pm index c91b8a79c..b8c03c7a9 100644 --- a/perllib/FixMyStreet/Cobrand/TfL.pm +++ b/perllib/FixMyStreet/Cobrand/TfL.pm @@ -7,6 +7,8 @@ use warnings; use POSIX qw(strcoll); use FixMyStreet::MapIt; +use mySociety::ArrayUtils; +use Utils; sub council_area_id { return [ 2511, 2489, 2494, 2488, 2482, 2505, 2512, 2481, 2484, 2495, @@ -362,4 +364,94 @@ sub munge_sendreport_params { $params->{To} = \@munged_to; } +sub report_new_is_on_tlrn { + my ( $self ) = @_; + + my ($x, $y) = Utils::convert_latlon_to_en( + $self->{c}->stash->{latitude}, + $self->{c}->stash->{longitude}, + 'G' + ); + + my $cfg = { + url => "https://tilma.mysociety.org/mapserver/tfl", + srsname => "urn:ogc:def:crs:EPSG::27700", + typename => "RedRoutes", + filter => "<Filter><Contains><PropertyName>geom</PropertyName><gml:Point><gml:coordinates>$x,$y</gml:coordinates></gml:Point></Contains></Filter>", + }; + + my $features = $self->_fetch_features($cfg, $x, $y); + return scalar @$features ? 1 : 0; +} + +sub munge_report_new_category_list { } + +sub munge_red_route_categories { + my ($self, $options, $contacts) = @_; + if ( $self->report_new_is_on_tlrn ) { + # We're on a red route - only send TfL categories (except the disabled + # one that directs the user to borough for street cleaning XXX TODO: make sure this is included when on the TfL cobrand) and borough + # street cleaning categories. + my %cleaning_cats = map { $_ => 1 } @{ $self->_cleaning_categories }; + @$contacts = grep { + ( $_->body->name eq 'TfL' && $_->category ne $self->_tfl_council_category ) + || $cleaning_cats{$_->category} + || @{ mySociety::ArrayUtils::intersection( $self->_cleaning_groups, $_->groups ) } + } @$contacts; + } else { + # We're not on a red route - send all categories except + # TfL red-route-only. + my %tlrn_cats = map { $_ => 1 } @{ $self->_tlrn_categories }; + @$contacts = grep { !( $_->body->name eq 'TfL' && $tlrn_cats{$_->category } ) } @$contacts; + } + my $seen = { map { $_->category => 1 } @$contacts }; + @$options = grep { my $c = ($_->{category} || $_->category); $c =~ 'Pick a category' || $seen->{ $c } } @$options; +} + +# Reports in these categories can only be made on a red route +sub _tlrn_categories { [ + "All out - three or more street lights in a row", + "Blocked drain", + "Damage - general (Trees)", + "Dead animal in the carriageway or footway", + "Debris in the carriageway", + "Fallen Tree", + "Flooding", + "Flytipping (TfL)", + "Graffiti / Flyposting (non-offensive)", + "Graffiti / Flyposting (offensive)", + "Graffiti / Flyposting on street light (non-offensive)", + "Graffiti / Flyposting on street light (offensive)", + "Grass Cutting and Hedges", + "Hoardings blocking carriageway or footway", + "Light on during daylight hours", + "Lights out in Pedestrian Subway", + "Low hanging branches and general maintenance", + "Manhole Cover - Damaged (rocking or noisy)", + "Manhole Cover - Missing", + "Mobile Crane Operation", + "Other (TfL)", + "Pavement Defect (uneven surface / cracked paving slab)", + "Pothole", + "Roadworks", + "Scaffolding blocking carriageway or footway", + "Single Light out (street light)", + "Standing water", + "Unstable hoardings", + "Unstable scaffolding", + "Worn out road markings", +] } + +sub _cleaning_categories { [ + 'Street cleaning', + 'Street Cleaning', + 'Accumulated Litter', + 'Street Cleaning Enquiry', + 'Street Cleansing', +] } + +sub _cleaning_groups { [ 'Street cleaning' ] } + +sub _tfl_council_category { 'General Litter / Rubbish Collection' } + 1; diff --git a/perllib/FixMyStreet/Cobrand/UKCouncils.pm b/perllib/FixMyStreet/Cobrand/UKCouncils.pm index ef0bcf4fb..97c8b6c81 100644 --- a/perllib/FixMyStreet/Cobrand/UKCouncils.pm +++ b/perllib/FixMyStreet/Cobrand/UKCouncils.pm @@ -289,6 +289,18 @@ sub prefill_report_fields_for_inspector { 1 } sub social_auth_disabled { 1 } +sub munge_report_new_category_list { + my ($self, $options, $contacts, $extras) = @_; + + my %bodies = map { $_->body->name => $_->body } @$contacts; + if ( $bodies{'TfL'} ) { + # Presented categories vary if we're on/off a red route + my $tfl = FixMyStreet::Cobrand->get_class_for_moniker( 'tfl' )->new({ c => $self->{c} }); + $tfl->munge_red_route_categories($options, $contacts); + } +} + + =head2 lookup_site_code Reports made via FMS.com or the app probably won't have a site code @@ -317,10 +329,16 @@ sub lookup_site_code { sub _fetch_features { my ($self, $cfg, $x, $y) = @_; - my $buffer = $cfg->{buffer}; - my ($w, $s, $e, $n) = ($x-$buffer, $y-$buffer, $x+$buffer, $y+$buffer); - my $uri = $self->_fetch_features_url($cfg, $w, $s, $e,$n); + # default to a buffered bounding box around the given point unless + # a custom filter parameter has been specified. + unless ( $cfg->{filter} ) { + my $buffer = $cfg->{buffer}; + my ($w, $s, $e, $n) = ($x-$buffer, $y-$buffer, $x+$buffer, $y+$buffer); + $cfg->{bbox} = "$w,$s,$e,$n"; + } + + my $uri = $self->_fetch_features_url($cfg); my $response = get($uri) or return; my $j = JSON->new->utf8->allow_nonref; @@ -336,7 +354,7 @@ sub _fetch_features { } sub _fetch_features_url { - my ($self, $cfg, $w, $s, $e, $n) = @_; + my ($self, $cfg) = @_; my $uri = URI->new($cfg->{url}); $uri->query_form( @@ -346,7 +364,7 @@ sub _fetch_features_url { TYPENAME => $cfg->{typename}, VERSION => "1.1.0", outputformat => "geojson", - BBOX => "$w,$s,$e,$n" + $cfg->{filter} ? ( Filter => $cfg->{filter} ) : ( BBOX => $cfg->{bbox} ), ); return $uri; @@ -372,7 +390,7 @@ sub _nearest_feature { next unless $accept_types->{$feature->{geometry}->{type}}; my @linestrings = @{ $feature->{geometry}->{coordinates} }; - if ( $feature->{geometry}->{type} eq 'LineString') { + if ( $feature->{geometry}->{type} eq 'LineString' ) { @linestrings = ([ @linestrings ]); } # If it is a point, upgrade it to a one-segment zero-length diff --git a/perllib/FixMyStreet/Cobrand/Westminster.pm b/perllib/FixMyStreet/Cobrand/Westminster.pm index 8adb7eb2c..7a3d7bbcd 100644 --- a/perllib/FixMyStreet/Cobrand/Westminster.pm +++ b/perllib/FixMyStreet/Cobrand/Westminster.pm @@ -122,7 +122,7 @@ sub lookup_site_code_config { } sub _fetch_features_url { - my ($self, $cfg, $w, $s, $e, $n) = @_; + my ($self, $cfg) = @_; # Westminster's asset proxy has a slightly different calling style to # a standard WFS server. @@ -132,7 +132,7 @@ sub _fetch_features_url { outSR => "27700", f => "geojson", outFields => $cfg->{property}, - geometry => "$w,$s,$e,$n", + geometry => $cfg->{bbox}, ); return $cfg->{proxy_url} . "?" . $uri->as_string; diff --git a/t/Mock/MapIt.pm b/t/Mock/MapIt.pm index 96be429e4..cd441856a 100644 --- a/t/Mock/MapIt.pm +++ b/t/Mock/MapIt.pm @@ -39,6 +39,7 @@ my @PLACES = ( [ 'LE15 0GJ', 52.670447, -0.727877, 2600, 'Rutland County Council', 'CTY'], [ 'BR1 3UH', 51.4021, 0.01578, 2482, 'Bromley Council', 'LBO' ], [ 'BR1 3UH', 51.402096, 0.015784, 2482, 'Bromley Council', 'LBO' ], + [ 'BR1 3EF', 51.4039, 0.018697, 2482, 'Bromley Council', 'LBO' ], [ 'NN1 1NS', 52.236251, -0.892052, 2234, 'Northamptonshire County Council', 'CTY', 2397, 'Northampton Borough Council', 'DIS' ], [ 'NN1 2NS', 52.238301, -0.889992, 2234, 'Northamptonshire County Council', 'CTY', 2397, 'Northampton Borough Council', 'DIS' ], [ '?', 52.238827, -0.894970, 2234, 'Northamptonshire County Council', 'CTY', 2397, 'Northampton Borough Council', 'DIS' ], diff --git a/t/Mock/Tilma.pm b/t/Mock/Tilma.pm new file mode 100644 index 000000000..5a11209e3 --- /dev/null +++ b/t/Mock/Tilma.pm @@ -0,0 +1,39 @@ +package t::Mock::Tilma; + +use JSON::MaybeXS; +use Web::Simple; + +has json => ( + is => 'lazy', + default => sub { + JSON->new->utf8->pretty->allow_blessed->convert_blessed; + }, +); + +sub dispatch_request { + my $self = shift; + + sub (GET + /mapserver/tfl + ?*) { + my ($self, $args) = @_; + my $features = []; + if ($args->{Filter} =~ /540512,169141/) { + $features = [ + { type => "Feature", properties => { HA_ID => "19" }, geometry => { type => "Polygon", coordinates => [ [ + [ 539408.94, 170607.58 ], + [ 539432.81, 170627.93 ], + [ 539437.24, 170623.48 ], + [ 539408.94, 170607.58 ], + ] ] } } ]; + } + my $json = mySociety::Locale::in_gb_locale { + $self->json->encode({ + type => "FeatureCollection", + crs => { type => "name", properties => { name => "urn:ogc:def:crs:EPSG::27700" } }, + features => $features, + }); + }; + return [ 200, [ 'Content-Type' => 'application/json' ], [ $json ] ]; + }, +} + +__PACKAGE__->run_if_script; diff --git a/t/app/controller/admin/templates.t b/t/app/controller/admin/templates.t index 200fbd727..ad5b3e77b 100644 --- a/t/app/controller/admin/templates.t +++ b/t/app/controller/admin/templates.t @@ -347,7 +347,7 @@ subtest "TfL cobrand only shows TfL templates" => sub { subtest "Bromley cobrand only shows Bromley templates" => sub { FixMyStreet::override_config { - ALLOWED_COBRANDS => [ 'bromley' ], + ALLOWED_COBRANDS => [ 'bromley', 'tfl' ], }, sub { $report->update({ category => $bromleycontact->category, bodies_str => $bromley->id }); $mech->log_in_ok( $bromleyuser->email ); diff --git a/t/cobrand/hounslow.t b/t/cobrand/hounslow.t index cb67ad397..91c1cb455 100644 --- a/t/cobrand/hounslow.t +++ b/t/cobrand/hounslow.t @@ -75,16 +75,6 @@ subtest "it shows the right things on an /around page" => sub { }; }; -subtest "does not show TfL traffic lights category" => sub { - FixMyStreet::override_config { - MAPIT_URL => 'http://mapit.uk/', - ALLOWED_COBRANDS => 'fixmystreet', - }, sub { - my $json = $mech->get_ok_json('/report/new/ajax?latitude=51.482286&longitude=-0.328163'); - is $json->{by_category}{"Traffic lights"}, undef; - }; -}; - subtest "Shows external ID on report page to staff users only" => sub { FixMyStreet::override_config { ALLOWED_COBRANDS => 'hounslow', diff --git a/t/cobrand/tfl.t b/t/cobrand/tfl.t index a327a00ef..33506e8c4 100644 --- a/t/cobrand/tfl.t +++ b/t/cobrand/tfl.t @@ -8,6 +8,11 @@ END { FixMyStreet::App->log->enable('info'); } my $mech = FixMyStreet::TestMech->new; +use t::Mock::Tilma; +my $tilma = t::Mock::Tilma->new; +LWP::Protocol::PSGI->register($tilma->to_psgi_app, host => 'tilma.mysociety.org'); + + my $body = $mech->create_body_ok(2482, 'TfL'); FixMyStreet::DB->resultset('BodyArea')->find_or_create({ area_id => 2483, # Hounslow @@ -27,7 +32,26 @@ my $user = $mech->create_user_ok('londonresident@example.com'); my $bromley = $mech->create_body_ok(2482, 'Bromley'); my $bromleyuser = $mech->create_user_ok('bromleyuser@bromley.example.com', name => 'Bromley Staff', from_body => $bromley); +$mech->create_contact_ok( + body_id => $bromley->id, + category => 'Accumulated Litter', + email => 'litter-bromley@example.com', +); +my $bromley_flooding = $mech->create_contact_ok( + body_id => $bromley->id, + category => 'Flooding (Bromley)', + email => 'litter-bromley@example.com', +); +$bromley_flooding->set_extra_metadata(display_name => 'Flooding'); +$bromley_flooding->update; +my $bromley_flytipping = $mech->create_contact_ok( + body_id => $bromley->id, + category => 'Flytipping (Bromley)', + email => 'flytipping-bromley@example.com', +); +$bromley_flytipping->set_extra_metadata(group => [ 'Street cleaning' ]); +$bromley_flytipping->update; my $contact1 = $mech->create_contact_ok( body_id => $body->id, @@ -660,6 +684,106 @@ FixMyStreet::override_config { }, }, sub { +for my $test ( + { + host => 'www.fixmystreet.com', + name => "test red route categories", + lat => 51.4039, + lon => 0.018697, + expected => [ + 'Accumulated Litter', # Tests TfL->_cleaning_categories + 'Bus stops', + 'Flooding', + 'Flytipping (Bromley)', # In the 'Street cleaning' group + 'Grit bins', + 'Pothole', + 'Traffic lights', + 'Trees' + ], + }, + { + host => 'www.fixmystreet.com', + name => "test non-red route categories", + lat => 51.4021, + lon => 0.01578, + expected => [ + 'Accumulated Litter', # Tests TfL->_cleaning_categories + 'Bus stops', + 'Flooding (Bromley)', + 'Flytipping (Bromley)', # In the 'Street cleaning' group + 'Grit bins', + 'Traffic lights', + 'Trees' + ], + }, + { + host => 'tfl.fixmystreet.com', + name => "test red route categories", + lat => 51.4039, + lon => 0.018697, + expected => [ + 'Bus stops', + 'Flooding', + 'Grit bins', + 'Pothole', + 'Traffic lights', + 'Trees' + ], + }, + { + host => 'tfl.fixmystreet.com', + name => "test non-red route categories", + lat => 51.4021, + lon => 0.01578, + expected => [ + 'Bus stops', + 'Flooding', + 'Grit bins', + 'Pothole', + 'Traffic lights', + 'Trees' + ], + }, + { + host => 'bromley.fixmystreet.com', + name => "test red route categories", + lat => 51.4039, + lon => 0.018697, + expected => [ + 'Accumulated Litter', + 'Bus stops', + 'Flooding', + 'Flytipping (Bromley)', + 'Grit bins', + 'Pothole', + 'Traffic lights', + 'Trees' + ], + }, + { + host => 'bromley.fixmystreet.com', + name => "test non-red route categories", + lat => 51.4021, + lon => 0.01578, + expected => [ + 'Accumulated Litter', + 'Bus stops', + 'Flooding (Bromley)', + 'Flytipping (Bromley)', + 'Grit bins', + 'Traffic lights', + 'Trees' + ], + }, +) { + subtest $test->{name} . ' on ' . $test->{host} => sub { + $mech->host($test->{host}); + my $resp = $mech->get_ok_json( '/report/new/ajax?latitude=' . $test->{lat} . '&longitude=' . $test->{lon} ); + my @actual = sort keys %{ $resp->{by_category} }; + is_deeply \@actual, $test->{expected}; + }; +} + for my $host ( 'tfl.fixmystreet.com', 'www.fixmystreet.com', 'bromley.fixmystreet.com' ) { for my $test ( { @@ -673,6 +797,7 @@ for my $host ( 'tfl.fixmystreet.com', 'www.fixmystreet.com', 'bromley.fixmystree safety_critical => 'yes', category => "Pothole", subject => "Dangerous Pothole Report: Test Report", + pc => "BR1 3EF", # this is on a red route (according to Mock::MapIt and Mock::Tilma anyway) }, { name => "test category extra field - safety critical", @@ -682,6 +807,7 @@ for my $host ( 'tfl.fixmystreet.com', 'www.fixmystreet.com', 'bromley.fixmystree location => "carriageway", }, subject => "Dangerous Flooding Report: Test Report", + pc => "BR1 3EF", # this is on a red route (according to Mock::MapIt and Mock::Tilma anyway) }, { name => "test category extra field - non-safety critical", @@ -691,13 +817,15 @@ for my $host ( 'tfl.fixmystreet.com', 'www.fixmystreet.com', 'bromley.fixmystree location => "footway", }, subject => "Problem Report: Test Report", + pc => "BR1 3EF", # this is on a red route (according to Mock::MapIt and Mock::Tilma anyway) }, ) { subtest $test->{name} . ' on ' . $host => sub { $mech->log_in_ok( $user->email ); $mech->host($host); $mech->get_ok('/around'); - $mech->submit_form_ok( { with_fields => { pc => 'BR1 3UH', } }, "submit location" ); + my $pc = $test->{pc} || 'BR1 3UH'; + $mech->submit_form_ok( { with_fields => { pc => $pc, } }, "submit location ($pc)" ); $mech->follow_link_ok( { text_regex => qr/skip this step/i, }, "follow 'skip this step' link" ); $mech->submit_form_ok( { diff --git a/web/cobrands/tfl/assets.js b/web/cobrands/tfl/assets.js index 96927600f..ca712934b 100644 --- a/web/cobrands/tfl/assets.js +++ b/web/cobrands/tfl/assets.js @@ -4,18 +4,6 @@ if (!fixmystreet.maps) { return; } -var cleaning_categories = [ - 'Street cleaning', - 'Street Cleaning', - 'Accumulated Litter', - 'Street Cleaning Enquiry', - 'Street Cleansing' -]; -var cleaning_groups = [ - 'Street cleaning' -]; -var tfl_council_category = 'General Litter / Rubbish Collection'; - var defaults = { http_options: { url: "https://tilma.mysociety.org/mapserver/tfl", @@ -94,12 +82,6 @@ function is_tlrn_category_only(category, bodies) { bodies.length <= 1; } -function regenerate_category_groups() { - var old_category_group = $('#category_group').val() || $('#filter_group').val(); - $('#category_group').remove(); - fixmystreet.set_up.category_groups(old_category_group, true); -} - var red_routes_layer = fixmystreet.assets.add(defaults, { http_options: { url: "https://tilma.mysociety.org/mapserver/tfl", @@ -117,65 +99,18 @@ var red_routes_layer = fixmystreet.assets.add(defaults, { stylemap: tlrn_stylemap, no_asset_msg_id: '#js-not-tfl-road', actions: { - found: function(layer, feature) { - fixmystreet.message_controller.road_found(layer, feature); - - if (fixmystreet.cobrand === 'tfl' || !fixmystreet.reporting_data) { + found: fixmystreet.message_controller.road_found, + not_found: function(layer) { + // Only care about this on TfL cobrand + if (fixmystreet.cobrand !== 'tfl') { return; } - - // On a TfL road, remove any council categories, except street cleaning - var changed = false; - $('#form_category').find('option').each(function(i, option) { - var val = option.value; - if (OpenLayers.Util.indexOf(cleaning_categories, val) > -1) { - return; - } - var optgroup = $(option).closest('optgroup').attr('label'); - if (OpenLayers.Util.indexOf(cleaning_groups, optgroup) > -1) { - return; - } - if (val === tfl_council_category) { - changed = true; - $(option).remove(); - } - var data = fixmystreet.reporting_data.by_category[val]; - if (data && OpenLayers.Util.indexOf(data.bodies, 'TfL') === -1) { - changed = true; - $(option).remove(); - } - }); - if (changed) { - regenerate_category_groups(); - } - }, - not_found: function(layer) { var category = $('#form_category').val(); if (is_tlrn_category_only(category, fixmystreet.bodies)) { fixmystreet.message_controller.road_not_found(layer); } else { fixmystreet.message_controller.road_found(layer); } - - if (fixmystreet.cobrand === 'tfl' || !fixmystreet.reporting_data) { - return; - } - - // On non-TfL, remove any TfL-road-only categories - var changed = false; - $('#form_category').find('option').each(function(i, option) { - if (option.value === tfl_council_category) { - $(option).remove(); - } - var data = fixmystreet.reporting_data.by_category[option.value]; - if (data && is_tlrn_category_only(option.value, data.bodies)) { - changed = true; - $(option).remove(); - } - }); - if (changed) { - regenerate_category_groups(); - } } } }); |