From 9d54735e4112610d381562465a17675cba34f60a Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 12 Sep 2017 11:12:26 -0400 Subject: [PATCH] converted geojsonify_place_details to return instead of modify --- helper/geojsonify.js | 2 +- helper/geojsonify_place_details.js | 22 ++-- test/unit/helper/geojsonify.js | 73 ++++++++----- test/unit/helper/geojsonify_place_details.js | 106 +++++-------------- 4 files changed, 85 insertions(+), 118 deletions(-) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index da7285a5..fdadc2b2 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -58,7 +58,7 @@ function geojsonifyPlace(params, place) { logger.warn(`doc ${doc.gid} does not contain name.default`); } - addDetails(params, place, doc); + _.assign(doc, addDetails(params, place)); return doc; } diff --git a/helper/geojsonify_place_details.js b/helper/geojsonify_place_details.js index 33879c39..bca94fab 100644 --- a/helper/geojsonify_place_details.js +++ b/helper/geojsonify_place_details.js @@ -53,19 +53,18 @@ function checkCategoryParam(params) { } /** - * Copy specified properties from source to dest. + * Collect the specified properties from source into an object and return it * Ignore missing properties. * * @param {object} params clean query params * @param {object} source * @param {object} dst */ -function copyProperties( params, source, dst ) { - DETAILS_PROPS.forEach( function ( prop ) { - - // if condition isn't met, just return without setting the property +function collectProperties( params, source ) { + return DETAILS_PROPS.reduce((result, prop) => { + // if condition isn't met, don't set the property if (_.isFunction(prop.condition) && !prop.condition(params)) { - return; + return result; } if ( source.hasOwnProperty( prop.name ) ) { @@ -84,10 +83,14 @@ function copyProperties( params, source, dst ) { } if (_.isNumber(value) || (value && !_.isEmpty(value))) { - dst[prop.name] = value; + result[prop.name] = value; } } - }); + + return result; + + }, {}); + } function getStringValue(property) { @@ -108,7 +111,6 @@ function getStringValue(property) { return _.toString(property); } - function getArrayValue(property) { // isEmpty check works for all types of values: strings, arrays, objects if (_.isEmpty(property)) { @@ -122,4 +124,4 @@ function getArrayValue(property) { return [property]; } -module.exports = copyProperties; +module.exports = collectProperties; diff --git a/test/unit/helper/geojsonify.js b/test/unit/helper/geojsonify.js index 8d6fc490..72ff6392 100644 --- a/test/unit/helper/geojsonify.js +++ b/test/unit/helper/geojsonify.js @@ -71,11 +71,15 @@ module.exports.tests.all = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 1') { - dst.property1 = 'property 1'; - dst.property2 = 'property 2'; + return { + property1: 'property 1', + property2: 'property 2' + }; } else if (source._id === 'id 2') { - dst.property3 = 'property 3'; - dst.property4 = 'property 4'; + return { + property3: 'property 3', + property4: 'property 4' + }; } } @@ -174,11 +178,15 @@ module.exports.tests.all = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 1') { - dst.property1 = 'property 1'; - dst.property2 = 'property 2'; + return { + property1: 'property 1', + property2: 'property 2' + }; } else if (source._id === 'id 2') { - dst.property3 = 'property 3'; - dst.property4 = 'property 4'; + return { + property3: 'property 3', + property4: 'property 4' + }; } } @@ -273,11 +281,15 @@ module.exports.tests.all = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 1') { - dst.property1 = 'property 1'; - dst.property2 = 'property 2'; + return { + property1: 'property 1', + property2: 'property 2' + }; } else if (source._id === 'id 2') { - dst.property3 = 'property 3'; - dst.property4 = 'property 4'; + return { + property3: 'property 3', + property4: 'property 4' + }; } } @@ -359,8 +371,10 @@ module.exports.tests.non_optimal_conditions = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 1') { - dst.property1 = 'property 1'; - dst.property2 = 'property 2'; + return { + property1: 'property 1', + property2: 'property 2' + }; } }, 'pelias-logger': logger @@ -429,8 +443,10 @@ module.exports.tests.non_optimal_conditions = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 2') { - dst.property3 = 'property 3'; - dst.property4 = 'property 4'; + return { + property3: 'property 3', + property4: 'property 4' + }; } }, 'pelias-logger': logger @@ -511,16 +527,21 @@ module.exports.tests.non_optimal_conditions = (test, common) => { const geojsonify = proxyquire('../../../helper/geojsonify', { './geojsonify_place_details': (params, source, dst) => { if (source._id === 'id 1') { - dst.property1 = 'property 1'; - dst.property2 = 'property 2'; - } - else if (source._id === 'id 2') { - dst.property3 = 'property 3'; - dst.property4 = 'property 4'; - } - else if (source._id === 'id 3') { - dst.property5 = 'property 5'; - dst.property6 = 'property 6'; + return { + property1: 'property 1', + property2: 'property 2' + }; + } else if (source._id === 'id 2') { + return { + property3: 'property 3', + property4: 'property 4' + }; + } else if (source._id === 'id 3') { + return { + property5: 'property 5', + property6: 'property 6' + }; + } }, 'pelias-logger': logger diff --git a/test/unit/helper/geojsonify_place_details.js b/test/unit/helper/geojsonify_place_details.js index 3bc96adc..97b444f7 100644 --- a/test/unit/helper/geojsonify_place_details.js +++ b/test/unit/helper/geojsonify_place_details.js @@ -5,7 +5,6 @@ module.exports.tests = {}; module.exports.tests.geojsonify_place_details = (test, common) => { test('plain old string values should be copied verbatim, replacing old values', t => { const source = { - unknown_field: 'original unknown_field value', housenumber: 'housenumber value', street: 'street value', postalcode: 'postalcode value', @@ -43,48 +42,8 @@ module.exports.tests.geojsonify_place_details = (test, common) => { neighbourhood_gid: 'neighbourhood_gid value', label: 'label value' }; - const destination = { - unknown_field: 'original unknown_field value', - housenumber: 'original housenumber value', - street: 'original street value', - postalcode: 'original postalcode value', - postalcode_gid: 'original postalcode_gid value', - match_type: 'original match_type value', - accuracy: 'original accuracy value', - country: 'original country value', - country_gid: 'original country_gid value', - country_a: 'original country_a value', - dependency: 'original dependency value', - dependency_gid: 'original dependency_gid value', - dependency_a: 'original dependency_a value', - macroregion: 'original macroregion value', - macroregion_gid: 'original macroregion_gid value', - macroregion_a: 'original macroregion_a value', - region: 'original region value', - region_gid: 'original region_gid value', - region_a: 'original region_a value', - macrocounty: 'original macrocounty value', - macrocounty_gid: 'original macrocounty_gid value', - macrocounty_a: 'original macrocounty_a value', - county: 'original county value', - county_gid: 'original county_gid value', - county_a: 'original county_a value', - localadmin: 'original localadmin value', - localadmin_gid: 'original localadmin_gid value', - localadmin_a: 'original localadmin_a value', - locality: 'original locality value', - locality_gid: 'original locality_gid value', - locality_a: 'original locality_a value', - borough: 'original borough value', - borough_gid: 'original borough_gid value', - borough_a: 'original borough_a value', - neighbourhood: 'original neighbourhood value', - neighbourhood_gid: 'original neighbourhood_gid value', - label: 'original label value' - }; const expected = { - unknown_field: 'original unknown_field value', housenumber: 'housenumber value', street: 'street value', postalcode: 'postalcode value', @@ -123,9 +82,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { label: 'label value' }; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -170,12 +129,11 @@ module.exports.tests.geojsonify_place_details = (test, common) => { neighbourhood_gid: empty_value, label: empty_value }; - const destination = {}; const expected = {}; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); }); @@ -222,7 +180,6 @@ module.exports.tests.geojsonify_place_details = (test, common) => { neighbourhood_gid: ['neighbourhood_gid value 1', 'neighbourhood_gid value 2'], label: ['label value 1', 'label value 2'] }; - const destination = { }; const expected = { housenumber: 'housenumber value 1', @@ -263,9 +220,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { label: 'label value 1' }; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -310,7 +267,6 @@ module.exports.tests.geojsonify_place_details = (test, common) => { neighbourhood_gid: { neighbourhood_gid: 'neighbourhood_gid value'}, label: { label: 'label value'} }; - const destination = { }; const expected = { housenumber: '[object Object]', @@ -351,9 +307,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { label: '[object Object]' }; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -365,11 +321,6 @@ module.exports.tests.geojsonify_place_details = (test, common) => { distance: value, bounding_box: value }; - const destination = { - confidence: 'original confidence value', - distance: 'original distance value', - bounding_box: 'original bounding_box value' - }; const expected = { confidence: value, @@ -377,9 +328,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { bounding_box: value }; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); }); @@ -394,16 +345,15 @@ module.exports.tests.geojsonify_place_details = (test, common) => { distance: value, bounding_box: value }; - const destination = {}; const expected = { confidence: value, distance: value, bounding_box: value }; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); }); @@ -418,12 +368,11 @@ module.exports.tests.geojsonify_place_details = (test, common) => { distance: value, bounding_box: value }; - const destination = {}; const expected = {}; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); }); @@ -435,12 +384,11 @@ module.exports.tests.geojsonify_place_details = (test, common) => { const source = { category: [] }; - const destination = {}; const expected = {}; - geojsonify({}, source, destination); + const actual = geojsonify({}, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -449,7 +397,6 @@ module.exports.tests.geojsonify_place_details = (test, common) => { const source = { category: [ 1, 2 ] }; - const destination = {}; const expected = { category: [ 1, 2 ] }; @@ -458,9 +405,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { categories: true }; - geojsonify(clean, source, destination); + const actual = geojsonify(clean, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -470,7 +417,6 @@ module.exports.tests.geojsonify_place_details = (test, common) => { const source = { category: value }; - const destination = {}; const expected = { category: [ value ] }; @@ -479,9 +425,9 @@ module.exports.tests.geojsonify_place_details = (test, common) => { categories: true }; - geojsonify(clean, source, destination); + const actual = geojsonify(clean, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); }); @@ -493,15 +439,14 @@ module.exports.tests.geojsonify_place_details = (test, common) => { const source = { category: [ 1, 2 ] }; - const destination = {}; const expected = { }; const clean = {}; - geojsonify(clean, source, destination); + const actual = geojsonify(clean, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); @@ -510,19 +455,18 @@ module.exports.tests.geojsonify_place_details = (test, common) => { const source = { category: [ 1, 2 ] }; - const destination = {}; const expected = { }; const clean = 'this is not an object'; - geojsonify(clean, source, destination); + const actual = geojsonify(clean, source); - t.deepEqual(destination, expected); + t.deepEqual(actual, expected); t.end(); }); - + }; module.exports.all = (tape, common) => {