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

Allow returning Geonames results from /reverse #1045

Merged
merged 2 commits into from
Oct 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 16 additions & 5 deletions sanitizer/_geonames_deprecation.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,32 @@
const _ = require('lodash');

/**
with the release of coarse reverse as a separate thing and ES reverse only
handling venues, addresses, and streets, geonames make no sense in the reverse context
* Now that we have the pip-service, we have stopped supporting returning Geonames for coarse reverse.
*
* However, until the `/nearby` endpoint is finalized, we still want to support Geonames for
* _non-coarse_ reverse.
**/

const coarse_reverse_message ='coarse /reverse does not support geonames. See https://github.com/pelias/pelias/issues/675 for more info';

function _sanitize( raw, clean, opts ) {
// error & warning messages
const messages = { errors: [], warnings: [] };

// return taking no action unless this is a coarse-only reverse request
const non_coarse_layers = ['address', 'street', 'venue'];
const is_coarse_reverse = !_.isEmpty(clean.layers) &&
_.isEmpty(_.intersection(clean.layers, non_coarse_layers));
if (!is_coarse_reverse) {
return messages;
}

if (_.isEqual(clean.sources, ['geonames']) || _.isEqual(clean.sources, ['gn'])) {
messages.errors.push('/reverse does not support geonames');
messages.errors.push(coarse_reverse_message);

} else if (_.includes(clean.sources, 'geonames') || _.includes(clean.sources, 'gn')) {
clean.sources = _.without(clean.sources, 'geonames', 'gn');
messages.warnings.push('/reverse does not support geonames');

messages.warnings.push(coarse_reverse_message);
}

return messages;
Expand Down
46 changes: 39 additions & 7 deletions test/unit/sanitizer/_geonames_deprecation.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ const sanitizer = require('../../../sanitizer/_geonames_deprecation')();

module.exports.tests = {};

const coarse_reverse_message ='coarse /reverse does not support geonames. See https://github.com/pelias/pelias/issues/675 for more info';

module.exports.tests.no_warnings_or_errors_conditions = (test, common) => {
test('undefined sources should add neither warnings nor errors', (t) => {
const clean = {};
Expand All @@ -27,41 +29,71 @@ module.exports.tests.no_warnings_or_errors_conditions = (test, common) => {

});

test('geonames/gn in sources with layers=venue should add neither warnings nor errors', (t) => {
const clean = {
sources: ['geonames'],
layers: ['venue']
};

const messages = sanitizer.sanitize(undefined, clean);

t.deepEquals(clean.sources, ['geonames']);
t.deepEquals(messages, { errors: [], warnings: [] });
t.end();
});
};

module.exports.tests.error_conditions = (test, common) => {
test('only geonames in sources should not modify clean.sources and add error message', (t) => {
test('only geonames in sources with layers=coarse should not modify clean.sources and add error message', (t) => {
['gn', 'geonames'].forEach((sources) => {
const clean = {
sources: [sources]
sources: [sources],
layers: ['coarse']
};

const messages = sanitizer.sanitize(undefined, clean);

t.deepEquals(clean.sources, [sources]);
t.deepEquals(messages.errors, ['/reverse does not support geonames']);
t.deepEquals(messages.errors, [ coarse_reverse_message ]);
t.deepEquals(messages.warnings, []);

});

t.end();

});

test('only geonames in sources with layers=locality should not modify clean.sources and add error message', (t) => {
['gn', 'geonames'].forEach((sources) => {
const clean = {
sources: [sources],
layers: ['locality']
};

const messages = sanitizer.sanitize(undefined, clean);

t.deepEquals(clean.sources, [sources]);
t.deepEquals(messages.errors, [ coarse_reverse_message ]);
t.deepEquals(messages.warnings, []);

});

t.end();
});
};

module.exports.tests.warning_conditions = (test, common) => {
test('only geonames in sources should not modify clean.sources and add error message', (t) => {
test('only geonames in sources and layers=coarse should not modify clean.sources and add error message', (t) => {
['gn', 'geonames'].forEach((sources) => {
const clean = {
sources: ['another source', sources, 'yet another source']
sources: ['another source', sources, 'yet another source'],
layers: ['coarse']
};

const messages = sanitizer.sanitize(undefined, clean);

t.deepEquals(clean.sources, ['another source', 'yet another source']);
t.deepEquals(messages.errors, []);
t.deepEquals(messages.warnings, ['/reverse does not support geonames']);
t.deepEquals(messages.warnings, [ coarse_reverse_message ]);

});

Expand Down