When you mass unsubscribe users, it would be good to have an option to inactivate instead of delete users.

Use case :

Currently :
- user joe subscribes
- user joe sends admin an email "Please remove me from your list"
- admin uses mass unsubscribe to remove user joe
- at a later stage some users are mass imported (including joe)
- joe is subscribed again
- joe is unhappy

With this feature :
- user joe subscribes
- user joe sends admin an email "Please remove me from your list"
- admin uses mass unsubscribe to remove user joe and checks a box "inactivate users" (or use a "mass inactivate" link)
- at a later stage some users are mass imported (including joe)
- joe, being inactive is not subscribed again
- joe is happy

Hope that makes sense

Files: 
CommentFileSizeAuthor
#16 simplenews-1315420-16.patch6.81 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 1,755 pass(es).
[ View ]
#14 simplenews-1315420-14.patch6.87 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 1,753 pass(es).
[ View ]
#12 simplenews-1315420-12.patch9.43 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 1,713 pass(es).
[ View ]
#10 simplenews-1315420-10.patch4.74 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 1,707 pass(es).
[ View ]
#8 simplenews-1315420-8.patch3.59 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simplenews-1315420-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Title:When mass unsubscribe allow to inactivate instead of deleteWhen 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"

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.

Issue tags:+test candidate

Oops, correct tagging.

Assigned:Unassigned» corvus_ch
Issue tags:-test candidate+Needs tests

Status:Active» Needs review
StatusFileSize
new3.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simplenews-1315420-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, simplenews-1315420-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 1,707 pass(es).
[ View ]

Diff against wrong branch. Next try.

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new9.43 KB
PASSED: [[SimpleTest]]: [MySQL] 1,713 pass(es).
[ View ]

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 1,753 pass(es).
[ View ]

Status:Needs review» Needs work

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.

StatusFileSize
new6.81 KB
PASSED: [[SimpleTest]]: [MySQL] 1,755 pass(es).
[ View ]

Status:Needs work» Needs review

The last submitted patch, simplenews-1315420-16.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests

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

Version:7.x-1.x-dev» 6.x-2.x-dev
Status:Needs review» Patch (to be ported)
Issue tags:-Needs tests

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.

Assigned:corvus_ch» Unassigned

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

Issue summary:View changes

corrected typo

has this patch been committed to the D6 version?

:-)