Improve use of OL ArgParser/missing zoom in URL.

Remove the server-side zoom-in-url fix, instead use an ArgParser subclass to default to the provided data if nothing in URL. Then we can switch to using short lat/lon in geocoder URLs.
mysociety · Feb 4, 2019 · c3ae406 · c3ae406
1 parent f15f04a
commit c3ae406
Show file tree

Hide file tree

Showing 16 changed files with 29 additions and 99 deletions.
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
@@ -420,27 +420,6 @@ sub uri_with {
  return $uri;
 }
 
-=head2 uri_for
-
- $uri = $c->uri_for( ... );
-
-Like C<uri_for> except that it passes the uri to the cobrand to be altered if
-needed.
-
-=cut
-
-sub uri_for {
- my $c = shift;
- my @args = @_;
-
- my $uri = $c->next::method(@args);
-
- my $cobranded_uri = $c->cobrand->uri($uri);
-
- # note that the returned uri may be a string not an object (eg cities)
- return $cobranded_uri;
-}
-
 =head2 uri_for_email
 
  $uri = $c->uri_for_email( ... );

diff --git a/perllib/FixMyStreet/Cobrand/Bristol.pm b/perllib/FixMyStreet/Cobrand/Bristol.pm
@@ -23,8 +23,6 @@ sub map_type {
  'Bristol';
 }
 
