Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

changed: added geocoding api #243

Closed
wants to merge 2 commits into from

Conversation

green3g
Copy link

@green3g green3g commented Mar 16, 2021

In response to enketo/enketo#1158

I created a service on /api/v2/geocoder.

It returns basic geojson with the lat/lon and Metadata from the provider (currently only mapbox)

New providers can be added without major rework.

Note, additional config needed:

    "geocoder": {
        "provider": "mapbox",
        "url": "https://api.mapbox.com/geocoding/v5/mapbox.places/{address}.json", // optional
        "apiKey": "pk....." // required
    },

Let me know your thoughts

@MartijnR
Copy link
Member

Thanks! Much appreciated. I haven't yet looked at the code, but just have a few initial comments:

  1. I think this should not be under /api/v2 and won't be documented in https://apidocs.enketo.org/v2 since that API is for other apps to communicate with Enketo. In this case, it's an internal API. How about a geo-controller.js that responds to /geo/geocoder?
  2. In order to merge this we'd need to add the Google Geocoding service as well. Note that some installations using only Mapbox/Leaflet tile layers and Google Geocoding do/could still work at the moment. If you'd have the time to do so, that would be great. Otherwise, it will have to wait until I have time.
  3. Please add a default value in default-config.json, defaulting to Google (to minimize required config changes for existing installations) when upgrading.
  4. In addition, to avoid problems with existing installations, I think we should use the google['api key'] as a backup if geocoder['api key'] is not available but google is selected as the geocoding service (which would be the default)
  5. Please use the existing naming convention for config items (so "api key" instead of "apiKey")
  6. Please document new config items in tutorials/10-configure.md

@green3g
Copy link
Author

green3g commented Mar 18, 2021

Good feedback, thanks! I'll take another stab at it.

@lognaturel
Copy link
Contributor

Closing for now but please re-open if this becomes active again. Thanks!

@lognaturel lognaturel closed this Apr 21, 2022
@green3g
Copy link
Author

green3g commented Mar 23, 2023

Hi @lognaturel I have updated this merge request with @MartijnR 's feedback and I think its in a better place. Would you be open to reviewing it again? I've also updated the enketo core MR as well.

@lognaturel
Copy link
Contributor

It looks like you'll need to open a new PR. We'll try to look at it soon!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants