search

mikker / passwordless

🗝 Authentication for your Rails app without the icky-ness of passwords
MIT License
1.25k stars 86 forks source link

Pass default_url_options from mailer #188

Closed djfpaagman closed 6 months ago

djfpaagman commented 7 months ago

This is a bit related to #187, with the same behavior where we set the hostname dynamically based on locale.

With the introduction of the context in 1.1.0 (I think?) (PR: #180). the url helpers are no longer using the default_url_options from the mailer (either via config.action_mailer.default_url_options or when overriding them in a mailer themselves via a default_url_options method).

This PR fixes that by passing them as arguments to the url_for method from the context.

This probably also means you no longer necessarily need to to set a default host for the routes as well, as mentioned here in the docs:

https://github.com/mikker/passwordless/blob/cd0e0ed7cdba745dcae92e84d800122b8d3368b6/README.md?plain=1#L148-L155

I also added a reload when using WithConfig, as the parent class for the mailer is set from the config and that can change based on usage of the helper. This probably also needs to be included in #187.

Let me know what you think!

mikker commented 7 months ago

Great solution!

mikker commented 7 months ago

The tests are failing. I think maybe due to your refresh mechanism not working as intended?

djfpaagman commented 7 months ago

I had some troubles running the tests locally (fixed that now) and indeed this does not seem to work without side effects. I'll have another look at it at a later point!

djfpaagman commented 7 months ago

I think I found a different solution, tests are constantly green for me now!

mikker commented 7 months ago

At least it's green! I think it'd be better to move all this from the general with_config to the specific test? It is only really relevant to these two examples.

djfpaagman commented 7 months ago

Yeah that could work as well 👍 but I think there will always be the possibility that a future test needs to override parent_mailer as well and then you'll run into the same issue again. So I think keeping the reloading behavior in the test helper for overriding configs makes most sense to me.

But happy to move it to the test itself as well, let me know.

mikker commented 7 months ago

Let's move it now and move it back when it becomes necessary. Thanks!

djfpaagman commented 7 months ago

@mikker: I've moved some things around with comments so that it's clear what's going on.

djfpaagman commented 6 months ago

@mikker is there anything I can do to get this merged in? would love to see it released :)

mikker commented 6 months ago

Sorry, no, it's good. Thank you!