-sub default_link_zoom { 6 }
-
 sub disambiguate_location {
  my $self = shift;
  my $string = shift;

diff --git a/perllib/FixMyStreet/Cobrand/Default.pm b/perllib/FixMyStreet/Cobrand/Default.pm
@@ -397,25 +397,6 @@ Return cobrand extra data for the problem
 
 sub cobrand_data_for_generic_problem { '' }
 
-=item uri
-
-Given a URL ($_[1]), QUERY, EXTRA_DATA, return a URL with any extra params
-needed appended to it.
-
-In the default case, we need to make sure zoom is always present if lat/lon
-are, to stop OpenLayers defaulting to null/0.
-
-=cut
-
-sub uri {
- my ( $self, $uri ) = @_;
- $uri->query_param( zoom => $self->default_link_zoom )
- if $uri->query_param('lat') && !$uri->query_param('zoom');
-
- return $uri;
-}
-
-
 =item header_params
 
 Return any params to be added to responses
@@ -1002,18 +983,14 @@ sub tweak_all_reports_map {}
 
 sub can_support_problems { return 0; }
 
-=item default_map_zoom / default_link_zoom
+=item default_map_zoom
 
 default_map_zoom is used when displaying a map overriding the
 default of max-4 or max-3 depending on population density.
 
-default_link_zoom is used in links that contain a 'lat' and no
-zoom, to stop e.g. OpenLayers defaulting to null/0.
-
 =cut
 
 sub default_map_zoom { undef };
-sub default_link_zoom { 3 }
 
 sub users_can_hide { return 0; }
 

diff --git a/perllib/FixMyStreet/Cobrand/Zurich.pm b/perllib/FixMyStreet/Cobrand/Zurich.pm
@@ -88,8 +88,6 @@ sub example_places {
 sub languages { [ 'de-ch,Deutsch,de_CH' ] }
 sub language_override { 'de-ch' }
 
-sub default_link_zoom { 6 }
-
 sub prettify_dt {
  my $self = shift;
  my $dt = shift;

diff --git a/t/app/controller/around.t b/t/app/controller/around.t
@@ -18,28 +18,6 @@ subtest "check that if no query we get sent back to the homepage" => sub {
  is $mech->uri->path, '/', "redirected to '/'";
 };
 
-# x,y requests were generated by the old map code. We keep the behavior for
-# historic links
-subtest "redirect x,y requests to lat/lon (301 - permanent)" => sub {
-
- FixMyStreet::override_config {
- MAPIT_URL => 'http://mapit.uk/',
- }, sub {
- $mech->get_ok('/around?x=3281&y=1113');
- };
-
- # did we redirect to lat,lon?
- is $mech->uri->path, '/around', "still on /around";
- is_deeply { $mech->uri->query_form },
- { lat => 51.499825, lon => -0.140137, zoom => 3 },
- "lat,lon correctly set";
-
- # was it a 301?
- is $mech->res->code, 200, "got 200 for final destination";
- is $mech->res->previous->code, 301, "got 301 for redirect";
-
-};
-
 # test various locations on inital search box
 foreach my $test (
  {

diff --git a/t/app/controller/index.t b/t/app/controller/index.t
@@ -29,15 +29,15 @@ subtest "does pc, (x,y), (e,n) or (lat,lon) go to /around" => sub {
  },
  {
  in => { lat => 51.50100, lon => -0.14158 },
- out => { lat => 51.50100, lon => -0.14158, zoom => 3 },
+ out => { lat => 51.50100, lon => -0.14158 },
  },
  {
  in => { x => 3281, y => 1113, },
- out => { lat => 51.499825, lon => -0.140137, zoom => 3 },
+ out => { lat => 51.499825, lon => -0.140137 },
  },
  {
  in => { e => 1234, n => 4567 },
- out => { lat => 49.808509, lon => -7.544784, zoom => 3 },
+ out => { lat => 49.808509, lon => -7.544784 },
  },
  )
  {

diff --git a/t/app/uri_for.t b/t/app/uri_for.t
@@ -13,7 +13,6 @@ use URI;
 use_ok('FixMyStreet::App');
 
 my $fms_c = ctx_request('http://www.fixmystreet.com/');
-my $fgm_c = ctx_request('http://www.fiksgatami.no/');
 
 is(
  $fms_c->uri_for('/bar/baz') . "",
@@ -33,11 +32,4 @@ is(
  'URI with query'
 );
 
-# fiksgatami
-is(
- $fgm_c->uri_for( '/foo', { lat => 1.23, } ) . "",
- 'http://www.fiksgatami.no/foo?lat=1.23&zoom=3',
- 'FiksGataMi url with lat not zoom'
-);
-
 done_testing();
diff --git a/templates/web/base/around/_error_multiple.html b/templates/web/base/around/_error_multiple.html
@@ -10,7 +10,7 @@
  [% IF match.icon %]
  <img src="[% match.icon %]" alt="">
  [% END %]
- <a href="/around?latitude=[% match.latitude | uri %];longitude=[% match.longitude | uri %][% IF c.req.params.category %];category=[% c.req.params.category | uri %][% END %]">[% match.address | html %]</a>
+ <a href="/around?lat=[% match.latitude | uri %]&amp;lon=[% match.longitude | uri %][% IF c.req.params.category %]&amp;category=[% c.req.params.category | uri %][% END %]">[% match.address | html %]</a>
  </li>
  [% END %]
  </ul>

diff --git a/web/js/map-OpenLayers.js b/web/js/map-OpenLayers.js
@@ -831,15 +831,6 @@ $.extend(fixmystreet.utils, {
  fixmystreet.map.addLayer(layer);
  }
 
- if (!fixmystreet.map.getCenter()) {
- var centre = new OpenLayers.LonLat( fixmystreet.longitude, fixmystreet.latitude );
- centre.transform(
- new OpenLayers.Projection("EPSG:4326"),
- fixmystreet.map.getProjectionObject()
- );
- fixmystreet.map.setCenter(centre, fixmystreet.zoom || 3);
- }
-
  // map.getCenter() returns a position in "map units", but sometimes you
  // want the center in GPS-style latitude/longitude coordinates (WGS84)
  // for example, to pass as GET params to fixmystreet.com/report/new.
@@ -921,6 +912,23 @@ OpenLayers.Control.PanZoomFMS = OpenLayers.Class(OpenLayers.Control.PanZoom, {
  }
 });
 
+OpenLayers.Control.ArgParserFMS = OpenLayers.Class(OpenLayers.Control.ArgParser, {
+ getParameters: function(url) {
+ var args = OpenLayers.Control.ArgParser.prototype.getParameters.apply(this, arguments);
+ // Get defaults from provided data if not in URL
+ if (!args.lat && !args.lon) {
+ args.lon = fixmystreet.longitude;
+ args.lat = fixmystreet.latitude;
+ }
+ if (args.lat && !args.zoom) {
+ args.zoom = fixmystreet.zoom || 3;
+ }
+ return args;
+ },
+
+ CLASS_NAME: "OpenLayers.Control.ArgParserFMS"
+});
+
 /* Overriding Permalink so that it can pass the correct zoom to OSM */
 OpenLayers.Control.PermalinkFMS = OpenLayers.Class(OpenLayers.Control.Permalink, {
  _updateLink: function(alter_zoom) {

diff --git a/web/js/map-OpenStreetMap.js b/web/js/map-OpenStreetMap.js
@@ -4,7 +4,7 @@ fixmystreet.maps.config = function() {
  permalink_id = 'map_permalink';
  }
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Attribution(),
  //new OpenLayers.Control.LayerSwitcher(),
  new OpenLayers.Control.Navigation(),

diff --git a/web/js/map-bing-ol.js b/web/js/map-bing-ol.js
@@ -6,7 +6,7 @@ fixmystreet.maps.config = function() {
 
  fixmystreet.controls = [
  new OpenLayers.Control.Attribution(),
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation(),
  new OpenLayers.Control.PermalinkFMS(permalink_id),
  new OpenLayers.Control.PanZoomFMS({id: 'fms_pan_zoom' })

diff --git a/web/js/map-google-ol.js b/web/js/map-google-ol.js
@@ -23,7 +23,7 @@ fixmystreet.maps.config = function() {
  }
 
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation(),
  new OpenLayers.Control.PermalinkFMS(permalink_id),
  new OpenLayers.Control.PanZoomFMS({id: 'fms_pan_zoom' })

diff --git a/web/js/map-streetview.js b/web/js/map-streetview.js
@@ -1,6 +1,6 @@
 fixmystreet.maps.config = function() {
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation(),
  new OpenLayers.Control.Permalink(),
  new OpenLayers.Control.PanZoomFMS()

diff --git a/web/js/map-toner-lite.js b/web/js/map-toner-lite.js
@@ -4,7 +4,7 @@ fixmystreet.maps.config = function() {
  permalink_id = 'map_permalink';
  }
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation(),
  new OpenLayers.Control.PermalinkFMS(permalink_id),
  new OpenLayers.Control.PanZoomFMS({id: 'fms_pan_zoom' })

diff --git a/web/js/map-wmts-bristol.js b/web/js/map-wmts-bristol.js
@@ -104,7 +104,7 @@ fixmystreet.maps.config = function() {
  }
 
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation(),
  new OpenLayers.Control.PermalinkFMS(permalink_id)
  ];

diff --git a/web/js/map-wmts-zurich.js b/web/js/map-wmts-zurich.js
@@ -135,7 +135,7 @@ fixmystreet.maps.config = function() {
  // This stuff is copied from js/map-bing-ol.js
 
  fixmystreet.controls = [
- new OpenLayers.Control.ArgParser(),
+ new OpenLayers.Control.ArgParserFMS(),
  new OpenLayers.Control.Navigation()
  ];
  if ( fixmystreet.page != 'report' || !$('html').hasClass('mobile') ) {