-
Notifications
You must be signed in to change notification settings - Fork 897
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
Prefix the method name with the class name for validation errors #17754
Conversation
LGTM, even the test failures show who this affects and they all seem like legitimate things to change in the tests. |
cc @mzazrivec |
e385a95
to
c70bf90
Compare
@@ -23,4 +23,8 @@ class ApplicationRecord < ActiveRecord::Base | |||
def self.display_name(number = 1) | |||
n_(model_name.singular.titleize, model_name.plural.titleize, number) | |||
end | |||
|
|||
def self.human_attribute_name(attribute, *args) | |||
"#{name}: #{super}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be display_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. @jrafanie and I talked about it briefly and we're leaning toward no, but ¯\_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, that seems to be masking the real model name failing validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display_name
would get you a nice looking model name, but at the same time the nice looking model name would be translated into current locale (shrug).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, 👎 to using display_name
, I don't think this should be translated.
@@ -0,0 +1,13 @@ | |||
describe ApplicationRecord do | |||
context ".human_attribute_name puts the class name in the validation error" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well reword this to not upset the puts
checker 😆
instance.errors.full_messages will now produce something like: ["SomeClass: The method can't be blank"]
c70bf90
to
8e53499
Compare
Checked commit bdunne@8e53499 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lean towards not localizing these alerts.
There's only one way to find out. We get burned by indirect validation failures often enough that we should just fix anything that this change affects. |
ManageIQ/manageiq#17754 added a prefixed model name in a number of exceptions which is causing a few specs to fail which were matching on the raised exception.
instance.errors.full_messages will now produce something like:
["SomeClass: The attribute can't be blank"]
Full disclosure... I have no idea what else this will effect (especially in the UI) ¯\_(ツ)_/¯