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

already_authenticated should set flash notice instead of flash alert #5302

Open
ryenski opened this issue Oct 19, 2020 · 4 comments · May be fixed by #5398
Open

already_authenticated should set flash notice instead of flash alert #5302

ryenski opened this issue Oct 19, 2020 · 4 comments · May be fixed by #5398

Comments

@ryenski
Copy link

ryenski commented Oct 19, 2020

if authenticated && resource = warden.user(resource_name)
set_flash_message(:alert, 'already_authenticated', scope: 'devise.failure')
redirect_to after_sign_in_path_for(resource)
end

This sets the flash[:alert], which in most apps is an error message. This is also evidenced by the already_authenticated key being set in the devise.failure scope.

In most apps, failure or alert messages are displayed with red text indicating that something went wrong. In my opinion this feels strange, because if you're already signed in, nothing went wrong. In fact, nothing is wrong, you just tried to sign in again unnecessarily.

It seems like it would be trivial to move the already_authenticated message from devise.failure to devise.sessions and set the flash key to notice instead of alert.

Would you be open to a change like this? If so, I can submit a PR.

@johansenja
Copy link

I think it would be neat to have an option to skip showing the message entirely, because for me this doesn't add that much to the UX (most users probably wouldn't care if they're already signed in, but just want to use the app). Equally open to submitting a quick PR to enable this

@timhaines
Copy link

@johansenja that makes a lot of sense to me. This alert is very odd.

@ryenski
Copy link
Author

ryenski commented Aug 23, 2021

I have introduced PR #5398 to change the flash message from 'alert' to 'notice'. Do you think this is the best way to do this? Should the flash level be configurable instead of hard-coded? Your feedback is requested.

@ryenski
Copy link
Author

ryenski commented Aug 23, 2021

I think it would be neat to have an option to skip showing the message entirely, because for me this doesn't add that much to the UX (most users probably wouldn't care if they're already signed in, but just want to use the app). Equally open to submitting a quick PR to enable this

@johansenja I agree it doesn't add much to the UI. Failing silently would be a good option. It seems like adding an option for this would be pretty heavy, since it would require:

  • adding a mattr_accessor in lib/devise.rb
  • adding documentation in the README
  • adding a line in the template for config/initializers/devise.rb
  • adding tests

Maybe there is an easier way? Looking at the set_flash_message method, it looks like it should be possible to simply blank out the already_authenticated key in devise.en.yml, since it seems to only set the flash if the message is present:

def set_flash_message(key, kind, options = {})
message = find_message(kind, options)
if options[:now]
flash.now[key] = message if message.present?
else
flash[key] = message if message.present?
end
end

However in my very quick experiment doing this resulted in a "translation missing" error.

Do you think it's still worth adding a config option for this?

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

Successfully merging a pull request may close this issue.

3 participants