Adding serialize and resume to guardianJs transactions #24

joseluisdiaz · 2017-02-22T18:51:08Z

new factory for client (websocket/pooling)
new factory for transaction (fromStartFlow/fromTransactionState)
new .serialize method on Transaction
new .resume method in guardianJS to continue from a seralized transaction.

nikolaseu · 2017-02-24T17:16:28Z

README.md

+```js
+var serializedTransaction = secureStorage.get('guardiantx');
+
+auth0GuardianJS.start({ transport: 'pooling' }, serializedTransaction, function(err, transaction) {


nikolaseu · 2017-02-24T17:19:09Z

README.md

@@ -292,6 +292,35 @@ auth0GuardianJS.start(function(err, transaction) {
  //...
 });
 ```
+### auth0GuardianJS.resume(options, transactionState, callback) 
+This continues a transaction saved by `transaction.serialize()`. The option parameter provides 
+the final user the opportunity to specify which kind of `client` to use. That been:


client ?? what's client? later it's used as transport: { transport: 'pooling' }

Something like this might sound better:

The option parameter provides the final user the opportunity to specify which kind of transport to use, the options being:

nikolaseu · 2017-02-24T17:21:13Z

README.md

@@ -389,6 +418,12 @@ recovery code is the new recovery code you must show to the user from him to sav
 transaction.recover({ recoveryCode: recoveryCode });
 ```

+#### transaction.serialize() 
+The .serialize() method creates a regular javascript Object that should remind opaque 


a "plain" object, or something better? A javascript object might have functions and other things in it. Maybe a plain JSON Object?

I like plain

nikolaseu · 2017-02-24T17:22:22Z

lib/index.js

-      self.socketClient = socketClient(self.serviceUrl);
-    }
-  }
+  self.socketClient = clientFactory({


clientFactory.create will look better, just like transactionFactory.fromXX
otherwise the require should have a better name, like:

var createClient = require('./utils/client_factory');

so the function call is more understandable

nikolaseu · 2017-02-24T17:24:02Z

lib/index.js

+        return;
+      }
+
+      callback(null, tx);


try { const tx = transactionFactory.fromTransactionState(transactionState, { transactionEventsReceiver: self.socketClient, httpClient: self.httpClient }); callback(null, tx); } catch (transactionBuildingErr) { callback(transactionBuildingErr); }

nikolaseu · 2017-02-24T17:24:32Z

lib/transaction/index.js

@@ -24,7 +24,7 @@ var eventSequencer = require('../utils/event_sequencer');
 var eventListenerHub = require('../utils/event_listener_hub');

 /**
- * @public
+ * @public||


nikolaseu · 2017-02-24T17:24:37Z

lib/transaction/index.js

@@ -74,6 +74,7 @@ function transaction(data, options) {
  self.txId = data.transactionToken.getDecoded().txid;
  self.transactionToken = data.transactionToken;
  self.transactionEventsReceiver = options.transactionEventsReceiver;
+


nikolaseu · 2017-02-24T17:24:42Z

lib/transaction/index.js

@@ -136,8 +137,34 @@ function transaction(data, options) {
  return self;
 }

+


nikolaseu · 2017-02-24T17:25:53Z

lib/utils/client_factory.js

+var pooling = require('./polling_client');
+var socketio = require('./socket_client');
+
+module.exports = function factory(options) {


export an object with a function create or something like that instead of the function directly. otherwise client_factory is not a good name

joseluisdiaz · 2017-02-24T19:06:09Z

@nikolaseu addressed most of your comments; i realized that we needed a new test for guardianjs.resume. I rewrite the commits history, sorry about this.

fiddur

Posting a partial review. Will continue soon.

fiddur · 2017-02-24T09:21:33Z

lib/entities/enrollment.js

@@ -48,4 +48,11 @@ enrollment.prototype.getPhoneNumber = function getPhoneNumber() {
  return this.data.phoneNumber;
 };

+/**
+ * @returns @see constructor.


I think only @see is enough here :)

fiddur · 2017-02-27T12:16:15Z

README.md

+the final user the opportunity to specify which kind of `transport` to use. That been:
+
+- `socket`: a socket.io transport
+- `pooling`: a pooling transport.


"polling". "pooling" refers to a "pool" with several connections to pick from, "polling" refers to "poll" to see if something is done...

"The option parameter" -> "The options parameter"

"That been:" -> "Options include:"

fiddur · 2017-02-27T13:50:29Z

README.md

@@ -389,6 +418,12 @@ recovery code is the new recovery code you must show to the user from him to sav
 transaction.recover({ recoveryCode: recoveryCode });
 ```

