Fix issue in README for multiple error messages #638
Conversation
README.md
Outdated
| flash[:error] = message, scope: "pundit", default: :default | ||
| message = | ||
| if e.reason | ||
| I18n.t("pundit.errors.#{e.reason}", scope: "pundit", default: :default) |
There was a problem hiding this comment.
Is scope required? Isn't it covered by starting the key with pundit?
Is the default: an essential part of this approach? It seems a bit odd to specify a default and then to also provide a default in the else clause? I wonder if we can just remove this and go back to the ternary if?
There was a problem hiding this comment.
Correct, I missed it. Just tested right now
There was a problem hiding this comment.
I agree about this if, fixed it and tested
holyketzer
force-pushed
the
fix-637
branch
2 times, most recently
from
f3c0c15
to
a31ae8e
Compare
README.md
Outdated
| @@ -549,7 +549,7 @@ class ProjectPolicy < ApplicationPolicy | |||
| def create? | |||
| if user.has_paid_subscription? | |||
| if user.project_limit_reached? | |||
| raise Pundit::NotAuthorizedError, reason: 'user.project_limit_reached' | |||
| raise Pundit::NotAuthorizedError, reason: 'user.project_limit_reached', record: user, query: :create | |||
There was a problem hiding this comment.
Do we need this record/query? It seems like they're not used - just reason.
There was a problem hiding this comment.
I suppose yes, because in this issue #637 it is what exactly happened when no corresponding reason is found in I18n then user sees default message, and to show it correctly Pundit is required record and query else it will look like not allowed to this NilClass
There was a problem hiding this comment.
Sure, but why would we want to display a default message in this case?
If there's no corresponding reason in I18n, then in my opinion it should fail: If I set a reason, then I'm saying "I want a specific message to be displayed - not the default message", so if it's not possible to display a specific message (because the user forgot to add a key in the locales) then the behaviour should be to fail, not to cover up the fact that it failed.
I'm open to the idea that there's some valid use-case where a reason is specified, but the corresponding locale text isn't specified, but I can't think of one: can you?
I think the controller logic you had before was reasonable:
"If there's a reason set on the error, then look it up in I18n, otherwise show the message from the error"
There was a problem hiding this comment.
@dgmstuart It's reasonable, I agree.
Fixed
| @@ -563,8 +563,7 @@ end | |||
| Then you can get this error message in exception handler: | |||
| ```ruby | |||
| rescue_from Pundit::NotAuthorizedError do |e| | |||
| message = e.reason ? I18n.t("pundit.errors.#{e.reason}") : e.message | |||
| flash[:error] = message, scope: "pundit", default: :default | |||
| flash[:error] = e.reason ? I18n.t("errors.#{e.reason}", scope: "pundit") : e.message | |||
There was a problem hiding this comment.
since "errors.#{nil}" == "errors.", I think you can drop the ternary altogether
| flash[:error] = e.reason ? I18n.t("errors.#{e.reason}", scope: "pundit") : e.message | |
| flash[:error] = I18n.t("errors.#{e.reason}", scope: "pundit", default: e.message) |
|
We've decided to take a different approach. #684 reverts the change that this PR is based on. |
Closes #637