Moved some of the location smarts from '/report/new' to '/around'

8 files changed, 266 insertions, 193 deletions

diff --git a/notes/location_related_flow.txt b/notes/location_related_flow.txt
new file mode 100644
index 000000000..5af3c88f0
--- /dev/null
+++ b/notes/location_related_flow.txt
@@ -0,0 +1,23 @@
+--- '/' ---
+
+    Homepage has a search form that submits to /around. There is no location
+    processing done on the home page. Any queries sent to the homepage are
+    redirected to /around.
+
+--- '/around' ---
+
+    SEARCH: allows user to search for a location using postcode or other text.
+    If nothing matched error shown. If multiple match show alternatives.
+
+    LIST: If a search could be resolved to a lat/lon, or a lot/lon was in query
+    show matches near that location.
+
+    PARTIAL: If there is a partial token show a message when searching. When a
+    match is found redirect to '/report/new' for the partial to be completed.
+
+--- '/report/new' ---
+
+    Requires a lat/lng, or a tile click, or a partial report with a location
+    stored. If no location can be deteremined redirects back to '/around'. All
+    form information is lost but the partial token is preserved.
+
diff --git a/perllib/FixMyStreet/App/Controller/Around.pm b/perllib/FixMyStreet/App/Controller/Around.pm
index 8df7b880f..6a6e82d02 100644
--- a/perllib/FixMyStreet/App/Controller/Around.pm
+++ b/perllib/FixMyStreet/App/Controller/Around.pm
@@ -6,6 +6,7 @@ BEGIN { extends 'Catalyst::Controller'; }
 
 use FixMyStreet::Map;
 use List::MoreUtils qw(any);
+use Encode;
 
 =head1 NAME
 
@@ -40,14 +41,139 @@ sub around_index : Path : Args(0) {
         return;
     }
 
-    # if there was no search then redirect to the homepage
-    if ( !any { $c->req->param($_) } qw(pc lat lon) ) {
-        return $c->res->redirect( $c->uri_for('/') );
-    }
-    
+    # Try to create a location for whatever we have
+    return
+      unless $c->forward('determine_location_from_coords')
+          || $c->forward('determine_location_from_pc');
+
+    # Check to see if the spot is covered by a council - if not show an error.
+    return unless $c->forward('check_location_is_acceptable');
+
+    # If we have a partial - redirect to /report/new so that it can be
+    # completed.
+    warn "FIXME - implement";
+
+    # Show the nearby reports
+    die "show nearby reports";
     
 }
 
