Skip to content
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

Adding current user available to pre/post save hooks #514

Conversation

estilles
Copy link

@estilles estilles commented Aug 6, 2014

This PR makes the current user available to pre and post save hooks.

I do not alter the model per se. I simply add a _user property to the item object just before it is saved in updateHandler, but since mongoose is unaware of it's existence, it does not save it to the database. The property is temporary in nature and does not survive the current transaction.

Here's a usage example.

List.schema.pre('save', function(next, done) {
    console.log('current user =>', this._user);
    next();
});

The idea of having the current user available to pre and post save hooks was originally raised in #458 in the context of using it for audit purposes (which I addressed in #490). However, having the current user available in pre and post save hooks is also quite useful in use cases other than this, which is why I decided to submit this as a separate pull request.

@JedWatson
Copy link
Member

This is pretty much exactly how I was planning to do this, nice work.

I can't remember why I hadn't added it yet, I think I wanted to (either) write tests first, or do some testing to make it really clear in which situations this will and won't work...

For example this._user won't be available in the following context:

new Thing.model({ name: "My New Thing" }).save();

... which is a bit of a trap, it means if you don't consistently use the UpdateHandler, or explicitly set _user when you're not, middleware won't necessarily work as expected.

I do think it's better to have this even with that limitation, I just wanted to make sure it was well tested and understood :)

Also, what do you think of calling it __user? or possibly even something like _req_user which is more explicit and probably never going to potentially conflict with actual schema paths.

@SharadKumar
Copy link

This is nice - I can use this on my project. It further sits well with #490
Would love to see a merge. ;-)

@estilles
Copy link
Author

estilles commented Aug 6, 2014

@JedWatson, you're absolutely correct. I should have included the caveat that it will not work in circumstances where UpdateHandler is not used.

I see your point about a potential schema conflict. I'll rename it to _req_user, as per your suggestion.

Let me work on this a bit. I was to include some tests as well.

@estilles
Copy link
Author

estilles commented Aug 6, 2014

@SharadKumar, thanks for the comments. The audit meta is something I've been using with Keystone since I started using it last year. I think it was one of the first mods I made. #490 is a slightly modifed/more testable version of my original code. Sorry I didn't share it with everyone until now.

JedWatson added a commit that referenced this pull request Aug 6, 2014
…er-in-pre-post-save

Adding current user available to pre/post save hooks
@JedWatson JedWatson merged commit eb581ed into keystonejs:master Aug 6, 2014
@JedWatson
Copy link
Member

Merged as is to unblock some work, will rename to _req_user in a moment. If you could do some tests that would be great, as a new PR though :)

JedWatson added a commit that referenced this pull request Aug 6, 2014
@estilles
Copy link
Author

estilles commented Aug 6, 2014

No problem. Will work on some tests tonight and submit a new PR.

@estilles estilles deleted the feature/adding-current-user-in-pre-post-save branch August 6, 2014 12:28
@alisajidcs
Copy link

This feature should be in pre/post remove too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants