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

Override reform full_messages to use ActiveModel full_messages #85

Merged
merged 3 commits into from
Nov 16, 2020
Merged

Override reform full_messages to use ActiveModel full_messages #85

merged 3 commits into from
Nov 16, 2020

Conversation

marcelolx
Copy link
Contributor

@marcelolx marcelolx commented Oct 19, 2020

Reform has its own full_messages implementation as we can see here
https://github.com/trailblazer/reform/blob/master/lib/reform/errors.rb#L29
and reform-rails ResultErrors class inherits from
Reform::Contract::Result::Errors, so when you call "full_messages" to
get the full error messages, it was calling reform full_messages
implementation instead of delegating to ActiveModel and this is a
problem for whom need to translate attribute names, because reform only
humanizes the attribute name.

I've do some "hacks" to get the full error messages of collections/nested properties of the form, if there is another better way to get the full_messages from AM, I'm open to the possibilities.

Fixes: #56
Fixes: #83

If I'm missing anything, please let me know.

Reform has its own full_messages implementation as we can see here
https://github.com/trailblazer/reform/blob/master/lib/reform/errors.rb#L29
and reform-rails ResultErrors class inherits from
Reform::Contract::Result::Errors, so when you call "full_messages" to
get the full error messages, it was calling reform full_messages
implementation instead of delegating to ActiveModel and this is a
problem for whom need to translate attribute names, because reform only
humanizes the attribute name.

Fixes: #56
We iterate over each field and for fields which is value is a twin::collection we get the instance variable of @amv_errors and call full_messages which will return the full translated messages of the collection fields.
@marcelolx marcelolx marked this pull request as ready for review October 21, 2020 13:06
@marcelolx
Copy link
Contributor Author

@apotonick @emaglio Could someone of you take a look on this?

@apotonick
Copy link
Member

Wow! 😍 This looks very good!

Can you remind me what the "original" Reform-version of full_messages did?

@marcelolx
Copy link
Contributor Author

Wow! 😍 This looks very good!

Can you remind me what the "original" Reform-version of full_messages did?

The original behavior was that the reform full_messages was called instead delegated to ActiveModel full_messages. With the changes I did here, reform-rails full_messages has the "same" behavior it has before, the only difference is that the call is delegated to ActiveModel which give us the ability to get the messages with the field names translated.

Example, rails project ->

en.yml

activemodel:
    attributes:
      my_form:
          name: User name

my_form.rb

  validates :name, presence: true

The original behavior would return the following error message Name can't be blank and this is because reform full_messages implementation only humanizes the field name while delegating the call to activemodel will return the following message User name can't be blank, and that is what we would expect when using reform-rails.

@valscion
Copy link

Anything I could do to help this PR advance? It would be nice to be able to update the version of reform and reform-rails used in our application but without this bug fix, we won't be able to do that. See #83 (comment)

@emaglio
Copy link
Member

emaglio commented Nov 16, 2020

@marcelolx thanks for this! @valscion want to add some small extra specs but I'm going to release a new version for this gem between today and tomorrow 👍

@emaglio emaglio merged commit d891c5b into trailblazer:master Nov 16, 2020
@emaglio
Copy link
Member

emaglio commented Nov 16, 2020

v0.2.1 released please test 👍 - thanks again for this

@valscion
Copy link

Thanks @emaglio! I can verify that v0.2.1 fixed my issue and was now able to update reform-rails v0.1.7 -> v0.2.1 and reform v2.2.4 -> 2.3.3 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants