[fixes #1444] Model.findOrCreate() should send back an object without {fetch: true} meta #1445

Merged
merged 5 commits into from Feb 10, 2017

Conversation

5 participants
@mrded
Contributor

mrded commented Feb 7, 2017

Waterline version: 0.13.0-3
Node version: v7.5.0
NPM version: 4.1.2
Operating system: macOS 10.12.3 (16D32)


Hello,

I'm trying to use latest version of waterline with SailsJS 1.0.0-22, and found a problem with Model.findOrCreate(). It behaves the same as Model.create() rather than Model.find(), i.e it requires {fetch: true} meta to return a created/found object.

Model.findOrCreate({ type: 'dog' }, { name: 'Pretend pet type', type: 'dog' }).then(console.log);
// undefined

Model.findOrCreate({ type: 'dog' }, { name: 'Pretend pet type', type: 'dog' }).meta({fetch: true}).then(console.log);
// { id: 1, name: 'Pretend pet type', type: 'dog' }
@sailsbot

This comment has been minimized.

Show comment
Hide comment
@sailsbot

sailsbot Feb 7, 2017

Hi @mrded! It looks like your pull request title doesn’t quite conform to our guidelines. Please edit the title so that it starts with [proposal], [patch], [fixes #], or [implements #]. Once you've fixed it, post a comment below (e.g. "ok, fixed!") and we'll take a look!

sailsbot commented Feb 7, 2017

Hi @mrded! It looks like your pull request title doesn’t quite conform to our guidelines. Please edit the title so that it starts with [proposal], [patch], [fixes #], or [implements #]. Once you've fixed it, post a comment below (e.g. "ok, fixed!") and we'll take a look!

@mrded mrded changed the title from With Model.findOrCreate() send back an object without {fetch: true} meta to [fixes #1444] Model.findOrCreate() should send back an object without {fetch: true} meta Feb 7, 2017

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 7, 2017

Contributor

ok, fixed!

Contributor

mrded commented Feb 7, 2017

ok, fixed!

@sailsbot sailsbot removed the Needs cleanup label Feb 7, 2017

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 7, 2017

Contributor

That's weird, because tests pass on my local env.

Contributor

mrded commented Feb 7, 2017

That's weird, because tests pass on my local env.

@gil0mendes

This comment has been minimized.

Show comment
Hide comment
@gil0mendes

gil0mendes Feb 8, 2017

That is because the Object.assign isn't available in the older version of Node.js

That is because the Object.assign isn't available in the older version of Node.js

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 8, 2017

Contributor

Oh right, I'll change it to something else. Thanks!

Contributor

mrded commented Feb 8, 2017

Oh right, I'll change it to something else. Thanks!

@gil0mendes

This comment has been minimized.

Show comment
Hide comment
@gil0mendes

gil0mendes Feb 8, 2017

I think that is time to drop some version, but is just my opinion :)

I think that is time to drop some version, but is just my opinion :)

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 8, 2017

Contributor

New node for new sails ;)

Contributor

mrded commented Feb 8, 2017

New node for new sails ;)

@particlebanana

This comment has been minimized.

Show comment
Hide comment
@particlebanana

particlebanana Feb 8, 2017

Contributor

@mrded good catch. We changed the behavior of create mid-stream and I forgot about the findOrCreate method.

Contributor

particlebanana commented Feb 8, 2017

@mrded good catch. We changed the behavior of create mid-stream and I forgot about the findOrCreate method.

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 8, 2017

Contributor

Looks like you have changed it already.

Contributor

mrded commented Feb 8, 2017

Looks like you have changed it already.

mrded added some commits Feb 8, 2017

Merge https://github.com/balderdashy/waterline
# Conflicts:
#	lib/waterline/methods/find-or-create.js
@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Feb 8, 2017

Member

@mrded crap sorry about that, bad timing-- I just saw this. For the purposes of tracking this for the future, we could still get this merged if you'd like

Member

mikermcneil commented Feb 8, 2017

@mrded crap sorry about that, bad timing-- I just saw this. For the purposes of tracking this for the future, we could still get this merged if you'd like

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 8, 2017

Contributor

It's ok. I would like to get it merged, let me solve conflicts first :)

Contributor

mrded commented Feb 8, 2017

It's ok. I would like to get it merged, let me solve conflicts first :)

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Feb 8, 2017

Member

@mrded and thanks for bringing it to our attention!! Only way I even knew about this was because @particlebanana said it out loud- I just didn't realize there was already a PR

Member

mikermcneil commented Feb 8, 2017

@mrded and thanks for bringing it to our attention!! Only way I even knew about this was because @particlebanana said it out loud- I just didn't realize there was already a PR

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 8, 2017

Contributor

@mikermcneil can you please check, what's a problem with tests?

Contributor

mrded commented Feb 8, 2017

@mikermcneil can you please check, what's a problem with tests?

mikermcneil referenced this pull request Feb 8, 2017

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 10, 2017

Contributor

Will it be possible to merge it now? Otherwise it will never be merged :(

Thanks

Contributor

mrded commented Feb 10, 2017

Will it be possible to merge it now? Otherwise it will never be merged :(

Thanks

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Feb 10, 2017

Member

@mrded totally-- I'm in there right now plugging in the optimized Deferred (parley) and I was actually just thinking about this and potential conflicts. Thanks for the reminder!

Member

mikermcneil commented Feb 10, 2017

@mrded totally-- I'm in there right now plugging in the optimized Deferred (parley) and I was actually just thinking about this and potential conflicts. Thanks for the reminder!

@mikermcneil mikermcneil merged commit 674c567 into balderdashy:master Feb 10, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mikermcneil added a commit that referenced this pull request Feb 10, 2017

@mrded

This comment has been minimized.

Show comment
Hide comment
@mrded

mrded Feb 10, 2017

Contributor

@mikermcneil thank you for merging. Unfortunately you can see none of my changes in here https://github.com/balderdashy/waterline/commits/master/lib/waterline/methods/find-or-create.js

Looks like you merged my changes as empty commit. Anyway, problem is solved, and it's too late to complain.

Thanks!

Contributor

mrded commented Feb 10, 2017

@mikermcneil thank you for merging. Unfortunately you can see none of my changes in here https://github.com/balderdashy/waterline/commits/master/lib/waterline/methods/find-or-create.js

Looks like you merged my changes as empty commit. Anyway, problem is solved, and it's too late to complain.

Thanks!

@mikermcneil

This comment has been minimized.

Show comment
Hide comment
@mikermcneil

mikermcneil Feb 12, 2017

Member

@mrded hmm weird... I definitely merged this branch locally, fixed the conflict and then committed-- so maybe the commits aren't included in the commit history of the master branch because there was a merge conflict? Git is a source of endless mysteries. But regardless, it's in place now.

Thanks again for your help!

Member

mikermcneil commented Feb 12, 2017

@mrded hmm weird... I definitely merged this branch locally, fixed the conflict and then committed-- so maybe the commits aren't included in the commit history of the master branch because there was a merge conflict? Git is a source of endless mysteries. But regardless, it's in place now.

Thanks again for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment