markets / maily

📫 Rails Engine to preview emails in the browser
https://rubygems.org/gems/maily
MIT License
707 stars 31 forks source link

Inherited methods dont show on ui #28

Closed pcriv closed 6 years ago

pcriv commented 6 years ago

Give the following mailer and hooks:

# app/mailers/user_mailer.rb

class UserMailer < Devise::Mailer
  private

  # headers for devise mailers
  def headers_for(action, opts)
    super action, opts.merge(template_path: "/mailers/user_mailer")
  end
end
# lib/maily_hooks.rb

user = User.first
token = "token"

Maily.hooks_for("UserMailer") do |mailer|
  mailer.register_hook(:reset_password_instructions, user, token, template_path: "mailers/user_mailer")
  mailer.register_hook(:confirmation_instructions, user, token, template_path: "mailers/user_mailer")
  mailer.register_hook(:invitation_instructions, user, token, template_path: "mailers/user_mailer")
  mailer.register_hook(:unlock_instructions, user, token, template_path: "mailers/user_mailer")
  mailer.register_hook(:password_change, user, template_path: "mailers/user_mailer")
  mailer.register_hook(:email_changed, user, template_path: "mailers/user_mailer")
end

On the ui i get this: (text version for simplicity)

User mailer (0)
markets commented 6 years ago

Hi again @pablocrivella 👋

Yes, at this point, this is "normal"... I mean, by design :sweat_smile:. I use this code to get all emails (instance methods at the end) from a mailer:

https://github.com/markets/maily/blob/be46fa5fbfc8d5e85dc40ab13449b843969d7193/lib/maily/mailer.rb#L56

And public_instance_methods called with false, doesn't return any method from ancestors. I built it this way because I sometimes use mailers like:

So both mailers share things (settings, layout, ...) with ancestor, but I don't want to list all Notifier emails when initializing the UserNotifier class: UserNotifier.send(:public_instance_methods, true) will return all emails from Notifier as well.

I never thought/experimented with an structure similar to yours (neither Devise support). Not sure how to solve or workaround this "feature", let me think about it 😄. If you have any idea or you want to contribute, feel free!

Thanks!

pcriv commented 6 years ago

I have been poking around the codebase for a bit, i'll let you know if i find a nice workaround!

markets commented 6 years ago

Hi @pablocrivella, I just made a quick analysis and I have some ideas to allow "inherited" mailers:

a) When registering a hook, force mailer class to look for this email, even if it belongs to the ancestor class. This change wouldn't imply new Maily public api or options for the user... Cons: a hook is necessary even if the email doesn't require parameters (not sure if this is really a "problem", most emails needs params anyway ...).

# code changes in Mailer class
def register_hook(email_name, *args)
  email = find_email(email_name)

  if email
    email.register_hook(args)
  else
    # email not found, try to look for instance methods in parent class
  end
end

b.1) Introduce new method mailer.add_email:

Maily.hooks_for("UserMailer") do |mailer|
  mailer.add_email(:reset_password_instructions, user, token, template_path: "mailers/user_mailer")

Needs new api for the developer, but internally should be something like (a) + current register_hook.

b.2) Introduce new method mailer.add_email:

Maily.hooks_for("UserMailer") do |mailer|
  mailer.add_email(:reset_password_instructions)
  mailer.register_hook(:reset_password_instructions, user, token, template_path: "mailers/user_mailer")

Needs new api for the developer, internally should be something like (b.1), but with different api. We also have a mailer.hide_email method with same signature, maybe this is like a complementary operation?

c) Introduce new option inheritance (or other more meaningful name) in register_hook:

Maily.hooks_for("UserMailer") do |mailer|
  mailer.register_hook(:reset_password_instructions, user, token, template_path: "mailers/user_mailer", inheritance: true)

Needs new api for the developer, but internally should be something like (a) + taking into account this flag.

d) Introduce new option inheritance (or other more meaningful name) in hooks_for:

Maily.hooks_for('UserMailer', inheritance: true) do |mailer|

This will include all emails from parent class by simply calling klass.send(:public_instance_methods, true) internally. Needs new api for the developer.


What do you think? What option you would like more as a developer integrating Maily? What api feels more natural (or less surprising)?

markets commented 6 years ago

@AlexLarra as a contributor and user of Maily, what do you think? Would be nice to hear your thoughts.

pcriv commented 6 years ago

Hey @markets, really nice summary you made!

IMHO option a makes more sense. Even tho you will have to register the hooks in order for the emails to show, but actually that is what i would expect to happen. So maybe less ruby "magic" is not actually a con here.

What do you think?

AlexLarra commented 6 years ago

Yes @markets, I would like to use it just as in a proposal. Even though we already have hide_email I'm not sure if in this case would be very friendly to use something similar. The c with inheritance argument I would like it too but it seems more natural a.

markets commented 6 years ago

@pablocrivella @AlexLarra Maily v0.8.0 released :rocket: