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

confirmation validation error attribute #5942

Merged
merged 4 commits into from Apr 29, 2012

Conversation

Projects
None yet
@bcardarella
Contributor

bcardarella commented Apr 23, 2012

This will render the error message on :#{attribute}_confirmation instead
of on attribute itself. When rendering confirmation errors inline on the
form with form builders such as SimpleForm and Formtastic it is
confusing to the end user to see the confirmation error message on the
attribute element. Instead it makes more sense to have this validation
error render on the confirmation field instead.

The i18n message has been updated for the confirmation validator error
message to include the original attribute name.

confirmation validation error attribute
This will render the error message on :#{attribute}_confirmation instead
of on attribute itself. When rendering confirmation errors inline on the
form with form builders such as SimpleForm and Formtastic it is
confusing to the ender user to see the confirmation error message on the
attribute element. Instead it makes more sense to have this validation
error render on the confirmation field instead.

The i18n message has been updated for the confirmation validator error
message to include the original attribute name.
@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 23, 2012

Contributor

Think about it this way, when I type in my password and it doesn't match the confirmation the error message should render on the password_confirmation field instead of the password field. Every other web app not using Rails does it this way. I believe users expect this behavior and Rails (IMO) does it backwards.

Contributor

bcardarella commented Apr 23, 2012

Think about it this way, when I type in my password and it doesn't match the confirmation the error message should render on the password_confirmation field instead of the password field. Every other web app not using Rails does it this way. I believe users expect this behavior and Rails (IMO) does it backwards.

@shedd

This comment has been minimized.

Show comment
Hide comment
@shedd

shedd commented Apr 23, 2012

+1

@CodeOfficer

This comment has been minimized.

Show comment
Hide comment
@CodeOfficer

CodeOfficer Apr 23, 2012

+1, this makes too much sense

CodeOfficer commented Apr 23, 2012

+1, this makes too much sense

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen Apr 23, 2012

Contributor

+1

Contributor

jamesarosen commented Apr 23, 2012

+1

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 23, 2012

Member

I agree with this. Let we see what the core team have to say.

Member

rafaelfranca commented Apr 23, 2012

I agree with this. Let we see what the core team have to say.

@Aupajo

This comment has been minimized.

Show comment
Hide comment
@Aupajo

Aupajo commented Apr 23, 2012

+1

@coryschires

This comment has been minimized.

Show comment
Hide comment
@coryschires

coryschires commented Apr 24, 2012

+1

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 24, 2012

Contributor

Small update to the changelog in hope that core will accept the PR

Contributor

bcardarella commented Apr 24, 2012

Small update to the changelog in hope that core will accept the PR

@rafaelfranca

View changes

Show outdated Hide outdated activemodel/CHANGELOG.md
@amrnt

This comment has been minimized.

Show comment
Hide comment
@amrnt

amrnt Apr 24, 2012

Contributor

+1

Contributor

amrnt commented Apr 24, 2012

+1

@chloerei

This comment has been minimized.

Show comment
Hide comment
@chloerei

chloerei Apr 24, 2012

Contributor

👍

Contributor

chloerei commented Apr 24, 2012

👍

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 24, 2012

Contributor

I am fine with this change but the reason given (simple form and formtastic use cases) is not enough for such abrupt change of behavior. That said, I would rather add a new option to confirmation's validator rather than changing it by default.

Contributor

josevalim commented Apr 24, 2012

I am fine with this change but the reason given (simple form and formtastic use cases) is not enough for such abrupt change of behavior. That said, I would rather add a new option to confirmation's validator rather than changing it by default.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 24, 2012

Contributor

The SimpleForm and Formtastic use cases are just examples of it manifesting. The current way the error is attaching is basically saying that the attribute doesn't match the confirmation. It should be that the confirmation doesn't match the attribute. (which is the behavior that this patch gives) I am resistant to an option on the validator as I think the proper behavior is as I submitted in the PR.

