Enable search box geocoder provider selection #14622

jesusbotella · 2019-01-18T09:35:51Z

This PR enables geocoder configuration for Builder's search box by configuring it in app_config.yml.

The way it was working until now didn't allow to configure the geocoding provider even if we were injecting API Keys of each provider into HTML templates.

I researched this matter deeply and the more straightforward way to do it is injecting the geocoding configuration properties to search overlay's configuration held in vizjson. But as we're caching them and we might not want to update them all or just not exposing those variables in vizjson for safety reasons I had to look for another way to do it.

So, I added a new property to app_config.yml named provider inside geocoder configuration that will hold the string of the selected geocoding provider.

From now on, when rendering Builder, Datasets or Embeds page templates, there will be new injected variable named geocoderConfiguration that replicates app_config.yml to be used by CARTO.js to configure search box geocoder.

CARTO.js will read that configuration and configure geocoding for search box. This code is modified in this CARTO.js PR.

I took the opportunity to remove injected useless variables like mapzenApiKey, mapboxApiKey and tomtomApiKey because they were not used anymore as we changed the way we configure geocoder in CARTO.js' search box.

Related to CartoDB/carto.js#2223.

jesusbotella · 2019-01-18T09:39:52Z

@javitonino @ivanmalagon Let me know what you think of this approach! :)

javitonino

Looks like a good approach. Some comments:

This requires platform config changes
When we deploy this, we must not forget to tick the invalidate public pages checkbox.

javitonino · 2019-01-18T13:30:40Z

config/app_config.yml.sample

@@ -201,6 +201,7 @@ defaults: &defaults
  geocoder:
    #force_batch: true
    #disable_cache: true
+    provider: ''


I'd rename to something more explicit like search_bar_provider or similar.

javitonino · 2019-01-18T13:33:19Z

lib/carto/configuration.rb


-    def tomtom_api_key
-      Cartodb.get_config(:geocoder, 'tomtom', 'search_bar_api_key')
+      geocoder_config


The above code is equivalent to:

geocoder_config = { provider: Cartodb.get_config(:geocoder, 'provider'), mapbox: Cartodb.config[:geocoder]['mapbox'], tomtom: Cartodb.config[:geocoder]['tomtom'] }

assuming you don't care that missing parameters are marked as null. e.g, where current code would return {}, mine returns { provider: null, mapbox: null, tomtom: null }.

That would be perfectly fine! Just one question, why do you use Cartodb.get_config in first place and then Cartodb.config? Is there any major difference?

I meant to use get_config for everything. The main difference is that if the string is blank (e.g: mapbox: ''), then config returns '' and get_config returns null.

I have changed this a bit to avoid injecting unneeded parameters

I had this idea, maybe too complicated:

GEOCODER_PROVIDERS = ['tomtom', 'mapbox', 'mapzen'].freeze def geocoder_config configured_providers = GEOCODER_PROVIDERS.select { |p| Cartodb.get_config(:geocoder, p, 'search_bar_api_key') } selected_provider = Cartodb.get_config(:geocoder, 'search_bar_provider') selected_provider = configured_providers.first unless configured_providers.include?(selected_provider) provider_configuration = Cartodb.get_config(:geocoder, selected_provider) { :provider => selected_provider, selected_provider => provider_configuration } end

It:

List all configured providers

Try to use the configured one

If not, use the first in the array (priority system)

This should work in all cases, but we should write some tests, since the behaviour starts to get complicated :/.

If not, we can revert to your previous version of listing all keys. It's simpler although it has the problem you mentioned (exposing too much) which I don't think it's a big problem, but it's certainly ugly.

Yes, maybe the flow above is too complicated for this :/

The previous version was good. I didn't discard it because its ugliness but for "security" and to optimize the rendered HTML, but if it's not a big deal for you I am not going to say so 😂

I've reverted the change that I did. So now it is back as it was before :)

jesusbotella · 2019-01-18T15:53:17Z

I assume that the platform changes would be because of the new app_config.yml property. It would be good to add the property to our production/development configurations but we covered that case by adding a default provider that is tomtom.

I would take care of the deploy so I'll write that down so I don't forget to tick the checkbox! :)

houndci-bot · 2019-01-18T16:40:48Z

lib/carto/configuration.rb

