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.

Sign up for GitHub

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

only translate name.default when translation is same source and source_id #1555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

only translate name.default when translation is same source and source_id #1555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions middleware/changeLanguage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ function updateDocs( req, res, translations ){
var match = attr.match(/^(.*)_id$/);
if( !match ){ continue; }

// adminKey is the property name without the '_id'
// eg. for 'country_id', adminKey would be 'country'.
var adminKey = match[1];
var adminValues = doc.parent[adminKey];
// adminLayer is the property name without the '_id'
// eg. for 'country_id', adminLayer would be 'country'.
var adminLayer = match[1];
var adminValues = _.get(doc.parent, adminLayer);

// skip invalid/empty arrays
if( !Array.isArray( adminValues ) || !adminValues.length ){ continue; }
Expand All @@ -92,29 +92,39 @@ function updateDocs( req, res, translations ){
for( var i in adminValues ){

// find the corresponding key from the '_id' Array
var id = doc.parent[attr][i];
if( !id ){ continue; }

// id not found in translation service response
if( !_.has(translations, id)){
logger.debug( `[language] [debug] failed to find translations for ${id}` );
let adminID = _.get(doc.parent[attr], i);
if (!adminID) { continue; }

// find the corresponding key from the '_source' Array
// dataset the parent property was sourced from, else 'whosonfirst'.
// the language service *at time of writing* only returns translations
// from whosonfirst, so we skip translating fields from other sources.
// see: https://github.com/pelias/schema/pull/459
// see: https://github.com/pelias/model/pull/128
let adminSource = _.get(doc.parent, `${match}_source[${i}]`, 'whosonfirst');
if (adminSource !== 'whosonfirst') { continue; }

// adminID not found in translation service response
if( !_.has(translations, adminID)){
logger.debug( `[language] [debug] failed to find translations for ${adminID}` );
continue;
}

// requested language is not available
if (_.isEmpty(_.get(translations[id].names, requestLanguage, [] ))) {
logger.debug( `[language] [debug] missing translation ${requestLanguage} ${id}` );
if (_.isEmpty(_.get(translations[adminID].names, requestLanguage, [] ))) {
logger.debug( `[language] [debug] missing translation ${requestLanguage} ${adminID}` );
continue;
}

// translate 'parent.*' property
adminValues[i] = translations[id].names[ requestLanguage ][0];

// if the record is an admin record we also translate
// the 'name.default' property.
if( adminKey === doc.layer ){
doc.name.default = translations[id].names[ requestLanguage ][0];
}
adminValues[i] = translations[adminID].names[ requestLanguage ][0];

// if the record is an admin record we also translate the 'name.default' property.
// note: ensure the records have the same 'layer' 'source' and 'source_id'.
if (adminLayer !== doc.layer){ continue; }
if (adminSource !== doc.source){ continue; }
if (_.toString(adminID) !== _.toString(doc.source_id)){ continue; }
doc.name.default = translations[adminID].names[requestLanguage][0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we actually need this line now. Looking through the history of this file, the translateNameDefault method was added in 2019 in #1301, whereas the rest of the file is mostly much older.

It might be that the name translation is already handled well now? I'm not 100% sure, the intention behind this code is definitely hard to read.

}
}
});
Expand Down
111 changes: 111 additions & 0 deletions test/unit/middleware/changeLanguage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ module.exports.tests.success_conditions = (test, common) => {
default: 'original name for 1st result'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1'],
Expand All @@ -165,6 +167,8 @@ module.exports.tests.success_conditions = (test, common) => {
default: 'original name for 2nd result'
},
layer: 'layer10',
source: 'whosonfirst',
source_id: '2',
parent: {
layer3_id: ['3', '4'],
layer3: ['original name 1 for layer3', 'original name 2 for layer3'],
Expand All @@ -190,6 +194,7 @@ module.exports.tests.success_conditions = (test, common) => {
},
// note that this is address!
layer: 'address',
source: 'whosonfirst',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1']
Expand All @@ -203,6 +208,7 @@ module.exports.tests.success_conditions = (test, common) => {
},
// note that this is address!
layer: 'address',
source: 'whosonfirst',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1']
Expand All @@ -225,6 +231,8 @@ module.exports.tests.success_conditions = (test, common) => {
default: 'replacement name for layer1'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1'],
Expand All @@ -239,6 +247,8 @@ module.exports.tests.success_conditions = (test, common) => {
default: 'original name for 2nd result'
},
layer: 'layer10',
source: 'whosonfirst',
source_id: '2',
parent: {
layer3_id: ['3', '4'],
layer3: ['replacement name 1 for layer3', 'replacement name 2 for layer3'],
Expand All @@ -258,6 +268,7 @@ module.exports.tests.success_conditions = (test, common) => {
'requested language': 'translated name'
},
layer: 'address',
source: 'whosonfirst',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1']
Expand All @@ -269,6 +280,7 @@ module.exports.tests.success_conditions = (test, common) => {
'random language': 'translated name'
},
layer: 'address',
source: 'whosonfirst',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1']
Expand All @@ -280,7 +292,106 @@ module.exports.tests.success_conditions = (test, common) => {
t.end();

});
});

test('admin fields from non-whosonfirst source should be skipped', t => {

const service = (req, res, callback) => {
callback(null, { '1': { names: { 'requested language': ['replacement name for layer1'] } } });
};

const logger = require('pelias-mock-logger')();

const controller = proxyquire('../../../middleware/changeLanguage', {
'pelias-logger': logger
})(service, () => true);

const req = {
clean: {
lang: {
iso6393: 'requested language',
iso6391: 'requested language',
defaulted: false
}
}
};

const res = {
data: [{
name: {
default: 'original default name 1'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1']
}
}, {
name: {
default: 'original default name 2'
},
layer: 'layer1',
source: 'foo',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1']
}
}, {
name: {
default: 'original default name 3'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '2',
parent: {
layer1_id: ['1'],
layer1: ['original name for layer1']
}
}]
};

controller(req, res, () => {
t.deepEquals(res, {
data: [{
name: {
default: 'replacement name for layer1'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1']
}
}, {
name: {
default: 'original default name 2'
},
layer: 'layer1',
source: 'foo',
source_id: '1',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1']
}
},{
name: {
default: 'original default name 3'
},
layer: 'layer1',
source: 'whosonfirst',
source_id: '2',
parent: {
layer1_id: ['1'],
layer1: ['replacement name for layer1']
}
}]
});
t.end();
});
});

};
Expand Down