As far as an abrupt change, this is only meant for the master branch which will be a major version release so I think abrupt changes should be implied.

Contributor

bcardarella commented Apr 24, 2012

The SimpleForm and Formtastic use cases are just examples of it manifesting. The current way the error is attaching is basically saying that the attribute doesn't match the confirmation. It should be that the confirmation doesn't match the attribute. (which is the behavior that this patch gives) I am resistant to an option on the validator as I think the proper behavior is as I submitted in the PR.

As far as an abrupt change, this is only meant for the master branch which will be a major version release so I think abrupt changes should be implied.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 24, 2012

Contributor

In addition, this PR shouldn't introduce any functional changes to the any applications that would be upgrading. The confirmation validator still works just the same, the only difference is which attribute the error message is attached to and the default error message. Any upgrade challenges should be pretty minimal.

Contributor

bcardarella commented Apr 24, 2012

In addition, this PR shouldn't introduce any functional changes to the any applications that would be upgrading. The confirmation validator still works just the same, the only difference is which attribute the error message is attached to and the default error message. Any upgrade challenges should be pretty minimal.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 24, 2012

Contributor

Another issue with this PR (unless I am missing something) is that the attribute does not get translated before sent to I18n, so it will end up showing the attribute name in english when my locale is portuguese.

Contributor

josevalim commented Apr 24, 2012

Another issue with this PR (unless I am missing something) is that the attribute does not get translated before sent to I18n, so it will end up showing the attribute name in english when my locale is portuguese.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 24, 2012

Contributor

@josevalim ah, good point. I can update to cover that case.

Contributor

bcardarella commented Apr 24, 2012

@josevalim ah, good point. I can update to cover that case.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella
Contributor

bcardarella commented Apr 24, 2012

@josevalim done

@abhijeetmisra

This comment has been minimized.

Show comment
Hide comment
@abhijeetmisra

abhijeetmisra Apr 24, 2012

+1 for confirmation

abhijeetmisra commented Apr 24, 2012

+1 for confirmation

@coorasse

This comment has been minimized.

Show comment
Hide comment
@coorasse

coorasse Apr 24, 2012

Contributor

+1

Contributor

coorasse commented Apr 24, 2012

+1

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 25, 2012

Contributor

@josevalim is there any additional feedback on this? I'm happy to tackle any blockers

Contributor

bcardarella commented Apr 25, 2012

@josevalim is there any additional feedback on this? I'm happy to tackle any blockers

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 29, 2012

Member

👍, though this does break behavior. I'm not sure what the policy is about that re:rails 4.

Member

steveklabnik commented Apr 29, 2012

👍, though this does break behavior. I'm not sure what the policy is about that re:rails 4.

@@ -9,7 +9,7 @@ en:
inclusion: "is not included in the list"
exclusion: "is reserved"
invalid: "is invalid"
confirmation: "doesn't match confirmation"
confirmation: "doesn't match %{attribute}"

This comment has been minimized.

@isaacsanders

isaacsanders Apr 29, 2012

Contributor

Is this right? I mean the %{}. Should it be #{}?

@isaacsanders

isaacsanders Apr 29, 2012

Contributor

Is this right? I mean the %{}. Should it be #{}?

This comment has been minimized.

@carlosantoniodasilva
@carlosantoniodasilva

carlosantoniodasilva Apr 29, 2012

Member

Yes it is, the pattern is %{} :)

This comment has been minimized.

@isaacsanders

isaacsanders Apr 29, 2012

Contributor

okay. Just checking :D

@isaacsanders

isaacsanders Apr 29, 2012

Contributor

okay. Just checking :D

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Apr 29, 2012

Member

No problem, always good to have code review ;)

@carlosantoniodasilva

carlosantoniodasilva Apr 29, 2012

Member

No problem, always good to have code review ;)

tenderlove added a commit that referenced this pull request Apr 29, 2012

