Mail Helper Refactor - Call for Feedback

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.

@awwa

@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!

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

@gurgelrenan

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

@thinkingserious

Hi @thinkingserious ,

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

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.
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')

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. 😄

@awwa

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

@awwa

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?

@awwa

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"

@brian4d

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.

@brian4d

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

@gurgelrenan

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

@thinkingserious

Hi,

@thinkingserious asked me to post my comments from #175 here, so here they are, in a new and improved version.

The dependency on Sinatra and rack-protection is a problem for people who don't need it. I'm guessing both of these are only needed for inbound processing. They each add dependencies of their own, so including the sendgrid-ruby gem in my projects adds a lot of unneeded gems. I saw in this comment that you will split the outgoing and incoming processing libraries in the future. Even before you do that, you can simply make the sinatra dependency optional. Put in the readme that if the user wants to do incoming processing, they have to add sinatra to the Gemfile. This isn't ideal because one can't control versions and such with as much precision and it might complicate the test suite setup a but, but overall it should be quite feasible.
ruby_http_client

i say: it looks like this is a standalone http client library developed by sendgrid? Why did you chose to make an http client library? There are several fantastic choices in ruby. Using an existing library would possibly reduce the weight of the dependencies, make it easier for users to work with your code and add new features, save you development time, let you benefit from the experience and history of those projects... this list goes on :)
@thinkingserious said: it's a thin wrapper over 'net/http' and 'net/https'. We split it out for two reasons: 1) to avoid any dependencies at all (which is another reason we need to break out that Sinatra dependency) and 2) we wanted to have a stand alone http client for simple generic API access. For the next version of this SDK, we may bring the http client back into the this SDK for simplicity. We would also entertain bringing in a third party http client dependency if the community desires it. Personally, I'd like it to be an option, unless there is a clear http client champion we can all agree on.
my response: gotcha. in that case, I'd say merge it into the main gem to simplify things and avoid confusion. if you decided to go with a third-party library, i think you will be safe. they are very aware they need to maintain backward compatibility. faraday for example hasn't had any breaking changes in years, except for dropping ruby 1.8.6 support. https://github.com/lostisland/faraday/releases (and i would recommend faraday with Net::HTTP as a replacement for ruby_http_client)

John

@jjb

Thanks @jjb!

Review this blog post with regards to error handling: https://betta.io/blog/2018/03/30/graceful-errors-in-ruby-sdks

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. Please feel free to reopen or create a new issue if you still require assistance. Thank you!

Navigation Menu

Repository navigation

Mail Helper Refactor - Call for Feedback #169

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Mail Helper Refactor - Call for Feedback #169

Description

Activity

thinkingserious commented on Jul 14, 2017

gurgelrenan commented on Jul 14, 2017

thinkingserious commented on Jul 14, 2017

awwa commented on Jul 19, 2017

maxcodes commented on Jul 19, 2017

thinkingserious commented on Jul 19, 2017

brian4d commented on Jul 19, 2017

thinkingserious commented on Jul 19, 2017

thinkingserious commented on Jul 20, 2017

thinkingserious commented on Jul 20, 2017

jjb commented on Sep 20, 2017

thinkingserious commented on Sep 23, 2017

thinkingserious commented on Apr 3, 2018

thinkingserious commented on Mar 11, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions