-
Notifications
You must be signed in to change notification settings - Fork 30
Attachments support #36
base: master
Are you sure you want to change the base?
Conversation
still work in progress made the pr so you can see already what it kind of looks like and maybe already give suggestions for improvement |
ready for review now. note: attachments must be passed in as byte64 encoded strings, this is what mandrill expect (lookup attachments under https://mandrillapp.com/api/docs/messages.html). I've added this into the readme. |
}); | ||
|
||
it('attachments object', function(done) { | ||
wrappedTransport.sendMail({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont merge yet, i thought of something that needs to be tested:
i should call sendMail with full details (from, to etc...) and make sure that it doesn't somehow interfere with the attachments logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done already.
What's the status on this, out of curiosity? Currently I'm pointing to @samzilverberg's repo in my package.json but it would be better if this were merged here so we can have attachment support. |
there, added the assertions i wanted, all worked as expected actually, but the assertions make me sleep better at night! ;) merge it if you feel good about it |
@devinus Is this mergeable ? Is it possible to create an npm release with this feature? |
var mandrillTransport = require('../'); | ||
|
||
var packageData = require('../package.json'); | ||
var sinon = require('sinon'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sry for changing the project style, just happened out of habit
I will review this PR this weekend and merge if acceptable. It's doing the right thing, just concerned about the private API usage. Will investigate further. |
+1 |
1 similar comment
+1 |
Any possibility of getting this merged? |
No description provided.