search

veda-consulting-company / uk.co.vedaconsulting.mailchimp

Other
22 stars 43 forks source link

Interests incorrectly updated on Profile update Webhook #238

Closed xurizaemon closed 7 years ago

xurizaemon commented 7 years ago

When updating interests at Mailchimp, we found that only one Interest group would be reflected in the group memberships in CiviCRM. I identified relevant code in CRM_Mailchimp_Sync::splitMailchimpWebhookGroupsToCiviGroupIds().

Mailchimp webhook POST data includes Mailchimp Group names in merges.INTERESTS as a comma-space separated list: Cats, Dogs, Birds. (A Mailchimp Group name containing a comma will be sent with two slashes prefixed to the comma. @mailchimp maybe providing this list as more structured data would be a good thing? Mailchimp's docs indicate that there is no space submitted.)

To match against local groups, this extension's code surrounds that in commas (,Cats, Dogs, Birds) and then scans for each possible match with surrounding commas (,Cats, or ,Dogs, or ,Birds,).

The variation in spaces (,Cats, then , Dogs,) prevents any but the first supplied Mailchimp Group being matched.

It feels kinda funky that we don't just explode() the string, but there's a bit of other juggling happening there so I shot for adding a test and then minimal changes to the code in question 😸

... I had some additional comments here on how the Mailchimp Groups were ordered in that same function, but I've moved them to #240 to keep this clear.

xurizaemon commented 7 years ago

Since there's inconsistency between @mailchimp docs and observed behaviour, I've reached out to apihelp at mailchimp dot com for a clarification. I will update this issue when they respond.

Would be interested if anyone else is able to confirm this behaviour I'm seeing - might help Mailchimp also.

xurizaemon commented 7 years ago

@mailchimp have replied, and "it's an outdated docs bug" I think is the message here.

The site you are looking at is for our old API v2.0 and older versions. That documentation has not been kept up to date in about a year and a half, and is going to be taken down at the end of the year when we formally retire old versions of the API. Currently, there is no such documentation for webhooks on our newer site for API v3.0 [ http://developer.mailchimp.com/ ] so the best thing to do is exactly what you have been doing, making calls and seeing how things are formatted. That said, I can definitely see how having up to date documentation on that is very useful, and will gladly pass along your feedback to our developers.

So, I think despite the current/outdated docs saying we should expect the old One,Two,Three behaviour, for APIv3 (and preferably once someone else confirms this behaviour) we should expect One, Two, Three.

I see also that in my test data, there's only one Group (the first) reflected in merge.INTERESTS, and that all Groups are reflected in a second structure merge.GROUPINGS. It may be we should scan that instead for APIv3. That would be a separate issue though! (Update: #241.)

deepak-srivastava commented 7 years ago

Work on the ticket seems to have been merged. And v2.x is out. Please re-open if the original query still exists in relation to latest v2.x release.