Problem/Motivation

on $op == 'insert', 'delete', or 'after_update' in hook_user (line 71) The lists to act agains are generated from _mailchimp_get_required_lists. However, not every site has required lists. For those instances, no lists will be returned and the user will not get updated.

It seems like a more appropriate call would be to _mailchimp_get_available_lists, such as occurs in op == 'register' (line 22).

Proposed resolution

Change line 72 from:
foreach ((array)_mailchimp_get_required_lists() as $list) {
to
foreach ((array)_mailchimp_get_available_lists($account) as $list) {
so that users will get updated, regardless of whether lists are required or not.

This issue is related to but not the same as
http://drupal.org/node/708910
http://drupal.org/node/859334

CommentFileSizeAuthor
#5 mailchimp-sync-optional-lists-1226958.patch1.33 KBthebuckst0p
#4 mailchimp.module.patch559 byteskanani
#2 mailchimp.module.patch947 bytesNetrics
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Netrics’s picture

Netrics CreditAttribution: Netrics commented

Also change the 'after_update' on line 71 to 'update' as they use reference to the $edit parameter to check the old edited e-mail address. But now this allow changing the e-mail address, but doesn't resolve the conflict with this issue: http://drupal.org/node/1097718#comment-4759712 NVM I tried creating new profile fields in drupal, then creating them as MERGE tags in MailChimp admin, and linked them via the module admin. I then tested this with many accounts changing the profile fields and e-mail addresses for all accounts. They all synced correctly.

Netrics’s picture

Netrics CreditAttribution: Netrics commented
Status: Active » Needs review
FileSize
mailchimp.module.patch947 bytes

I've submitted a patch that will make these changes to the module. Whoever has CVS/Git access please review it and summit it through.

kanani’s picture

kanani CreditAttribution: kanani commented

Thanks for the patch.

But I think so that we don't conflate the two we need to keep the two issues your addressing separated.

One for the update/after_update issue, and a second for the required vs available lists. I was actually hoping to get some feedback from levelos as to desired behavior since there has been some back and forth regarding update vs after_update.

kanani’s picture

kanani CreditAttribution: kanani commented
FileSize
mailchimp.module.patch559 bytes 
--- a/sites/default/modules/contrib/mailchimp/mailchimp.module.orig	2011-07-25 11:11:43.000000000 -0700
+++ b/sites/default/modules/contrib/mailchimp/mailchimp.module	2011-07-25 11:12:08.000000000 -0700
@@ -69,7 +69,7 @@
   }
 
   if (in_array($op, array('insert', 'delete', 'after_update')) && $q = _mailchimp_get_api_object()) {
-    foreach ((array)_mailchimp_get_required_lists() as $list) {
+    foreach ((array)_mailchimp_get_available_lists($account) as $list) {
       $action_taken = FALSE;
       switch ($op) {
       // delete a user from MC list
thebuckst0p’s picture

thebuckst0p CreditAttribution: thebuckst0p commented
Version: 6.x-2.5 » 7.x-2.0
Component: Code » General
FileSize
mailchimp-sync-optional-lists-1226958.patch1.33 KB

Porting the patch to 7.x-2.0. However, this is not a good approach for this version -- if it filters by optional lists, then it excludes required lists! There needs to be some hierarchy, so optional equals optional+required but required only equals required... or something like that. Thoughts?

levelos’s picture

levelos CreditAttribution: levelos commented
Version: 7.x-2.0 » 6.x-2.x-dev

This does not apply to 7.x-2.x.

levelos’s picture

levelos CreditAttribution: levelos commented
Title: hook_user fails to update users on $op == 'insert', 'delete', 'after_update' when a site is not configured to have required list » process optional as well as required lists during hook_user() and cron
Category: bug » feature
Status: Needs review » Active

The hook_user questions is being discussed over at #1097718. At this, time, we are not going to re-architect the 6.x version of module to process optional lists in hook_user. It's not as simple as changing _mailchimp_get_required_lists() to _mailchimp_get_available_lists(). For one thing, optional lists are not processed via cron, so if the cron flag is set, none of the update code will fire. The intent of the design to mirror what happens with required lists during cron, but do it synchronously.

I've changed the title and category of the ticket to reflect the source issue. Happy to leave it open for now.

j0rd’s picture

j0rd CreditAttribution: j0rd commented

I've got a solution for this over at
#859334: Update/sync optional lists as well as required lists with cron for D6 version.

I don't believe my version deals with hook_user though, but it does do cron.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
Category: Feature request » Bug report
Issue summary: View changes

We just ran into this, and I assure you it is a bug.

  1. We have our configuration set to update a user NOT at Cron, but when they update their account. This makes sense because I'd prefer to have the change happen immediately.
  2. Our mailing lists are not "required" because this doesn't make sense. We want people to be able to Opt-out (we automatically have them opted-in during the registration process).

The expected behavior is the User's email address gets updated at MailChimp if they change it on the Drupal side. The actual behavior is that nothing gets updated except for in Drupal. Even the description for the Cron checkbox is: "Sync Required Lists During Cron. If this is set, users will be subscribed to the required list during cron runs. Otherwise subscription will take place when a user is added/edited." All of these added together makes it seem as though you must have your lists set to "Required" to make sure email addresses are synced with MailChimp.

This is a bug, or at least a major design flaw.

For now it seems I will have to enable the Cron updates, but this is not ideal.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
Status: Active » Reviewed & tested by the community

FYI, I just tested the patch in #4 and it seems to work exactly as described (and as one might expect):

1. With Cron sync disabled, the update happens and the MailChimp API sync is fired.
2. With Cron sync enabled, the user gets added to the queue.

Our lists are not Required, however, and so I'm not sure if this feature requires more testing - but IMO, it shouldn't matter what the list type is. If a user is changing their email address, it should update wherever it is subscribed. Otherwise what's the point of having an update hook?

I'd say this is RTBC - or if the maintainer believes more attention needs to be in place due to the Required Lists/Cron interaction, I'd be happy to work on a more in-depth patch.

The last submitted patch, 2: mailchimp.module.patch, failed testing.

The last submitted patch, 4: mailchimp.module.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: mailchimp-sync-optional-lists-1226958.patch, failed testing.

nrackleff’s picture

nrackleff CreditAttribution: nrackleff commented
Status: Needs work » Closed (won't fix)

“And now our watch [for support of the 6.x version of the MailChimp module] has ended…” With the end of Drupal 6 support, I’m sad to say we too must turn the page.

Fret not! The 7.x-4.x and 8.x versions come highly recommended. Both are using Mailchimp’s new API 3.0 and are being actively maintained. “What is dead may never die, but rises again, harder and stronger!”