Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Params for mail templates #2196

Closed
tigris opened this Issue Jan 2, 2013 · 3 comments

Comments

Projects
None yet
2 participants
Contributor

tigris commented Jan 2, 2013

Currently there is no way to pass any extra parameters to a mail template. The simplest instance of this I've needed in the past is if you wanted the email to include the IP address of the person who requested a password recovery. As per this StackExchange question.

There's also more complex needs, such as hostname context, and probably many other needs as well.

On a branch, I have made some changes to make this easy. The changes are quite small, and the main part is devise controller having a params_for_mailer method that is empty hash by default, the controller passes the result from that method into the resource and all model methods deal with having an optional argument.

My concern with this change, and the reason I didn't just send a pull request, is that it means send_devise_notification is now going to send an extra argument to all the mailer methods, such as recover_password_instructions, which at the moment only take 1 argument, the resource itself. A large number of people will have overloaded these methods, and I can't think of a nice way to have them transition nicely.

Then there's the whole rails mentality of "you should try and write request agnostic mail views", so that User.find(1).send_reset_password_instructions works the same from the command line as it does from a web request. But that's a debate for another time.

Anyway, if anyone has any thoughts on my above changes, I will clean them up and put the pull request in?

Owner

josevalim commented Jan 8, 2013

I couple days ago I have pushed a modification that allows mailers to receive an extra parameter that are sent by send_devise_notification. So you can achieve this now by simply overriding send_devise_notification. Please give it a go on master and let us know how it goes.

@josevalim josevalim closed this Jan 8, 2013

Owner

josevalim commented Jan 8, 2013

And thank you for writing this ticket and taking the time to improve Devise!

Contributor

tigris commented Jan 9, 2013

Hi Jose. I looked through the changes, and mostly they are the same as what I made. I was worried about the mailer methods like reset_password_instructions in Devise::Mailer needing to be redefined by anyone who upgraded.

Anyway, my main concern is that there is still not a way for the controller to pass options into things like the class method version of send_reset_password_instructions. Shall I show you what I have in a pull request and you can either reject or let me know what you don't like about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment