Overriding a controller shouldn't mess with Devise translation rules

[rosenfeld]

Devise's behavior for an attempt to load the sign-in path while already authenticated is to forward the request to the main (or configured) page. But when the session times out in our application we redirect to a static page explaining how to re-authenticate (this is done in a third-party application) rather than redirecting to the log-in page. So, if the user navigates to the log-in page he will be redirected to the main page since the user is authenticated but then it will redirect again to the timed-out page since the session timed out. In order to avoid this behavior that some users have complained we decided to always show the log-in page to the user when requested instead of redirecting it when authenticated by skipping one of the filters for SessionsController in a inherited controller from our own. This problem happened in a moment when there was a problem with the third-party authenticator and they asked us to allow users to use a shared account until they fixed the authenticator. I've written about this before in the mailing list:

https://groups.google.com/forum/#!topic/plataformatec-devise/w5SJcBGI_Ec

As you can see in the solution we used another controller inheriting from Devise::SessionsController.

Now I noted that some translations are missing in some flash messages set by this new inherited controller. Then I found how find_message works:

/~https://github.com/plataformatec/devise/blob/master/app/controllers/devise_controller.rb#L163

It uses the controller's name for building the translation scope. It would help already if that line was replaced by another method like "devise_scope" so that we could easily override it in the overridden controller but it would be even better if overriding a Devise's controller worked more seamless without having to worry about translations at all.

Maybe a guide on the Wiki explaining how to override Devise's controller would be helpful. Maybe the scope could be passed as a parameter to find_message so that overriding a Devise's controller wouldn't have such side effects.

What do you think?

[josevalim]

Instead of making it a scope, it should likely just be a fallback. We could always make it fallback to Devise module. Thoughts?

[rosenfeld]

Sorry, I didn't quite understand what you meant. I'm not much aware of how i18n works. Would you mind further explaining it?

[josevalim]

@rosenfeld I18n allows multiple fallbacks to be given. So today, if we are looking at :"devise.session.invalid" and in the custom controller we lookup for :"my_session.invalid", we could add :"devise.session.invalid" as an I18n fallback for such lookup, so if people override, it doesn't matter how deep, we always lookup :"devise.session.invalid" in case the specific one isn't available.

[rosenfeld]

Sounds like a good plan but I'm still unsure on how this would be implemented. I can see two basic approaches:

1 - explicit approach: the scope would always be passed as a parameter to Devise#find_message so that we could use it as the fallback
2 - dynamic approach: the fallbacks would be created dynamically, one for each inherited controller's name in the chain up to ::DeviseController.

Personally I prefer explicit approaches but I'm not sure how you see this happening on Devise.

Am I missing something?

[josevalim]

+1 for explicit.

[rosenfeld]

What about this. DeviseController could implement a protected translation_default_scope method like this:

def translation_default_scope
  "devise.#{controller_name}"
end

Then all bundled controllers would override it so that Devise::SessionsController would have it overridden as:

def translation_default_scope
  "devise.sessions"
end

This way we could use that as the fallback rather than passing it to find_message. What do you think?

[josevalim]

That sounds great, yes!

[rosenfeld]

Great. I need to set up some time to understand how fallbacks work in I18n and how to write tests for Devise before I can work on this. Right now I'm in a rush to finish quite some tasks before the end of the year and I can't work on this out of my work journey because I have to take care of my baby so it's possible that I will only be able to work on this next year. For now I've just overriden find_message to fix it in the application and get my work done in the due date but later I'll try to work on a proper fix by sending a pull request to Devise. You may close this ticket in the meanwhile if you prefer.

[rosenfeld]

@josevalim when I searched for fallbacks in I18n I found an unrelated subject:

/~https://github.com/svenfuchs/i18n/wiki/Fallbacks

I believe you meant something else by fallbacks, like the multiple options passed to the :default key. But if we change the :scope argument it would be backward incompatible and might break someone's translations. Are you sure you want to stop using this scope?

The changes I suggested is unlikely to break any existing translations. It could, for instance, try to get the translation from "devise.#{controller_name}" first. Then it could try with translation_default_scope as a fallback mechanism. Do you think it would worth the backward compatibility or would you prefer to not try to be backward compatible?

[josevalim]

Yeah, let's go simply go with your translation_default_scope suggestion then. There is no need for fallbacks.

[rosenfeld]

Ok, great, I have a simple task for this morning and maybe I would have some little free time to start working on this until the end of the morning but I don't think I'll be able to write the tests that fast, as I'm not sure on how to write the tests yet :)

[rosenfeld]

I've spent the first week of this year sleeping in the Hospital where my daughter was admitted, but now I could find some time to work on this. Actually I think the name translation_scope makes more sense than translation_default_scope. I'll start working on it and will let you know if I have any questions about how to write tests for it...