Skip to content
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

Pass default_url_options from mailer #188

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

djfpaagman
Copy link
Contributor

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:

passwordless/README.md

Lines 148 to 155 in cd0e0ed

Also be sure to
[specify ActionMailer's `default_url_options.host`](http://guides.rubyonrails.org/action_mailer_basics.html#generating-urls-in-action-mailer-views) and tell the routes as well:
```ruby
# config/application.rb for example:
config.action_mailer.default_url_options = {host: "www.example.com"}
routes.default_url_options[:host] ||= "www.example.com"
```

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
Copy link
Owner

mikker commented Nov 30, 2023

Great solution!

@mikker
Copy link
Owner

mikker commented Nov 30, 2023

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

@djfpaagman
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mikker
Copy link
Owner

mikker commented Dec 6, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

mikker commented Dec 13, 2023

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

@djfpaagman
Copy link
Contributor Author

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

@djfpaagman
Copy link
Contributor Author

djfpaagman commented Jan 11, 2024

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

@mikker
Copy link
Owner

mikker commented Jan 11, 2024

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

@mikker mikker merged commit 0428330 into mikker:master Jan 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants