added CC and BCC sugar #194
Conversation
|
Haven't looked at the code at all but just want to be sure that: side note: why do we not mention |
|
It definitely needs more documentation around it. I always look at the docs here. |
|
Interesting... I did a CMD+f search and a search in the search input for "header_to" and got no results
… On Nov 26, 2016, at 11:21 AM, Avi Goldman ***@***.***> wrote:
It definitely needs more documentation around it. I always look at the docs here.
https://developers.sparkpost.com/api/recipient-lists.html#header-address-attributes
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
While I'm taking a look at this code, @avrahamgoldman can you use |
avigoldman
force-pushed
the
cc-and-bcc
branch
from
4c82900
to
23164f6
Compare
| const reqOpts = { | ||
| uri: api, | ||
| json: transmission, | ||
| json: modifiedTransmission, |
There was a problem hiding this comment.
I think it'd be nice to move all of the checks into formatPayload, and also collapse the first _.has check and the _.isArray check because _.isArray will return false if transmission.recipients isn't defined.
Then you don't need the const here, you can do the clone in formatPayload, and do 2 individual checks with early returns, and comments about why for each (e.g. " // return as-is if recipients is not an array") which I think will be a little clearer.
| } | ||
|
|
||
| function addListToRecipients(transmission, listName) { | ||
| if (_.has(transmission, listName) && _.isArray(transmission[listName])) { |
There was a problem hiding this comment.
I think the isArray check is sufficient here too.
| // default the headers to a blank object | ||
| transmission.content.headers = transmission.content.headers || {}; | ||
|
|
||
| transmission.content.headers.CC = _.map(transmission.cc, (ccRecipient) => addressToString(ccRecipient.address)).join(', '); |
There was a problem hiding this comment.
Use _.set() here, like _.set(transmission, 'content.headers.CC', <value>)
| } | ||
|
|
||
| addListToRecipients(transmission, 'cc'); | ||
| addListToRecipients(transmission, 'bcc'); |
There was a problem hiding this comment.
For these add* methods (addCCHeader and addListToRecipients) I think I'd usually prefer to see them return values that get added to the modified object here, instead of modifying/mutating in place. That'd mean we'd probably need to do the delete separately, here, but that might be worth it? Do you have thoughts on mutation lol
| } | ||
|
|
||
| function isEmail(email) { | ||
| const emailRegex = /^[-a-z0-9~!$%^&*_=+}{\'?]+(\.[-a-z0-9~!$%^&*_=+}{\'?]+)*@([a-z0-9_][-a-z0-9_]*(\.[-a-z0-9_]+[a-z][a-z])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}))(:[0-9]{1,5})?$/i; |
There was a problem hiding this comment.
Just curious, why are we pulling down this hell upon ourselves?
There was a problem hiding this comment.
I agree. We should let the API determine if it's a valid email address.
| } | ||
|
|
||
| function generateHeaderTo(recipients) { | ||
| const originalRecipients = _.filter(recipients, (recipient) => !_.has(recipient.address, 'header_to')); |
There was a problem hiding this comment.
What does this line do? I don't see any examples where header_to is ever defined so I forget why this scenario would arise.
There was a problem hiding this comment.
If they do a weird mix of the full syntax and the shortcut - i.e. they set the header_to for one cc'd recipient and use the shortcut for another.
| } | ||
|
|
||
| function addListToRecipients(transmission, listName, headerTo) { | ||
| if (!_.isArray(transmission[listName])) { |
There was a problem hiding this comment.
Do we still need that check for list_id ?
There was a problem hiding this comment.
Nope, that check was incorrect. I just reviewed the docs.
No description provided.