forward context options in user.verify by ebarault · Pull Request #3344 · strongloop/loopback

ebarault · 2017-04-07T18:37:48Z

Description

change original "options" argument name to "verifyOptions"
adds ctx options argument as "options"
forward context "options" down to relevant downstream functions
review "verifyOptions" assertions
overall code cleaning

Related issues

required by #3314 (see comment)

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

ebarault · 2017-04-08T01:19:42Z

@bajtos

commit 9a8c9d1 addresses the main PR's purpose
commit e76d693 reviews verifyOptions assertions and brings several code cleanups

The second commit inherently implies some code churn.
If you don't mind reviewing the two together in the same PR, we could finalize #3314 quicker 🥇 and you'll make my day! Otherwise i'll cherrypick the second one in another branch.

ebarault · 2017-04-08T09:28:37Z

-      verifyOptions.subject = verifyOptions.subject || g.f('Thanks for Registering');
-
-      verifyOptions.headers = verifyOptions.headers || {};
-


these 3 assignments should be done while still sync so we can call assertVerifyOptions() before going async
(here we are already async as we saved the user with new verification token)

ebarault · 2017-04-08T09:29:24Z

+    assert(verifyOptions.mailer, 'A mailer function must be provided');
+    assert(typeof verifyOptions.mailer.send === 'function', 'mailer.send must be a function ');
+  }
+


verifyOptions assertions grouped outside the main block

ebarault · 2017-04-08T09:32:04Z

-    var tokenGenerator = verifyOptions.generateVerificationToken || User.generateVerificationToken;
-    assert(typeof tokenGenerator === 'function', 'generateVerificationToken must be a function');
+    // assert the verifyOptions params that might have been badly defined
+    assertVerifyOptions(verifyOptions);


verifyOptions assertion is now done right before generating token and going async

ebarault · 2017-04-08T09:34:10Z

        'this link in a web browser:\n\t%s', verifyOptions.verifyHref);

      verifyOptions.text = verifyOptions.text.replace(/\{href\}/g, verifyOptions.verifyHref);



these 4 assignments can't be done before since they depend on user toke generation
we could bring back verifyOptions assertion right after token generation but we'd sacrifice on performance in case of badly defined options

ebarault · 2017-04-08T09:36:33Z

+    // Set a default mailer function if none provided
+    verifyOptions.mailer = verifyOptions.mailer || userModel.email ||
+      registry.getModelByType(loopback.Email);
+


grouping verifyOptions function defaulting logic at the top

ebarault · 2017-04-08T09:39:35Z

+        sendEmail(user);
      });
-    });
+    }


encapsulating actions done once user save in a callback function to ease tokenGenerator call with 2 or 3 args

ebarault · 2017-04-08T09:43:51Z


