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 note to readme to explain DoubleRenderError #108

Merged
merged 1 commit into from
Feb 14, 2014
Merged

Add note to readme to explain DoubleRenderError #108

merged 1 commit into from
Feb 14, 2014

Conversation

michaelem
Copy link
Contributor

Since there have been a few issues opened on the subject I propose to modify the readme to reflect the potential DoubleRenderError that Rails users might encounter using verify_policy_scoped and verify_authorized.

@thomasklemm
Copy link
Collaborator

Hi Michael, thanks for your PR. It's a good addition explaining how the DoubleRenderError came to be and thus differs from #53.

thomasklemm added a commit that referenced this pull request Feb 14, 2014
Add note to readme to explain DoubleRenderError
@thomasklemm thomasklemm merged commit 49eace6 into varvet:master Feb 14, 2014
@jnicklas
Copy link
Collaborator

I still don't understand how you'd get a double render error. The only reasonable explanation I have is that you'd use a rescue_from for all StandardErrors in your ApplicationController. In development. Why would anyone do that? That seems like a horrible development experience and I can't for the life of me understand why you'd want to do that.

@michaelem
Copy link
Contributor Author

@jnicklas I do not have such a catch all. What I do have is a rescue_from Pundit::NotAuthorizedError as recomended in the readme: https://github.com/michaelem/pundit#rescuing-a-denied-authorization-in-rails

(Also: please don't assume the worst of other humans on the internet.)

@jnicklas
Copy link
Collaborator

@michaelem maybe too much OSS has left me jaded, I apologize. I didn't mean any offence though, I was genuinely trying to understand what's going on. I just checked the code and it turns out I was wrong, and it is indeed possible to cause the DoubleRenderError.

I actually didn't remember correctly how we'd defined errors, I'd assumed that we raised a different error when verify_authorized kicked in, but it seems that we raise the same NotAuthorizedError in both cases. So rescue_from kicks in for verify_authorized as well. If it didn't, this problem would go away.

So we have a better solution in raising a different exception on verify_authorized, that way we fix the problem and we don't need any documentation on how to work around it.

@jnicklas
Copy link
Collaborator

I just created #109 which fixes this issue by raising a different error for verify_authorized, more details inside the PR. @michaelem, @thomasklemm what do you guys think?

jnicklas added a commit that referenced this pull request Apr 4, 2014
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