New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to retreive deleted paranoid models in queries #2083

Merged
merged 2 commits into from Jul 25, 2014

Conversation

4 participants
@ryanpon
Contributor

ryanpon commented Jul 24, 2014

Implementation as suggested in #1862

@janmeier

This comment has been minimized.

Member

janmeier commented Jul 24, 2014

I'm pretty sure your implementation already supports it, but could you add a test to ensure that it works for includes as well?

@@ -629,6 +629,7 @@ module.exports = (function() {
* @param {Object} [options] A hash of options to describe the scope of the search
* @param {Object} [options.where] A hash of attributes to describe your search. See above for examples.
* @param {Array<String>} [options.attributes] A list of the attributes that you want to select. To rename an attribute, you can pass an array, with two elements - the first is the name of the attribute in the DB (or some kind of expression such as `Sequelize.literal`, `Sequelize.fn` and so on), and the second is the name you want the attribute to have in the returned instance
* @param {Boolean} [options.paranoid] If false, will include columns which have a non-null deletedAt column.

This comment has been minimized.

@janmeier

janmeier Jul 24, 2014

Member

Remember to add default value (=true). Perhaps is should also just say "both existing and deleted records"

This comment has been minimized.

@ryanpon

ryanpon Jul 24, 2014

Contributor

It's slightly misleading since the default is undefined and it's comparing against false to see if they set the flag. Still ok to do default as true?

This comment has been minimized.

@mickhansen

mickhansen Jul 24, 2014

Contributor

@ryanpon the behaviour is still as if it was true. So for the sake of the user it's best to tell them that this is turned on by default :)

@janmeier

This comment has been minimized.

Member

janmeier commented Jul 24, 2014

Something like

Model.find({
  include: [
    {model: RelatedModel, paranoid: false }
  ]
});

Should only return model instances that are not deleted, but all related instances of relatedModel, both deleted and not

@ryanpon

This comment has been minimized.

Contributor

ryanpon commented Jul 24, 2014

Sure, I'll add the tests for includes as well.

{username: 'Max'}
]);
})
.success(function() { return User.find(1) })

This comment has been minimized.

@mickhansen

mickhansen Jul 24, 2014

Contributor

could you change all these success to then?
then is the correct method, success is just our BC method.

This comment has been minimized.

@ryanpon

ryanpon Jul 24, 2014

Contributor

Good to know. I use Q promises normally, and I assumed that .success was just part of the bluebird api.

This comment has been minimized.

@mickhansen

mickhansen Jul 24, 2014

Contributor

Nope :) .success is just a backwards compat remnant from the event emitter pattern that we switched away from here on 2.0/master

@ryanpon

This comment has been minimized.

Contributor

ryanpon commented Jul 24, 2014

Added and revised tests, added the default value as requested.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jul 25, 2014

Beautiful. Thank you.

mickhansen added a commit that referenced this pull request Jul 25, 2014

Merge pull request #2083 from meetearnest/find-paranoid
Add flag to retreive deleted paranoid models in queries

@mickhansen mickhansen merged commit 052b5f1 into sequelize:master Jul 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@cusspvz

This comment has been minimized.

cusspvz commented Jul 30, 2014

@brunocasanova take a peek at this!

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