Feature/wayback machine #286

adaveinthelife · 2018-06-04T17:48:08Z

This is my first iteration of a new feature for the Google Action: the Wayback Machine. The Wayback Engine function makes requests to metadata on archive.org and alexa rankings to get:

the earliest year that archiving began
the most recent year of archiving
the total number of unique URLs (uses initial URL count and then iterates through each new url count per year)
the Alexa World Rank
the Alexa US Rank

Still lots of work to be done here. I've attempted to write a basic mechanism for slot filling but it probably needs more helper methods. Hopefully Eugene can help me with that!

…archive-google-action into feature/Wayback-Machine

…g parsing

hyzhak · 2018-06-20T11:37:44Z

functions/src/actions/wayback-machine.js

@@ -0,0 +1,134 @@
+const request = require('request');


it is not critical, but I'd recommend to reorganize imports:

put apart third part imports and local imports

put improts in the alphabet order by name of lib:

const mustache = require('mustache'); const traverse = require('traverse'); const xml2js = require('xml2js');

You could check such organization in any of my js-files.

Such organization would help to read imports.

btw, we don't have request libs here, we already use axios (https://github.com/axios/axios). The reason why we use axios (in the single place - interceptors):

use single user-agent for all request

track request performance

https://github.com/internetarchive/internet-archive-google-action/blob/master/functions/src/setup.js#L31

Actually we could use another lib, but should have real motivation to switch to it.

hyzhak · 2018-06-20T11:39:31Z

functions/src/actions/wayback-machine.js