Merge pull request #5942 from bcardarella/confirmation_error_message_…
…on_confirmation_attribute

confirmation validation error attribute

@tenderlove tenderlove merged commit f975a86 into rails:master Apr 29, 2012

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Apr 30, 2012

Contributor

@tenderlove awesome!

Contributor

bcardarella commented Apr 30, 2012

@tenderlove awesome!

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 30, 2012

Contributor

This change needs to be added to the Upgrade to Rails 4 Guide.

Contributor

josevalim commented Apr 30, 2012

This change needs to be added to the Upgrade to Rails 4 Guide.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella
Contributor

bcardarella commented Apr 30, 2012

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 30, 2012

Contributor

Awesome, tks!

Contributor

josevalim commented Apr 30, 2012

Awesome, tks!

@Dinuz

This comment has been minimized.

Show comment
Hide comment
@Dinuz

Dinuz Sep 5, 2012

Hi everybody,
I don't get it, my rails version is the 3.2.8 (and I saw that this change are implemented in it), but still using the client_side_validation gem (of @bcardarella ), with simple_form and twitter bootstrap, it attach the not matching confirmation error to the password field instead of the confirmation field.

What is wrong?

Dinuz commented Sep 5, 2012

Hi everybody,
I don't get it, my rails version is the 3.2.8 (and I saw that this change are implemented in it), but still using the client_side_validation gem (of @bcardarella ), with simple_form and twitter bootstrap, it attach the not matching confirmation error to the password field instead of the confirmation field.

What is wrong?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 5, 2012

Member

@Dinuz it is only implemented at master branch (4.0.0)

Member

rafaelfranca commented Sep 5, 2012

@Dinuz it is only implemented at master branch (4.0.0)

@Dinuz

This comment has been minimized.

Show comment
Hide comment
@Dinuz

Dinuz Sep 5, 2012

Oh oh....well than that is the issue (but the changes are present in the version 3.2.8., I just checked or I am wrong in something?).

Thanks anyway @rafaelfranca

Dinuz commented Sep 5, 2012

Oh oh....well than that is the issue (but the changes are present in the version 3.2.8., I just checked or I am wrong in something?).

Thanks anyway @rafaelfranca

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Sep 5, 2012

Contributor

@Dinuz the changes are only supposed to be on master which is currently the development branch for Rails 4.0.

Contributor

bcardarella commented Sep 5, 2012

@Dinuz the changes are only supposed to be on master which is currently the development branch for Rails 4.0.

@Dinuz

This comment has been minimized.

Show comment
Hide comment
@Dinuz

Dinuz Sep 5, 2012

Yes Thanks....I saw!!!
I apologize, my mistake I was checking the master branch and not the local files on my machine of the 3.2.8 (I saw the changes, but that were obviously on the wrong release)...

My Mistake.

Dinuz commented Sep 5, 2012

Yes Thanks....I saw!!!
I apologize, my mistake I was checking the master branch and not the local files on my machine of the 3.2.8 (I saw the changes, but that were obviously on the wrong release)...

My Mistake.

rbudiharso pushed a commit to rbudiharso/rails-i18n that referenced this pull request Apr 15, 2014

nanaya added a commit to nanaya/rails that referenced this pull request Aug 23, 2014

senny added a commit that referenced this pull request Aug 27, 2014

Merge pull request #16661 from edogawaconan/doc-fix
Update documentation to match change in #5942 [ci skip]

epidemian added a commit to epidemian/rails-i18n that referenced this pull request Sep 18, 2014

Change confirmation error message for es-* locales
This follows the changes introduced in rails/rails#5942, but does not include the attribute name in the message because it reads weird and it would require adding an article, which in turn would require knowing the grammatical gender of the attribute. E.g., "no coincide con *la* contraseña" or "no coincide con *el* número de cuenta".

trungpham added a commit to trungpham/rails that referenced this pull request Sep 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment