Fix Google reverse lookup in getPlacenameFromCoordinates #232

nstjean · 2020-01-09T18:59:48Z

Issue #231

Added error checking in the getPlacenameFromCoodinates response from Google
Display error message in console
updated the API key in line with what is listed in Location search not working #214

I'm considering adding in a option to set the displayed value as something other than "Location unavailable" - in plots2 I'd like for it to be blank if there's an error.

…al page load

nstjean · 2020-01-09T20:55:00Z

I found that the API call was being triggered continuously during a map move, it was firing off a huge number of API calls. Now it will only update the location name once at the end of the map move!

I added in the option to set a default message if the google api call fails. This way I can control what shows up in the location modal.

        InterfaceOptions: {
          placenameDisplayOnError: 'Location message'
        },

If all looks good I'll add it to the documentation!

nstjean · 2020-01-09T22:00:21Z

I think I did something wrong when publishing to gh-pages. :( It's broken now.

jywarren · 2020-01-09T22:20:18Z

hm, i think @crisner has recently published to gh-pages on LEL, maybe can offer some input? did you include the node_modules folder and the dist files? Don't stress!

…

On Thu, Jan 9, 2020 at 5:00 PM Natalie St Jean ***@***.***> wrote: I think I did something wrong when publishing to gh-pages. :( It's broken now. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#232?email_source=notifications&email_token=AAAF6J2TFKNGLL5BIHB3MM3Q46M7NA5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIR55GA#issuecomment-572776088>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JZOM5TFJLWJD5QT5CDQ46M7NANCNFSM4KE457VQ> .

crisner · 2020-01-10T05:51:53Z

@nstjean, I see a bunch of 404 errors. Check if the node modules exist in the gh-pages branch. If not, you will need to push them along with your dist files if you haven't pushed them already. Let me know how this goes. :)

nstjean · 2020-01-10T16:36:15Z

Yes that fixed it! It works now. :)

nstjean · 2020-01-10T18:10:38Z

Ok the gh-pages is up to date with this branch:
https://publiclab.github.io/leaflet-blurred-location/examples/

There is still an error in the response from google: "API keys with referer restrictions cannot be used with this API."
Here's what google says https://developers.google.com/maps/faq#browser-keys-blocked-error

You are using any of the web service APIs with an API key restricted to an HTTP referer. For security reasons, web service APIs need to use API keys restricted to IP addresses. Switch your key restriction type from an HTTP referer restriction to an IP address restriction, or create a new API key if your key is already used with the Maps JavaScript API.

jywarren · 2020-01-10T22:33:49Z

Hm, ok, I'll try to make another key with a different type of restriction.

jywarren · 2020-01-10T22:37:42Z

Ah, ok, so that didn't make sense because it's supposed to be an HTML referrer restriction for client-side requests. The issue may be different:

https://stackoverflow.com/questions/42167695/api-key-browser-api-keys-cannot-have-referer-restrictions-when-used-with-this-ap

If server-side geocoding is not an option, you should use the geocoder from the Google Javascript API. You can set HTTP referer restrictions on that API.

Google itself says to avoid the Non-Javascript Geocoder API for dynamic geocoding:

This service is generally designed for geocoding static (known in advance) addresses for placement of application content on a map; this service is not designed to respond in real time to user input. For dynamic geocoding (for example, within a user interface element), consult the documentation for the Maps JavaScript API client geocoder and/or the Google Play services Location APIs.

I'll be sure the existing key has permissions for the Google Javascript API.

jywarren · 2020-01-10T22:41:12Z

src/core/Geocoding.js

-                country = result.results[i].formatted_address;
-              }
+    $.ajax({
+      url:"https://maps.googleapis.com/maps/api/geocode/json?latlng="+lat+","+lng + "&key=AIzaSyAOLUQngEmJv0_zcG1xkGq-CXIPpLQY8iQ",


So i guess we have to change this to the JS version?

Here seems to be the docs for that: https://developers.google.com/maps/documentation/javascript/geocoding#ReverseGeocoding

Ahhh!! Ok! I will get it switched over.

nstjean · 2020-01-13T20:20:54Z

@jywarren @sagarpreet-chadha @crisner Please take a look at the console on https://publiclab.github.io/leaflet-blurred-location/examples/index.html
When I do a search for a location using the client-side API the result[0].geometry.location has no lat or lng. I have tried all kinds of different things and I just don't understand it. It is supposed to have a float for location.lat and for location.lng. It's doing a search, it's getting a result, it's parsing the result... there's simply no numbers there. Here's the code for it:

  function geocodeStringAndPan(string) {
    if(typeof map.spin == 'function'){
      map.spin(true) ;
    }

    geocoder.geocode( { 'address': string }, function(results, status) {
      if(status === "OK") {
        var location = results[0].geometry.location;
        console.log(results);
          // $("#lat").val(location.lat);
          // $("#lng").val(location.lng);
          //map.setView([location.lat, location.lng], options.zoom);
      } else {
        console.log("Geocode not successful: " + status);
      }
      if(typeof map.spin == 'function'){
        map.spin(false) ;
      }
    });

  }

nstjean · 2020-01-13T20:23:54Z

Here's a picture of the resulting object when I search:

crisner · 2020-01-14T10:42:29Z

@nstjean, I am not sure why lat and lng has functions for values but calling it as results[0].geometry.location.lat() returns a float.

crisner · 2020-01-14T10:45:29Z

You could perhaps call the lat and lng functions and store them in variables to use here?

$("#lat").val(location.lat);
$("#lng").val(location.lng);
map.setView([location.lat, location.lng], options.zoom);

crisner · 2020-01-14T10:58:38Z

Upon looking at different posts it looks like you will have to get the lat and lng values as results[0].geometry.location.lat() and results[0].geometry.location.lng() respectively. So in your code, you may have to get the lat and lng values as location.lat() and location.lng(). I am attaching a couple of posts I looked at for your reference.
https://stackoverflow.com/questions/33379960/google-maps-geocoding-does-not-return-location
https://stackoverflow.com/questions/14143600/how-to-use-viewport-from-google-geocoder-in-leaflet-js

nstjean · 2020-01-14T13:53:43Z

Thank you!! I was so frustrated yesterday I had to walk away.

nstjean · 2020-01-14T15:01:51Z

This works now! You can see it in action on gh-pges: https://publiclab.github.io/leaflet-blurred-location/examples/index.html

All tests are passing but it's giving me a warning ReferenceError: Can't find variable: google. The link to the google maps api script is in the spec folder, the tests all pass, so I'm not sure if I should spend more time trying to figure out why it's complaining or just leave it?

nstjean · 2020-01-14T15:10:20Z

The other question I have is about the format of the Placename. When you are zoomed out it displays only the Country, which is fine. When zoomed in sometimes it shows city, zipcode, country, or zoomed in further displays street number/name, city, zipcode, country. The format changes from country to country. So let me know if the location names that display are what we want or if it needs to be changed.

sagarpreet-chadha · 2020-01-14T16:39:58Z

Hey,
We do not have tests for placename, right? If we will write tests for it, then that test will fail.

nstjean · 2020-01-14T16:42:55Z

Yes that is true. I'm not sure how we would go about testing things that rely on the Google api data.

…

On Tue, Jan 14, 2020, 11:40 AM Sagarpreet Chadha ***@***.***> wrote: Hey, We do not have tests for placename, right? If we will write tests for it, then that test will fail. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#232?email_source=notifications&email_token=ALZLKMKKKHTKFUK2UHED7V3Q5XTF7A5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI5JDVI#issuecomment-574263765>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALZLKMPGZLPW7AXCTM2F7BDQ5XTF7ANCNFSM4KE457VQ> .

sagarpreet-chadha · 2020-01-14T17:28:17Z

Okay i was thinking about this, one way to test is mocking data.
But this tests should be part of end to end tests (which will test API and its effect on UI). We can ignore the warning for now i think, for writing e2e test i would prefer different framework that is seperate discussion. Thanks!

nstjean · 2020-01-14T17:29:07Z

Ok! Sounds good!

…

On Tue, Jan 14, 2020 at 12:28 PM Sagarpreet Chadha ***@***.***> wrote: Okay i was thinking about this, one way to test is mocking data. But this tests should be part of end to end tests (which will test API and its effect on UI). We can ignore the warning for now i think, for writing e2e test i would prefer different framework that is seperate discussion. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#232?email_source=notifications&email_token=ALZLKMKPDZY2VVEZDYZKODTQ5XY3FA5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI5OMQI#issuecomment-574285377>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALZLKMOVML5AE2UA62MJVLLQ5XY3FANCNFSM4KE457VQ> .

nstjean · 2020-01-14T17:57:33Z

In order to have the placename field be overwritten sometimes but left alone at other times I've included a check for the value of a new data attribute:
<input id="placenameDisplay" type="text" class="form-control" data-preventOverwrite="false" />

If data-preventOverwrite="true" then it will not overwrite it with the current location name when the map is panned.

If data-preventOverwrite does not exist, or data-preventOverwrite="false" then it will overwrite the field as usual.

nstjean · 2020-01-14T19:25:31Z

I bumped the version to 1.6.0 for adding the new placename data attribute check.

sagarpreet-chadha

Looks good to me :)

jywarren · 2020-01-15T15:59:19Z

This is awesome. Thanks all for getting this figured out! I'll publish shortly!

emilyashley · 2020-01-15T16:01:02Z

Just went over this with @jywarren. Awesome! 🎉

jywarren · 2020-01-15T16:02:53Z

😄 and published as v1.6.0!

jywarren · 2020-01-15T16:05:13Z

want us to bump dependabot?

jywarren · 2020-01-15T16:07:26Z

I just did that! watch for a PR on plots2!

nstjean added 4 commits January 9, 2020 13:00

add error checking in getPlacenameFromCoordinates

3b0a990

update api key

511142c

add option to specify error message in placename display

c8c5b59

change trigger from 'move' to 'moveend', add function to run on initi…

cd4fc52

…al page load

nstjean force-pushed the fix-location-placename branch from 60100cd to fe8154c Compare January 9, 2020 20:48

fix jasmine error

ae024b2

nstjean force-pushed the fix-location-placename branch from fe8154c to ae024b2 Compare January 9, 2020 20:50

github-pages bot deployed to github-pages January 9, 2020 20:58 View deployment

nstjean mentioned this pull request Jan 9, 2020

[WIP] Redesign of location modal form publiclab/plots2#7125

Merged

jywarren reviewed Jan 10, 2020

View reviewed changes

change to google javascript api format

a9eb52f

nstjean mentioned this pull request Jan 13, 2020

Simple editor template publiclab/plots2#6817

Closed

5 tasks

nstjean added 4 commits January 14, 2020 09:21

test to fix geocode error

a4a99f8

fix google api errors

d10bd6f

change api key to publiclab

5726337

update test spec with new api

ea66dd9

nstjean mentioned this pull request Jan 14, 2020

Outreachy project planning publiclab/plots2#6887

Closed

add check for data-preventOverwrite

5ce3337

bump version to 1.6.0

a7be318

sagarpreet-chadha approved these changes Jan 15, 2020

View reviewed changes

jywarren approved these changes Jan 15, 2020

View reviewed changes

jywarren merged commit ef66dcb into publiclab:main Jan 15, 2020

nstjean deleted the fix-location-placename branch January 15, 2020 16:09

nstjean mentioned this pull request Jan 16, 2020

Combined people/content inline maps publiclab/plots2#3090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Google reverse lookup in getPlacenameFromCoordinates #232

Fix Google reverse lookup in getPlacenameFromCoordinates #232

nstjean commented Jan 9, 2020

nstjean commented Jan 9, 2020

nstjean commented Jan 9, 2020

jywarren commented Jan 9, 2020 via email

crisner commented Jan 10, 2020

nstjean commented Jan 10, 2020

nstjean commented Jan 10, 2020

jywarren commented Jan 10, 2020

jywarren commented Jan 10, 2020

jywarren Jan 10, 2020

nstjean Jan 13, 2020

nstjean commented Jan 13, 2020

nstjean commented Jan 13, 2020

crisner commented Jan 14, 2020

crisner commented Jan 14, 2020

crisner commented Jan 14, 2020

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

sagarpreet-chadha commented Jan 14, 2020

nstjean commented Jan 14, 2020 via email

sagarpreet-chadha commented Jan 14, 2020

nstjean commented Jan 14, 2020 via email

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

sagarpreet-chadha left a comment

jywarren commented Jan 15, 2020

emilyashley commented Jan 15, 2020

jywarren commented Jan 15, 2020

jywarren commented Jan 15, 2020

jywarren commented Jan 15, 2020

Fix Google reverse lookup in getPlacenameFromCoordinates #232

Fix Google reverse lookup in getPlacenameFromCoordinates #232

Conversation

nstjean commented Jan 9, 2020

nstjean commented Jan 9, 2020

nstjean commented Jan 9, 2020

jywarren commented Jan 9, 2020 via email

crisner commented Jan 10, 2020

nstjean commented Jan 10, 2020

nstjean commented Jan 10, 2020

jywarren commented Jan 10, 2020

jywarren commented Jan 10, 2020

jywarren Jan 10, 2020

Choose a reason for hiding this comment

nstjean Jan 13, 2020

Choose a reason for hiding this comment

nstjean commented Jan 13, 2020

nstjean commented Jan 13, 2020

crisner commented Jan 14, 2020

crisner commented Jan 14, 2020

crisner commented Jan 14, 2020

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

sagarpreet-chadha commented Jan 14, 2020

nstjean commented Jan 14, 2020 via email

sagarpreet-chadha commented Jan 14, 2020

nstjean commented Jan 14, 2020 via email

nstjean commented Jan 14, 2020

nstjean commented Jan 14, 2020

sagarpreet-chadha left a comment

Choose a reason for hiding this comment

jywarren commented Jan 15, 2020

emilyashley commented Jan 15, 2020

jywarren commented Jan 15, 2020

jywarren commented Jan 15, 2020

jywarren commented Jan 15, 2020