Added ability to specify what to populate on save()

[tobalsgithub]

This is the same as #954 but with the function signature changed to have cb as the last parameter.

All credit to @nkibler7.

[dmarcelino]

Hey @tobalsgithub, did you branch @nkibler7's save-populate-feature branch? I don't see his commits in this PR only 1 commit from you...

[tobalsgithub]

Oh, no, I just forked the main waterline repo and created all his changes in one commit. Need me to do it the other way?

[dmarcelino]

I think it would be fair thing to do as then the PR would reflect the history of the changes (and their author).

[tobalsgithub]

Ok, tried again. Is this what you were thinking?

[dmarcelino]

Much better :)

[dmarcelino]

@tobalsgithub, would you mind adding a couple of tests to assert:

1. previous behaviour (populate if no arguments are supplied) remains intact;
2. only specified associations are populated (this should fail before the fix is applied).

Perhaps in https://github.com/balderdashy/waterline/tree/master/test/integration?

[tobalsgithub]

Happy to try. I took a peak at this and think there should be 3 tests.

1. previous behaviour (populate if no arguments are supplied) remains intact;
2. only specified associations are populated (this should fail before the fix is applied).
3. no associations are populated if options.populate = false (this should fail before the fix is applied).

It's not at all clear to me how I should go about this though. Any guidance on what I can look at to determine if the associations are getting populated?

[tobalsgithub]

To give a bit more context, I'm in https://github.com/balderdashy/waterline/tree/master/test/integration/model/save.js. From looking at the other tests, I'm not sure how I can tell if the associations are getting populated just by calling person.save.

The other tests in this file have stubbed out the adapter methods. Is there a particular adapter method I could stub that would be useful here? Or, can I reuse one of the ones already stubbed?

[dmarcelino]

Hey @tobalsgithub, you are right, 3 tests!

After calling person.save(cb) we can check the person model instance to check if it has the populated records or not.

Yes, you can reuse the ones already stubbed.

[tobalsgithub]

Hey @dmarcelino, that's what I originally thought, but either it doesn't work or I'm not understanding how to set it up.

I added a car collection and associated it with the person collection in order to test populating only the car collection and not the pet collection. But it appears that after adding a couple instances of these models to the person model, regardless of which options are passed into the save function, all associations still exist. From looking at the actual save function, this seems to make sense given that proto is not updated after the final query takes place.

I thought about checking values instead of person, but values never actually gets the associations added to it, I think because of the way we're stubbing things.

All 3 of my tests are setup the same way:

var options;
var person = new collection._model({ id: 4, first_name: 'jane', last_name: 'doe' }, {showJoins: true});

// Update collection      
person.pets.push({type: 'dog'});
person.pets.push({type: 'frog'});
person.pets.push({type: 'log'});

person.cars.push({type: 'truck'});
person.cars.push({type: 'bike'});

But regardless of how I call save, person.pets.length is always 3, and person.cars.length is always 2.

person.save(function (err, values) {
  // person.pets.length = 3
  // person.cars.length = 2
});

person.save({populate: false}, function (err, values) {
  // person.pets.length = 3
  // person.cars.length = 2
});

person.save({populate: [{key: 'cars'}]}, function (err, values) {
  // person.pets.length = 3
  // person.cars.length = 2
});

[dmarcelino]

I thought about checking values instead of person, but values never actually gets the associations added to it, I think because of the way we're stubbing things.

Is it worth using the stubs from https://github.com/balderdashy/waterline/blob/master/test/integration/model/association.add.manyToMany.object.js? In those tests the associations seem to work fine...

[tobalsgithub]

See what you think of this. I'm sort of guessing that the adapter.find method gets called for each populate. Let me know if that's wrong.
@dmarcelino

[tobalsgithub]

Hey @dmarcelino,
Anything you need from me to get this merged in? Looks like there are some merge conflicts. Anything I can do to help with those?

[dmarcelino]

@tobalsgithub, apologies, I've been busy and forgot this PR 😓 😊

Can you please merge the latest master and fix the merge conflicts? Currently it's not possible to merge...

@devinivy, can you also take a look please?

[tobalsgithub]

Looks like it can be merged now. Let me know if there are any issues.
@dmarcelino

[dmarcelino]

I like what you've done with populate!

Finally checked the tests and they looked good to me. ✅

@devinivy will you do the honours?

UPDATE: @tobalsgithub, would you mind putting some docs together in https://github.com/balderdashy/waterline-docs?

[devinivy]

It looks good to me. I do want to make sure that docs get written though!

[dmarcelino]

@devinivy, can the docs be written after the merge?

[devinivy]

Yep– after merge and before publish should be okay :)

[dmarcelino]

Cool! Thanks @tobalsgithub and @nkibler7!

[coveralls]

Changes Unknown when pulling d16a65f on tobalsgithub:save-populate-feature into ** on balderdashy:master**.