+=head2 determine_location_from_coords
+
+Use latitude and longitude if provided in parameters.
+
+=cut 
+
+sub determine_location_from_coords : Private {
+    my ( $self, $c ) = @_;
+
+    my $latitude  = $c->req->param('latitude');
+    my $longitude = $c->req->param('longitude');
+
+    if ( defined $latitude && defined $longitude ) {
+        $c->stash->{latitude}  = $latitude;
+        $c->stash->{longitude} = $longitude;
+
+        # Also save the pc if there is one
+        if ( my $pc = $c->req->param('pc') ) {
+            $c->stash->{pc} = $pc;
+        }
+
+        return 1;
+    }
+
+    return;
+}
+
+=head2 determine_location_from_pc
+
+User has searched for a location - try to find it for them.
+
+If one match is found returns true and lat/lng is set.
+
+If several possible matches are found puts an array onto stash so that user can be prompted to pick one and returns false.
+
+If no matches are found returns false.
+
+=cut 
+
+sub determine_location_from_pc : Private {
+    my ( $self, $c ) = @_;
+
+    # check for something to search
+    my $pc = $c->req->param('pc') || return;
+    $c->stash->{pc} = $pc;    # for template
+
+    my ( $latitude, $longitude, $error ) =
+      eval { FixMyStreet::Geocode::lookup( $pc, $c->req ) };
+
+    # Check that nothing blew up
+    if ($@) {
+        warn "Error: $@";
+        return;
+    }
+
+    # If we got a lat/lng set to stash and return true
+    if ( defined $latitude && defined $longitude ) {
+        $c->stash->{latitude}  = $latitude;
+        $c->stash->{longitude} = $longitude;
+        return 1;
+    }
+
+    # $error doubles up to return multiple choices by being an array
+    if ( ref($error) eq 'ARRAY' ) {
+        @$error = map {
+            decode_utf8($_);
+            s/, United Kingdom//;
+            s/, UK//;
+            $_;
+        } @$error;
+        $c->stash->{possible_location_matches} = $error;
+        return;
+    }
+
+    # pass errors back to the template
+    $c->stash->{pc_error} = $error;
+    return;
+}
+
+=head2 check_location_is_acceptable
+
+Find the lat and lon in stash and check that they are acceptable to the council,
+and that they are in UK (if we are in UK).
+
+=cut
+
+sub check_location_is_acceptable : Private {
+    my ( $self, $c ) = @_;
+
+    # These should be set now
+    my $lat = $c->stash->{latitude};
+    my $lon = $c->stash->{longitude};
+
+    # Check this location is okay to be displayed for the cobrand
+    my ( $success, $error_msg ) = $c->cobrand->council_check(    #
+        { lat => $lat, lon => $lon },
+        'submit_problem'
+    );
+
+    # If in UK and we have a lat,lon coocdinate check it is in UK
+    if ( !$error_msg && $lat && $c->config->{COUNTRY} eq 'GB' ) {
+        eval { Utils::convert_latlon_to_en( $lat, $lon ); };
+        $error_msg =
+          _( "We had a problem with the supplied co-ordinates - outside the UK?"
+          ) if $@;
+    }
+
+    # all good
+    return 1 if !$error_msg;
+
+    # show error
+    $c->stash->{pc_error} = $error_msg;
+    return;
+
+}
+
 __PACKAGE__->meta->make_immutable;
 
 1;
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index 4e077f32b..2dbec4235 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -375,34 +375,12 @@ sub determine_location : Private {
 
     return
       unless $c->forward('determine_location_from_tile_click')
-          || $c->forward('determine_location_from_coords')
-          || $c->forward('determine_location_from_pc')
+          || $c->forward('/around/determine_location_from_coords')
+          || $c->forward('/around/determine_location_from_pc')
           || $c->forward('determine_location_from_report');
 
-    # These should be set now
-    my $lat = $c->stash->{latitude};
-    my $lon = $c->stash->{longitude};
 
-    # Check this location is okay to be displayed for the cobrand
-    my ( $success, $error_msg ) = $c->cobrand->council_check(    #
-        { lat => $lat, lon => $lon },
-        'submit_problem'
-    );
-
-    # If in UK and we have a lat,lon coocdinate check it is in UK
-    if ( !$error_msg && $lat && $c->config->{COUNTRY} eq 'GB' ) {
-        eval { Utils::convert_latlon_to_en( $lat, $lon ); };
-        $error_msg =
-          _( "We had a problem with the supplied co-ordinates - outside the UK?"
-          ) if $@;
-    }
-
-    # all good
-    return 1 if !$error_msg;
-
-    # show error
-    $c->stash->{pc_error} = $error_msg;
-    return;
+    return $c->forward('/around/check_location_is_acceptable');
 }
 
 =head2 determine_location_from_tile_click
@@ -456,85 +434,6 @@ sub determine_location_from_tile_click : Private {
     return 1;
 }
 
