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

Unexpected Mail instance categories, categories= behavior #86

Closed
subvertallchris opened this issue Aug 10, 2016 · 3 comments
Closed

Unexpected Mail instance categories, categories= behavior #86

subvertallchris opened this issue Aug 10, 2016 · 3 comments
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap

Comments

@subvertallchris
Copy link

Issue Summary

SendGrid::Mail categories behavior makes it too easy to set empty values.

Steps to Reproduce

Instantiate a new instance

sg_mail = SendGrid::Mail.new

I know it will end up being an Arrary, so try to add something to it...

sg_mail.categories << 'foo'
# NoMethodError: undefined method `<<' for nil:NilClass

Oh, categories starts nil. I guess we need to instantiate it?

sg_mail.categories = []
sg_mail.categories
=> [nil]

An Array with nil in it. ???

More misleading is that if someone expects it to accept the String category, it will not complain, it will just add nil

sg_mail.categories = 'foo'
=> "foo"

sg_mail.categories
=> [nil, nil]

Despite its own awareness that it is an Array and its delegation of methods, it doesn't actually let me treat it like an array, so I don't have a clear way of fixing this.

sg_mail.categories.compact
=> []

sg_mail.categories
=> [nil, nil]

You could improve this by doing two things:

  • New instances of Sendgrid::Mail should set categories to an empty Array. If nothing else, that would let callers know to treat it like an array and avoid categories= in the first place.
    def categories
      @categories ||= []
    end

https://github.com/sendgrid/sendgrid-ruby/blob/master/lib/sendgrid/helpers/mail/mail.rb#L908

  • Fix categories=. I don't think that what you have defined works at all. When you call [string] on a String, you just select that pattern from the string. So if someone did do this:
# I guess this was what you had in mind?
sg_mail.categories = { categories: 'foo' }

# It would become this:
{ category: 'foo' }.to_json
=> "{\"category\":\"foo\"}"

# And then you're just pulling the word `category` out of it and storing it in your array
"{\"category\":\"foo\"}"['category']
=> "category"

This is also why calling categories= results in nil entries: it converts everything to a String with to_json and then whatever_your_string_is['categories'] always returns nil.

Technical details:

  • sendgrid-ruby Version: 4.0.2
  • Ruby Version: 2.2.3
@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: help wanted requesting help from the community labels Aug 11, 2016
@thinkingserious
Copy link
Contributor

Excellent feedback @subvertallchris!

We would like to offer you some swag. Please email us at dx@sendgrid.com referencing this ticket with your mailing address and T-shirt size.

For this issue to rise up in our queue, we need +1s, comments or a pull request with a signed CLA.

Thanks!

tkbky pushed a commit to tkbky/sendgrid-ruby that referenced this issue Oct 9, 2016
@tkbky
Copy link

tkbky commented Oct 12, 2016

Related issue: #95

@thinkingserious
Copy link
Contributor

Please follow progress on this issue here: #95

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

3 participants