Added title normalization and tests for wiktionary #1078
Conversation
Modified title in lib/normalize_title_filter.js to check for term in wiktionary before defaulting to title Added tests to check case sensitivty and namespaces in wiktionary Added wiktionary url to test/utils/server.js Modified v1/definition.yaml to register the filter
lib/normalize_title_filter.js
Outdated
| @@ -29,6 +29,7 @@ module.exports = (hyper, req, next, options, specInfo) => { | |||
| } | |||
| }); | |||
| } | |||
| rp.title = rp[options.title] ? rp[options.title] : rp.title; | |||
There was a problem hiding this comment.
- rp[options.title] || rp.title is nicer
- if options.title is undefined we'd be in trouble. so we need to go
(options.title && rp[options.title]) || rp.title the brackets are not really needed, but it's easier to read without thinking.
| @@ -9,6 +9,57 @@ describe('redirects', () => { | |||
| before(() => { return server.start(); }); | |||
|
|
|||
| describe('', () => { | |||
| it('should direct to a lowercase version of a title in wiktionary', () => { | |||
| }); | ||
| }); | ||
|
|
||
| it('should direct to an uppercase version of a title in wiktionary', () => { |
Updated lib/normalize_title_filter.js to better follow format of lib/mwUtil.js
Modified lib/mwUtil.js to use {term} if present
Removed previous wiktionary test and replaced with one test that checks filter
lib/mwUtil.js
Outdated
| @@ -341,7 +341,8 @@ mwUtil.createRelativeTitleRedirect = (path, req, options) => { | |||
| const backString = Array.apply(null, { length: pathSuffixCount }).map(() => '../').join(''); | |||
| let location; | |||
| if (!options.dropPathAfterTitle) { | |||
| const pathPatternAfterTitle = path.substring(path.indexOf('{title}') - 1); | |||
| let pathPatternAfterTitle = path.substring(path.indexOf(`{${titleParamName}`) - 1); | |||
| pathPatternAfterTitle = pathPatternAfterTitle.replace(/term/, 'title'); | |||
There was a problem hiding this comment.
No way! This is super specific to the case that we have here right now. What if we're gonna have a different endpoint in the future where we decide to call the page title 'ugabugafoobarxxyyblombalabam' - we would need to replace that with 'title' as well.
This is a utility function, and it does take titleParamName as an option, so it has to work correctly whatever the titleParamName was provided to it right? Now it will only work if it's 'title' or a 'term'
lib/normalize_title_filter.js
Outdated
| @@ -29,6 +29,7 @@ module.exports = (hyper, req, next, options, specInfo) => { | |||
| } | |||
| }); | |||
| } | |||
| rp.title = (options.title && rp[options.title]) || rp.title; | |||
There was a problem hiding this comment.
Our overall goal here is to get rid of the hard-coded property name title right? So why are we assigning the request parameter title? We know that that the page name that we need to normalize will be located inside the options.title property right?
Also, having the options property named title is a bit confusing isn't it? now we have so many titles in the code that it's hard to understand which one refers to what. Would it be more understandable if the options property be called 'titleParamName' here as well as in the utility function?
In v1/definition.yaml updated title to title_param_name to better track code In lib/mwUtil.js and lib/normalize_title_filter.js: changed hard coded title to be more dynamic towards the passed in variable
| it('should redirect to a normalized version of a title', () => { | ||
| return preq.get({ | ||
| uri: `${server.config.bucketURL}/html/Main%20Page?test=mwAQ`, | ||
| followRedirect: false | ||
| }) | ||
| .then((res) => { | ||
| console.log(res); |
|
That's what we're talking about! but you forgot a console.log in the test. Otherwise LGTM |
|
Ok, let's ait for travis |
Modified title in lib/normalize_title_filter.js to check for term in wiktionary before defaulting to title
Added tests to check case sensitivty and namespaces in wiktionary
Added wiktionary url to test/utils/server.js
Modified v1/definition.yaml to register the filter
Bug: T207904