-=head2 determine_location_from_coords
-
-Use latitude and longitude if provided in parameters.
-
-=cut 
-
-sub determine_location_from_coords : Private {
-    my ( $self, $c ) = @_;
-
-    my $latitude  = $c->req->param('latitude');
-    my $longitude = $c->req->param('longitude');
-
-    if ( defined $latitude && defined $longitude ) {
-        $c->stash->{latitude}  = $latitude;
-        $c->stash->{longitude} = $longitude;
-
-        # Also save the pc if there is one
-        if ( my $pc = $c->req->param('pc') ) {
-            $c->stash->{pc} = $pc;
-        }
-
-        return 1;
-    }
-
-    return;
-}
-
-=head2 determine_location_from_pc
-
-User has searched for a location - try to find it for them.
-
-If one match is found returns true and lat/lng is set.
-
-If several possible matches are found puts an array onto stash so that user can be prompted to pick one and returns false.
-
-If no matches are found returns false.
-
-=cut 
-
-sub determine_location_from_pc : Private {
-    my ( $self, $c ) = @_;
-
-    # check for something to search
-    my $pc = $c->req->param('pc') || return;
-    $c->stash->{pc} = $pc;    # for template
-
-    my ( $latitude, $longitude, $error ) =
-      eval { FixMyStreet::Geocode::lookup( $pc, $c->req ) };
-
-    # Check that nothing blew up
-    if ($@) {
-        warn "Error: $@";
-        return;
-    }
-
-    # If we got a lat/lng set to stash and return true
-    if ( defined $latitude && defined $longitude ) {
-        $c->stash->{latitude}  = $latitude;
-        $c->stash->{longitude} = $longitude;
-        return 1;
-    }
-
-    # $error doubles up to return multiple choices by being an array
-    if ( ref($error) eq 'ARRAY' ) {
-        @$error = map {
-            decode_utf8($_);
-            s/, United Kingdom//;
-            s/, UK//;
-            $_;
-        } @$error;
-        $c->stash->{possible_location_matches} = $error;
-        return;
-    }
-
-    # pass errors back to the template
-    $c->stash->{pc_error} = $error;
-    return;
-}
-
 =head2 determine_location_from_report
 
 Use latitude and longitude stored in the report - this is probably result of a
diff --git a/perllib/FixMyStreet/TestMech.pm b/perllib/FixMyStreet/TestMech.pm
index 26738556d..6a4382c3d 100644
--- a/perllib/FixMyStreet/TestMech.pm
+++ b/perllib/FixMyStreet/TestMech.pm
@@ -204,6 +204,24 @@ sub form_errors {
     return $result->{errors} || [];
 }
 
+=head2 page_errors
+
+    my $arrayref = $mech->page_errors;
+
+Find all the form errors on the current page and return them in page order as an
+arrayref of TEXTs. If none found return empty arrayref.
+
+=cut
+
+sub page_errors {
+    my $mech   = shift;
+    my $result = scraper {
+        process 'p.error', 'errors[]', 'TEXT';
+    }
+    ->scrape( $mech->response );
+    return $result->{errors} || [];
+}
+
 =head2 import_errors
 
     my $arrayref = $mech->import_errors;
diff --git a/t/app/controller/around.t b/t/app/controller/around.t
index 021cf8f7d..069ec605a 100644
--- a/t/app/controller/around.t
+++ b/t/app/controller/around.t
@@ -7,7 +7,7 @@ my $mech = FixMyStreet::TestMech->new;
 
 subtest "check that if no query we get sent back to the homepage" => sub {
     $mech->get_ok('/around');
-    is $mech->uri->path, '/', "sent back to '/'";
+    is $mech->uri->path, '/around', "still at  '/around'";
 };
 
 # x,y requests were generated by the old map code. We keep the behavior for
@@ -28,4 +28,76 @@ subtest "redirect x,y requests to lat/lon (301 - permanent)" => sub {
 
 };
 
