From 9f7f4aa3fcf95eb3ceb86e9965ee286d1d94f0c0 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 23 Dec 2018 01:04:35 -0500 Subject: [PATCH] Fix language fallback processing, caching bug. --- modules/services/osm_wikibase.js | 66 ++-- modules/ui/tag_reference.js | 2 +- test/spec/services/osm_wikibase.js | 470 +++++++++++++++-------------- 3 files changed, 278 insertions(+), 260 deletions(-) diff --git a/modules/services/osm_wikibase.js b/modules/services/osm_wikibase.js index 7570c5a56..366f6c5fc 100644 --- a/modules/services/osm_wikibase.js +++ b/modules/services/osm_wikibase.js @@ -9,7 +9,7 @@ import { utilQsString } from '../util'; var apibase = 'https://wiki.openstreetmap.org/w/api.php'; var _inflight = {}; var _wikibaseCache = {}; -var _localeIds = {}; +var _localeIds = { en: false }; var debouncedRequest = _debounce(request, 500, { leading: false }); @@ -24,6 +24,22 @@ function request(url, callback) { } +/** + * Get the best string value from the descriptions/labels result + * Note that if mediawiki doesn't recognize language code, it will return all values. + * In that case, fallback to use English. + * @param values object - either descriptions or labels + * @param langCode String + * @returns localized string + */ +function localizedToString(values, langCode) { + if (values) { + values = values[langCode] || values.en; + } + return values ? values.value : ''; +} + + export default { init: function() { @@ -72,21 +88,6 @@ export default { }, - getDescription: function(entity) { - if (entity.descriptions) { - // Assume that there will be at most two languages because of - // how we request it: English + possibly another one. - // Pick non-English description if available (if we have more than one) - var langs = Object.keys(entity.descriptions); - if (langs.length) { - var lng = langs.length > 1 && langs[0] === 'en' ? langs[1] : langs[0]; - return entity.descriptions[lng].value; - } - } - return undefined; - }, - - toSitelink: function(key, value) { var result = value ? 'Tag:' + key + '=' + value : 'Key:' + key; return result.replace(/_/g, ' ').trim(); @@ -97,21 +98,17 @@ export default { var doRequest = params.debounce ? debouncedRequest : request; var self = this; var titles = []; - var languages = ['en']; var result = {}; var keySitelink = this.toSitelink(params.key); var tagSitelink = params.value ? this.toSitelink(params.key, params.value) : false; var localeSitelink; - if (params.langCode && params.langCode !== 'en') { - languages.push(params.langCode); - if (!_localeIds[params.langCode]) { - // This is the first time we are asking about this locale - // Fetch corresponding entity (if it exists), and cache it. - // If there is no such entry, cache `true` to avoid re-requesting it. - localeSitelink = ('Locale:' + params.langCode).replace(/_/g, ' ').trim(); - titles.push(localeSitelink); - } + if (params.langCode && _localeIds[params.langCode] === undefined) { + // If this is the first time we are asking about this locale, + // fetch corresponding entity (if it exists), and cache it. + // If there is no such entry, cache `false` value to avoid re-requesting it. + localeSitelink = ('Locale:' + params.langCode).replace(/_/g, ' ').trim(); + titles.push(localeSitelink); } if (_wikibaseCache[keySitelink]) { @@ -122,7 +119,7 @@ export default { if (tagSitelink) { if (_wikibaseCache[tagSitelink]) { - result.key = _wikibaseCache[tagSitelink]; + result.tag = _wikibaseCache[tagSitelink]; } else { titles.push(tagSitelink); } @@ -133,11 +130,17 @@ export default { return callback(null, result); } + // Requesting just the user language code + // If backend recognizes the code, it will perform proper fallbacks, + // and the result will contain the requested code. If not, all values are returned: + // {"zh-tw":{"value":"...","language":"zh-tw","source-language":"zh-hant"} + // {"pt-br":{"value":"...","language":"pt","for-language":"pt-br"}} var obj = { action: 'wbgetentities', sites: 'wiki', titles: titles.join('|'), - languages: languages.join('|'), + languages: params.langCode, + languagefallback: 1, origin: '*', format: 'json', // There is an MW Wikibase API bug https://phabricator.wikimedia.org/T212069 @@ -152,10 +155,13 @@ export default { } else if (!d.success || d.error) { callback(d.error.messages.map(function(v) { return v.html['*']; }).join('
')); } else { - var localeId = true; + var localeId = false; _forEach(d.entities, function(res) { if (res.missing !== '') { var title = res.sitelinks.wiki.title; + // Simplify access to the localized values + res.description = localizedToString(res.descriptions, params.langCode); + res.label = localizedToString(res.labels, params.langCode); if (title === keySitelink) { _wikibaseCache[keySitelink] = res; result.key = res; @@ -171,7 +177,7 @@ export default { }); if (localeSitelink) { - // If locale ID is not found, set cache to true to prevent repeated queries + // If locale ID is not found, store false to prevent repeated queries self.addLocale(params.langCode, localeId); } diff --git a/modules/ui/tag_reference.js b/modules/ui/tag_reference.js index 8fa05a1b8..6215250b9 100644 --- a/modules/ui/tag_reference.js +++ b/modules/ui/tag_reference.js @@ -28,7 +28,7 @@ export function uiTagReference(tag) { var result = { title: entity.title, - description: wikibase.getDescription(entity), + description: entity.description, }; if (entity.claims) { diff --git a/test/spec/services/osm_wikibase.js b/test/spec/services/osm_wikibase.js index aaccc16f4..61951abba 100644 --- a/test/spec/services/osm_wikibase.js +++ b/test/spec/services/osm_wikibase.js @@ -24,226 +24,239 @@ describe('iD.serviceOsmWikibase', function () { return iD.utilStringQs(url.substring(url.indexOf('?') + 1)); } - var keyData = { - pageid: 205725, - ns: 120, - title: 'Item:Q61', - lastrevid: 1721242, - modified: '2018-12-18T07:00:43Z', - type: 'item', - id: 'Q61', - labels: { - en: {language: 'en', value: 'amenity'} - }, - descriptions: { - en: {language: 'en', value: 'For describing useful and important facilities for visitors and residents.'} - }, - aliases: {}, - claims: { - P2: [ // instance of - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q7'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'normal' - } - ], - P16: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'string', - datavalue: {value: 'amenity', type: 'string'} - }, - type: 'statement', - rank: 'normal' - } - ], - P25: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q4679'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'normal' - } - ], - P9: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q8'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'normal' - } - ], - P6: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q15'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'preferred' - }, - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q14'}, type: 'wikibase-entityid'} - }, - type: 'statement', - qualifiers: { - P26: [ - { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q6994'}, type: 'wikibase-entityid'} - } - ] - }, - rank: 'normal' - } - ], - P28: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'string', - datavalue: {value: 'Mapping-Features-Parking-Lot.png', type: 'string'} - }, - type: 'statement', - rank: 'normal' - } - ] - }, - sitelinks: { - wiki: { - site: 'wiki', - title: 'Key:amenity', - badges: [] + function adjust(params, data) { + if (params) { + if (params.norm) { + data.description = data.descriptions.fr.value; + data.label = data.labels.fr.value; } } - }; + return data; + } - var tagData = { - pageid: 210934, - ns: 120, - title: 'Item:Q4904', - lastrevid: 1718041, - modified: '2018-12-18T03:51:05Z', - type: 'item', - id: 'Q4904', - labels: { - en: {language: 'en', value: 'amenity=parking'} - }, - descriptions: { - en: {language: 'en', value: 'A place for parking cars'}, - fr: {language: 'fr', value: 'Un lieu pour garer des voitures'} - }, - aliases: {}, - claims: { - P2: [ // instance of = Q2 (tag) - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q2'}, type: 'wikibase-entityid'} + function keyData(params) { + return adjust(params, { + pageid: 205725, + ns: 120, + title: 'Item:Q42', + lastrevid: 1721242, + modified: '2018-12-18T07:00:43Z', + type: 'item', + id: 'Q42', + labels: { + fr: {language: 'en', value: 'amenity', 'for-language': 'fr'} + }, + descriptions: { + fr: {language: 'en', value: 'English description', 'for-language': 'fr'} + }, + aliases: {}, + claims: { + P2: [ // instance of + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q7'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ], + P16: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'string', + datavalue: {value: 'amenity', type: 'string'} + }, + type: 'statement', + rank: 'normal' + } + ], + P25: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q4679'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ], + P9: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q8'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ], + P6: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q15'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'preferred' }, - type: 'statement', - rank: 'normal' + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q14'}, type: 'wikibase-entityid'} + }, + type: 'statement', + qualifiers: { + P26: [ + { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q6994'}, type: 'wikibase-entityid'} + } + ] + }, + rank: 'normal' + } + ], + P28: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'string', + datavalue: {value: 'Mapping-Features-Parking-Lot.png', type: 'string'} + }, + type: 'statement', + rank: 'normal' + } + ] + }, + sitelinks: { + wiki: { + site: 'wiki', + title: 'Key:amenity', + badges: [] } - ], - P19: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'string', - datavalue: {value: 'amenity=parking', type: 'string'} - }, - type: 'statement', - rank: 'normal' - } - ], - P10: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q61'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'normal' - } - ], - P4: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'commonsMedia', - datavalue: {value: 'Primary image.jpg', type: 'string'} - }, - type: 'statement', - rank: 'preferred' - } - ], - P6: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q14'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'preferred' - }, - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q13'}, type: 'wikibase-entityid'} - }, - type: 'statement', - qualifiers: { - P26: [ - { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q6994'}, type: 'wikibase-entityid'} - } - ] - }, - rank: 'normal' - } - ], - P25: [ - { - mainsnak: { - snaktype: 'value', - datatype: 'wikibase-item', - datavalue: {value: {'entity-type': 'item', id: 'Q4679'}, type: 'wikibase-entityid'} - }, - type: 'statement', - rank: 'normal' - } - ] - }, - sitelinks: { - wiki: { - site: 'wiki', - title: 'Tag:amenity=parking', - badges: [] } - } - }; + }); + } + + function tagData(params) { + return adjust(params, { + pageid: 210934, + ns: 120, + title: 'Item:Q13', + lastrevid: 1718041, + modified: '2018-12-18T03:51:05Z', + type: 'item', + id: 'Q13', + labels: { + fr: {language: 'en', value: 'amenity=parking', 'for-language': 'fr'} + }, + descriptions: { + fr: {language: 'fr', value: 'French description'} + }, + aliases: {}, + claims: { + P2: [ // instance of = Q2 (tag) + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q2'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ], + P19: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'string', + datavalue: {value: 'amenity=parking', type: 'string'} + }, + type: 'statement', + rank: 'normal' + } + ], + P10: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q42'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ], + P4: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'commonsMedia', + datavalue: {value: 'Primary image.jpg', type: 'string'} + }, + type: 'statement', + rank: 'preferred' + } + ], + P6: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q14'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'preferred' + }, + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q13'}, type: 'wikibase-entityid'} + }, + type: 'statement', + qualifiers: { + P26: [ + { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q6994'}, type: 'wikibase-entityid'} + } + ] + }, + rank: 'normal' + } + ], + P25: [ + { + mainsnak: { + snaktype: 'value', + datatype: 'wikibase-item', + datavalue: {value: {'entity-type': 'item', id: 'Q4679'}, type: 'wikibase-entityid'} + }, + type: 'statement', + rank: 'normal' + } + ] + }, + sitelinks: { + wiki: { + site: 'wiki', + title: 'Tag:amenity=parking', + badges: [] + } + } + }); + } var localeData = { @@ -259,8 +272,8 @@ describe('iD.serviceOsmWikibase', function () { server.respondWith('GET', /action=wbgetentities/, [200, {'Content-Type': 'application/json'}, JSON.stringify({ entities: { - Q61: keyData, - Q4904: tagData, + Q42: keyData(), + Q13: tagData(), Q7792: localeData, }, success: 1 @@ -271,14 +284,18 @@ describe('iD.serviceOsmWikibase', function () { expect(query(server.requests[0].url)).to.eql( { action: 'wbgetentities', - format: 'json', - languages: 'en|fr', - origin: '*', sites: 'wiki', titles: 'Locale:fr|Key:amenity|Tag:amenity=parking', + languages: 'fr', + languagefallback: '1', + origin: '*', + format: 'json', } ); - expect(callback).to.have.been.calledWith(null, {key: keyData, tag: tagData}); + expect(callback).to.have.been.calledWith(null, { + key: keyData({norm: true}), + tag: tagData({norm: true}) + }); }); }); @@ -296,15 +313,10 @@ describe('iD.serviceOsmWikibase', function () { it('gets correct value from entity', function () { wikibase.addLocale('de', 'Q6994'); wikibase.addLocale('fr', 'Q7792'); - expect(wikibase.claimToValue(tagData, 'P4', 'en')).to.eql('Primary image.jpg'); - expect(wikibase.claimToValue(keyData, 'P6', 'en')).to.eql('Q15'); - expect(wikibase.claimToValue(keyData, 'P6', 'fr')).to.eql('Q15'); - expect(wikibase.claimToValue(keyData, 'P6', 'de')).to.eql('Q14'); - }); - - it('gets correct description from entity', function () { - expect(wikibase.getDescription(tagData)).to.eql('Un lieu pour garer des voitures'); - expect(wikibase.getDescription(keyData)).to.eql('For describing useful and important facilities for visitors and residents.'); + expect(wikibase.claimToValue(tagData(), 'P4', 'en')).to.eql('Primary image.jpg'); + expect(wikibase.claimToValue(keyData(), 'P6', 'en')).to.eql('Q15'); + expect(wikibase.claimToValue(keyData(), 'P6', 'fr')).to.eql('Q15'); + expect(wikibase.claimToValue(keyData(), 'P6', 'de')).to.eql('Q14'); }); });