When mass subscribe exclude unsubscribed subscribers

We don't delete users.

Simplenews stores who has been unsubscribed and also allow to export unsubscribed users / subscribers.
However it doesn't store a full history about subscribe / unsubscribe per newsletter. It only stores last subscribe / unsubscribe date.

We should only consider the state is the user on mass import.
This can be provided as a (default) option:
Unsubscribe state:
- Don't resubscribe unsubscribed subscribers... (default)
- Subscribe provided list without any filter

Possibly the option needs better wording. Any suggestion?

Some Reference
#528808: distinguish between "not subscribed" and "unsubscribed"

Closing #1068608: Prevent mass subscription of previously manually unsubscripted email addresses as a duplicate

Tagging issues for test-first candidates

Related to
#706904: List of outstanding, unconfirmed, anonymous subscription requests.

There's some thoughts more about how statuses should work in future.

I fixed this for myself for Drupal 6 / Simplenews 2-0 Alpha2. See here: #1379060: When mass subscribe exclude unsubscribed subscribers (Drupal 6 Version, fixed, plz apply patch)

maybe, when #1379060: When mass subscribe exclude unsubscribed subscribers (Drupal 6 Version, fixed, plz apply patch) is reviewed and fixed, then the Drupal 6 fix could be frontported for this ticket here.

Oops, correct tagging.

Diff against wrong branch. Next try.