-  User.prototype.verify = function(options, fn) {
-    fn = fn || utils.createPromiseCallback();
+  User.prototype.verify = function(verifyOptions, options, cb) {


changing "options" to "verifyOption"
adding context options as "options"
changing "fn" to "cb"

ebarault · 2017-04-08T09:45:54Z

+        function handleAfterSend(err, email) {
+          if (err) return cb(err);
+          cb(null, {email: email, token: user.verificationToken, uid: user[pkName]});
+        }


encapsulating the handling of actions on email.send callback in this function to ease call to email.send with 2 or 3 args

ebarault · 2017-04-08T09:47:17Z

+      }

-      options.to = options.to || user.email;
+      function setHtmlContentAndSend(err, html) {


adding the handling of templateFn callback error inside this function to ease the call to templateFn with 2 or 3 args

ebarault · 2017-04-08T09:50:03Z

+      }
+
+      function setHtmlContentAndSend(err, html) {
+        if (err) return cb(err);


adding the handling of templateFn callback error inside this function to ease the call to templateFn with 2 or 3 args

ebarault · 2017-04-08T09:50:57Z

+          Email.send(verifyOptions, handleAfterSend);
+        }
+
+        function handleAfterSend(err, email) {


encapsulating the handling of actions on email.send callback in this function to ease call to email.send with 2 or 3 args

bajtos · 2017-04-10T12:43:09Z

Nice 👏 I am afraid I don't have bandwidth to review this pull request today, I'll do my best to review it in the next day or two.

bajtos

Great job! The changes look mostly good to me, see my comments below.

bajtos · 2017-04-10T15:02:26Z

+        if (Email.send.length == 3) {
+          Email.send(verifyOptions, options, handleAfterSend);
+        } else {
+          Email.send(verifyOptions, handleAfterSend);


Email.send does not support options yet - see

loopback/lib/connectors/mail.js

Line 142 in 9524059

Mailer.send = function(options, fn) {

It would be nice to eventually fix Email's API and add support for options too. Let's leave that out of scope of this pull request though.

bajtos · 2017-04-10T15:03:17Z

   * @param {Function} cb The generator must pass back the new token with this function call
   */
-  User.generateVerificationToken = function(user, cb) {
+  User.generateVerificationToken = function(user, options, cb) {


Please describe the new options argument in the jsdoc comment.

bajtos · 2017-04-10T15:05:39Z

+        User.observe('before save', function(ctx, next) {
+          if (!ctx.isNewInstance) {
+            // not checking equality since other properties are added by user.save()
+            expect(ctx.options).to.contain({testFlag: true});


This is fragile. If the hook is not called then the assertion is never executed.

Here is a better way for testing the context passed to hooks: save the context to an array of observed contexts in the hook callback/handler and then assert the content of observed contexts after user.verify finished.

BTW you can install the observer after User.create finished, that way you don't have to check for isNewInstance.

bajtos · 2017-04-10T15:06:08Z

+          from: 'noreply@myapp.org',
+          templateFn: (verifyOptions, options, cb) => {
+            // not checking equality since other properties are added by user.save()
+            expect(options).to.contain({testFlag: true});


ditto - assert after user.verify() finished

bajtos · 2017-04-10T15:06:23Z

+          from: 'noreply@example.com',
+          generateVerificationToken: function(user, options, cb) {
+            // not checking equality since other properties are added by user.save()
+            expect(options).to.contain({testFlag: true});


ditto - assert after user.verify() finished

bajtos · 2017-04-10T15:08:01Z

+        const mailer = function() {};
+        mailer.send = function(verifyOptions, options, cb) {
+          // not checking equality since other properties are added by user.save()
+          expect(options).to.contain({testFlag: true});


ditto - assert after user.verify() finished

bajtos · 2017-04-10T15:08:32Z

+        const verifyOptions = {
+          type: 'email',
+          from: 'noreply@myapp.org',
+        };


This block is repeated in every test, extract it in a shared variable please.

bajtos · 2017-04-10T15:10:17Z

+          next();
+        });
+
+        return User.create({email: 'test@example.com', password: 'pass'})


This block is repeated in every test, let's extract it into a shared piece please. At minimum, extract the credentials. I think it would be even better to move User.create into a beforeEach hook.

let user; beforeEach(givenUser); // tests ... function givenUser() { return User.create(...).then(u => user = u); }

bajtos · 2017-04-10T15:10:42Z

+
+      it('forwards the "options" argument to user.save() ' +
+        'when adding verification token', function() {
+        const ctxOptions = {testFlag: true};


ctxOptions are shared by all test cases, please extract into a shared constant.

bajtos · 2017-04-10T15:15:04Z

+        // argument "options" is passed depending on Email.send function requirements
+        var Email = verifyOptions.mailer;
+        if (Email.send.length == 3) {
+          Email.send(verifyOptions, options, handleAfterSend);


Email.send does not support options yet - see

loopback/lib/connectors/mail.js

Line 142 in 9524059

Mailer.send = function(options, fn) {

It would be nice to eventually fix Email's API and add support for options too. Let's leave that out of scope of this pull request though.

ebarault · 2017-04-10T15:19:53Z

@bajtos : thanks for reviewing 👍 . Your change requests look good to me. I'm in the middle of something right now, but will try applying tomorrow to keep it fresh.

ebarault

@bajtos : PTAL, sharing verifyOptions constant led me to refactor existing tests where they were used. The result is much cleaner now.
Once i have your green light, i'll squash.

ebarault · 2017-04-10T22:20:52Z

+      credentials => User.create(credentials)
+    ).then(users => {
+      validCredentialsUser = user = users[0];
+      validCredentialsEmailVerifiedUser = users[1];


refactoring cb -> promises when creating users
assigning a shortcut user to use in tests rather than the heavy validCredentialsUser

ebarault · 2017-04-10T22:21:51Z

+          from: 'noreply@example.org',
+        };
+      });
+


base verifyOptions used in all tests
overloaded specifically in each test

ebarault · 2017-04-10T22:24:12Z

-            protocol: ctx.req.protocol,
-            host: ctx.req.get('host'),
-          };
-


refactoring existing tests to use the shared verifyOptions
we can simplify as most of these options are defaulted in user.verify implementation : redirect, to, protocol, host

ebarault · 2017-04-10T22:24:50Z

-            host: ctx.req.get('host'),
-            headers: {'message-id': 'custom-header-value'},
-          };
+          verifyOptions.headers = {'message-id': 'custom-header-value'};


actually only the headers are not standard in this test

ebarault · 2017-04-10T22:25:22Z

-              cb(null, 'custom template  - verify url: ' + verifyOptions.verifyHref);
-            },
+          verifyOptions.templateFn = function(verifyOptions, cb) {
+            cb(null, 'custom template  - verify url: ' + verifyOptions.verifyHref);


only the templateFn function is not standard

ebarault · 2017-04-10T22:26:38Z

+        return user.verify(verifyOptions, ctxOptions)
+          .then(() => {
+            // not checking equality since other properties are added by user.save()
+            expect(templateFnOptions).to.contain({testFlag: true});


now checking options once verify is done

ebarault · 2017-04-10T22:27:49Z

          assert(user, 'afterRemote should include result');

-          options = {
+          verifyOptions = {


changing options name to verifyOptions here too so be consistent

ebarault · 2017-04-10T22:32:50Z

+          },
+        });
+
+        return user.verify(verifyOptions)


reusing one user created in the main beforeEach script

ebarault · 2017-04-10T22:35:01Z

+        });
+
+        return user.verify(verifyOptions)
+          .then(() => actualVerificationToken)


ebarault · 2017-04-10T22:35:14Z

-        return User.create({email: 'test@example.com', password: 'pass'})
-          .then(user => {
-            return user.verify(verifyOptions, ctxOptions);
+        return user.verify(verifyOptions, ctxOptions)


bajtos

Lovely, the code looks so much better now! 👏

I found few small details to improve (nitpicks?), the rest of the patch LGTM. No further review is needed as far as I am concerned.

bajtos · 2017-04-11T13:41:07Z

-            redirect: '/',
+          Object.assign(verifyOptions, {
            host: 'myapp.org',
-            port: 3000,


I think it's better to keep explicit port: 3000, because we are asserting this port number few lines below (http://myapp.org:3000/test-users/confirm).

When the default port is used, it's not clear to readers of this test where is the port 3000 coming from.

bajtos · 2017-04-11T13:43:01Z

-            .then(() => actualVerificationToken);
-          })
+      it('verify that verifyOptions.templateFn receives ' +
+        'verifyOptions.verificationToken', function() {


I think the following would be more consistent with our style:

it('verifies that verifyOptions.templateFn receives verifyOptions.verificationToken', function() {

bajtos · 2017-04-11T13:44:20Z

+        return user.verify(verifyOptions, ctxOptions)
+          .then(() => {
+            // not checking equality since other properties are added by user.save()
+            expect(onBeforeSaveCtx.options).to.contain({testFlag: true});


If the hook was not called, then this would throw a TypeError because onBeforeSaveCtx is not defined. A possible solution: initalise onBeforeSaveCtx with an empty object.

change original "options" argument to "verifyOptions" adds ctx options argument as "options" forward context "options" down to relevant downstream functions review verifyOptions assertions code cleaning tests code cleaning

ebarault · 2017-04-11T17:57:17Z

@bajtos : 1 downstream test fails, i can't see why, PTAL

ssh24 · 2017-04-11T22:59:47Z

@ebarault Looks like the make command failed for node-gyp.

@slnode test please

bajtos · 2017-04-12T07:12:42Z

[cis-jenkins] PR Builder — Build finished.

This is the "master" job that triggers build of dependent jobs like x64 && linux && nvm.4 and all downstream:-prefixed jobs. It passes when all dependent pass, fails when at least dependent failed.

[cis-jenkins] downstream: gateway-director-bluemix@develop

This build is failing because the binary addon in node-rdkafka@0.10.0 cannot be compiled. Since this problem is not related to LoopBack changes at all, we can safely ignore this particular build failure.

bajtos · 2017-04-12T07:13:45Z

Landed, thank you @ebarault for your contribution!

ebarault self-assigned this Apr 7, 2017

ebarault requested a review from bajtos April 7, 2017 18:37

0candy added review needs-priority and removed review labels Apr 7, 2017

crandmck added the #review label Apr 7, 2017

ebarault removed the needs-priority label Apr 7, 2017

ebarault force-pushed the maintenance/passing-context-options-in-user.verify branch 3 times, most recently from ddff2ae to e76d693 Compare April 8, 2017 09:25

ebarault commented Apr 8, 2017

View reviewed changes

bajtos suggested changes Apr 10, 2017

View reviewed changes

bajtos reviewed Apr 10, 2017

View reviewed changes

ebarault force-pushed the maintenance/passing-context-options-in-user.verify branch from e76d693 to 65e7d11 Compare April 10, 2017 22:18

ebarault commented Apr 10, 2017

View reviewed changes

ebarault force-pushed the maintenance/passing-context-options-in-user.verify branch from 65e7d11 to 0c37a74 Compare April 10, 2017 22:30

ebarault commented Apr 10, 2017

View reviewed changes

ebarault force-pushed the maintenance/passing-context-options-in-user.verify branch from 0c37a74 to 575d4cc Compare April 10, 2017 22:34

ebarault commented Apr 10, 2017

View reviewed changes

bajtos approved these changes Apr 11, 2017

View reviewed changes

forwarding context options in user.verify

912aad8

change original "options" argument to "verifyOptions" adds ctx options argument as "options" forward context "options" down to relevant downstream functions review verifyOptions assertions code cleaning tests code cleaning

ebarault force-pushed the maintenance/passing-context-options-in-user.verify branch from 575d4cc to 912aad8 Compare April 11, 2017 15:55

bajtos merged commit faa4975 into master Apr 12, 2017

bajtos deleted the maintenance/passing-context-options-in-user.verify branch April 12, 2017 07:13

bajtos removed the #review label Apr 12, 2017

bajtos mentioned this pull request Apr 20, 2017

Provide a solution to finish the user's registration when the email verification token has expired #3314

Merged

2 tasks

bajtos mentioned this pull request May 25, 2017

New password flow and email reverification strongloop/loopback-example-user-management#58

Merged

		verifyOptions.subject = verifyOptions.subject \|\| g.f('Thanks for Registering');

		verifyOptions.headers = verifyOptions.headers \|\| {};

		'this link in a web browser:\n\t%s', verifyOptions.verifyHref);

		verifyOptions.text = verifyOptions.text.replace(/\{href\}/g, verifyOptions.verifyHref);

Uh oh!

Conversation

ebarault commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

ebarault commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 10, 2017

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault commented Apr 10, 2017

Uh oh!

ebarault left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault commented Apr 7, 2017 •

edited

Loading

ebarault commented Apr 8, 2017 •

edited

Loading

ebarault Apr 8, 2017 •

edited

Loading

ebarault left a comment •

edited

Loading

bajtos left a comment •

edited

Loading

ebarault commented Apr 11, 2017 •

edited

Loading