Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add support for entire Newsletter API area #32

Closed
wants to merge 1 commit into from

3 participants

@kenperkins

Caveat

I don't expect this PR to be accepted as is, but I wanted to start the dialog about how and what form this might take.

Overview

I've added the support for all of the newsletter APIs with very crude error handling, input validation, minimal documentation, and no unit tests.

The APIs that are supported are:

  • Newsletter
  • Newsletter - Lists
  • Newsletter - Lists - Email
  • Newsletter - Schedule
  • Newsletter - Recipients
  • Newsletter - Identity

All of these are implemented using the Request module which nicely abstracts away the particulars of using HTTP/HTTPS calls.

I've tentatively added these as sub objects of the master Sendgrid object, but I'm not convinced that is the best way to factor these, but this is good enough to get the ball rolling.

Here's a couple of examples:

var Sendgrid = require('sendgrid'),
    sendgrid = new Sendgrid('myuser', 'mypassword');

sendgrid.Newsletter.Lists.Email.add('mylist', {
        email: 'ken@mydomain.com',
        name: 'ken',
        displayName: 'Ken Perkins'
    }, function(err, result) {
        if (err) {
            console.dir(err);
            process.exit(1);
        }

        console.dir(result.body);
});

sendgrid.Newsletter.Identity.add({
        identity: 'testIdentity',
        name: 'My Identity',
        email: 'support@mydomain.com',
        address: '12345 Anystreet S',
        city: 'Seattle',
        state: 'WA',
        zip: '98104',
        country: 'usa'
    }, function(err, result) {

        if (err) {
            console.dir(err);
        }

        console.dir(result.body);
});
@partkyle
Owner

Sorry for the late response on this, we've been a little busy. This code looks good and is well documented, but we can't accept this for now without some form of automated testing.

Unfortunately, we don't really have the resources to write unit tests for this portion of code right now. If you were able to do so, then we could consider adding it to the library.

If I get some spare time I will look more into it. This looks like a lot of work and I would love to see it integrated.

@kenperkins

I completely understand the test requirement. Part of the reason I worked fast on this was I wanted to vet the model with you guys before making a huge investment of time that might end up being throw-away.

If you're happy with the general model, I'll start work to add unit tests and further clean things up.

@theycallmeswift

Where are we on this? I can pitch in if needed.

@kenperkins

Sorry I've been swamped with a big release this week. feel free to help out but part of the problem is I need to author unit tests.

@partkyle
Owner

I'm wondering if we can do something like a feature branch and a beta release to npm, so people who want to use the newsletter stuff can use it while we work on tests.

As of right now, there are only 2 issues with this PR:

  • There are no tests
  • The code style does not match (not a huge issue, but I like consistency in a project. Admittedly, our style may not be like other node projects, so something needs to change)

I'd be comfortable with a beta branch/npm version if that would help get this some use.

Thoughts?

@kenperkins

I have no problems getting the code style in line, nor with unit tests. I just haven't had a lot of bandwidth.

@theycallmeswift

I'm closing the pull request as we don't have plans ATM to add newsletter functionality to this library. We would want to make a separate newsletter library for node instead. Happy to discuss this further if people are interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 17, 2012
  1. @kenperkins
Something went wrong with that request. Please try again.