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

Offer the option to use parameterization for shared processing of headers and ivars #27825

Merged
merged 9 commits into from Jan 28, 2017

Conversation

Projects
None yet
6 participants
@dhh
Member

dhh commented Jan 27, 2017

Consider this example that does not use parameterization:

class InvitationsMailer < ApplicationMailer
  def account_invitation(inviter, invitee)
    @account = inviter.account
    @inviter = inviter
    @invitee = invitee

    subject = "#{@inviter.name} invited you to their Basecamp (#{@account.name})"

    mail \
      subject:   subject,
      to:        invitee.email_address,
      from:      common_address(inviter),
      reply_to:  inviter.email_address_with_name
  end

  def project_invitation(project, inviter, invitee)
    @account = inviter.account
    @project = project
    @inviter = inviter
    @invitee = invitee
    @summarizer = ProjectInvitationSummarizer.new(@project.bucket)

    subject = "#{@inviter.name.familiar} added you to a project in Basecamp (#{@account.name})"

    mail \
      subject:   subject,
      to:        invitee.email_address,
      from:      common_address(inviter),
      reply_to:  inviter.email_address_with_name
  end

  def bulk_project_invitation(projects, inviter, invitee)
    @account  = inviter.account
    @projects = projects.sort_by(&:name)
    @inviter  = inviter
    @invitee  = invitee

    subject = "#{@inviter.name.familiar} added you to some new stuff in Basecamp (#{@account.name})"

    mail \
      subject:   subject,
      to:        invitee.email_address,
      from:      common_address(inviter),
      reply_to:  inviter.email_address_with_name
  end
end

InvitationsMailer.account_invitation('david@basecamp.com', 'jason@basecamp.com').deliver_later

Using parameterized mailers, this can be rewritten as:

class InvitationsMailer < ApplicationMailer
  before_action { @inviter, @invitee = params[:inviter], params[:invitee] }
  before_action { @account = params[:inviter].account }

  default to:       -> { @invitee.email_address }, 
          from:     -> { common_address(@inviter) },
          reply_to: -> { @inviter.email_address_with_name }

  def account_invitation
    mail subject: "#{@inviter.name} invited you to their Basecamp (#{@account.name})"
  end

  def project_invitation
    @project    = params[:project]
    @summarizer = ProjectInvitationSummarizer.new(@project.bucket)

    mail subject: "#{@inviter.name.familiar} added you to a project in Basecamp (#{@account.name})"
  end

  def bulk_project_invitation
    @projects = params[:projects].sort_by(&:name)

    mail subject: "#{@inviter.name.familiar} added you to some new stuff in Basecamp (#{@account.name})"
  end
end

InvitationsMailer.params(inviter: 'david@basecamp.com', invitee: 'jason@basecamp.com').account_invitation.deliver_later

That's a big improvement! It's also fully backwards compatible. So you can start to gradually transition
mailers that stand to benefit the most from parameterization one by one and leave the others behind.

@@ -0,0 +1,141 @@
module ActionMailer
# Provides the option to parameterize mailers in other to share ivar setup, processing, and common headers.

This comment has been minimized.

@alexcameron89

alexcameron89 Jan 27, 2017

Member

"in other" -> "in order"

@alexcameron89

alexcameron89 Jan 27, 2017

Member

"in other" -> "in order"

This comment has been minimized.

@kaspth

kaspth Jan 27, 2017

Member

I'd also expand instance variable.

@kaspth

kaspth Jan 27, 2017

Member

I'd also expand instance variable.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 27, 2017

Member

Digging it, especially now that we can extract things into default 👍

I'm curious why the method is calledparams? Is it to bridge the terminology gap between controllers and mailers? I'm asking because I'd have thought this would be called assigns, which eludes to the view aspects of mailers.

Especially since most of the mailer actions I've written just blankly assign their parameters to an instance variable so I'm curious if we should take this a step further with:

class InvitationsMailer < ApplicationMailer
  assigns :inviter, :invitee
  assigns(:account) { @inviter.account } # Personally, meh. But throwing it out there :)
end

This would then just do @inviter = assigns[:inviter].

In case a specific action haven't passed, say, :invitee, the action would just have a blank @invitee ivar and nothing would blow up.

This way I think we could also avoid the params method while keeping backwardscompatibility:

InvitationsMailer.account_invitation(inviter: 'david@basecamp.com', invitee: 'jason@basecamp.com').deliver_later

Regarding Proc.new in default. It feels outside of the Rails aesthetic to me and I'm curious if we should do something to support:

default to: -> { @invitee.email_address }

Or if we're cool enough with proc :)

Member

kaspth commented Jan 27, 2017

Digging it, especially now that we can extract things into default 👍

