Implement Email Verification #583

drew-gross · 2016-02-22T23:23:50Z

Still missing:

Password resets
Setting emailVerified to false and sending a new verification email when email is changed after creation.
Use of AdapterLoader interface.

flovilmart · 2016-02-22T23:38:06Z

src/verifyEmail.js

@@ -0,0 +1,27 @@
+function verifyEmail(appId, serverURL) {


To keep with the current pattern, maybe we should have an class EmailVerificationController extends PromiseRouter {}.

The email related routes aren't really grouped together in any logical way, so I'm not sure that makes sense.

your call, but it smells the need of decoupling the invalidLink, and verify success calls too, maybe on an adapter.

The EmailVerificationController would have the validation route, then the adapter would provide the behaviour for invalid link and success.

Yeah some parts of those are potential customization points. I think it might make more sense as a separate adapter though.

If a class is never overridden, doesn't inherit any meaningful behaviour, and doesn't have any instance-specific data, I don't see any need to put it in a class. Classes are also slower due to chasing pointers around the prototype chain, if you have few instantiations and many calls (this applies to our controllers)

Sure. Let's leave it like that for now.

I actually feel strongly here - we should be creating a MailController here, since we need a databaseAdapter here, which is state of a controller, which is not injected right now - but we are definitely moving that direction.

Not 100% sure I follow

If you look at it right now - it could a simply function, sure, but as we are trying to decouple and move away from using globals - the DatabaseAdapter piece as a first line in the function is actually a global, and should be injected at construction time of a controller - since it is required for it to function.
The idea is to have a single DatabaseAdapter per server, which is what we allow right now, and stop doing things like getDatabaseConnection from a global.

Which at this point will make sense to move this entire piece into UsersController, which will make it much simpler to extend.
Also, we might as well split some logic that deals with request/response here into UsersPublicRouter (because you don't need app specific security and friends).

nlutsenko · 2016-02-23T01:25:08Z

src/Routers/UsersRouter.js

-import RestWrite from '../RestWrite';
-import { newToken } from '../cryptoUtils';
+import RestWrite      from '../RestWrite';
+let cryptoUtils = require('../cryptoUtils');


Nit: Use ES6 import.

facebook-github-bot · 2016-02-23T02:07:28Z

@drew-gross updated the pull request.

nlutsenko · 2016-02-23T03:15:44Z

src/Routers/UsersRouter.js

@@ -150,6 +170,7 @@ export class UsersRouter extends ClassesRouter {
    this.route('POST', '/requestPasswordReset', () => {
      throw new Parse.Error(Parse.Error.COMMAND_UNAVAILABLE, 'This path is not implemented yet.');
    });
+    this.route('POST', '/requestPasswordReset', req => this.handleReset(req));


Looks like this might be coming, but is currently missing, just putting the comment here to remind :P

facebook-github-bot · 2016-02-23T20:41:14Z

@drew-gross updated the pull request.

flovilmart · 2016-02-23T21:30:23Z

src/Adapters/loadAdapter.js

+
+  if (options.module) {
+    //Configuring via module name + options
+    return require(options.module)(options.options)


should check if require(options.module).default for ES6 exported objects, is set, if set and is function, should create a new instance.

flovilmart · 2016-02-23T21:32:58Z

@drew-gross for the load adapter, es6 will break if you try to call Class() without the new operator. Your load adapter design is factory oriented. That can work too, but so far all the adapters we have are not factory oriented but class/instance oriented.

> class A {}
[Function: A]
> A()
TypeError: Class constructors cannot be invoked without 'new'

flovilmart · 2016-02-23T21:40:44Z

src/Adapters/Email/SimpleMailgunAdapter.js

@@ -0,0 +1,39 @@
+import Mailgun from 'mailgun-js';
+
+let SimpleMailgunAdapter = mailgunOptions => {


Actually, you still need to return an Object in the end, I'm not sure of the performance advantage of returning the object as you do instead of doing

export class SimpleMailgunAdapter { constructor(options) { this.mailgun = Mailgun(mailgunOptions); } sendMail(to, subject, text) {} sendVerificationEmail({ link, user, appName, }) {} }

This way you can use the default loadAdapter.

Typically, using classes is faster to instantiate (due to less allocation for the functions), using objects is faster to call methods (due to not needing to walk the prototype chain). Since these objects are called far more often than they are instantiated, this way should be better for perf.

drew-gross · 2016-02-23T22:01:45Z

@flovilmart the ones we've done so far are pretty much ad-hoc and there are only 3 of them. I'd rather pick a good interface now and stick with it for the future, than stick with a bad interface for consistency with the earliest 3 adapters. Also, it's pretty easy to accept legacy config options, for backwards compatibility, so it would be fairly easy to convert the 3 existing adapters to the new style while not forcing old servers to upgrade.

I didn't account for classes because it's slower if you are only instantiating the object once (this should be true of most/all adapters) and I'm just generally against classes. Douglas Crockford explains why pretty clearly in this talk: https://www.youtube.com/watch?v=PSGEjv3Tqo0

nlutsenko · 2016-02-23T22:20:28Z

Do we use it for public-facing adapters only? Do we use this as a rule of thumb everywhere?
Ok, let's just pick a single thing, because I feel we have 2 fighting architectures here, that actually follow the same flow, just use different pieces of JavaScript, Classes vs Factories of Functions.

I agree to any approach you guys take, because I hope @drew-gross, you have a better idea on what makes best sense in the latest modern JavaScript that will scale as we go.

The only thing that I don't see how it fits - injected state that is required for most/all methods of Controllers and the state that should live on the Controllers themselves. For example, FilesController requires a FilesAdapter, which is a parameter on the instance, for most of it's operations.
The same will apply to ObjectsController aka what we have right now split between RestWrite/RestQuery/transform.js/rest.js.

I understand an extra call implication of performance here, but I don't think it actually regresses the performance by much (the stats that I found actually suggest the same performance implication).
Also, clearly - Object-oriented JavaScript is the future.

drew-gross · 2016-02-23T22:57:07Z

Do we use it for public-facing adapters only?

So far we have no concept of a non-public facing adapter, so I'm not sure what that means.

Object-oriented JavaScript is the future.

Or, like Douglas Crockford is advocating, object oriented without classes :)

injected state that is required for most/all methods of Controllers and the state that should live on the Controllers themselves.

How you construct the adapters is not related to where the state lives. Classes are also not required for injected state.

I don't think it actually regresses the performance by much

True, for shallow inheritance trees on modern JS engines the difference should be minimal.

flovilmart · 2016-02-23T23:10:56Z

In the end, the goal is the flexibility for the 3rd party modules. In an ideal world, I'd support both options from a single loadAdapter. What we can do is:

If it's an es6 module, (default is set when calling require) create an instance. Otherwise, just call the method with the options. Any way to tell at runtime the difference from a constructor function from a factory? Size of the prototype property maybe?

drew-gross · 2016-02-23T23:17:19Z

the goal is the flexibility for the 3rd party modules

I would somewhat disagree with this. I would suggest that we would rather there be exactly one way to implement adapters, and it should be lightweight and low-overhead, to reduce confusion and reduce the learning curve for people making many adapters (I'm envisioning a future where most extensibility is provided through adapters, so most parse app writers will be making at least a handful of them).

drew-gross · 2016-02-23T23:21:02Z

If we did want to detect constructor requiring new or not, the way would be to try constructing without new and in the catch block try constructing with new.

flovilmart · 2016-02-23T23:29:44Z

@drew-gross that seems reasonable, and I checked the babelified code and it injects the proper check:

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

Do you think you can try merging your adapter paradigm with .module, .class etc.. with the loadAdapter that takes care of instantiation + the trying?

drew-gross · 2016-02-23T23:38:14Z

Yeah I can do that. I have a lot on my plate right now and I'm also sorta sick this week so it probably won't be soon. I'll try to get it in for 2.1.3 though.

flovilmart · 2016-02-23T23:39:50Z

@drew-gross I can take over from where you are :) Get well, that's what matters! I'll PR on your branch.

drew-gross · 2016-02-23T23:44:32Z

Cool, thanks. Please try to include some tests that demonstrate what the users of the interface will see, like I've done in index.spec.js. They serve as a good demonstration as well as being an actual test.

flovilmart · 2016-02-23T23:46:33Z

No problem, I had a full test suite on the loadAdapter (100%) for demonstration purposes too :)

drew-gross · 2016-02-24T00:21:54Z

Yeah that one didn't show what it looks like when you actually instantiate the ParseServer which is the most important part.

flovilmart · 2016-02-24T01:02:17Z

watched Douglas's talk, very insightful! This pattern makes a lot of sense, should we move towards that kind of architecture? I actually may use it in Swift too.

nlutsenko · 2016-02-24T01:08:36Z

Talked to @drew-gross off GitHub and yeah - this is the future! Don't think we need to move everything right away, but rather - move as we go, so we don't spend time moving stuff around, but actually building things.
I am going to use this approach as a rule of thumb for all the new things or things I touch. :)

Swift: you actually don't need to, since Seift inherently has this, re closures/functions/classes/structs/protocol extensions.
Swift really shines with Peotocols and Peotocol Extensions coupled with Structs.

drew-gross · 2016-02-24T01:09:18Z

Hah, @nlutsenko was just saying that this architecture makes a lot of sense in swift because scopes are closures in swift (apparently - I've probably written no more than 50 lines of swift yet)

flovilmart · 2016-02-24T01:10:34Z

just the stupid let that means const and var that mean let... besides that we're good :)

flovilmart · 2016-02-24T01:14:34Z

@nlutsenko started writing an express like server in swift, based on protocol extensions just as an experiment. That actually is very nice code.

flovilmart · 2016-02-27T21:29:45Z

Closing as superset by #627

facebook-github-bot added the GH Review: review-needed label Feb 22, 2016

flovilmart reviewed Feb 22, 2016
View reviewed changes

drew-gross assigned nlutsenko Feb 23, 2016

nlutsenko reviewed Feb 23, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels Feb 23, 2016

nlutsenko assigned drew-gross and unassigned nlutsenko Feb 23, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Feb 23, 2016

nlutsenko reviewed Feb 23, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels Feb 23, 2016

Alex Kwan and others added 8 commits February 23, 2016 07:55

fix spacing

24f9176

FIX : User Roles not added to create, update or delete calls

40d37e9

Exploring the interface of a mail adapter

3c31e1f

Implement and test sending of signup email

e953953

Finish implementation of email verification on create

8f643c7

Cleanup reset logic (that will come in another PR)

817ef97

Remove reset routes

317d6ea

Add some tests and demonstrate the adapter loading interface

0a982b7

drew-gross force-pushed the mail-adapter branch from 79a5238 to 0a982b7 Compare February 23, 2016 20:38

facebook-github-bot removed the GH Review: needs-revision label Feb 23, 2016

flovilmart reviewed Feb 23, 2016
View reviewed changes

flovilmart mentioned this pull request Feb 24, 2016

Email Validation #627

Merged

taylorstine mentioned this pull request Feb 24, 2016

MailAdapter design #275

Closed

flovilmart closed this Feb 27, 2016

goggelj mentioned this pull request Mar 28, 2016

verifyUserEmails option + mailgun #1219

Closed

drew-gross deleted the mail-adapter branch April 5, 2016 21:52

This was referenced Feb 20, 2024

refactor: Upgrade semver from 7.5.2 to 7.5.4 #8934

Merged

refactor: Upgrade semver from 7.5.2 to 7.5.4 #8940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Email Verification #583

Implement Email Verification #583

drew-gross commented Feb 22, 2016

flovilmart Feb 22, 2016

drew-gross Feb 23, 2016

flovilmart Feb 23, 2016

drew-gross Feb 23, 2016

flovilmart Feb 23, 2016

nlutsenko Feb 23, 2016

drew-gross Feb 23, 2016

nlutsenko Feb 23, 2016

nlutsenko Feb 23, 2016

facebook-github-bot commented Feb 23, 2016

nlutsenko Feb 23, 2016

facebook-github-bot commented Feb 23, 2016

flovilmart Feb 23, 2016

flovilmart commented Feb 23, 2016

flovilmart Feb 23, 2016

drew-gross Feb 23, 2016

drew-gross commented Feb 23, 2016

nlutsenko commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 24, 2016

flovilmart commented Feb 24, 2016

nlutsenko commented Feb 24, 2016

drew-gross commented Feb 24, 2016

flovilmart commented Feb 24, 2016

flovilmart commented Feb 24, 2016

flovilmart commented Feb 27, 2016

		@@ -0,0 +1,39 @@
		import Mailgun from 'mailgun-js';

		let SimpleMailgunAdapter = mailgunOptions => {

Implement Email Verification #583

Implement Email Verification #583

Conversation

drew-gross commented Feb 22, 2016

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

facebook-github-bot commented Feb 23, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Feb 23, 2016

Choose a reason for hiding this comment

flovilmart commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drew-gross commented Feb 23, 2016

nlutsenko commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 23, 2016

flovilmart commented Feb 23, 2016

drew-gross commented Feb 24, 2016

flovilmart commented Feb 24, 2016

nlutsenko commented Feb 24, 2016

drew-gross commented Feb 24, 2016

flovilmart commented Feb 24, 2016

flovilmart commented Feb 24, 2016

flovilmart commented Feb 27, 2016