-    def tomtom_api_key
-      Cartodb.get_config(:geocoder, 'tomtom', 'search_bar_api_key')
+    def geocoder_config
+      geocoder_config = {


Useless assignment to variable - geocoder_config.

javitonino · 2019-01-22T11:33:09Z

lib/carto/configuration.rb

+
+      if provider_configuration.present?
+        geocoder_config[provider] = Cartodb.config[:geocoder][provider]
+      end


This change breaks backwards compatibility. If a user does not update his configuration file, the search bar will break. We try to really minimize them, so we should make this to keep working if the geocoder -> provider option is not specified.

Yes, I thought about it too. I thought this change wouldn't break anything. So, what would be the best way to handle it without breaking compatibility for a rails noob like me? 😂

I guess we can just use a default value: Cartodb.get_config(:geocoder, 'search_bar_provider') || 'tomtom' (for example)

That was the old code so no need to do this anymore

alrocar · 2019-01-24T11:47:28Z

lib/carto/configuration.rb

-      Cartodb.get_config(:geocoder, 'tomtom', 'search_bar_api_key')
+    def geocoder_config
+      {
+        provider: Cartodb.get_config(:geocoder, 'provider'),


After your last commit this should be search_bar_provider to match the app_config.yml.sample.

Woah! Yes, that's wrong!

alrocar · 2019-01-24T11:49:48Z

app/views/carto/builder/datasets/show.html.erb

  var dashboardNotifications = <%= safe_js_object @dashboard_notifications.to_json %>;
  var ACTIVE_LOCALE = 'en';
+  var geocoderConfiguration = <%= safe_js_object geocoder_config.to_json %>;


Just a question, where is this being used?

It is being used in CARTO.js through Builder, you can see the code here: https://github.com/CartoDB/carto.js/pull/2224/files#diff-7ead12936599aa5b9c6b5340db29cf33R214

alrocar · 2019-01-24T11:50:24Z

lib/carto/configuration.rb

@@ -69,16 +69,12 @@ def saas?
      Cartodb.config[:cartodb_com_hosted] == false
    end

-    def mapzen_api_key


looks like we are getting rid of mapzen api key, should we remove it from app_config.yml.sample as well?

Well, yes, I think we should now

alrocar · 2019-01-24T11:52:48Z

lib/carto/configuration.rb

+
+      if provider_configuration.present?
+        geocoder_config[provider] = Cartodb.config[:geocoder][provider]
+      end


I guess we can just use a default value: Cartodb.get_config(:geocoder, 'search_bar_provider') || 'tomtom' (for example)

alrocar

A couple of questions and something you missed :)

alrocar

👍

jesusbotella added 4 commits January 18, 2019 10:10

Add geocoder_config to configuration module

8142001

Add new provider property to app_config.yml.sample

bd4cd66

Remove unneeded properties passing to deepInsights.createDashboard

1920a44

Inject geocoder_config to needed templates

16a3792

Inject only necessary variables into geocoding configuration in template

3812771

javitonino reviewed Jan 18, 2019

View reviewed changes

Rename 'provider' to search_bar_provider. Simplify configuration.

9b9c95e

houndci-bot reviewed Jan 18, 2019

View reviewed changes

Fix Hound

8d56107

jesusbotella force-pushed the 2223-search-box-provider branch from 8ffcab5 to 8d56107 Compare January 21, 2019 09:26

jesusbotella mentioned this pull request Jan 21, 2019

Enable search box geocoder provider selection CartoDB/carto.js#2224

Merged

Update internal-carto.js

fa1531c

jesusbotella force-pushed the 2223-search-box-provider branch from c9b0896 to fa1531c Compare January 21, 2019 13:36

jesusbotella added 7 commits January 21, 2019 16:06

Return only configuration provider related data

c346d93

Merge 'master' into 223-search-box-provider

3558b07

Update package.json version for staging

fc98659

Update NEWS.md

e51835c

Simplify configuration for geocoder_config

27cfbf5

Update internal-carto.js

9347f07

Merge branch 'master' into 2223-search-box-provider

1c37994

javitonino suggested changes Jan 22, 2019

View reviewed changes

Revert configuration change

bc75768

alrocar self-assigned this Jan 24, 2019

alrocar reviewed Jan 24, 2019

View reviewed changes

alrocar suggested changes Jan 24, 2019

View reviewed changes

alrocar assigned jesusbotella and unassigned alrocar Jan 24, 2019

Apply CR suggestions

6fcae9b

alrocar approved these changes Jan 24, 2019

View reviewed changes

alrocar added do not merge yet and removed do not merge yet labels Jan 24, 2019

jesusbotella added 3 commits January 24, 2019 15:32

Merge branch 'master' into 2223-search-box-provider

be4c83b

Update package.json

8d353db

Revert package.json version update

2c2c683

jesusbotella merged commit 06f053f into master Jan 24, 2019

jesusbotella deleted the 2223-search-box-provider branch January 24, 2019 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable search box geocoder provider selection #14622

Enable search box geocoder provider selection #14622

jesusbotella commented Jan 18, 2019

jesusbotella commented Jan 18, 2019

javitonino left a comment

javitonino Jan 18, 2019

javitonino Jan 18, 2019

jesusbotella Jan 18, 2019

javitonino Jan 21, 2019

jesusbotella Jan 21, 2019

javitonino Jan 22, 2019 •

edited

Loading

javitonino Jan 22, 2019

jesusbotella Jan 22, 2019 •

edited

Loading

jesusbotella Jan 22, 2019

jesusbotella commented Jan 18, 2019

houndci-bot Jan 18, 2019

javitonino Jan 22, 2019

jesusbotella Jan 22, 2019

alrocar Jan 24, 2019

jesusbotella Jan 24, 2019

alrocar Jan 24, 2019 •

edited

Loading

jesusbotella Jan 24, 2019

alrocar Jan 24, 2019

jesusbotella Jan 24, 2019

alrocar Jan 24, 2019

jesusbotella Jan 24, 2019

alrocar Jan 24, 2019

alrocar left a comment

alrocar left a comment

Enable search box geocoder provider selection #14622

Enable search box geocoder provider selection #14622

Conversation

jesusbotella commented Jan 18, 2019

jesusbotella commented Jan 18, 2019

javitonino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javitonino Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusbotella Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusbotella commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

javitonino Jan 22, 2019 •

edited

Loading

jesusbotella Jan 22, 2019 •

edited

Loading

alrocar Jan 24, 2019 •

edited

Loading