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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow from address to fall back to parent mailer #187

Conversation

djfpaagman
Copy link
Contributor

@djfpaagman djfpaagman commented Nov 28, 2023

I'm in a situation where I want the parent mailer class to set the from address.

The use case is that based on the I18n.locale we set a different from address/domain.

Right now it's not possible to do this, as even with nil for default_from_address it will not fall back to the parent (the mailer will fail: ArgumentError (SMTP From address may not be blank)). That basically means only one, fixed from address is supported.

I've come up with a small PR that makes that work by not setting the default value if it's nil.

Curious to hear what you think, I'm not entirely sure about:

  • Is the config the right place for that mailer_defaults method? Feel like it could also live in the Passwordless::Mailer class.
  • Does it even need it's own method like this, since there's only one default being set right now. It's easily extendable in the future like this, though.
  • I've had some issues with getting all tests to run, not sure if this is correct now. Since we need to change the config just for this test, I think setting it like this and resetting at the end is the best way to do it. edit: found the WithConfig.with_config helper that solves my problem 馃槉
  • Probably needs some documentation.

Happy to take it in the direction you want with some pointers 馃榿

@djfpaagman
Copy link
Contributor Author

While writing this PR I had another look at my code and realized I can just put a lambda with a method call in the initializer as well:

config.default_from_address = -> { mailer_from }

This works as long as that method exists in the parent mailer, it's the same I do there to set up the default.

I want to keep this PR open though, still think it would be a nice addition!

@mikker
Copy link
Owner

mikker commented Nov 30, 2023

The tests look like they're failing. I appreciate your efforts and it looks like a good solution - however, I'm hesitant to add code to solve a problem we already, as you noted, has sort of a solution for.

@djfpaagman
Copy link
Contributor Author

Yeah I'm fine with it working with the lambda now, i'll close this PR 馃

@djfpaagman djfpaagman closed this Dec 13, 2023
@djfpaagman djfpaagman deleted the default_from_fallback_to_parent_mailer branch December 13, 2023 08:35
@mikker
Copy link
Owner

mikker commented Dec 13, 2023

Thanks!

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