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

onLoggedIn - data.extra (coming soon) - when? #194

Closed
stevenhornung opened this issue Mar 11, 2016 · 6 comments
Closed

onLoggedIn - data.extra (coming soon) - when? #194

stevenhornung opened this issue Mar 11, 2016 · 6 comments

Comments

@stevenhornung
Copy link
Contributor

You mentioned in the README that "Any returned data will be added to the response body as data.extra" for the onLoggedIn hook. Any idea when this would be implemented? I need to append some data to the response and this would be perfect (since I can't directly override the route without renaming the route and coping most of the logic which seems silly). Just trying to get a timeframe.

Thanks!

@kahmali
Copy link
Owner

kahmali commented Mar 12, 2016

I'm just waiting for tests to be added for PR #168 to verify the functionality and then I can pull it in. If you want to add the tests in for it I'll update the docs and release a new version. I'll send a reminder to the developer that submitted the PR. Otherwise just grab a fork of their package and use that if you need for now. Take a look at that PR for more info.

@kahmali
Copy link
Owner

kahmali commented Mar 14, 2016

This was released in v0.8.9 (hot off the presses). Please let me know if that works as expected for you.

@stevenhornung
Copy link
Contributor Author

@kahmali awesome! I actually just overrode the methods with new names and implemented the hooks myself (and was going to do a PR for the original login/logout routes until I saw this :) ). This does do as I'd expect, but it looks like the onLoggedOut hook wasn't also implemented? It should be the exact same logic as the onLoggedIn addition made.

@kahmali
Copy link
Owner

kahmali commented Mar 14, 2016

Yup. If you want to submit a PR for that I'll gladly accept it. Just please add the accompanying tests in with it. And please add the tests in a separate unit test, not in with the others as the first tests for the extra field were done. Bonus points if you move those other tests for the onLoggedIn hook's data.extra field to a separate unit test.

Thanks again for the request! I'm going to close this issue since it was only a request for the onLoggedIn hook, and that appears to be all set.

@kahmali kahmali closed this as completed Mar 14, 2016
@stevenhornung
Copy link
Contributor Author

Yeah I'll do that when I have a chance. The unit tests are pretty straight-forward looking. Maybe expect a PR with that implemented and the unit tests in the next couple of days.

Thanks for the great package!

@stevenhornung
Copy link
Contributor Author

@kahmali just submitted that PR. Let me know if you have any issues with it but pretty straight forward, I think.

If you don't mind pinging me when you release the update to Atmosphere, that would be awesome :). I can then update and remove my override routes.

Thanks!

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

No branches or pull requests

2 participants