Downcase authentication keys and humanize error message

[grantzau]

To fix #4095

[feliperenan]

Could we add tests for it? 🤔

[grantzau]

Sorry, created the pull request a bit to early!

[grantzau]

Edited the existing tests. They check, that the downcasing are done. There's not really a test to check the added humanize method though, not sure, if that's necessary?

[feliperenan]

I would add tests to ensure that it's the expected behavior.

[feliperenan]

@grantzau What do you think about squash the commits? All of them is about the same change - downcase authentication keys and humanize error message, so for me, just one commit is enough.

Here a great blog post about it: https://blog.carbonfive.com/2017/08/28/always-squash-and-rebase-your-git-commits/

[grantzau]

Great! I will look at it and squash it during the weekend. Was editing directly on Github from the browser, which wasn't a good idea ..! fre. 6. apr. 2018 kl. 03.22 skrev Felipe Renan <notifications@github.com>:

…

@grantzau <https://github.com/grantzau> What do you think about squash the commits? All of them is about the same change - downcase authentication keys and humanize error message, so for me, just one commit is enough. Here a greate blog post about it: https://blog.carbonfive.com/2017/08/28/always-squash-and-rebase-your-git-commits/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWVadtJRbJSR8Tcj5rQuHw3mir16bM5ks5tlsM0gaJpZM4TJIQ7> .

[grantzau]

Added tests and rebased commits.

[tegon]

I'm not sure about this one, I think it should have stayed as it was before. This is a custom message defined here: https://github.com/plataformatec/devise/blob/88724e10adaf9ffd1d8dbfbaadda2b9d40de756a/test/support/locale/en.yml#L5 and we're passing the user's name Steve in a capitalized form here: https://github.com/grantzau/devise/blob/cca5a7b8e9f699de4299e001d95e3e269470f47f/test/failure_app_test.rb#L27, which gets lost when we call #humanize

[3] pry(main)> "User Steve does not exist".humanize
=> "User steve does not exist"

I'm not sure what's the best solution here. Maybe we should only call #humanize when the message is :invalid 🤔

[smoyte]

Note that you can control i18n of parameters to failure messages by doing something like this:

In your models:

module People
  module Users
    # Overriding the i18n_options method in the Devise FailureApp so that we can have control
    # over how the `authentication_keys` parameter to the messages under devise.failure is translated.
    # By default the `human_attribute_name` of the attribute is used, but those are typically capitalized,
    # while the messages call for a lowercase value.
    # This custom failure app must be activated in the config/initializers/devise.rb file.
    class CustomDeviseFailureApp < Devise::FailureApp
      def i18n_options(options)
        options.merge(authentication_keys: I18n.t("devise.authentication_keys.email"))
      end
    end
  end
end

and then in config/initializers/devise.rb:

  config.warden do |manager|
    manager.failure_app = People::Users::CustomDeviseFailureApp
  end

and then in your devise.yml file:

  devise:
    ...
    authentication_keys:
      email: "email"

Ideally Devise would support an optional authentication_keys key in the YML file out of the box, so you could have control over these params without a custom failure app, but this works right now and is not particularly hacky. There is support for this method in the tests even, see here.

Hope this helps!

[tegon]

@grantzau are you still going to work on this?

I'm asking to know if I should assign it to someone else.