lynndylanhurley / devise_token_auth

Token based authentication for Rails JSON APIs. Designed to work with jToker and ng-token-auth.
Do What The F*ck You Want To Public License
3.52k stars 1.14k forks source link

Refactoring : Confirmations controller - useless `if redirect_url` condition #1611

Open newfylox opened 9 months ago

newfylox commented 9 months ago

Version: 1.2.2

in this section https://github.com/lynndylanhurley/devise_token_auth/blob/eeed6422a025813010ac1dcb922661d05266e36e/app/controllers/devise_token_auth/confirmations_controller.rb#L29-L33 I was wondering why we are checking if redirect_url because it's necessary of having DeviseTokenAuth.default_confirm_success_url or params[:redirect_url] set.

In other words, imagine having this method returning nil https://github.com/lynndylanhurley/devise_token_auth/blob/eeed6422a025813010ac1dcb922661d05266e36e/app/controllers/devise_token_auth/confirmations_controller.rb#L82-L87 It means that the sign_up flow would failed in this section https://github.com/lynndylanhurley/devise_token_auth/blob/eeed6422a025813010ac1dcb922661d05266e36e/app/controllers/devise_token_auth/confirmations_controller.rb#L22-L25 and throw an exception of type ArgumentError saying bad argument (expected URI object or URI string).

Maybe I missed something?

newfylox commented 9 months ago

Also, it could be nice to say if email is already confirmed. I know it's possible to get the answer by calling this:

@resource.errors.of_kind? :email, :already_confirmed

So it would be nice to specify if account_confirmation_success AND something like already_confirmed in redirect_header_options so that we know if it's because the link is expired or because the email is already confirmed, giving us the chance to show a better message on frontend. There can also have something like link_expired param to tell if it's because the link is expired.