Create getAllTransactions wrapper around getTransactions #148

evanlimanto · 2018-03-20T20:33:18Z

Fixes #145.

evanlimanto · 2018-03-20T21:23:05Z

Seems like we need async/await support in circleci to successfully run the tests.

ntindall · 2018-03-21T17:22:34Z

lib/PlaidClient.js

@@ -210,6 +210,42 @@ Client.prototype.getTransactions =
    }, options, cb);
  };

+// getAllTransactions(String, Date, Date, Object?, Function)
+Client.prototype.getAllTransactions =
+  async function(access_token, start_date, end_date, cb) {


@evanlimanto, lets write this without using mode modern node features. I can't imagine that all of our clients have node on 7.x or greater right now.

I would consider looking at http://bluebirdjs.com/docs/api/promise.coroutine.html

evanlimanto · 2018-03-22T04:39:24Z

I've updated it to use bluebird, thanks!

ntindall · 2018-03-28T15:12:27Z

lib/PlaidClient.js

@@ -1,5 +1,6 @@
 'use strict';

+var Promise = require('bluebird');


rather than shadowing Promise, it would be more idiomatic to give the bluebird promise implementation a different name.

canonically @ plaid, we use P

var P = require('bluebird')

ntindall · 2018-03-28T15:14:59Z

lib/PlaidClient.js

+
+    var handleResponse = function(transactionsResponse) {
+      total_transactions = transactionsResponse.total_transactions;
+      transactions.push(...transactionsResponse.transactions);


we should not use the spread operator, since it assumes that the client is using a version of node that supports it.

even though node4 is almost at end of life... we should not do this.

consider:

transactions = R.concat(transactions, transactionsResponse.transactions);

ntindall · 2018-03-28T15:15:30Z

lib/PlaidClient.js

+Client.prototype.getAllTransactions =
+  function(access_token, start_date, end_date, cb) {
+    var transactions = [];
+    var offset = 0, total_transactions = null;


nit: two statements per line

we don't have a linter set up for this project, but it would probably not like this :).

ntindall · 2018-03-28T15:19:56Z

lib/PlaidClient.js

+          break;
+        }
+
+        yield self.getTransactions(


since we are in a generator, we can do something like

const intermediaryTransactionsResponse = yield self.getTransactions( /* ... */ ); handleResponse(intermediaryTransactionsResponse);

In that world, its probably best to have handleTransactions be inline within the loop.

ntindall · 2018-03-28T15:21:44Z

lib/PlaidClient.js

+        });
+      }
+    })().then(function() {
+      return cb(null, transactions);


this seems reasonable to me... I agree with you that we should be using the Promise api internally...

ntindall · 2018-03-28T15:23:44Z

test/PlaidClientTest.js

+        });
+
+        it('all transactions', cb => {
+          pCl.getAllTransactions(accessToken, now, now,


this is good for an end-to-end test, but I'm not sure it tests anything other than that the function doesn't throw an exception. This is generally OK for the other functions, since they don't do much other than call the API, but now that we are implementing some real logic, I think we need more.

@evanlimanto could we add some unit tests for the pagination? I would suggest using sinon to stub out the implementation of self.getTransactions to return fake data in pages (and assert that the aggregate result is returned).

added unit tests!

ntindall · 2018-03-28T15:26:04Z

lib/PlaidClient.js

+    var self = this;
+
+    var handleResponse = function(transactionsResponse) {
+      total_transactions = transactionsResponse.total_transactions;


the documentation is somewhat ambiguous, but I believe that this represents the total transactions available within the date range (e.g. that this is the proper way to terminate the pagination).

cc @michaelckelly @pbernasconi we should consider clarifying the documentation here: https://plaid.com/docs/api/#transactions

evanlimanto · 2018-04-02T06:43:28Z

i've fixed all the issues you mentioned.

ntindall · 2018-04-28T15:02:33Z

test/PlaidClientTest.js

+                  transactions: R.range(0, 200),
+                  total_transactions: 200,
+                });
+              }


i think we should throw an error in any other case

ntindall · 2018-04-28T15:03:32Z

test/PlaidClientTest.js

+                transactionsResponse.transactions = R.range(500, 1000);
+              } else if (options.offset === 1000) {
+                transactionsResponse.transactions = R.range(1000, 1200);
+              }


any other case should be an error

ntindall · 2018-04-28T15:05:16Z

lib/PlaidClient.js

