jeremyevans / rodauth

Ruby's Most Advanced Authentication Framework
http://rodauth.jeremyevans.net
MIT License
1.69k stars 95 forks source link

Set flash notifications if message is not nil #370

Closed igor-alexandrov closed 10 months ago

igor-alexandrov commented 11 months ago

Currently, there is no way to disable flash notifications completely. The example below will still set flash keys with nil message. I added a check for nil in methods that set flash notifications. If a message is not present, flash will not be set.

# frozen_string_literal: true

class RodauthImpersonate < Rodauth::Rails::Auth
  configure do
    enable :login, :logout

    prefix '/impersonate'

    session_key_prefix 'impersonate_'

    logout_redirect '/'
    logout_notice_flash nil

    login_redirect nil
    login_route nil
    login_notice_flash nil
  end

  def clear_session
    session.keys.each do |key|
      session.delete(key) if key.start_with?(session_key_prefix)
    end
  end
end
jeremyevans commented 11 months ago

I'm OK with this, but you'll need to not make unnecessary whitespace notifications, and you'll need to add covering tests (Rodauth requires 100% line and branch coverage). That's going to be at least four separate tests (or test assertions), one for each of the cases where you are adding a conditional.

igor-alexandrov commented 11 months ago

@jeremyevans sure, will do! I have some problems running tests locally. How it is better to show them to you?

jeremyevans commented 11 months ago

You can post the issues you are having with running the tests here. Note that the repository has CI, so if you can't get the tests working locally, you can use the CI to check.

For making sure the tests are covering, temporarily add raise unless message in each of the four methods where you add the conditional, and write tests that will hit that and raise the RuntimeError. Then you can remove the code and have the test check that no flash is set (not even an empty flash).

jeremyevans commented 10 months ago

This has been open for over a month without activity. I'm going to close this now. If you would still like this in Rodauth, please push a commit that adds specs and I can reopen. While I'm not against this approach, the current behavior of setting a nil flash message doesn't seem problematic, because most applications are going to do something like flash['error'], and a missing versus nil value will be the same in that case.