Undestroy method for paranoid models #2540
Conversation
| var identifier; | ||
|
|
||
| if (!self.Model._timestampAttributes.deletedAt) { | ||
| return Promise.reject(new Error("Model is not paranoid")); |
There was a problem hiding this comment.
I think we should fail early if deletedAt is not define (before hooks)
|
On of the tests is failing under pg. Other than that, just some small comments, please ask if you have problems with promise style in tests |
|
Might |
|
@janmeier Do those promise tests look right? Don't have much experience with them. |
|
@mickhansen Agreed |
|
@janmeier Is it better to return a Promise.reject or throw a error synchronously? |
|
@seth-admittedly usually they prefer an error throw :) +1, thanks for the PR! |
| * @return {Promise<undefined>} | ||
| */ | ||
| Instance.prototype.restore = function(options) { | ||
| if (!this.Model._timestampAttributes.deletedAt) return Promise.reject(new Error("Model is not paranoid")); |
There was a problem hiding this comment.
Usually they prefer an error throw since you should always call you methods right.
An error should be triggered into the promise if isn't at your handling scope. :)
|
@seth-admittedly I'd prefer promise tests that are not so nested. So rather this: something.then(function () {
return somethingelse()
}).then(function () {
return anotherPromise();
})Than something.then(function () {
return somethingelse().then(function () {
return anotherPromise();
})
})But it's fine for now :) |
|
As @cusspvz we usually just throw the error synchronously, since the error is not because of a DB error or something other async thing, but because of a programming error from the user. You can also change the assertion to expect(self.User.restore({where : {username : "Peter"}})).to.throw(Error, 'Model is not paranoid')To avoid the awkvard expect false to be true in the succes handler |
|
The only way I could get the throw to test correct was with the kludgey looking try/catch. Otherwise the throw seemed to interfere with the promise chain being returned correctly. I couldn't find an example of a promisey throw test, suggestions on how to make it better are very welcome. |
| return this.User.create({username : "Peter", secretValue : "42"}) | ||
| .then(function(user){ | ||
| try{ | ||
| expect(self.User.destroy({where : {secretValue : "42"}})).to.throw(Error, "Model is not paranoid"); |
There was a problem hiding this comment.
For some reason you have to wrap it in a function - still not pretty, but better
return this.User.create({username : "Peter", secretValue : "42"})
.then(function(user){
expect(function () {
self.User.restore({where : {secretValue : "42"}})
}).to.throw(Error, "Model is not paranoid");
})
|
Thanks for the typo catch and the function wrapper suggestion, which makes sense once you see it. |
Undestroy method for paranoid models
|
Good work as always @seth-admittedly ! |
Undestroy allows for restoring destroyed paranoid models. Mirrors the syntax of destroy, with both model (bulk) and instance options.
Implementing this without resorting to manual SQL required tweaking the query generator to allow for omitNull to be passed as an option with an individual query.