+function handler (app) {
+  // Check parameter for Wayback qualifier
+  let wayback = app.params.getByName('wayback');
+  if (wayback.includes('wayback') === false) {


it doesn't look javascript style way. I'd recommend to reformat it to:

if (!wayback.includes('wayback')) {

hyzhak · 2018-06-20T11:40:04Z

functions/src/actions/wayback-machine.js

+  }
+
+  // Get url parameter
+  var url = app.params.getByName('url');


I'd recommend to replace var with const and let. It would be safer.

hyzhak · 2018-06-20T11:42:27Z

functions/src/actions/wayback-machine.js

+  var url = app.params.getByName('url');
+
+  // Create wayback object
+  var waybackObject = {


why don't set waybackObject just in place? I'd recommend to rewrite it in:

waybackObject = { ulr, earliestYear: 0,

so you would need later assignment

waybackObject.url = url;

hyzhak · 2018-06-20T11:44:45Z

functions/src/actions/wayback-machine.js

+  };
+
+  waybackObject.url = url;
+  var archiveQueryURL = 'http://web.archive.org/__wb/search/metadata?q=' + url;


we store all string constants in src/functions/config.js so it would be much simpler to rewrite all endpoints in once if we decide to use a proxy, new API or something else.

hyzhak · 2018-06-20T11:52:48Z

functions/src/actions/wayback-machine.js

@@ -0,0 +1,134 @@
+const request = require('request');


btw, we don't have request libs here, we already use axios (https://github.com/axios/axios). The reason why we use axios (in the single place - interceptors):

use single user-agent for all request

track request performance

https://github.com/internetarchive/internet-archive-google-action/blob/master/functions/src/setup.js#L31

Actually we could use another lib, but should have real motivation to switch to it.

hyzhak · 2018-06-20T11:55:24Z

functions/src/actions/wayback-machine.js

+      var XMLparser = new xml2js.Parser();
+      XMLparser.parseString(allData[1], function (err, result) {
+        if (err) {
+          debug('Uh-oh, the XML parser didn\'t work');


we should gracefully continue dialog here, for example, propose to retry or ask something else.

hyzhak · 2018-06-20T11:57:12Z

functions/src/actions/wayback-machine.js

@@ -0,0 +1,134 @@
+const request = require('request');
+const traverse = require('traverse');


I think we should lodash (https://lodash.com/) instead, it is much more powerful and we already have it in this app

hyzhak · 2018-06-20T11:58:19Z

functions/src/actions/wayback-machine.js

@@ -0,0 +1,134 @@
+const request = require('request');
+const traverse = require('traverse');
+const xml2js = require('xml2js');


all new imports should be installed by:

npm install --save xml2js

when you would need new lib for testing you should use instead:

npm install --save-dev xml2js

hyzhak · 2018-06-20T12:02:19Z

functions/src/actions/wayback-machine.js

+      // Construct response dialog for action
+      if (waybackObject.alexaUSRank !== 0) {
+        waybackObject.speech = mustache.render(waybackStrings.speech, waybackObject);
+        waybackObject.speech += ' and <say-as interpret-as="ordinal">' + waybackObject.alexaUSRank + '</say-as> in the United States';


I'd recommend to use template literals instead https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

hyzhak · 2018-06-20T12:07:42Z

functions/src/actions/wayback-machine.js

+  });
+}
+
+function archiveEngine (archiveJSON, waybackObject) {


Just general comments. I'd recommend writing pure functions everywhere it possible.

Motivation:

they are much easy to test

easy to reuse

hyzhak · 2018-06-26T14:17:37Z

functions/src/actions/wayback-machine.js

+        if (err) {
+          debug('The XML parser didn\'t work');
+          waybackObject.speech = waybackStrings.error;
+          dialog.ask(app, waybackObject);


if we would like to change dialog flow here we should embed XMLparser.parseString in Promises pipeline. For example by wrapping it to:

new Promise((resolve, reject) => { XMLparser.parseString(allData[1].data, function (err, result) { //... })

Because right now we will reach dialog.close(app, waybackObject); whether result you would get in XMLparser.parseString.

Actually, I'd recommend to create unit tests for that part of the code - it would help a lot. All async scenarios are very tricky.

hyzhak · 2018-06-26T20:22:44Z

functions/src/actions/wayback-machine.js

+const _ = require('lodash');
+const axios = require('axios');
+const mustache = require('mustache');
+const xml2js = require('xml2js');


you should install xml2js with:

npm install --save xml2js

to add it to package.json

hyzhak · 2018-06-26T20:24:56Z

functions/src/actions/wayback-machine.js

+  waybackObject.latestYear = yearsArray[yearsArray.length - 1];
+
+  // Traverse URL category
+  const traverse = obj => {


pls put this function to ./utils directory. Btw it is a good candidate for unit tests :)

…for it

hyzhak · 2018-07-06T12:27:56Z

functions/package.json

@@ -37,7 +37,8 @@
    "object.entries": "^1.0.4",
    "raven": "^2.6.0",
    "replaceall": "^0.1.6",
-    "supports-color": "^5.4.0"
+    "supports-color": "^5.4.0",


hm, how do you use this lib here?

Hmm I'm not sure how that ended up here! I believe that's an atom module I use for formatting in the editor. That can come out.

hyzhak · 2018-07-06T12:30:18Z

functions/src/actions/wayback-machine.js

+            waybackObject.speech = waybackStrings.error;
+            dialog.ask(app, waybackObject);
+          } else {
+            alexaJSON = JSON.parse(JSON.stringify(result));


it seems that you forgot to run resolve here, and btw, you don't need alexaJSON here, because you can just pass a result to resolve.

hyzhak · 2018-07-06T12:30:38Z

functions/src/actions/wayback-machine.js

+          if (err) {
+            debug('The XML parser didn\'t work');
+            waybackObject.speech = waybackStrings.error;
+            dialog.ask(app, waybackObject);


is it a good place for reject?

hyzhak · 2018-07-06T12:31:26Z

functions/src/actions/wayback-machine.js

+        .catch(function (error) {
+          debug(error.message);
+        });
+      alexaEngine(alexaJSON, waybackObject);


it seems alexaEngine should be inside of Promise then pipeline

hyzhak · 2018-07-06T12:37:36Z

functions/src/utils/traverse.js

@@ -0,0 +1,31 @@
+const _ = require('lodash');
+const {debug} = require('../utils/logger')('ia:actions:wayback-machine');


'ia:actions:wayback-machine' should be 'ia:actions:utils:traverse'

hyzhak · 2018-07-06T12:38:59Z

functions/tests/actions/wayback-machine.spec.js

+      dialog = mockDialog();
+      action.__set__('dialog', dialog);
+    });
+    it('check to see that a promise is returned with network requests', () => {


please give some space here ;)

hyzhak · 2018-07-06T17:24:28Z

functions/src/actions/wayback-machine.js

+            resolve(result);
+          }
+        });
+      });


I don't think you need to store convertXML promise. You are already inside of Promise.then pipeline so you can return new Promise. And btw you need to pass archiveJSON down the promises chain. So you could use something like:

return Promise.all([archiveJSON, new Promise((resolve, reject) => { //... }])

and btw it would need it because part of code from // Construct response dialog for action will be called before: alexaEngine(JSON.parse(JSON.stringify(fulfilled)), waybackObject);

Btw I'd recommend to write unit test of this pipeline, so you would see how promise will fire one after another.

In additional I'd recommend to put:

XMLparser.parseString(allData[1].data, function (err, result) {

in separate function so it would improve readability of this pipeline.

…ipeline

hyzhak · 2018-07-13T14:09:24Z

functions/tests/actions/wayback-machine.spec.js

+      expect(Promise.resolve()).to.be.a('promise');
+    });
+
+    it('check to see that axios request promises are working', () => {


we shouldn't test axios library.

hyzhak · 2018-07-13T14:11:44Z

functions/tests/actions/wayback-machine.spec.js

+  describe('wayback machine', () => {
+    let app;
+    let dialog;
+    let archiveRequest = axios.get('http://web.archive.org/__wb/search/metadata?q=cnn.com');


We can't request data for a test from an ourside. If you would need any data you should create fixture data.

Examples: https://github.com/internetarchive/internet-archive-google-action/tree/master/functions/tests/fixtures

hyzhak · 2018-07-13T14:23:52Z

functions/tests/actions/wayback-machine.spec.js

+      let obj = { earliestYear: 0, latestYear: 0, totalUniqueURLs: 0 };
+      let result;
+
+      action.__set__('archiveEngine', function (a, b) {


it seems this test has never come inside of this mock. You never call handler in this test on others. Actually, it is not clear what: check to see that archiveEngine is working means here, could you be more specific?

hyzhak · 2018-07-13T14:31:58Z

functions/src/actions/wayback-machine.js

+    config.wayback.ALEXA, app, waybackObject
+  );
+
+  return Promise.all([axios.get(archiveQueryURL), axios.get(alexaQueryURL)])


Please don't use callback nesting here. One of the main feature of Promises is avoiding callback hell. It should be something like:

action.then(a => { return Promise.all(/*....*/); }) .then(b => { return Promise.all(/*....*/); }) .then(c => { });

hyzhak · 2018-07-13T15:30:32Z

functions/src/actions/wayback-machine.js

+    // debug('Inside archiveEngine promise...');
+    // Create array of capture years and then find earliest year
+    //  and most recent year.
+    let yearsArray = Object.keys(archiveJSON.captures);


btw, it is better to sort years, to be sure that they are in the order

hyzhak

much better but still need to make few improvements

hyzhak · 2018-07-14T11:18:56Z

functions/src/actions/wayback-machine.js

+      return Promise.all([archiveEngine(requestData[0].data, waybackObject), xmlConverter(requestData[1].data)]);
+    })
+    .then(response => {
+      return Promise.all([alexaEngine(response[1], waybackObject)]);


you don't need to wrap alexaEngine( call to Promise.all here. Because you have single async function here. and btw you don't really need alexaEngine to be async (with Promise) because it has sync logic.

hyzhak · 2018-07-14T11:21:19Z

functions/src/actions/wayback-machine.js

+      debug('Country not found');
+      debug(e);
+    }
+    if (waybackObject.alexaWorldRank > 0) {


Why do you need to check that alexaWorldRank > 0 here, and what should be in case when alexaWorldRank <= 0?

I'd recommend that function alexaEngine would be in the pure-function way: https://en.wikipedia.org/wiki/Pure_function

You don't need to mutate waybackObject here, just return rands:

function getAlexaRanks (alexaJSON) { //.... some in logic to extract ranks from alexa json return { alexaWorldRank: 123456, alexsUSRank: 1345, }; }

and then you can merge the result inside of handler:

res = {} // it merges res to empty object and then merge getAlexaRanks res = Object.assign({}, res, getAlexaRanks (alexaJSON));

hyzhak · 2018-07-14T11:25:23Z

functions/src/actions/wayback-machine.js

+    });
+} // End of handler
+
+function archiveEngine (archiveJSON, waybackObject) {


this function also should be pure-function and it doesn't need to be async

hyzhak · 2018-07-14T11:26:21Z

functions/src/actions/wayback-machine.js

+    .then(response => {
+      return Promise.all([alexaEngine(response[1], waybackObject)]);
+    })
+    .then(a => {


you don't need argument in arrow function if you don't use it:

.then(() => { //... logic })

hyzhak

Thanks for pure functions! We have significant progress here 👍 But still need to make some improvements

hyzhak · 2018-07-14T21:30:13Z

functions/src/actions/wayback-machine.js

+    });
+} // End of handler
+
+function archiveEngine (archiveJSON) {


the inner logic of archiveEngine and alexaEngine are not asynchronous, so you don't need Promises here and there.

hyzhak · 2018-07-14T21:30:48Z

functions/src/actions/wayback-machine.js

+function archiveEngine (archiveJSON) {
+  return new Promise(function (resolve, reject) {
+    debug('Inside archiveEngine promise...');
+    let obj = { earliestYear: 0, latestYear: 0, totalUniqueURLs: 0 };


it could be const and res (result) is a more canonic variable name here

hyzhak · 2018-07-14T21:32:13Z

functions/src/actions/wayback-machine.js

+      return Promise.all([archiveEngine(requestData[0].data), xmlConverter(requestData[1].data)]);
+    })
+    .then(response => {
+      let res = {};


why don't merge response right to waybackObject?

hyzhak · 2018-07-14T21:33:55Z

functions/src/actions/wayback-machine.js

+
+  return Promise.all([axios.get(archiveQueryURL), axios.get(alexaQueryURL)])
+    .then(function (requestData) {
+      return Promise.all([archiveEngine(requestData[0].data), xmlConverter(requestData[1].data)]);


Because archiveEngine and alexaEngine are actually sync functions you don't need to wait for .then before process them.

Confused about this one... Don't I have to wait on Axios requests to complete before feeding them to archiveEngine and alexaEngine? Even if those functions are sync functions, how can they operate before that data has arrived?

In other words, you don't do any async actions inside of archiveEngine or alexaEngine function, in particular, there are no any Axios requests inside of them. So you don't need to wrap them to Promise and return a result, you just can get a result right away. It should simplify all pipeline a lot ;)

Sure you need to get responses from axios.get above.

hyzhak · 2018-07-14T21:36:11Z

functions/src/actions/wayback-machine.js

+        waybackObject.speech += '.';
+      }
+      dialog.close(app, waybackObject);
+    });


You may want gracefully cover .catch case here because you throw reject/error in the pipeline, and request to a server could get an exception

hyzhak · 2018-07-16T11:34:30Z

functions/src/actions/wayback-machine.js

+
+  return Promise.all([axios.get(archiveQueryURL), axios.get(alexaQueryURL)])
+    .then(function (requestData) {
+      return Promise.all([archiveEngine(requestData[0].data), xmlConverter(requestData[1].data)]);


In other words, you don't do any async actions inside of archiveEngine or alexaEngine function, in particular, there are no any Axios requests inside of them. So you don't need to wrap them to Promise and return a result, you just can get a result right away. It should simplify all pipeline a lot ;)

Sure you need to get responses from axios.get above.

hyzhak · 2018-07-16T11:51:08Z

functions/src/actions/wayback-machine.js

+  );
+
+  return Promise.all([axios.get(archiveQueryURL), axios.get(alexaQueryURL)])
+    .then(function (requestData) {


btw you can make this part of code more readable by using arguments unpacking:

.then(([archiveRes, alexaRes]) => { return Promise.all([archiveEngine(archiveRes.data), xmlConverter(alexaRes.data)]) })

…learer

hyzhak · 2018-07-16T14:16:27Z

functions/src/actions/wayback-machine.js

+    return res;
+  } else {
+    let error = new Error('alexaEngine didn\'t work.');
+    return error;


it seems we should throw an error here. return passes it as a successful response.

hyzhak · 2018-07-16T14:17:55Z

functions/src/actions/wayback-machine.js

+    })
+    .then(([archiveEngineRes, xmlRes]) => {
+      waybackObject = Object.assign(waybackObject, archiveEngineRes);
+      return alexaEngine(xmlRes);


alexaEngine is sync function it doesn't return Promise. So you can just merge its response right to waybackObject object without waiting then.

hyzhak · 2018-07-16T14:21:02Z

functions/src/actions/wayback-machine.js

+    })
+    .catch(err => {
+      debug('Wayback handler has an error: ', err);
+      waybackObject.speech = waybackObject.error;


it seems waybackObject.error would be undefined here

hyzhak · 2018-08-07T17:21:17Z

functions/tests/actions/wayback-machine.spec.js

+      expect(Promise.resolve()).to.be.a('promise');
+    });
+
+    it('check to see that archiveEngine is working', () => {


@adaveinthelife before merging this PR please fix those tests or note that they are not working and should be fixed

adaveinthelife added 4 commits June 4, 2018 13:40

Added wayback-machine action

9d61a6f

Merge branch 'master' of https://github.com/internetarchive/internet-…

35704a0

…archive-google-action into feature/Wayback-Machine

Further modifications to Wayback code. Fixed slots

d10b1ea

Run eslint and local testing on wayback code

92c7c8e

adaveinthelife closed this Jun 14, 2018

adaveinthelife reopened this Jun 14, 2018

adaveinthelife added 4 commits June 15, 2018 20:00

Rewrote wayback machine to return promise

c2b2cae

Created fallback for speech if country rank missing from alexa rankin…

9bc0f69

…g parsing

Reverted strings.js back to master to pass tests

efc09fe

Added Wayback dialog to strings.js

5514439

hyzhak reviewed Jun 20, 2018

View reviewed changes

hyzhak suggested changes Jun 20, 2018

View reviewed changes

adaveinthelife added 2 commits June 25, 2018 14:38

Wayback machine revisions for code review + changes to config.js file

d4184a2

Changes to string.js with additional dialog

565491b

hyzhak suggested changes Jun 26, 2018

View reviewed changes

adaveinthelife added 2 commits June 26, 2018 23:57

Changes to wayback spec file

734ec81

Break traverse off into separate util function and created unit test …

4ace6fc

…for it

hyzhak suggested changes Jul 6, 2018

View reviewed changes

Reworked XML parser promise to include resolve and reject

3cbd152

hyzhak suggested changes Jul 6, 2018

View reviewed changes

adaveinthelife added 6 commits July 10, 2018 00:57

Reworked all functions to return promises and then created promises p…

22c6015

…ipeline

Added unit tests for axios requests in wayback feature

0c7c183

Added more unit tests for axios requests in wayback feature

bcb2da7

Took some debug messages out of WB code and traverse util

ced199e

Added unit tests for archiveEngine and alexaEngine

d7b796c

Added unit test for xml converter in wb machine

167d287

hyzhak suggested changes Jul 13, 2018

View reviewed changes

hyzhak reviewed Jul 13, 2018

View reviewed changes

adaveinthelife added 2 commits July 13, 2018 13:42

Took out axios calls for wayback.spec and added them into fixtures

abb91d7

Changed promise pipeline to avoid callback

fa153ac

Took out missing axios variable in wayback spec

698830e

hyzhak suggested changes Jul 14, 2018

View reviewed changes

Rewrote archive and alexa engines to be pure functions

74f93e5

hyzhak suggested changes Jul 14, 2018

View reviewed changes

Rewrote pipeline responses to use obj assign

a3364d7

hyzhak suggested changes Jul 16, 2018

View reviewed changes

adaveinthelife added 2 commits July 16, 2018 10:08

Took unneeded promises out of pipeline, used unpacking to make code c…

d916d1e

…learer

Added catch statement for WB handler

f53d4f3

hyzhak suggested changes Jul 16, 2018

View reviewed changes

hyzhak reviewed Jul 16, 2018

View reviewed changes

adaveinthelife added 2 commits July 16, 2018 10:25

Simplified pipeline for WB machine

d76ca18

Fixed typo in catch statement

8e3ec2f

hyzhak approved these changes Jul 16, 2018

View reviewed changes

hyzhak suggested changes Aug 7, 2018

View reviewed changes

		@@ -0,0 +1,134 @@
		const request = require('request');
		const traverse = require('traverse');

		@@ -0,0 +1,31 @@
		const _ = require('lodash');
		const {debug} = require('../utils/logger')('ia:actions:wayback-machine');

Feature/wayback machine #286

Are you sure you want to change the base?

Feature/wayback machine #286

Conversation

adaveinthelife commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyzhak Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

hyzhak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyzhak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyzhak Jul 13, 2018 •

edited

Loading