-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
locale to support scoped routes #206 #208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, just a comment
lib/passwordless/config.rb
Outdated
default: lambda do |session, _request| | ||
Mailer.sign_in(session, session.token).deliver_now | ||
default: lambda do |session, request| | ||
locale = request.params[:locale] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think params[:locale]
is a built-in Rails thing? I've seen it before but is it not just a typical thing rather than an officially supported thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it's the recommended way to deal with a locale in the url or as a query parameter in the official i18n rails guide.
def switch_locale(&action)
locale = params[:locale] || I18n.default_locale
I18n.with_locale(locale, &action)
end
But the recommendation is different when the locale is taken from somewhere else like the domain name or the user preferences.
def switch_locale(&action)
locale = current_user.try(:locale) || I18n.default_locale
I18n.with_locale(locale, &action)
end
However if the locale does not affect the url, the mailer doesn't need to know about it to build the magic link, so it should be fine that params[:locale]
is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to include the params[:locale]
part by default. I definitely like there to be a way to support it, so the url_options
param is good. Is there a way we can support something like params[:locale]
(or whatever) without putting it in there?
I think it'll have to be like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the changes involving params[:locale]
(in sessions_controller.rb and context.rb).
For my use case it means I can use the passwordless Mailer with the new url_options, but the only way I can get things to work in my project is to implement my own equivalent of session_controller#create
and after_session_save
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution would be to add an optional configurable make_url_options(request)
function somewhere. If the user implements it, it would be used in the session_controller#create
and after_session_save
. That avoids introducing :locale
in the code, but adds an option that's difficult to explain...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I just found out how to get it working without touching the mailer. Using the default_url_options is enough!
config/initializers/passwordless.rb
Passwordless.configure do |config|
config.parent_mailer = "PasswordlessMailer"
end
app/mailers/passwordless_mailer.rb
class PasswordlessMailer < ActionMailer::Base
# adds params[:locale] to every url_for, link_to etc...
def default_url_options
Rails.application.config.action_mailer.default_url_options.merge(
locale: I18n.locale
)
end
end
The same mechanism should work with the session_controller
too when it inherits from the application_controller
that also defines a default_url_options
with the locale.
Unfortunately Passwordless.context.path_for
fails to use the default_url_options
. This pull request fixes it: #209
Thank you for working on this! |
* origin/master: Update changelog Use flash.alert (mikker#215) Update changelog Add support for scoped routes (mikker#209) Add "token" on show page to locale definition (mikker#214) v1.5.0 Update changelog Include TestHelpers in ActionDispatch::IntegrationTest (mikker#211) Add url_options param to sign_in email (mikker#208) Evaluate callable redirects in context of controller (mikker#203)
This seems to fix the problem.
The tests pass, however I don't know how to set up a test for a scoped locale. I'm new to rails, so it's better to check this very closely.