Mobrovac has uploaded a new change for review.
https://gerrit.wikimedia.org/r/230757
Change subject: Minor tweaks and fixes here and there before first deployment
......................................................................
Minor tweaks and fixes here and there before first deployment
It is expected the first deploy to happen in the next few days. To that
end I have performed a review of the current code-base. This patch
addresses some minor issues I found along the way, namely:
- set DEBUG as a application-level configuration directive (inspite of
the fact that logger.log('debug', ...) should be used to that end, and
not console.log()).
- lib/mwapi.js: check the MW API response directly after issuing the
request
- lib/gallery.js: galleryItemsPromise() is expected to return an Object,
not an Array
- routes/mobile-text.js: use mwapi.apiGet() instead of its own duplicate
method
- routes/mobile-html.js: need to return the request-handling promise
from the route handler to allow automatic error handling
- routes/mobile-html-sections.js: use res.send().end() to allow the
compression middleware to compress the contents if clients accept it
Change-Id: I4638fd6423db12bf0b345137118f20267ffc8178
Note: these changes need to land before the first deploy.
---
M config.dev.yaml
M config.prod.yaml
M lib/gallery.js
M lib/mwapi.js
M routes/mobile-html-sections.js
M routes/mobile-html.js
M routes/mobile-text.js
M spec.yaml
8 files changed, 49 insertions(+), 82 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps
refs/changes/57/230757/1
diff --git a/config.dev.yaml b/config.dev.yaml
index 80b5d04..ef07a32 100644
--- a/config.dev.yaml
+++ b/config.dev.yaml
@@ -48,3 +48,5 @@
# - domain1.com
# - domain2.org
restbase_uri: https://restbase.wikimedia.org
+ # whether to print extra debug info
+ debug: true
diff --git a/config.prod.yaml b/config.prod.yaml
index beed5e6..81dd8d3 100644
--- a/config.prod.yaml
+++ b/config.prod.yaml
@@ -34,3 +34,5 @@
# interface: localhost # uncomment to only listen on localhost
# more per-service config settings
restbase_uri: https://restbase.wikimedia.org
+ # whether to print extra debug info
+ debug: false
diff --git a/lib/gallery.js b/lib/gallery.js
index 1c101f2..d0b4c45 100644
--- a/lib/gallery.js
+++ b/lib/gallery.js
@@ -74,7 +74,6 @@
var items, key;
var output = [];
- mwapi.checkApiResponse(response);
mwapi.checkForQueryPagesInResponse(logger, response);
items = response.body.query.pages;
@@ -103,12 +102,11 @@
}
function onGalleryCollectionsResponse(logger, response, domain) {
- var detailsPromises = [], videos = [], images = [];
+ var detailsPromises = {}, videos = [], images = [];
var isVideo;
- mwapi.checkApiResponse(response);
if (!response.body.query || !response.body.query.pages) {
- return [];
+ return {};
}
// iterate over all items
@@ -197,4 +195,4 @@
module.exports = {
collectionPromise: collectionPromise
-};
\ No newline at end of file
+};
diff --git a/lib/mwapi.js b/lib/mwapi.js
index cf36dea..a07c7a7 100644
--- a/lib/mwapi.js
+++ b/lib/mwapi.js
@@ -25,6 +25,18 @@
return preq.get({
uri: 'http://' + domain + '/w/api.php',
query: params
+ }).then(function(response) {
+ // check if the query failed
+ if (response.status > 299) {
+ // there was an error in the MW API, propagate that
+ throw new HTTPError({
+ status: response.status,
+ type: 'api_error',
+ title: 'MW API error',
+ detail: response.body
+ });
+ }
+ return response;
});
}
@@ -61,23 +73,10 @@
};
}
-function checkApiResponse(response) {
- // check if the query failed
- if (response.status > 299) {
- // there was an error in the MW API, propagate that
- throw new HTTPError({
- status: response.status,
- type: 'api_error',
- title: 'MW API error',
- detail: response.body
- });
- }
-}
-
function checkForQueryPagesInResponse(logger, response) {
- if (!response.body.query || !response.body.query.pages) {
+ if (!(response && response.body && response.body.query &&
response.body.query.pages)) {
// we did not get our expected query.pages from the MW API, propagate
that
- logger.log('error', 'no query.pages in response: ' +
JSON.stringify(response, null, 2));
+ logger.log('error/mwapi', 'no query.pages in response: ');
throw new HTTPError({
status: response.status,
type: 'api_error',
@@ -91,6 +90,5 @@
apiGet: apiGet,
getAllSections: getAllSections,
buildLeadImageUrls: buildLeadImageUrls,
- checkApiResponse: checkApiResponse,
checkForQueryPagesInResponse: checkForQueryPagesInResponse
-};
\ No newline at end of file
+};
diff --git a/routes/mobile-html-sections.js b/routes/mobile-html-sections.js
index d3ddc69..26d56d7 100644
--- a/routes/mobile-html-sections.js
+++ b/routes/mobile-html-sections.js
@@ -33,16 +33,16 @@
var app;
function dbg(name, obj) {
- //console.log("DEBUG: " + name + ": " + JSON.stringify(obj, null, 2));
- app.logger.log('debug', name + ": " + JSON.stringify(obj));
+ if (app.conf.debug) {
+ //console.log("DEBUG: " + name + ": " + JSON.stringify(obj, null, 2));
+ app.logger.log('debug', name + ": " + JSON.stringify(obj));
+ }
}
/** Returns a promise to retrieve the page content from MW API mobileview */
function pageContentPromise(domain, title) {
return mwapi.getAllSections(domain, title)
.then(function (response) {
- mwapi.checkApiResponse(response);
-
// transform all sections
var sections = response.body.mobileview.sections;
for (var idx = 0; idx < sections.length; idx++) {
@@ -111,7 +111,7 @@
page: pageContentPromise(req.params.domain, req.params.title),
media: gallery.collectionPromise(req.logger, req.params.domain,
req.params.title)
}).then(function (response) {
-
res.status(200).type('json').end(JSON.stringify(buildOutput(response)));
+ res.status(200).type('json').send(buildOutput(response)).end();
});
});
@@ -123,7 +123,7 @@
return BBPromise.props({
page: pageContentPromise(req.params.domain, req.params.title)
}).then(function (response) {
-
res.status(200).type('json').end(JSON.stringify(buildOutput(response).lead));
+ res.status(200).type('json').send(buildOutput(response).lead).end();
});
});
@@ -136,7 +136,7 @@
page: pageContentPromise(req.params.domain, req.params.title),
media: gallery.collectionPromise(req.logger, req.params.domain,
req.params.title)
}).then(function (response) {
-
res.status(200).type('json').end(JSON.stringify(buildOutput(response).remaining));
+
res.status(200).type('json').send(buildOutput(response).remaining).end();
});
});
diff --git a/routes/mobile-html.js b/routes/mobile-html.js
index 24f5d75..038cf39 100644
--- a/routes/mobile-html.js
+++ b/routes/mobile-html.js
@@ -35,10 +35,8 @@
*/
var app;
-var DEBUG = true;
-
function dbg(name, obj) {
- if (DEBUG) {
+ if (app.conf.debug) {
console.log("DEBUG: " + name + ": " + util.inspect(obj));
//console.log("DEBUG: " + name + ": " + JSON.stringify(obj, null, 2));
//app.logger.log('debug', name + ": " + JSON.stringify(obj));
@@ -235,8 +233,6 @@
function pageContentPromise(domain, title) {
return mwapi.getAllSections(domain, title)
.then(function (response) {
- mwapi.checkApiResponse(response);
-
//// transform all sections
//var sections = response.body.mobileview.sections;
//for (var idx = 0; idx < sections.length; idx++) {
@@ -264,7 +260,7 @@
*/
router.get('/mobile-html/:title', function (req, res) {
//dbg("req.params", req.params);
- BBPromise.props({
+ return BBPromise.props({
html: preq.get({
uri: app.conf.restbase_uri + '/' +
req.params.domain.replace(/^(\w+\.)m\./, '$1')
+ '/v1/page/html/' + encodeURIComponent(req.params.title),
diff --git a/routes/mobile-text.js b/routes/mobile-text.js
index 4e0e204..7f29eb6 100644
--- a/routes/mobile-text.js
+++ b/routes/mobile-text.js
@@ -16,6 +16,7 @@
var preq = require('preq');
var domino = require('domino');
var sUtil = require('../lib/util');
+var mwapi = require('../lib/mwapi');
// shortcut
var HTTPError = sUtil.HTTPError;
@@ -31,22 +32,6 @@
*/
var app;
-
-/**
- * A helper function that obtains the HTML from the MW API and
- * loads it into a domino DOM document instance.
- *
- * @param {String} domain the domain to contact
- * @param {Object} params an Object with all the query parameters for the MW
API
- * @return {Promise} a promise resolving as the HTML element object
- */
-function apiGet(domain, params) {
- // get the page from the MW API
- return preq.get({
- uri: 'http://' + domain + '/w/api.php',
- query: params
- });
-}
function rmSelectorAll(doc, selector) {
var ps = doc.querySelectorAll(selector) || [];
@@ -140,9 +125,6 @@
itemIndex++;
}
-
-
-
delete section.text;
}
@@ -163,33 +145,22 @@
"noheadings": true
};
- return apiGet(req.params.domain, apiParams)
- // and then return it
- .then(function (apiRes) {
- // check if the query failed
- if (apiRes.status > 299) {
- // there was an error in the MW API, propagate that
- throw new HTTPError({
- status: apiRes.status,
- type: 'api_error',
- title: 'MW API error',
- detail: apiRes.body
- });
- }
+ return mwapi.apiGet(req.params.domain, apiParams)
+ // and then return it
+ .then(function (apiRes) {
+ // transform all sections
+ var sections = apiRes.body.mobileview.sections;
+ for (var idx = 0; idx < sections.length; idx++) {
+ var section = sections[idx];
+ // run DOM transforms on the section...
+ runDomTransforms(section);
+ }
- // transform all sections
- var sections = apiRes.body.mobileview.sections;
- for (var idx = 0; idx < sections.length; idx++) {
- var section = sections[idx];
- // run DOM transforms on the section...
- runDomTransforms(section);
- }
-
-
res.status(200).type('json').end(JSON.stringify(apiRes.body.mobileview));
-
//res.status(200).type('json').end(util.inspect(apiRes.body.mobileview));
- //res.status(200).type('html').end(apiRes.innerHTML);
- //res.status(200).type('json').end(apiRes);
- });
+
res.status(200).type('json').end(JSON.stringify(apiRes.body.mobileview));
+
//res.status(200).type('json').end(util.inspect(apiRes.body.mobileview));
+ //res.status(200).type('html').end(apiRes.innerHTML);
+ //res.status(200).type('json').end(apiRes);
+ });
});
module.exports = function (appObj) {
diff --git a/spec.yaml b/spec.yaml
index 4ad5ef6..a8a9322 100644
--- a/spec.yaml
+++ b/spec.yaml
@@ -64,7 +64,7 @@
produces:
- text/html
x-amples:
- - title: retrieve en.wp main page
+ - title: retrieve en.wp main page via mobile-html
request:
params:
title: Main_Page
@@ -82,7 +82,7 @@
produces:
- application/json
x-amples:
- - title: retrieve en.wp main page
+ - title: retrieve en.wp main page via mobile-html-sections
request:
params:
title: Main_Page
--
To view, visit https://gerrit.wikimedia.org/r/230757
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4638fd6423db12bf0b345137118f20267ffc8178
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mobrovac <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
