I'll use this issue to submit a patch that works with mailchimp version 8.x

CommentFileSizeAuthor
#14 interdiff_12-13.txt1.79 KBbrendanthinkshout
#14 mailchimp-retry_failed_communications-3029438-13.patch1.81 KBbrendanthinkshout
#12 3029438-12.patch1.59 KBcasey
#11 3029438-11.patch1.17 KBcasey
#10 interdiff-3029438-7-10.txt793 bytesaprice42
#10 retry-failed-communications-with-mailchimp-to-avoid-data-loss-3029438-10.patch961 bytesaprice42
#7 retry-failed-communications-with-mailchimp-to-avoid-data-loss-3029438-07.patch1.55 KBaprice42
#2 3029438-retry-failed-communications-with-mailchimp-to-avoid-data-loss.patch1.98 KBkarstenkmadsen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

karstenkmadsen created an issue. See original summary.

karstenkmadsen’s picture

karstenkmadsen CreditAttribution: karstenkmadsen commented
Assigned: karstenkmadsen » Unassigned
FileSize
3029438-retry-failed-communications-with-mailchimp-to-avoid-data-loss.patch1.98 KB

First of all: This module it really great. Great work guys.

This patch will fix the problem that rooby describes in parent issue.

To ad unsubscribe to the queue when the connection is off, I had to move the mailchimp_is_subscribed test down in the mailchimp_unsubscribe_process, to me this seems to be the correct solution. But I am wery open to feedback.

rjacobsen0’s picture

rjacobsen0 CreditAttribution: rjacobsen0 at ThinkShout for Mailchimp commented
Assigned: Unassigned » rjacobsen0
Status: Active » Needs review

Great! Nice patch! Would be nice to know how to reproduce the issue. @karstenkmadsen, Do you have a use case?

karstenkmadsen’s picture

karstenkmadsen CreditAttribution: karstenkmadsen commented

Use case:
1) Add a new user in drupal "and run cron", check in mailchimp that it got created there too.
2) alter the api-key to an invalid key, delete the user "and run cron". The user should now still be in the mailchimp list because the connection is broken.
3) alter the api-key back to the valid api-key "and run cron". With the patch installed the user should be deleted from mailchimp now. If the Patch is not installed mailchimp and drupal will be out of sync there.

Keep an eye in DB on the table "queue". Without the Patch: queue items get deleted even though the connenction to mailchimp is broken, thereby sync gets broken.

Karsten

rooby’s picture

rooby CreditAttribution: rooby commented
Version: 8.x-1.8 » 8.x-1.x-dev
Category: Feature request » Bug report
Priority: Normal » Major

I would say this is actually a bug, since it is a case of lost data when there is a broken transmission.

It's probably major, due to:

"(Lost user input, e.g. a failed form submission, is not the same thing as data loss and in most cases is major)" - From the priority handbook docs.

Patch still applies cleanly to latest dev.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson commented
Assigned: rjacobsen0 » aprice42
aprice42’s picture

aprice42 CreditAttribution: aprice42 at ThinkShout for Mailchimp commented
FileSize
retry-failed-communications-with-mailchimp-to-avoid-data-loss-3029438-07.patch1.55 KB

Re-rolled patch with the latest from 8.x-1.x.

aprice42’s picture

aprice42 CreditAttribution: aprice42 at ThinkShout for Mailchimp commented
Related issues: +#3073887: Can't unsubscribe from lists

This patch is not able to be tested without the patch from https://www.drupal.org/project/mailchimp/issues/3073887

aprice42’s picture

aprice42 CreditAttribution: aprice42 at ThinkShout for Mailchimp commented
Status: Needs review » Reviewed & tested by the community

This is working as outlined in #4 above.

aprice42’s picture

aprice42 CreditAttribution: aprice42 at ThinkShout for Mailchimp commented
FileSize
retry-failed-communications-with-mailchimp-to-avoid-data-loss-3029438-10.patch961 bytes
interdiff-3029438-7-10.txt793 bytes

I noticed something with this patch while working on another issue. This patch fixes the unintended reintroduction of some variables in mailchimp_unsubscribe(). It also fixes a minor coding standard issue.

casey’s picture

casey CreditAttribution: casey at SWIS commented
FileSize
3029438-11.patch1.17 KB

We've encountered that you can't keep failed items on the queue as that would result in the queue runner only retrying failing items. We also can't move failed item to the back of the queue as the order of the queue (for items regarding the same email address) is important.

Attached patch retries failed items once, immediately after failure. If it fails again, the item is moved to a separate "mailchimp_failed" queue for manual inspection.

This probably is not a final fix, but at least gives us some slack to find a better solution.

casey’s picture

casey CreditAttribution: casey at SWIS commented
FileSize
3029438-12.patch1.59 KB

Forgot to merge with patch from #10

aprice42’s picture

aprice42 CreditAttribution: aprice42 at ThinkShout for Mailchimp commented
Status: Reviewed & tested by the community » Needs review
brendanthinkshout’s picture

brendanthinkshout CreditAttribution: brendanthinkshout at ThinkShout for Mailchimp commented
FileSize
mailchimp-retry_failed_communications-3029438-13.patch1.81 KB
interdiff_12-13.txt1.79 KB

I really like the failed queue approach, @casey, but I think in the case of API outages, it's better to retry the next time a batch is processed instead of right away. I'd also like to specify a limited number of retries in a constant. What I'm not clear on is how this will behave with the queue ordering issues you mentioned. Do you have an example of how trying to process items with the same email address might go awry if one is pushed to the back of the queue?

gcb’s picture

gcb
he/him
CreditAttribution: gcb as a volunteer and at ThinkShout commented