+#### transaction.serialize() 
+The .serialize() method creates a plain javascript Object that should remind opaque 


s/remind/remain/

...I'm not sure what is meant here, with the object being opaque to the end user. In all examples, the object is not sent to the end user.

That's a good point; to 'library user' or 'library client' i meant.

topherMarie · 2017-02-27T15:25:21Z

lib/index.js

+
+/**
+ * @public
+ *
 * Post result to url using an standard form


"Post result to url using a standard form"

topherMarie · 2017-02-27T15:25:53Z

lib/transaction/factory.js

+ * @param {object} transactionState.availableAuthenticationMethods
+ *
+ * @param {EventEmitter} options.transactionEventsReceiver
+ *  Receiver for transaction events; it will receive  backend related transaction events


Extra space

topherMarie · 2017-02-27T15:29:08Z

lib/index.js

@@ -128,6 +122,70 @@ auth0GuardianJS.prototype.start = function start(callback) {
 /**
 * @public
 *
+ * This takes a parameter what transaction.serialize returns to
+ * resume the transaction with auth0-mfa-api


"Takes a parameter returned from transaction.serialize to resume the transaction with auth0-mfa-api"

Correct me if I'm not interpreting it correctly, though

topherMarie · 2017-02-27T15:30:42Z

README.md

+- `socket`: a socket.io transport
+- `pooling`: a pooling transport.
+
+The default one is `socket`. 


"If not set, the socket transport is used as default"

And polling again :)

fiddur

Looks good, but it's a lot of changes... We really should have a test coverage setup (like istanbul) to not miss adding tests for any added/changed function.

fiddur · 2017-02-28T08:05:58Z

lib/utils/client_factory.js

@@ -0,0 +1,20 @@
+var pooling = require('./polling_client');


joseluisdiaz force-pushed the serialize-deserialize branch 4 times, most recently from 204e4b1 to 8dab8ad Compare February 24, 2017 16:31

joseluisdiaz changed the title ~~WIP: adding serialize and resume to guardianJs transactions~~ Adding serialize and resume to guardianJs transactions Feb 24, 2017

nikolaseu reviewed Feb 24, 2017

View reviewed changes

adding serialize and resume to guardianJs transactions

e0f81d0

joseluisdiaz force-pushed the serialize-deserialize branch from 1d42675 to 81aa89f Compare February 24, 2017 19:04

fiddur reviewed Feb 27, 2017

View reviewed changes

joseluisdiaz added 2 commits February 27, 2017 12:07

unit tests

0396108

Documentation

6d9cdb5

joseluisdiaz force-pushed the serialize-deserialize branch from 81aa89f to 6d9cdb5 Compare February 27, 2017 15:08

topherMarie reviewed Feb 27, 2017

View reviewed changes

fiddur approved these changes Feb 28, 2017

View reviewed changes

lib/utils/client_factory.js Outdated

@@ -0,0 +1,20 @@

var pooling = require('./polling_client');

Copy link

fiddur Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polling!

Addressing comments

40821ba

joseluisdiaz force-pushed the serialize-deserialize branch from 10ee80e to 40821ba Compare March 1, 2017 12:31

joseluisdiaz merged commit 290c3aa into auth0:master Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding serialize and resume to guardianJs transactions #24

Adding serialize and resume to guardianJs transactions #24

joseluisdiaz commented Feb 22, 2017 •

edited

Loading

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

joseluisdiaz Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

nikolaseu Feb 24, 2017

joseluisdiaz commented Feb 24, 2017

fiddur left a comment

fiddur Feb 24, 2017

fiddur Feb 27, 2017

topherMarie Feb 27, 2017

fiddur Feb 27, 2017

fiddur Feb 27, 2017

joseluisdiaz Feb 27, 2017

topherMarie Feb 27, 2017

topherMarie Feb 27, 2017

topherMarie Feb 27, 2017

topherMarie Feb 27, 2017

fiddur Feb 28, 2017

fiddur left a comment

fiddur Feb 28, 2017

		@@ -136,8 +137,34 @@ function transaction(data, options) {
		return self;
		}

Adding serialize and resume to guardianJs transactions #24

Adding serialize and resume to guardianJs transactions #24

Conversation

joseluisdiaz commented Feb 22, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseluisdiaz commented Feb 24, 2017

fiddur 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

fiddur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseluisdiaz commented Feb 22, 2017 •

edited

Loading