publiclab / leaflet-blurred-location

A Leaflet-based interface for selecting a "blurred" or low-resolution location, to preserve privacy
https://publiclab.github.io/leaflet-blurred-location/examples/
GNU General Public License v3.0
35 stars 32 forks source link

Fix Google reverse lookup in getPlacenameFromCoordinates #232

Closed nstjean closed 4 years ago

nstjean commented 4 years ago

Issue #231

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.

nstjean commented 4 years ago

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 commented 4 years ago

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

jywarren commented 4 years ago

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 notifications@github.com 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 https://github.com/publiclab/leaflet-blurred-location/pull/232?email_source=notifications&email_token=AAAF6J2TFKNGLL5BIHB3MM3Q46M7NA5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIR55GA#issuecomment-572776088, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6JZOM5TFJLWJD5QT5CDQ46M7NANCNFSM4KE457VQ .

crisner commented 4 years ago

@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 commented 4 years ago

Yes that fixed it! It works now. :)

nstjean commented 4 years ago

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 commented 4 years ago

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

jywarren commented 4 years ago

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.

nstjean commented 4 years ago

@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 commented 4 years ago

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

Peek 2020-01-13 15-23

crisner commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

nstjean commented 4 years ago

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?

natalie@natalie-ubuntu: ~-Dev-Ruby-public-lab-leaflet-blurred-location_018

nstjean commented 4 years ago

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 commented 4 years ago

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

nstjean commented 4 years ago

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 notifications@github.com 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 https://github.com/publiclab/leaflet-blurred-location/pull/232?email_source=notifications&email_token=ALZLKMKKKHTKFUK2UHED7V3Q5XTF7A5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI5JDVI#issuecomment-574263765, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALZLKMPGZLPW7AXCTM2F7BDQ5XTF7ANCNFSM4KE457VQ .

sagarpreet-chadha commented 4 years ago

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 commented 4 years ago

Ok! Sounds good!

On Tue, Jan 14, 2020 at 12:28 PM Sagarpreet Chadha notifications@github.com 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 https://github.com/publiclab/leaflet-blurred-location/pull/232?email_source=notifications&email_token=ALZLKMKPDZY2VVEZDYZKODTQ5XY3FA5CNFSM4KE457V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI5OMQI#issuecomment-574285377, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALZLKMOVML5AE2UA62MJVLLQ5XY3FANCNFSM4KE457VQ .

nstjean commented 4 years ago

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 commented 4 years ago

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

jywarren commented 4 years ago

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

emilyashley commented 4 years ago

Just went over this with @jywarren. Awesome! :tada:

jywarren commented 4 years ago

😄 and published as v1.6.0!

jywarren commented 4 years ago

want us to bump dependabot?

jywarren commented 4 years ago

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