DynamoMTL / spree_chimpy

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

Add support for Mailchimp v3.0 API via Gibbon #76

Closed jkelleyj closed 8 years ago

jkelleyj commented 8 years ago

@braidn I've reworked the interface classes to use Gibbon instead of Mailchimp API since the v3.0 API is to be required soon. This took a bit more reworking than I'd expected because the API changes are significant.

The new api is pretty strictly restful, so many of the convenience methods are gone, and we require a more chatty approach to add an order. Basically, we need to ensure the products exist, the customer exists, and then either update or create the order based on whether it already exists.

A few notes:

  1. The old logic did not include the mc_eid on the order when the email address connected to that eid and the order's email address do not match. This has been noted by a TODO, but I'm not sure on the best approach. I'm thinking that creating it with the email in the actual order when it does not match the one returned by the mc_eid would lead to more consistent Customer information in Mailchimp. It's currently implemented the other way, and I'd be happy to tweak if you agree.
  2. The v3.0 api has limits on which verbs are allowed on which resources (e.g. Orders can't be PUT, so gibbon upsert doesn't work). It's worth checking their docs here before any refactors: http://developer.mailchimp.com/documentation/mailchimp/reference/overview/
  3. One issue I couldn't easily get around was how to handle orders where a different email is used than is connected to the user_id on the order. Mailchimp requires a "foreign" customer id (which I've added as customer_XXXX). You can't change the email address for a customer, so I assume we would just connect it to the user_id on the order. This is how I've implemented it in the upsert_customer method.
  4. Not sure if the way I'm generating routes for the product url is acceptable. It works, but has a code smell. Any thoughts on accessing the spree routes from this engine?
  5. The specs are killing me. They all pass and I added a few to test smaller pieces of the interfaces. We could refactor the customer and product related concerns into separate classes from interface/order, but I've let that class balloon a bit for now. I don't think we have fantastic test coverage because I had to sub all the interfaces on Gibbon. I'd recommend moving towards webmocks and vcr to record actual transactions with the API. This would be brittle in its own way, but more amenable to refactoring and would be easier to confirm that the api is being called correctly. I spent many iterations making sure I was calling Gibbon correctly and stubbing it only to find that I needed customers.id not just id in the fields parameter. VCR would catch these better than stubbing IMHO.
jkelleyj commented 8 years ago

Just noticed two of your commits are listed here. I thought I rebased my branch off of the v2 stable branch. I'm no git wizard, so happy to adjust my branch to remove those if there's something screwy with my history... may just need some help with that.

braidn commented 8 years ago

noice @jkelleyj will have a look at this tomorrow AM!

jkelleyj commented 8 years ago

Noticed that one of the info fields was missing and is used in the sync rake task and added a commit for that.

braidn commented 8 years ago

Pinging you here @jkelleyj . What cha think about some of these code review requests? Would love to get this merged.

jkelleyj commented 8 years ago

I'm pretty pinned down for the next week or two. There's a chance I can squeeze it in sooner but most likely it is going to end up being mid month before I can get back to it.

On Sunday, October 30, 2016, Braden Douglass notifications@github.com wrote:

Pinging you here @jkelleyj https://github.com/jkelleyj . What cha think about some of these code review requests? Would love to get this merged.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DynamoMTL/spree_chimpy/pull/76#issuecomment-257171369, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjW5NE_z5ZP5rfmqAudYAqoqxlILfUMks5q5OpxgaJpZM4KVLwI .

jkelleyj commented 8 years ago

Hey @braidn I know we need to get this done, so I went ahead and made some of the changes and refactored a bit. The orders interface was out of control, so I split it into more specific classes and broke out the specs as well. https://github.com/jkelleyj/spree_chimpy/tree/feature/mailchimp-3.0_refactor

If you like the look of this, I can merge into this branch. It handles everything in your comments except for the Rails URL helper. Are we able to simply remove that? You can see how I'm trying to use it in the products interface class in the refactor branch. Basically, using the rails helper if it responds to that or building it with URI otherwise. Let me know what I should do about that one.

jkelleyj commented 8 years ago

Just pulled in a refactored set of classes to clean up the order insert/update with some Updaters.