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

Updating the email delivery method #300

Closed
wants to merge 5 commits into from

Conversation

tomrossi7
Copy link

@tomrossi7 tomrossi7 commented Jul 27, 2023

This introduces a new option to the email delivery method to pass in arguments to mailers.

Sometimes a mailer will accept arguments:

class UserMailer < ApplicationMailer
  def comment_notification(comment)
    ...
  end
end

This can now be accounted for with an args option:

class EmailDeliveryWithArguments < Noticed::Base
  deliver_by :email, mailer: "UserMailer", method: :comment_notification, args: :email_args

  def email_args
    comment
  end
end

This will also work with a mailer that accepts named arguments with the named_args option:

class UserMailer < ApplicationMailer
  def comment_notification(comment:)
    ...
  end
end
class EmailDeliveryWithNamedArguments < Noticed::Base
  deliver_by :email, mailer: "UserMailer", method: :comment_notification_with, named_args: :email_named_args

  def email_named_args
    {comment: comment}
  end
end

@@ -50,6 +50,20 @@ def format
end
params.merge(recipient: recipient, record: record)
end

def args
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should standardize on args or arguments? I usually don't like to abbreviate, but Its kind of likeparams and parameters?

@@ -2,4 +2,8 @@ class UserMailer < ApplicationMailer
def comment_notification
mail(body: "")
end

def comment_notification_for(user)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new mailer action that accepts an argument.

@tomrossi7
Copy link
Author

tomrossi7 commented Jul 27, 2023

@excid3 in general what do you think about having options that just check if a method is defined rather than passing it as an option? For example, rather than passing the option arguments: :email_arguments to the delivery_by method, what if we just check to see if method_defined?(:email_arguments). This would codify the opinion that if you want to pass in arguments, you must define a method called email_arguments.

class EmailDeliveryWithArguments < Noticed::Base
  deliver_by :email, mailer: "UserMailer", method: :comment_notification_for, arguments: :email_arguments

  def email_arguments
    params[:recipient]
  end
end

would become:

class EmailDeliveryWithArguments < Noticed::Base
  deliver_by :email, mailer: "UserMailer", method: :comment_notification_for

  def email_arguments
    params[:recipient]
  end
end

You lose the flexibility to call the method whatever you want but gain convention-over-configuration goodness. Just a thought!

@excid3 excid3 closed this Jan 15, 2024
@excid3
Copy link
Owner

excid3 commented Jan 15, 2024

This is rolled into v2. 👍

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.

2 participants