Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable the selection of the search box provider #2223

Closed
ivanmalagon opened this issue Jan 16, 2019 · 7 comments
Closed

Enable the selection of the search box provider #2223

ivanmalagon opened this issue Jan 16, 2019 · 7 comments
Assignees

Comments

@ivanmalagon
Copy link
Contributor

Context

We've recently changed our search box provider to TomTom instead of Mapbox. See tickets:

The "problem" is that even if the code seems to select the provider using an option, in the reality we don't set that option in any place.

We need to address it somehow, to enable the configuration of the provider. This is a likely scenario in OnPremises, where a customer can have the chance of using their own credentials.

I don't a clear idea on how to tackle this in a simple way but one of the options is to provide a hierarchy on providers and then look into the credentials that backend sets. If TomTom key exists, use TomTom. If not, mapbox... and so on.

<script>
    var mapzenApiKey = 'mapzen-NYwhatever';
    var mapboxApiKey = 'pk.eyJ1IjoiY2FyWadusLadusFadus';
    var tomtomApiKey = 'supercalifragilisticoespialidoso';
  </script>

If you have any better idea please let's discuss it here.

cc @javitonino

cc @ramiroaznar @jesusbotella (Response Team)

@jesusbotella
Copy link
Contributor

jesusbotella commented Jan 16, 2019

User model has a property named geocoder_provider which sets the provider that you might want to use, which is not being used right now in Builder because it defaults to tomtom.

So why instead of relying on some window variables, we use that property to determine the geocoder and then look for the API Key on the window scope and if it's not there throw an error?.

@javitonino
Copy link

We cannot use the geocoder_provider for that. That is the configuration of the user, which may be an unsupported provider for search (e.g: heremaps). It has to be some global config.

Carto.js wise, we probably want to expose a provider parameter. Builder-side we probably want to add an option to app_config.yml to select that (or use some order if multiple keys are provided).

@jesusbotella
Copy link
Contributor

jesusbotella commented Jan 16, 2019

Yes, it might be much easier using that injected property rather than trying to determine which provider to use from CARTO.js and definitely easier to change.

@jesusbotella
Copy link
Contributor

I've noticed that we have some geocoder configuration options in app_config.yml already. So my proposal is, why don't we add a property to select which geocoder is the one to use and inject the configuration into JS?

The app_config.yml configuration is:

  geocoder:
    #force_batch: true
    #disable_cache: true
    app_id: ''
    token:  ''
    mailto: ''
    base_url: ''
    non_batch_base_url: ''
    internal:
      username: ''
      api_key: ''
    cache:
      base_url: ''
      api_key: ''
      table_name: ''
    mapzen:
      search_bar_api_key: ''
    mapbox:
      search_bar_api_key: ''
    tomtom:
      search_bar_api_key: ''
    api:
      host: 'localhost'
      port: '5432'
      dbname: 'dataservices_db'
      user: 'geocoder_api'

We can retrieve the properties in visualizations controller and then inject it into the template.

@javitonino @ivanmalagon What do you think about this approach?

@jesusbotella
Copy link
Contributor

I've tested it and the template injects geocoderConfiguration in Builder and Embed and it changes whenever provider property changes.

@csubira
Copy link
Contributor

csubira commented Jan 22, 2019

Tested:

  • geocoderConfiguration property is included in Builder, dataset and embed.
  • After changing the provider in app_config.yml geocoderConfiguration changes.
  • Search working in all of the tests.

@jesusbotella
Copy link
Contributor

Merged and working in all pages (including embed)! 🚀

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

No branches or pull requests

4 participants