With this PR, only the property name in the GeoJSON is internationalized.
Most of the time, we are using label property instead of name.
Maybe, this work should be done in changeLanguage midleware and not in geojsonify.

Looking at the code, I see both assignLabels and changeLanguage rely on / use name.default. Should I keep this behavior and substitute name.default with name.LANG's value if it makes sense?
Also - assignLabels happens before changeLanguage - when we construct a label like "name + rest_of_address", name should be translated already otherwise we'd get a different language in label than in name.

Thoughts?

I think you should update the name.default here

Because changeLanguage should do what it says, change the language.
I think it will be weird to do the job in assignLabels + geojsonify.

I think you should update the name.default here

api/middleware/changeLanguage.js

Lines 65 to 67 in ceaf1f0

	res.data.forEach( function( doc, p ){
	// skip invalid records

Because changeLanguage should do what it says, change the language.
I think it will be weird to do the job in assignLabels + geojsonify.

Indeed, no question there.
What I'm not sure of - it seems that by the tame we get to changeLanguage, assignLabels has already happened. And it has constructed the label field by including name.default (the non translated one). That would lead to some inconsistencies.

Is my observation correct? And if so, would it maybe make sense to do assignLabels after changeLanguage?

Nope, changeLanguage is executed before assignLabels 😄

My bad, I was scrolled into the postProc object 🤦‍♂ . Thanks! Addressing.

@Joxit I updated the code. I don't understand the Travis failure, could you please shed some light? npm test runs successfully on my machine.

Tests will fail if you do npm update
I think Geolib failed their 3.0.0 release, I opened #1303 to fix this 😄
Don't worry :) you will need to rebase to master when #1303 will be merged

@Joxit I rebased on top of the new master.

@Joxit testing this I realized that placeholder is required to be up for the translation to happen, even though the [services] page claims that for autocomplete you don't need placeholder. Do you see any quick win / way around this?
If not, maybe I could update that doc to say "optional - needed for translations".
Wdyt?

Hey @mihneadb,
This PR looks good in my testing so far. Note that we already use Placeholder for all admin translations, but this PR will help with venues, especially from OSM where they often have translations in many languages. I will give it a bit more testing just to make sure but I think it's good to merge.

The services page does already have a column at the right side noting that Placeholder is required for all admin translations. Maybe we can make it more clear somehow?

Hey @mihneadb,
This PR looks good in my testing so far. Note that we already use Placeholder for all admin translations, but this PR will help with venues, especially from OSM where they often have translations in many languages. I will give it a bit more testing just to make sure but I think it's good to merge.

The services page does already have a column at the right side noting that Placeholder is required for all admin translations. Maybe we can make it more clear somehow?

You're right, I see it now. Fail on my part. Nvm then :). I didn't read all of the Pelias code unfortunately so maybe this is a stupid question - but looking at changeLanguage.js, it would seem that all the necessary info to translate admin and venue names in in the ES documents themselves, which were imported with name.*. What's the part that I'm missing that's making placeholder required at runtime for translations?

Thanks for the help on this! Glad it turned out to work :).

You're right that all the name translations are present in the Elasticsearch document, but not all of the parent fields.

For example, we have 8 million addresses here in NYC. There aren't really translations for most address records, but if there were, we would store them in each record. However, there are about 100 translations for New York City and those are not stored with each record, or we would have 8 million * 100 = 8 billion copies of the same 100 translations in Elasticsearch. A relational database is much better for that, and that's why Placeholder uses SQLite

Is there anything else I need to help with on this or is it just about testing and playing with it some more?

Thanks!

Hi @mihneadb,
Nothing was needed on your end, just had to find a bit more time to finish testing this.

I've now done so, and am super excited, this is an important bit of functionality and I think it's good to go.

I found one thing that needs to be changed: it's important to only change the name.default value if our language middleware detected a language value either through headers or the lang param, rather than defaulting to English. Without this, any such request will translate all venue names to english, rather than the local language, which was a significant change in behavior.

I've made that change, squashed your commits, updated the commit message to our format that will automatically add this feature to the package changelog, and pushed all that back to your branch so it can be merged easily. I also made some acceptance tests to verify all this behavior over in pelias/acceptance-tests#500.

Thanks for taking the time to look at this!

Right, that makes sense. Thanks a lot for taking care of this!