default_url_options in SessionsController.create

[andreaskundig]

This allows to define the locale as a default_url_option in the application_controller and goes some way to implement #206

The tests pass

[mikker]

I'm not sure where default_url_options comes from in this context? Do we not need to define it in the controller so it's overridable? Could you add a test that covers this code path?

[andreaskundig]

default_url_options would be implemented in the ApplicationController, or whatever other class you set as config.parent_controller in your configuration.

This is recommended in the rails i18n guide . It says ApplicationController#default_url_options is the Rails infrastructure for "centralizing dynamic decisions about the URLs". So when you add a property to default_url_options, it's expected that it's passed as argument to the generated urls. You can see it's tested in rails, for example here's one url_helper_test that I just found with a quick grep. This PR brings the expected rails behavior to the magic_link generated in the controller.

The same mechanism already works in the Mailer thanks to the recent PR #188: , that added **default_url_options here.

I'll try to write a test for this, that PR and the rails test I found are probably a good place to start, I'll see if I have time this week-end.

By the way I'm impressed your reactivity. I've read in several places it's hard to be a maintainer because people only seek you when there's a problem. So in my name and in the name of your many users who never ran into a problem: thank you, you're doing a great job!

[andreaskundig]

I wrote some tests for a i18n setup that uses the locale in scoped routes. If you look at my commits in order:

• 6e88b48 sets up a scoped route, adds a fr.yml translation file, and adds some tests that pass. This follows the rails i18n guide pretty closely.
• 3222c1a adds a test that fails to generate the proper redirection.
• 866d446 finally introduces my proposed change that adds default_url_options to SessionController#create's path_for(), and now the test passes.

The locale tests are more verbose that I would like, each one redefines a Passwordless::LocaleParentController. They are all identical, but I didn't manage to put them in one place. I'm no ruby metaprogramming wizard, any help on that is welcome.

[andreaskundig]

I managed to make things work for me with this workaround: I subclass the SessionsController and override the create method:

class MyPasswordlessSessionsController < Passwordless::SessionsController
    def create
    # same as in superclass but with 
    # the additional line that adds 
    # ...
    **defaul_url_options

Then in the routes I set

  scope "/:locale" do
    passwordless_for :users, controller: 'my_passwordless_sessions'

But this feels like a really clumsy hack. I would love to have your opinion on how to do this cleanly.

[mikker]

Thank you for reporting and contributing all this back. I'm away from work this week but I'll get back to you soon!

[mikker]

I …

1. Removed the locale file (I don't want to support locales although I would love it if you could add your translation to the readme!)
2. Moved all the added tests to a dedicated test case so they're close to each other and can share setup/teardown

[mikker]

To clarify: I want to support locales but I don't want to bundle them as I'd have to keep them up to date for forever going forward

[mikker]

Thank you for working on this! ❤️🧡💛💚💙💜

[andreaskundig]

Thanks for merging it !

One thing I don't understand though is where you want me to put my translation ?

1. Removed the locale file (I don't want to support locales although I would love it if you could add your translation to the readme!)

[mikker]

Here: https://github.com/mikker/passwordless/wiki/User-submitted-locale-files