Mobrovac has uploaded a new change for review.
https://gerrit.wikimedia.org/r/309004
Change subject: Feed endpoints: Various bug fixes
......................................................................
Feed endpoints: Various bug fixes
- When an individual endpoint is called with aggregated=true, ensure
that (a) the ETag is not set; and (b) the response contains at least
an empty object, so as not to break client connections
- In lib/feed/most-read.js, module.exports.promise should not have the
aggregated param, but should be deduced from the request query
- Add a test for the tfa endpoint with aggregated=true
Bug: T143912
Change-Id: I99d28e8c21548143c6d3a3d886d5028635ba6bab
---
M lib/feed/featured-image.js
M lib/feed/featured.js
M lib/feed/most-read.js
M lib/feed/news.js
M lib/mobile-util.js
M routes/featured-image.js
M routes/featured.js
M routes/most-read.js
M routes/news.js
M test/features/featured/pagecontent.js
10 files changed, 33 insertions(+), 17 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps
refs/changes/04/309004/1
diff --git a/lib/feed/featured-image.js b/lib/feed/featured-image.js
index 93e9514..cfd0c1b 100644
--- a/lib/feed/featured-image.js
+++ b/lib/feed/featured-image.js
@@ -4,6 +4,7 @@
'use strict';
+var BBPromise = require('bluebird');
var preq = require('preq');
var api = require('../api-util');
var dateUtil = require('../dateUtil');
@@ -118,7 +119,7 @@
}).catch(function(err) {
if (err.status === 504) {
if (aggregated) {
- return { payload: undefined, meta: undefined };
+ return BBPromise.resolve({ payload: undefined, meta: undefined
});
}
throw new HTTPError({
status: 404,
diff --git a/lib/feed/featured.js b/lib/feed/featured.js
index 46e3772..3ea0538 100644
--- a/lib/feed/featured.js
+++ b/lib/feed/featured.js
@@ -76,7 +76,7 @@
var aggregated = !!req.query.aggregated;
if (req.params.domain.indexOf('en') !== 0) {
if (aggregated) {
- return {};
+ return BBPromise.resolve({});
} else {
throw new HTTPError({
status: 501,
diff --git a/lib/feed/most-read.js b/lib/feed/most-read.js
index a996ba8..8e95f14 100644
--- a/lib/feed/most-read.js
+++ b/lib/feed/most-read.js
@@ -4,6 +4,7 @@
'use strict';
+var BBPromise = require('bluebird');
var mUtil = require('../mobile-util');
var dateUtil = require('../dateUtil');
var api = require('../api-util');
@@ -22,8 +23,7 @@
return result;
}
-function getTopPageviews(app, req) {
- var aggregated = !!req.query.aggregated;
+function getTopPageviews(app, req, aggregated) {
var yesterday = new Date(dateUtil.getRequestedDate(req) -
dateUtil.ONE_DAY);
var year = aggregated ? yesterday.getUTCFullYear() : req.params.yyyy;
var month = aggregated ? dateUtil.pad(yesterday.getUTCMonth() + 1) :
req.params.mm;
@@ -46,7 +46,8 @@
});
}
-function promise(app, req, aggregated) {
+function promise(app, req) {
+ var aggregated = !!req.query.aggregated;
var goodTitles, resultsDate;
dateUtil.validate(dateUtil.hyphenDelimitedDateString(req));
return getTopPageviews(app, req, aggregated).then(function (response) {
@@ -109,7 +110,7 @@
// Catch and handle the error if this is an aggregated request and the
// pageview data are not yet loaded.
if (aggregated && err.status === 404) {
- return {};
+ return BBPromise.resolve({});
}
throw err;
});
diff --git a/lib/feed/news.js b/lib/feed/news.js
index 081c268..27449a6 100644
--- a/lib/feed/news.js
+++ b/lib/feed/news.js
@@ -1,5 +1,6 @@
'use strict';
+var BBPromise = require('bluebird');
var domino = require('domino');
var api = require('../api-util');
var mUtil = require('../mobile-util');
@@ -40,7 +41,7 @@
var aggregated = !!req.query.aggregated;
if (!newsTemplates[lang]) {
if (aggregated) {
- return { payload: undefined, meta: undefined };
+ return BBPromise.resolve({ payload: undefined, meta: undefined });
}
throw new HTTPError({
status: 501,
diff --git a/lib/mobile-util.js b/lib/mobile-util.js
index 74b1cbe..d32108e 100644
--- a/lib/mobile-util.js
+++ b/lib/mobile-util.js
@@ -69,7 +69,9 @@
* @param {Object} value to set the ETag to
*/
mUtil.setETagToValue = function(response, value) {
- response.set('etag', '' + value);
+ if (value) {
+ response.set('etag', '' + value);
+ }
};
/**
diff --git a/routes/featured-image.js b/routes/featured-image.js
index 60b1263..934402c 100644
--- a/routes/featured-image.js
+++ b/routes/featured-image.js
@@ -27,8 +27,8 @@
return featured.promise(app, req)
.then(function (response) {
res.status(200);
- mUtil.setETagToValue(res, response.meta.etag);
- res.json(response.payload).end();
+ mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ res.json(response.payload || {}).end();
});
});
diff --git a/routes/featured.js b/routes/featured.js
index 752410b..d2fc2c9 100644
--- a/routes/featured.js
+++ b/routes/featured.js
@@ -27,9 +27,9 @@
return featured.promise(app, req)
.then(function (response) {
res.status(200);
- mUtil.setETagToValue(res, response.meta.etag);
+ mUtil.setETagToValue(res, response.meta && response.meta.etag);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
- res.json(response.payload).end();
+ res.json(response.payload || {}).end();
});
});
diff --git a/routes/most-read.js b/routes/most-read.js
index 24f59b7..2f54193 100644
--- a/routes/most-read.js
+++ b/routes/most-read.js
@@ -25,9 +25,9 @@
return mostRead.promise(app, req)
.then(function (response) {
res.status(200);
- mUtil.setETagToValue(res, response.meta.etag);
+ mUtil.setETagToValue(res, response.meta && response.meta.etag);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
- res.json(response.payload).end();
+ res.json(response.payload || {}).end();
});
});
diff --git a/routes/news.js b/routes/news.js
index 72dd15a..1b22967 100644
--- a/routes/news.js
+++ b/routes/news.js
@@ -24,8 +24,8 @@
return news.promise(app, req)
.then(function (response) {
res.status(200);
- mUtil.setETagToValue(res, response.meta.etag);
- res.json(response.payload).end();
+ mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ res.json(response.payload || {}).end();
});
});
@@ -36,4 +36,4 @@
api_version: 1,
router: router
};
-};
\ No newline at end of file
+};
diff --git a/test/features/featured/pagecontent.js
b/test/features/featured/pagecontent.js
index 8634766..5400a40 100644
--- a/test/features/featured/pagecontent.js
+++ b/test/features/featured/pagecontent.js
@@ -68,6 +68,17 @@
});
});
+ it('unsupported language with aggregated=true should return 200',
function() {
+ return preq.get({
+ uri: server.config.uri +
'zh.wikipedia.org/v1/page/featured/2016/04/15',
+ query: { aggregated: true }
+ })
+ .then(function(res) {
+ assert.status(res, 200);
+ assert.deepEqual(res.body, {}, 'Expected the body to be empty');
+ });
+ });
+
it('featured article of an old date should return 404', function() {
return preq.get({ uri: server.config.uri +
'en.wikipedia.org/v1/page/featured/1970/12/31' })
.then(function(res) {
--
To view, visit https://gerrit.wikimedia.org/r/309004
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99d28e8c21548143c6d3a3d886d5028635ba6bab
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mobrovac <mobro...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