+++ b/includes/simplenews.admin.incundefined
@@ -725,6 +725,12 @@ function simplenews_subscription_list_add($form, &$form_state) {
+    '#description' => t('By default simplenews ignores users priviously subscribed. Setting this option will force that those users ar getting resubscribed. Consider that this might be aginst the will of your users.'),

Hm, can we maybe simplify/shorten the description a bit?

"If checked, previously unsubscribed e-mail addresses will be resubscribed. Consider that this might be against the will of your users."

Not sure about the last sentence but we can leave it like that for now.

+++ b/includes/simplenews.admin.incundefined
@@ -774,10 +781,17 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
-        simplenews_subscribe_user($email, $category->tid, FALSE, 'mass subscribe', $langcode);
-        $added[] = $email;
+        $is_unsubscribed = $subscriber && array_key_exists($category->tid, $subscriber->newsletter_subscription)
+              && $subscriber->newsletter_subscription[$category->tid]->status == SIMPLENEWS_SUBSCRIPTION_STATUS_UNSUBSCRIBED;

Can we add a comment above this line? Explain that we are checking if the has a record for that newsletter and is unsubscribed or something like that.

+++ b/includes/simplenews.admin.incundefined
@@ -774,10 +781,17 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
+        }
+        else {
+          $unsubscribed[$category->tid] = $email;

This will currently result in saving only a single e-mail per category. Should use [].

If you extend your tests to use multiple e-mail addresses, you should be able to reproduce this.

+++ b/includes/simplenews.admin.incundefined
@@ -802,6 +816,12 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
+    drupal_set_message(t('If you like to resubscribe the skipped addresses use the \'Force resubscription\' option.'), 'warning');

"If you would like to resubscribe them, use the 'Force resubscription' option."

Also, try to avoid \ in translatable strings, instead use "", some translation tools tend to double-escape these \.

+++ b/tests/simplenews.testundefined
@@ -1458,6 +1458,31 @@ class SimpleNewsAdministrationTestCase extends SimplenewsTestCase {
+    $edit = array(
+      'emails' => $subscriber->mail,
+      'newsletters[' . reset($subscriber->tids) . ']' => TRUE,

Let's extend this and test three mail addresses, two unsubscribed and a subscribed one.

+++ b/tests/simplenews.testundefined
@@ -1458,6 +1458,31 @@ class SimpleNewsAdministrationTestCase extends SimplenewsTestCase {
+    $this->assertTrue(simplenews_user_is_subscribed($subscriber->mail, reset($subscriber->tids), t('Subscriber resubscribed throught mass subscription.')));

"throught" => "through". Same above.

+++ b/includes/simplenews.admin.incundefined
@@ -802,6 +817,19 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
+      drupal_set_message(t('The following addresses were skipped because they have previously unsubscribed: %unsubscribed.', array('%unsubscribed' => $subscribers)), 'warning');

As discussed, we need to add the category name here.

+++ b/tests/simplenews.testundefined
@@ -1499,6 +1458,46 @@ class SimpleNewsAdministrationTestCase extends SimplenewsTestCase {
+    $this->assertFalse(simplenews_user_is_subscribed( $tested_subscribers[0], $first, t('Subscriber not resubscribed trough mass subscription.')));

through :)

+++ b/tests/simplenews.testundefined
@@ -1499,6 +1458,46 @@ class SimpleNewsAdministrationTestCase extends SimplenewsTestCase {
+    $this->assertText(t('If you would like to resubscribe them, use the "Force resubscription" option.'));

I'd suggest doing this the other way round, sorry for not being clear. " ... ' ... '.. ", aka having the single quotes in the string, not the otherway round.

One last time, promised!

+++ b/includes/simplenews.admin.incundefined
@@ -774,10 +781,18 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
+        // If there is a valid subscriber, check if there is a subscription for the current category and if this subscription has the status unsubscribed.

The comment should be wrapped at 80 characters.

+++ b/includes/simplenews.admin.incundefined
@@ -802,6 +817,19 @@ function simplenews_subscription_list_add_submit($form, &$form_state) {
+  $show_hint = FALSE;
+  foreach ($unsubscribed as $name => $subscribers) {
+    if ( $subscribers ) {
+      $subscribers = implode(", ", $subscribers);
+      drupal_set_message(t('The following addresses were skipped because they have previously unsubscribed from %name: %unsubscribed.', array('%name' => $name, '%unsubscribed' => $subscribers)), 'warning');
+      $show_hint = TRUE;
+    }

Do we really need the $show_hint variable? Can't we simply do a !empty($unsubscribed) in the if below?. Because if we never added anything, it will be an empty array. It is not possible to have a category in there but no subscribers...

Which also means that the if here is not necessary either.

+++ b/tests/simplenews.testundefined
@@ -1437,6 +1437,46 @@ class SimpleNewsAdministrationTestCase extends SimplenewsTestCase {
+    $this->assertFalse(simplenews_user_is_subscribed( $tested_subscribers[0], $first, t('Subscriber not resubscribed through mass subscription.')));
+    $this->assertFalse(simplenews_user_is_subscribed( $tested_subscribers[1], $first, t('Subscriber not resubscribed through mass subscription.')));
+    $this->assertTrue(simplenews_user_is_subscribed( $tested_subscribers[2], $first, t('Subscriber subscribed through mass subscription.')));

While you're at it, the space in front of $tested_subscribers isn't necessary.

Tests otherwise look great and should give us the ability to quickly verify my suggested simplifications of the above code.

#16: simplenews-1315420-16.patch queued for re-testing.

Commited. Marking as a possible backport and closing #1379060: When mass subscribe exclude unsubscribed subscribers (Drupal 6 Version, fixed, plz apply patch) as a duplicate.

Hi guys,
Not sure if there's an update to this one, but I'm confused as to how it's supposed to be working on the 6.x-2.x-.dev branch currently?
In my tests, we're always able to re-subscribe users regardless of whether they unsubscribed themselves, or indeed, even if their listing is shown as deactivated. In this case SN simply allows the mass subscribe operation to proceed, and reports that all users have been subscribed to a selected newsletter.
When we check, we find the deactivated user has indeed been ticked on as a subscriber to the newsletter, even though they are shown as a deactivated user. Surely this is an odd outcome to silently re-subscribe deactivated users?
This is reasonably mind-bending territory, so I apologise in advance if we've missed the "correct" application/intent of this otherwise very useful-sounding feature.
Thanks in advance!

It's not supposed to work. That's why the status is at "patch (to be ported)", which means it has been commited 7.x-1.x needs to update the patch to work with 6.x-2.x so that it can be commited there as well.

Thanks for pointing me at the obvious: guess I was being a bit optimisitic. Thanks again for your energies and contributions,

a 6x-2x patch would be greatly appreciated

corrected typo

has this patch been committed to the D6 version?

:-)

Back to 7.x where it was fixed.
Will not happen for 6.x