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

Add _req_user to removed item so you can use this in the remove pre/p… #4831

Merged
merged 1 commit into from
Apr 5, 2019
Merged

Add _req_user to removed item so you can use this in the remove pre/p… #4831

merged 1 commit into from
Apr 5, 2019

Conversation

laurenskling
Copy link
Contributor

…ost hook

As discussed in keystonejs/keystone#1857 the remove pre/post hooks are "missing" the _req_user. I, amongst others, am using this object to handle some permissions. It's available in the updateHandler (in create and update) but missing in remove.

it's added on the updateHandler here: https://github.com/keystonejs/keystone/blob/d34f45662eb359e2cb18b397f2ffea21f9883141/lib/list/updateItem.js#L157

Remove doesn't use the updateHandler, so this is missing.

Description of changes

Related issues (if any)

Testing

  • List browser version(s) any admin UI changes were tested in:
  • Please confirm you've added (or verified) test coverage for this change.
  • Please confirm npm run test-all ran successfully.

…ost hook

As discussed in keystonejs/keystone#1857 the remove pre/post hooks are "missing" the _req_user. I, amongst others, am using this object to handle some permissions. It's available in the updateHandler (in `create` and `update`) but missing in `remove`.

it's added on the updateHandler here: https://github.com/keystonejs/keystone/blob/d34f45662eb359e2cb18b397f2ffea21f9883141/lib/list/updateItem.js#L157

Remove doesn't use the updateHandler, so this is missing.
@autoboxer
Copy link
Contributor

this seems like a worthwhile addition, and would be consistent with the other hooks. My concern is that a larger change seemed pending in #3187, but discussion died. I'd like to think this change is independent of #3187, and to prevent a breaking change, both options should live side by side, however, I'd feel more comfortable if @dominikwilkowski or @JedWatson weighed in before continuing.

@laurenskling
Copy link
Contributor Author

Would be great if we could get this PR sorted. I am sure many like me are awaiting this value to settle permissions on deleting records. A simple line of code, endless possibilities :D

@JedWatson JedWatson merged commit 7cfc752 into keystonejs:master Apr 5, 2019
@JedWatson
Copy link
Member

Thanks @laurenskling.

Sorry this went unmerged for so long, will push a new release with the fix this weekend.

@laurenskling
Copy link
Contributor Author

Awesome @JedWatson thanks so much

@laurenskling laurenskling deleted the patch-3 branch April 5, 2019 15:17
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.

3 participants