I'm curious why the method is calledparams? Is it to bridge the terminology gap between controllers and mailers? I'm asking because I'd have thought this would be called assigns, which eludes to the view aspects of mailers.

Especially since most of the mailer actions I've written just blankly assign their parameters to an instance variable so I'm curious if we should take this a step further with:

class InvitationsMailer < ApplicationMailer
  assigns :inviter, :invitee
  assigns(:account) { @inviter.account } # Personally, meh. But throwing it out there :)
end

This would then just do @inviter = assigns[:inviter].

In case a specific action haven't passed, say, :invitee, the action would just have a blank @invitee ivar and nothing would blow up.

This way I think we could also avoid the params method while keeping backwardscompatibility:

InvitationsMailer.account_invitation(inviter: 'david@basecamp.com', invitee: 'jason@basecamp.com').deliver_later

Regarding Proc.new in default. It feels outside of the Rails aesthetic to me and I'm curious if we should do something to support:

default to: -> { @invitee.email_address }

Or if we're cool enough with proc :)

@kaspth

kaspth approved these changes Jan 27, 2017

Implementation seems fine 👍

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 27, 2017

Member

Reading your example again, we'd probably need to update:

InvitationsMailer.account_invitation(inviter: 'david@basecamp.com', invitee: 'jason@basecamp.com').deliver_later

Since those strings won't respond to account, name etc. 😁

Member

kaspth commented Jan 27, 2017

Reading your example again, we'd probably need to update:

InvitationsMailer.account_invitation(inviter: 'david@basecamp.com', invitee: 'jason@basecamp.com').deliver_later

Since those strings won't respond to account, name etc. 😁

@maclover7

IMHO, I don't see this as a big improvement over the current usage. Introducing a ActionMailer::Base#params method which you then call the mailer method on seems kind of messy to me. I'm a big fan of explicitly passing arguments to the mailer method, over more magical params handling 😬

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 28, 2017

Member
Member

dhh commented Jan 28, 2017

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 28, 2017

Member
Member

dhh commented Jan 28, 2017

dhh added some commits Jan 28, 2017

Use #with rather than #params as the context-setter
Confusing to have both the context setter as a unqualified method and
the accessor as well. Considered #with_params, but I think that’s
implied well enough using just #with.
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 28, 2017

Member

Neat! I always thought you had to check the arity to get lambda support.

Member

kaspth commented on actionmailer/lib/action_mailer/base.rb in bb7b923 Jan 28, 2017

Neat! I always thought you had to check the arity to get lambda support.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 28, 2017

Member

Uses person_b twice ;)

Member

kaspth commented on actionmailer/lib/action_mailer/parameterized.rb in fbbf892 Jan 28, 2017

Uses person_b twice ;)

[ci skip] Use person_a as the first argument.
Inviting yourself? Your life must be a party! 😄

@dhh dhh merged commit 1cec84a into master Jan 28, 2017

1 check passed

codeclimate no new or fixed issues
Details

@dhh dhh deleted the parameterized-mailers branch Jan 28, 2017

@kaspth kaspth removed the needs feedback label Jan 28, 2017

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 28, 2017

Member

Here we can now talk of a new concept: parameterized mailers. The method follows that name. And it makes the before_action and other filters feel immediately familiar to anyone who've done the same work with action controllers.

Ha! It took me until now to fully grasp what you meant by this. params allows for better use of before_action in Mailers and perfectly mirrors Action Controller's API of the same 😄

Member

kaspth commented Jan 28, 2017

Here we can now talk of a new concept: parameterized mailers. The method follows that name. And it makes the before_action and other filters feel immediately familiar to anyone who've done the same work with action controllers.

Ha! It took me until now to fully grasp what you meant by this. params allows for better use of before_action in Mailers and perfectly mirrors Action Controller's API of the same 😄

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 30, 2017

Member

This is great! I did some improvements in the implementation here 341fab8...2dadf73

Member

rafaelfranca commented Jan 30, 2017

This is great! I did some improvements in the implementation here 341fab8...2dadf73

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jan 30, 2017

Member

@rafaelfranca and now it's even better 😃🎉

Member

kaspth commented Jan 30, 2017

@rafaelfranca and now it's even better 😃🎉

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 31, 2017

Member

Lovely, thanks @rafaelfranca!

Member

dhh commented Jan 31, 2017

Lovely, thanks @rafaelfranca!

@williamromero

This comment has been minimized.

Show comment
Hide comment
@williamromero

williamromero Nov 14, 2017

Excuse me if i don't understand, but it means now we can send mails with a particular name for each mail for our mailing lists? Right? Sorry if don't understand it well.

williamromero commented Nov 14, 2017

Excuse me if i don't understand, but it means now we can send mails with a particular name for each mail for our mailing lists? Right? Sorry if don't understand it well.

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