+    var self = this;
+
+    P.coroutine(function*() {
+      while (true) {


makes sense, we will break out when we get the correct number of transactions or if any of the requests throws an error.

assuming the plaid API works correctly, this looks right to me.

ntindall · 2018-04-28T15:06:05Z

@jacksiegel anything to add here? it looks right to me (just left a few nits for @evanlimanto)

ghost · 2018-04-29T00:42:33Z

@ntindall nope, looks good to me.

evanlimanto · 2018-05-06T03:53:32Z

sorry it took a while — i've fixed the nits you mentioned.

ntindall

Looks good after the final (yes, really this time) comment @evanlimanto !

ntindall · 2018-05-06T14:52:27Z

lib/PlaidClient.js

+
+        transactions = R.concat(
+          transactions, transactionsResponse.transactions);
+        if (transactions.length === transactionsResponse.total_transactions) {


can we change this to be >= ? I am somewhat concerned about the possibility of this loop not terminating.

makes sense, though something could be wrong with the plaid API in that case

yes -- I think that it "should never happen"

better to be safe!

ntindall · 2018-05-06T17:05:33Z

thanks @evanlimanto -- see you soon!

snyamathi · 2018-05-19T04:54:06Z

@ntindall @evanlimanto It doesn't look like this function returns a promise. Is that something which can easily be fixed?

evanlimanto · 2018-05-19T05:10:30Z

Good catch — I think it should be an easy fix with wrapPromise.

ntindall · 2018-05-19T08:47:23Z

Ah, sorry about that @snyamathi, we'll work on a fix. @evanlimanto is right, we already have an easy way to support both the cps and promise styles internally.

ntindall · 2018-05-20T14:25:31Z

#157

ntindall reviewed Mar 21, 2018

View reviewed changes

evanlimanto force-pushed the get-all-transactions branch 5 times, most recently from 137d623 to 9f27b83 Compare March 22, 2018 04:34

ntindall reviewed Mar 28, 2018

View reviewed changes

evanlimanto force-pushed the get-all-transactions branch 6 times, most recently from 010b942 to a581158 Compare April 2, 2018 06:42

ntindall reviewed Apr 28, 2018

View reviewed changes

ntindall requested a review from a user April 28, 2018 15:05

ntindall added the enhancement label Apr 28, 2018

evanlimanto force-pushed the get-all-transactions branch from a581158 to a7776bf Compare May 6, 2018 03:52

ntindall reviewed May 6, 2018

View reviewed changes

Create getAllTransactions wrapper around getTransactions

ddd1bbb

evanlimanto force-pushed the get-all-transactions branch from a7776bf to ddd1bbb Compare May 6, 2018 16:30

ntindall added enhancement and removed enhancement labels May 6, 2018

ntindall merged commit 73cbd39 into plaid:master May 6, 2018

This was referenced May 6, 2018

Test improvements #155

Merged

getting all transactions? #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create getAllTransactions wrapper around getTransactions #148

Create getAllTransactions wrapper around getTransactions #148

evanlimanto commented Mar 20, 2018

evanlimanto commented Mar 20, 2018

ntindall Mar 21, 2018 •

edited

Loading

evanlimanto commented Mar 22, 2018 •

edited

Loading

ntindall Mar 28, 2018

ntindall Mar 28, 2018

ntindall Mar 28, 2018

ntindall Mar 28, 2018

ntindall Mar 28, 2018

ntindall Mar 28, 2018

evanlimanto Apr 2, 2018

ntindall Mar 28, 2018

evanlimanto commented Apr 2, 2018

ntindall Apr 28, 2018

ntindall Apr 28, 2018

ntindall Apr 28, 2018

ntindall commented Apr 28, 2018

ghost commented Apr 29, 2018

evanlimanto commented May 6, 2018

ntindall left a comment

ntindall May 6, 2018

evanlimanto May 6, 2018

ntindall May 6, 2018

ntindall commented May 6, 2018

snyamathi commented May 19, 2018

evanlimanto commented May 19, 2018

ntindall commented May 19, 2018

ntindall commented May 20, 2018

		@@ -1,5 +1,6 @@
		'use strict';

		var Promise = require('bluebird');

Create getAllTransactions wrapper around getTransactions #148

Create getAllTransactions wrapper around getTransactions #148

Conversation

evanlimanto commented Mar 20, 2018

evanlimanto commented Mar 20, 2018

ntindall Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

evanlimanto commented Mar 22, 2018 • edited Loading

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

evanlimanto commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntindall commented Apr 28, 2018

ghost commented Apr 29, 2018

evanlimanto commented May 6, 2018

ntindall 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

ntindall commented May 6, 2018

snyamathi commented May 19, 2018

evanlimanto commented May 19, 2018

ntindall commented May 19, 2018

ntindall commented May 20, 2018

ntindall Mar 21, 2018 •

edited

Loading

evanlimanto commented Mar 22, 2018 •

edited

Loading