Mail Helper Refactor - Call for Feedback

[thinkingserious]

Hello!

It is now time to implement the final piece of our v2 to v3 migration. Before we dig into writing the code, we would love to get feedback on the proposed interface.

[thinkingserious]

@awwa, @pjurewicz, @skalee, @tricknotes, @Gwash3189, @maxcodes, @tkbky, @swifthand, @mootpointer, @brian4d, @alikhan-io, @safetymonkey

If you are tagged on this message, it means we are particularly interested in your feedback :)

If you don't have the time, no worries and my apologies for the disturbance.

If you do have the time, please take a look at the proposed helper upgrade above and let us know what you think. Any and all feedback is greatly appreciated.

Thanks in advance!

[gurgelrenan]

Sendgrid docs in Overview section says: The total number of recipients must be less than 1000. This includes all recipients defined within the to, cc, and bcc parameters, across each object that you include in the personalizations array..

In my opinion is a good ideia shows a example to a use case where send email to more than 1K at once. I'm currently solving this with the follow approach:

def send_emails(user_type)
  # Get all emails that needs to send. Could be more than 1K, so we need to make a
  # little bit more configuration
  # 'all_recipients' is a array of Personalization objects with their respective 
  # substitution tags
  all_recipients = recipients

  sg = SendGrid::API.new(api_key: ENV['SENDGRID_API_KEY'])

  all_recipients.each_slice(1000) do |thousand_recipients|
    # This is the way to send a Mail object with 1000 recipients and follow Sendgrid recommendations
    mail = prepare_email(thousand_recipients)

    begin
      response = sg.client.mail._('send').post(request_body: mail.to_json)
    rescue Exception => e
      puts e.message
    end

    puts '-------------------'
    puts "Status: #{response.status_code}"
    puts "Body: #{response.body}"
    puts "Headers: #{response.headers}"
    puts '------------------'
  end
  puts "#{all_recipients.size} emails sent!"
end

def recipients
  recipients = []

  User.all.find_each do |user|
    personalization = Personalization.new
    personalization.add_to(Email.new(email: user.email))
    personalization.add_substitution(Substitution.new(key: '-name-', value: user.name))
    recipients << personalization
  end
  recipients
end

def prepare_email(thousand_recipients)
  mail = Mail.new
  mail.from = Email.new(email: 'test@example.com', name: 'Example User')
  mail.subject = 'Sending with SendGrid is Fun'

  mail.template_id = '12345-6789'

  # Add 1000 recipients inside mail object as recommended by Sendgrid
  thousand_recipients.each do |recipient|
    mail.add_personalization(recipient)
  end
  mail
end

Perhaps this is not the better way, but I will leave my example here, maybe could be useful for some other users

[thinkingserious]

Thanks @gurgelrenan,

My hope is that with the updated design this will be easier, for example, using the Kitchen Sink example, you should not have to manipulate the Personalization object directly.

For this release, we are limiting the scope to the use cases described above; however, we will add this follow up use case to the next iteration. Thanks again!

With Best Regards,

Elmer

[awwa]

Hi @thinkingserious ,

I will show my opinion. I would like to hear any comments from other Rubyist about No.3.

1. The substitution keys on the contents
   For expediting the understanding how substitutions work, the email contents is better to include the substitution keys. For example, -name1- of the Send Multiple Emails to Multiple Recipients or %name4% and %city2% of the Kitchen Sink.

2. The attachment implementation
   It is better to include the file read and encode implementations for easy to use. For example:

require 'base64'
bytes = File.read('/path/to/the/attachment.pdf')
encoded = Base64.encode64(bytes)
msg.add_attachment('attachment.pdf',
                   encoded,
                   'application/pdf',
                   'attachment',
                   'Balance Sheet')

3. About creating email implementation

These three methods are clear and easy to understand.

SendGrid::Mail.create_single_email()
SendGrid::Mail.create_single_email_to_multiple_recipients()
SendGrid::Mail.create_multiple_emails_to_multiple_recipients()

But these methods can be implemented in one method too. For example:

def create_email(from, to, subject, plain_text_content, html_content, substitutions = [])

to and subject parameter could be passed a single instance or Array. The method create a message instance for single email if the user pass a single instance to to parameter. Also, the method create multiple message instances for multiple recipients if the user pass Array instance to to and subject.
This implementation may reduce the amount of the memory for developers brain. 😄

[maxcodes]

I agree with @awwa. Is there any reason to have all those separate methods?

Also, I have some comments regarding naming:

• Why call it ClientFactory, when you can call it Client? Client.new seems pretty straightforward to me.
• SendGrid::SendGridMessage seems redundant. I think it would be more consistent to use SendGrid::Message.
• add_to(email) seems kind of clunky. It could be interpreted as adding the message to the email. And since "Email" refers to the Email address, not to the actual email (you called this a "Message"), I think this could lead to confusion. What do you think of add_recipient and add_recipients?
• This is just an idea, but it seems it would be simpler (at least to me) to rename Email to EmailAddress and Message to Email, but I'm fine either way. Thoughts?

And lastly:
I'm not sure I understand the use case for sending multiple emails. It seems to add a lot of complexity when you could just create a new message and send it. Or maybe I'm just not understanding how it works. In the example above, are you randomly sending different subjects to different users? Or are you sending Subject 1 to Recipient 1? Or sending all 3 subjects to each of the users?

My 2c

[thinkingserious]

Thanks @awwa & @maxcodes! Great feedback! I'll respond in detail shortly.

For now, I don't believe we have given @maxcodes any swag. Can you please fill out this form so we can show some gratitude for your feedback?

[brian4d]

Would it be possible to use an options hash (Ruby 1.x) or keyword arguments (Ruby 2.x) instead of positional arguments? For example:

# Positional arguments
msg = SendGrid::Mail.create_single_email(from, to, subject, plain_text_content, html_content)

# Options hash and keyword arguments syntax is identical
msg = SendGrid::Mail.create_single_email(
    from: from,
    to: to,
    subject: subject,
    plain_text: plain_text_content,
    html: html_content)

This makes my code self-documenting and much less prone to errors caused by my arguments being out of order. It also lets you incorporate optional parameters:

msg = SendGrid::Mail.create_single_email(
    from: from,
    to: to,
    subject: subject,
    plain_text: plain_text_content,
    html: html_content,
    cc: cc_email)

Also, a lot of Ruby interfaces allow some parameters to optionally be an array. The advantage is that your API syntax is easier to read and remember. @awwa mentioned this in #3 of his comments. Just wanted to add +1 to his suggestion. For example, the "Single Email to Multiple Recipients" could just be:

to_list = [ 
    SendGrid::Email.new('test1@example.com', 'Example User1'),
    SendGrid::Email.new('test2@example.com', 'Example User2'),
    SendGrid::Email.new('test3@example.com', 'Example User3')
]
msg = SendGrid::Mail.create_single_email(
    from: from,
    to: to_list,
    subject: subject,
    plain_text: plain_text_content,
    html: html_content)

One last small suggestion. Most Ruby code seems to follow the convention of writing "set_" methods as "attr_name=". For example:

# Good
msg.set_subject("This is the subject")

# Better
msg.subject = "This is the subject"

[thinkingserious]

Fantastic feedback @brian4d! I'll have a link to some swag for you shortly, thanks!

Also, I'll be spending some deeper time later to properly address all the great feedback here and make adjustments to the proposal.

[thinkingserious]

@brian4d please take a moment to fill out this form so we can send you some swag. Thanks!

[thinkingserious]

@gurgelrenan please take a moment to fill out this form so we can send you some swag. Thanks!