+# test various locations on inital search box
+foreach my $test (
+    {
+        pc              => '',    #
+        errors          => [],
+        pc_alternatives => [],
+    },
+    {
+        pc              => 'xxxxxxxxxxxxxxxxxxxxxxxxxxx',
+        errors          => ['Sorry, we could not find that location.'],
+        pc_alternatives => [],
+    },
+    {
+        pc     => 'glenthorpe',
+        errors => [],
+        pc_alternatives => [   # TODO - should filter out these non-UK addresses
+            'Glenthorpe Crescent, Leeds LS9 7',
+            'Glenthorpe Rd, Merton, Greater London SM4 4',
+            'Glenthorpe Ln, Katy, TX 77494, USA',
+            'Glenthorpe Dr, Walnut, CA 91789, USA',
+            'Glenthorpe Ave, Leeds LS9 7',
+            'Glenthorpe Ct, Katy, TX 77494, USA',
+        ],
+    },
+    {
+        pc => 'Glenthorpe Ct, Katy, TX 77494, USA',
+        errors =>
+          ['We had a problem with the supplied co-ordinates - outside the UK?'],
+        pc_alternatives => [],
+    },
+  )
+{
+    subtest "test bad pc value '$test->{pc}'" => sub {
+        $mech->get_ok('/');
+        $mech->submit_form_ok( { with_fields => { pc => $test->{pc} } },
+            "bad location" );
+        is_deeply $mech->page_errors, $test->{errors},
+          "expected errors for pc '$test->{pc}'";
+        is_deeply $mech->pc_alternatives, $test->{pc_alternatives},
+          "expected alternatives for pc '$test->{pc}'";
+    };
+}
+
+# check that exact queries result in the correct lat,lng
+foreach my $test (
+    {
+        pc        => 'SW1A 1AA',
+        latitude  => '51.5010096115539',
+        longitude => '-0.141587067110009',
+    },
+    {
+        pc        => 'Manchester',
+        latitude  => '53.4807125',
+        longitude => '-2.2343765',
+    },
+    {
+        pc        => 'Glenthorpe Rd, Merton, Greater London SM4 4, UK',
+        latitude  => '51.3937997',
+        longitude => '-0.2209596',
+    },
+  )
+{
+    subtest "check lat/lng for '$test->{pc}'" => sub {
+        $mech->get_ok('/');
+        $mech->submit_form_ok( { with_fields => { pc => $test->{pc} } },
+            "good location" );
+        is_deeply $mech->form_errors, [], "no errors for pc '$test->{pc}'";
+        is_deeply $mech->extract_location, $test,
+          "got expected location for pc '$test->{pc}'";
+    };
+}
+
 done_testing();
