DynamoMTL / spree_chimpy

Spree/MailChimp Integration
BSD 3-Clause "New" or "Revised" License
35 stars 123 forks source link

Use V2 of MailChimp API #41

Closed bryanmtl closed 9 years ago

bryanmtl commented 10 years ago

Hey @braidn -

If you have some time in the next week or so, do you want to take a run at this? We're currently using an old version of the mailchimp gem that communicates with an older (deprecated) version of the API.

I'd like to upgrade to use https://rubygems.org/gems/mailchimp-api.

cc: @joshnuss @DynamoMTL/mickeyren

joshnuss commented 10 years ago

:+1:

And we can fix the bug where it doesnt find the list when there are more than 25 (it currently only looks at the first page) by using the filtering api

mailchimp.lists.list({list_name: config.list_name})
bryanmtl commented 10 years ago

We also have an issue on H&C where it looks like multiple calls to "addOrder" are made and it's inflating results. MailChimp has suggested that upgrading to 2.0 might resolve it.

braidn commented 10 years ago

@bryanmtl,

I will take a stab at it likely today/tomorrow. I am going to opt out of list name completely though and move to just list_id. This way we can explicitly call the list we want without having to look/search for it. I hope this is ok? -- 

On June 12, 2014 at 9:01:58 AM, Bryan Mahoney (notifications@github.com) wrote:

We also have an issue on H&C where it looks like multiple calls to "addOrder" are made and it's inflating results. MailChimp has suggested that upgrading to 2.0 might resolve it.

— Reply to this email directly or view it on GitHub.

joshnuss commented 10 years ago

I think we can add a list_id to the config options (and note it as the preferred method in the readme), but we should still support list_name, otherwise it will be a breaking change for a bunch of sites.

bryanmtl commented 10 years ago

@joshnuss - good call.

braidn commented 10 years ago

@joshnuss I am running into some problems with the following specs: https://github.com/DynamoMTL/spree_chimpy/blob/master/spec/lib/list_interface_spec.rb

When lists is used in this section of the api it is used to find the list_id. From there you call list_subscribe directly on the api. However, in 2.0 we need to call api.lists.subscribe(params). I am having a hard time modifying the tests to this since api.lists is a hash which has no clue about the subscribe method. Here is a gist showing my approach:

https://gist.github.com/braidn/0706dd1b0d640f540e8a

Thank you.

joshnuss commented 10 years ago

@braidn I see,

Thinking it must have something to do with api.stub(:lists).and_return(data), cause data is a hash. Maybe you can add another double for api.lists?

joshnuss commented 10 years ago

or maybe instead of expect(api.lists).to receive.. it should be expect(data).to receive..

braidn commented 10 years ago

@joshnuss Yeah both of these methods cause the tests to error out at the fact that subscribe is not a valid method for Hash and that api.lists was called with an unexpected [](original errors). The data attribute passes info to the subscribe method, so I am not sure how to stub subscribe to it.

joshnuss commented 10 years ago

@braidn wanna push up the branch with the failing specs and I'll take a look? or screen share? not totally sure what the problem could be

braidn commented 9 years ago

Ok all! I think we are good to move this into master. Everyone's thoughts?

cc: @bryanmtl @joshnuss

joshnuss commented 9 years ago

@braidn looks awesome!

huge :+1:

braidn commented 9 years ago

Does everyone think that it is ok to operate master at Spree 2.1 and up? and maintain the 1.3 branch?

@bryanmtl @joshnuss

joshnuss commented 9 years ago

Yup, sounds good to me