SeedDB Refactoring

[lirantal]

Refactoring the seeddb logic to work with promises all over due to all the async behavior.
This lays the ground for further work on adding tests for SeedDB functionality.

[lirantal]

@mleanos care to review?

[mleanos]

This should be checking for a length greater than zero, or just users.length

Even with that fix, this still won't work. We're already in the callback of the remove(), so it's too late to check if we should be removing this user.

I think it might be a good idea to move the remove functionality outside of the saveUser method. We probably don't want to enter this function unless we are sure we want to add the new user.

[mleanos]

@lirantal I left a line comment. Overall, I like what you've done. Other than what I pointed out, everything is working pretty well.. Tested this with the production env as well.

One thing, that may just be preference, is the use of the callbacks for the promises. Rather than using closures. Is there a specific reason to use one way over the other?

Overall, I really like these changes & how you tied the seeding into the app & mongoose configs. That seems like the best way to make this testable and scaleable.

[lirantal]

@mleanos Thanks, I'll address the admin user issue for the production environment state.

What do you suggest we use instead of closures and callbacks? Can you show an example?

[lirantal]

@mleanos I updated the PR with the fix for the admin user account.

[lirantal]

Check it out, I also added meanjs-core server tests so we can test our own code base.
This is of course just the beginning with the Seed DB feature so there will be more to come.

[lirantal]

Closing and re-opening this PR because it seems that Travis CI was not working at all.

[codydaig]

@lirantal Travis is having technical issues right now.
https://www.traviscistatus.com/incidents/1r8qwcwdfk90

[lirantal]

Ahh, explains it :)

[ilanbiala]

I'd prefer this returns a promise.

[lirantal]

I agree, but let's do this in another PR and keep the promises refactoring contained to the seed functionality.

[mleanos]

@lirantal I've tested this, and the issue with the remove is fixed. However, in the original version of this seeding, the admin & user accounts were removed & re-created each time when the node env is Development. With these changes, you're not re-creating the admin account if in the Development env. Was that intentional?

In addition to my line comment, I think you can do a couple things differently to reduce the amount of code & keep the logic's SOC in tact...

What about moving the remove functionality outside of the saveUser function? If you burden a new removeUser function with the task of determining whether or not to remove the user (based on env, and admin already existing) then your saveUser can be simplified. Also, by doing so you can reduce the code of your performSeedUser function even further.

For example..

// resolve with success, or reject when there's an error or we don't want to touch, or add, the user
function removeUser(user) {
  return new Promise...
}

And the body of performSeedUser will look something like this..

removeUser(user)
.then(generatePassword)
.then(saveUser(user))
.then(userSeedSuccess)
.then(function () {
  // add your admin user here
})
.catch(reportError);

An alternative to the last call in the above chain, you can refactor performSeedUser to accept a user and call it from start each time you'd like to add a seeded user.

With all that said, I think this is a great start. And I agree that this can be the foundation for a grandeur refactoring of the back-end to make it more testable. How we implement these changes here can set the style for how we refactor, and implement promises, throughout the rest of the back-end. That's part of the reason why I'm so interested in getting this right, or at least understanding the style.

[lirantal]

Thanks for the feedback guys.
I'm all about doing things right, so let's work through this PR together to get everything out right as it should be.

[lirantal]

@ilanbiala and @mleanos
I've refactored and fixed formatting styles to conform with the comments, please review.

P.S. Going with the closures inside the functions is preferred IMO on moving the user object around.

[mleanos]

You don't need the closure here, if you're not using it to pass any parameters.

[lirantal]

We need the closure there so that saveUser can act as a promise, and thus it will call the resolve() with the data variable of theuser.
Try commenting out the closure function and test to see what I mean.

[mleanos]

@lirantal I see what you mean. However, I think this behavior is stemming from the previous promise resolve in the chain. In both uses of the saveUser method, the previous promise is resolving with no data: resolve();

Hence, the saveUser method is expecting to handle that resolve, but if you don't have that closure there it doesn't know what to do with the resolve.

Let's say you resolved removeUser with data: resolve(user);
The next operation in the chain could look like this .then(saveUser); in this case you wouldn't need that empty closure since it would get the user from the resolve.

Ok... Now with all that said. I think what you have is the correct way to handle it. I just wanted to clarify what my findings were, as to why we're seeing this behavior & need for the empty closure; and I'm sure you're aware of most of this. Furthermore, I think you're absolutely correct to resolve most of those functions with no data. For instance, it wouldn't make sense to resolve removeUser with the user, and we want to maintain the user object from the scope that the chain is in.

[lirantal]

Exactly.

[mleanos]

@lirantal This looks really good! It's much cleaner & easier to follow. And I think it can be extended further in the future if need be.

I left a couple of minor comments. I'm going to spend some time with the tests now. I think we're pretty much there with this. I'm a little concerned that seedTheUser is still dependent on checking the environment & username, but I don't think we have a choice here with this specific use case; and it's a very minor concern for me.

One thing we might want to introduce at this time, is adding seed db options to the environmental configs. For instance, we could add the user info to the config. I can see users wanting to control that specifically based on the environment that the app is running in.

I had an issue with the username being set to "admin" in my development environment. I already had a user with "admin" for the username. The seeding removed this user & I lost context of data that was related to this user. Now might be as good of a time as any to do this, if we can see the need for this to be added to the config.

[lirantal]

@ilanbiala @mleanos ready. even increased our coverage by 2%! ;-)
@mleanos about further refactoring to environment variables, how about we work on that in another PR?

[codydaig]

@lirantal @ilanbiala @mleanos

• I prefer closures as well!
• Great job! Looks great. :-D
• Let's leave the env variables refactor for another PR.
• I think travis is failing on 0.10 because native promises wern't supported until 0.12. If we want to keep 0.10 support, we need to use Q or Bluebird.

[lirantal]

Great, thanks Cody.
With regards to versions, v0.10 did receive a backport for Promises support with an update for the rest of the V8 features so we're ok with v0.10 users and no need to deprecate support for that. The build failed for some other reason with db timeouts so I restarted it and it passes just fine now.

[codydaig]

@lirantal Good to know. Thanks!

LGTM

[mleanos]

@lirantal I left a comment about the empty closure. I agree with your use of the closure there. However, I wanted to clarify why I think we're seeing that behavior.

I'm very pleased with how this turned out! Thanks for working through this with me. I've definitely learned a few things. I think this is ready. LGTM.

P.S. 60% Coverage. Let's keep that upward trend going :)

[mleanos]

@lirantal One thing I forgot to mention.. The tests are logging the seed db events to the console. This is disruptive to the output of the tests. Let's work on that in a separate PR, so we can get this out.

[lirantal]

Sure. I noticed that too but didn't have much idea how to further work it out.
You're welcome to submit an enhancement PR with more tests as well.

But hey, we already increased coverage by 2% to 60%, that's awesome if I may say so! :)

[codydaig]

👍

[mleanos]

@lirantal Once this is merged, I'll work on adding the user info the configs, and adding tests :)

And yea, pretty awesome, we're at 60%! Great job leading the charge on the test coverage!!