diff --git a/t/app/controller/report_import.t b/t/app/controller/report_import.t
index 5c16324d3..764357b57 100644
--- a/t/app/controller/report_import.t
+++ b/t/app/controller/report_import.t
@@ -105,12 +105,16 @@ subtest "Submit a correct entry" => sub {
     is_deeply $mech->visible_form_values, { pc => '' },
       "check only pc field is shown";
 
+    is $mech->uri->path, '/around', "sent to report page";
+
     $mech->submit_form_ok(    #
         { with_fields => { pc => 'SW1A 1AA' } },
         "fill in postcode"
     );
 
-    # check that we are not shown anything as we don't have a location yet
+    is $mech->uri->path, '/report/new', "sent to report page";
+
+    # check that fields are prefilled for us
     is_deeply $mech->visible_form_values,
       {
         name          => 'Test User',
@@ -124,7 +128,7 @@ subtest "Submit a correct entry" => sub {
       "check imported fields are shown";
 
   TODO: {
-        local $TODO = "'/report/123' urls not srved by catalyst yet";
+        local $TODO = "'/report/123' urls not served by catalyst yet";
 
         # change the details
         $mech->submit_form_ok(    #
diff --git a/t/app/controller/report_new.t b/t/app/controller/report_new.t
index dca86db77..1310a3e35 100644
--- a/t/app/controller/report_new.t
+++ b/t/app/controller/report_new.t
@@ -8,77 +8,10 @@ use Web::Scraper;
 my $mech = FixMyStreet::TestMech->new;
 $mech->get_ok('/report/new');
 
-# test various locations on inital search box
-foreach my $test (
-    {
-        pc              => '',    #
-        errors          => [],
-        pc_alternatives => [],
-    },
-    {
-        pc              => 'xxxxxxxxxxxxxxxxxxxxxxxxxxx',
-        errors          => ['Sorry, we could not find that location.'],
-        pc_alternatives => [],
-    },
-    {
-        pc     => 'glenthorpe',
-        errors => [],
-        pc_alternatives => [   # TODO - should filter out these non-UK addresses
-            'Glenthorpe Crescent, Leeds LS9 7, UK',
-            'Glenthorpe Rd, Merton, Greater London SM4 4, UK',
-            'Glenthorpe Ln, Katy, TX 77494, USA',
-            'Glenthorpe Dr, Walnut, CA 91789, USA',
-            'Glenthorpe Ave, Leeds LS9 7, UK',
-            'Glenthorpe Ct, Katy, TX 77494, USA',
-        ],
-    },
-    {
-        pc => 'Glenthorpe Ct, Katy, TX 77494, USA',
-        errors =>
-          ['We had a problem with the supplied co-ordinates - outside the UK?'],
-        pc_alternatives => [],
-    },
-  )
-{
-    subtest "test bad pc value '$test->{pc}'" => sub {
-        $mech->get_ok('/report/new');
-        $mech->submit_form_ok( { with_fields => { pc => $test->{pc} } },
-            "bad location" );
-        is_deeply $mech->form_errors, $test->{errors},
-          "expected errors for pc '$test->{pc}'";
-        is_deeply $mech->pc_alternatives, $test->{pc_alternatives},
-          "expected alternatives for pc '$test->{pc}'";
-    };
-}
+fail "test that lat,lon not covered by council goes to '/around'";
+
+fail "test that partial code is transferred";
 
-# check that exact queries result in the correct lat,lng
-foreach my $test (
-    {
-        pc        => 'SW1A 1AA',
-        latitude  => '51.5010096115539',
-        longitude => '-0.141587067110009',
-    },
-    {
-        pc        => 'Manchester',
-        latitude  => '53.4807125',
-        longitude => '-2.2343765',
-    },
-    {
-        pc        => 'Glenthorpe Rd, Merton, Greater London SM4 4, UK',
-        latitude  => '51.3937997',
-        longitude => '-0.2209596',
-    },
-  )
-{
-    subtest "check lat/lng for '$test->{pc}'" => sub {
-        $mech->get_ok('/report/new');
-        $mech->submit_form_ok( { with_fields => { pc => $test->{pc} } },
-            "good location" );
-        is_deeply $mech->form_errors, [], "no errors for pc '$test->{pc}'";
-        is_deeply $mech->extract_location, $test,
-          "got expected location for pc '$test->{pc}'";
-    };
-}
 
 # test that the various bit of form get filled in and errors correctly
 # generated.
diff --git a/templates/web/default/report/new/report_new.html b/templates/web/default/around/around_index.html
index 434da9c6b..07b3cee4d 100644
--- a/templates/web/default/report/new/report_new.html
+++ b/templates/web/default/around/around_index.html
@@ -8,8 +8,16 @@
 %]
 
 
-<h1>[% loc('Reporting a problem') %]</h1>
+<form action="[% partial_token ? '/report/new' : '/' %]" method="get" name="postcodeForm" id="postcodeForm"><label for="pc">[% loc("Enter a nearby GB postcode, or street name and area") %]:</label>
+&nbsp;<input type="text" name="pc" value="[% pc | html %]" id="pc" size="10" maxlength="200">
+&nbsp;<input type="submit" value="[% loc('Go') %]" id="submit">
+[% c.cobrand.form_elements %]
+
+[% IF partial_token %]
+    <input type="hidden" name="partial" value="[% partial_token.token %]">
+[% END %]
 
+</form>
 [% IF location_error %]
     <div class="error">[% location_error %]</div>
 [% END %]
@@ -34,15 +42,5 @@
 <p class="error">[% pc_error %]</p>
 [% END %]
 
-<form action="[% partial_token ? '/report/new' : '/' %]" method="get" name="postcodeForm" id="postcodeForm"><label for="pc">[% loc("Enter a nearby GB postcode, or street name and area") %]:</label>
-&nbsp;<input type="text" name="pc" value="[% pc | html %]" id="pc" size="10" maxlength="200">
-&nbsp;<input type="submit" value="[% loc('Go') %]" id="submit">
-[% c.cobrand.form_elements %]
-
-[% IF partial_token %]
-    <input type="hidden" name="partial" value="[% partial_token.token %]">
-[% END %]
-
-</form>
 
 [% INCLUDE 'footer.html' %]