Sending an item to multiple newsletters

Sorry for not providing any feedback earlier.

Haven't looked at everything yet, what I don't really like is that you force the field to cardinality multiple and control that in the widget then. I wondering if we can do better. We now have our own field type, and instead of hardcoding that field, I wonder if we can find a way to support multiple fields of that type. We already kind of do in places, see simplenews_get_content_types(), we'd have to for example not hardcode the field name there but loop over them.

Then you could define your own field if you want to send out a newsletter multiple times. Getting there would however probably be quite an efffort. All the hardcoded "->simplenews_issue" logic would have to change.

I'll try to read through the patch soon and provide more feedback. Don't start with implementing that above, I need to think about it some more anyway.

Maybe this can solve the problem #2302133: Replace getInfo() in tests with phpDoc. Also please @arit_nath configure your code editor properly, you have a lot of extra lines that should not be there.

There has been some problems with CI errors, actually I am not sure how exactly we can fix them, maybe the reference is not the fix for it. Besides that, thank you so much for fixing the extra lines but also we should fix the indentation.

+++ b/src/Form/NodeTabForm.php
@@ -30,7 +30,18 @@ class NodeTabForm extends FormBase {
+	// Handling the multi-newsletters options
+	$subcriber_ids = array();
+	foreach($node->simplenews_issue as $simplenews_issue){
+		$subscribers = db_select('simplenews_subscriber__subscriptions', 'r')->fields('r', array('entity_id'))->condition('subscriptions_target_id', $simplenews_issue->target_id)->condition('subscriptions_status', 1)->execute()->fetchAll();
+		foreach($subscribers as $subscribe){
+			 if (!in_array($subscribe->entity_id, $subcriber_ids)){
+				$subcriber_ids[] = $subscribe->entity_id;
+			 }
+		}

The extra indentation here and in some parts of the last patch are considered unrelated changes.

The indentation looks perfect now.

A first try.

1. +++ b/src/Form/NodeTabForm.php
   @@ -30,7 +30,18 @@ class NodeTabForm extends FormBase {
   +     $subscribers = db_select('simplenews_subscriber__subscriptions', 'r')->fields('r', array('entity_id'))->condition('subscriptions_target_id', $simplenews_issue->target_id)->condition('subscriptions_status', 1)->execute()->fetchAll();
   ...
   +          $subcriber_ids[] = $subscribe->entity_id;
   

   It means with large recipient counts you temporary have a list of ALL recipients here - and this for counting only.
   I think a grouped query should be possible though.

2. +++ b/src/Plugin/Field/FieldWidget/SimplenewsIssueWidget.php
   @@ -0,0 +1,146 @@
   +    $summary[] = t('Use field label: @allow_multiple', array('@allow_multiple' => ($display_label ? t('Yes') : 'No')));
   

   "Use field label:"? I guess more "Allow multiple:"

3. +++ b/src/Plugin/Field/FieldWidget/SimplenewsIssueWidget.php
   @@ -0,0 +1,146 @@
   +   if($field_storage->getCardinality()!=1){
   ...
   +     if($this->getSetting('allow_multiple')=="1"){
   

   Codestyle.. And similarly many other occurrences.

4. +++ b/src/Spool/SpoolStorage.php
   @@ -238,32 +238,35 @@ class SpoolStorage implements SpoolStorageInterface {
   +      $select->condition('s.id', db_select('simplenews_mail_spool', 'r')->fields('r', array('snid'))->condition('entity_id', $node->id(), '='), 'NOT IN');
   

   Interesting exclude. It seems to work this way.
   Not sure how this fits our receiver pluggability.

We will need to further discuss this request before providing a qualified statement.

A client of mine who has been sending his newsletters with Simplenews for many years now, decided to distinguish between different topics in the future and hence this exact issue became relevant as a newsletter might cover more than one topic.

Thus, I would like to ask about the current state of the possibility to send a single issue to multiple newsletters, ideally with a system preventing user from getting the issue multiple times when subscribed to more than one topic.

So more specific:

1. @jonathanshaw: Do you have a working system in place? Is it stable? Would you mind sharing your setup?
2. @Berdir: Is this feature still part of the roadmap or would you mark it as incompatible with Simplenews architecture?

@jonathanshaw

The result is that the user can leave the field cardinality as single and use the select widget, or set te cardinality to multiple and select the widget form this patch, and either way the email sending works fine.

Or even better have a single widget that works correctly whatever the cardinality on the field.

@Paul Broon As far as I know this feature is still part of the roadmap. I think the patch in this issue is going in the right direction but I haven't tested/reviewed it and I think it might be missing "preventing user from getting the issue multiple times".

@jonathanshaw #37 I'm dubious about your approach, and feel you might be better following the direction in the patch.

The simplenews_issue field is the key to determine configuration for mail building/delivery and is hard-coded into the code in many places so I think you will be in trouble if that field isn't set. It determines which recipient hander gets called. It determines which newsletter settings get used for "email settings", "sender information", "newsletter subject".

A key question is what does it even mean to send to multiple newsletters. One approach is simply to process each one in turn and I think the patch does that, but then a subscriber will get multiple mails if they are on multiple lists.

So it is tempting to try and avoid duplicates by tracking which subscribers have been added and avoiding adding them again. Beware the "duplicate" emails won't be identical - they can have different subjects, formats, sending info and (crucially) unsubscribe links. So it becomes relevant to determine which newsletter has priority - perhaps newsletters could have a weight and be rearranged in order on /admin/config/services/simplenews?

The unsubscribe link could be replaced with two (similar to MailChimp I think) "manage subscriptions" takes you to /newsletter/subscriptions and "unsubscribe" unsubscribes from all. This change would be desirable even without "multiple newsletters" - if a user subscribed to multiple lists originally, they will be frustrated to have to unsubscribe multiple times (legally GDPR requires that unsubscribe must be as easy as subscribe).

I am willing to review/commit this issue if someone will work on it to resolve outstanding issues.

Thanks very much for creating a patch - for sure this is a step in the right direction. I had a quick test and I found some problems.

1. Design problem: with multiple values selected for the newsletter, this leads to storing multiple copies of all of the properties including status. However the node should have a single send status. Perhaps the solution is to keep Newsletter as single-valued, and a an extra ordinary entity reference field "additional newsletters" and these are used only for picking up subscribers?
2. Related bug: send later and cancel only affect the first of these multiple copies.
3. Related bug: duplicate rows in /admin/content/simplenews (can fix with the query settings of the view).
4. In NodeTabForm you have replaced a call to simplenews_count_subscriptions with a block of code. This code needs to be in a function.
5. Related bug: Various other places in the code that currently call simplenews_count_subscriptions also need to be changed to call the new function, including SendStatus.php.
6. Related bug: The new function must match simplenews_count_subscriptions more closely, including the check on ss.subscriptions_status.
7. I wonder if SimplenewIssueWidget is needed - multiple values seems to work fine using a options_select.
8. Missing: prevent sending the same mail twice to a subscriber on multiple newsletters.

My current idea is that this could be solved with a new recipient handler RecipientHandlerMultiple. It would implement a form with a single field "additional newsletters" which is a multi-valued reference to newsletter. It should be possible for RecipientHandlerMultiple->buildRecipientQuery() to avoid sending to the same subscriber twice.

This would need fixes to some of the problems in #3052744: [META] Problems with RecipientHandler.

@Paul Broon You can try to make this work by writing RecipientHandlerMultiple in your own contrib module. If you do please post a link to your code.

On further thought, I see problems with the RecipientHandler approach from #40. Suppose that the main newsletter is set to AA and additional newsletter to BB.

• BUG: A subscriber of BB will see mails with the subject, from address etc from AA (should be BB).
• BUG: If an view based on the newsletter admin page admin/content/simplenews with a filter on newsletter = BB then this issue would be missing (should be present).

So I am back to favouring the multi-valued field approach. Here's I how I see it working.

1. Patch makes no change to field settings for allowed number of values - leave this to site admin to decide.
2. Parch makes no change to manage form display settings, and does not create a new widget - leave this to site admin to decide.
3. Fix SpoolStorage to work correctly if it finds a multi-valued field - including avoiding of duplicates to the same email address.
4. Fix UI (NodeTabForm and SendStatus) to display clear & correct information for a multi-valued field.

The SpoolStorage work should start with fixing some tasks to consolidate code into clear functions within the SpoolStorage service.

1. #3052727: Create clear consolidated functions to send/cancel newsletters
2. #3030432: Simplenews list page confusing/wrong "send status"
3. #3052744: [META] Problems with RecipientHandler - rely on creation of SpoolStorage::countSubscriptions so that this issue can extend it; rely on returning a list of email addresses to avoid duplicates.

Would welcome any comment from @Berdir thanks!

IS updated to match latest thinking/comments.

If anyone is keep to work on this issue please let me know.

Thanks to both on you for suggesting possible solutions to me. I have been deep into developing a new module and didn't have time to look into this issue. I was quite familiar with Simplenews for D7 and even wrote some patches, however, I haven't had time, yet, to dig into the D8 version in order to help a lot.

As this feature is not a hard requirement of my client for the relaunch of their website, I will only start to look into this around mid-June. My approach is to first get a grasp on how Simplenews works in the D8 universe, especially the dependencies Adam mentioned about the hard-coded simplenews_issue field occurrences. All the points are also valid for consideration (like Views of newsletter issues). Right now, it seems like an overwhelming task to get that into core Simplenews.

I'll keep you posted when I start working on it.

We have a need for this functionality as well.

+1 to making the field work as multivalue. This is what site builders would expect, as is a common pattern.

I poked around a bit with some ideas, to try and understand the situation described in the IS and comments, however its clear that things have moved around quite a bit since the #31 patch...

Looking at 3.x - what would the best approach look like?

Thanks @jonathanshaw, seems like a new field and a custom recipient handler would get the job done for us. I will give it a shot (we'll need to upgrade to 2.x first...)

#43/47 seems good to me. To clarify:

• The simplenews_issue field type already has a target_id which would be the main newsletter to use for format etc.
• So the custom recipient handler needs just one new field which is a multi-valued entity reference called something like "Extra newsletters" that would also be sent to.

OK so on and off I've been hacking at this, and so far managed to get a recipient handler going that will send to multiple newsletters selected by the user on an extra entity reference field (referencing newslettters).

So that works great, except in the case where 1 email address is subscribed to multiple newsletters, in which case the email gets sent twice to the same recipient. I have not been able to "deduplicate" on the recipient handler though, has anyone gotten around this?

Here is my code thus far, you'll note that I am extending RecipientHandlerAll - not sure if this is limiting what we can do here or not...

/**
 * This handler sends a newsletter issue to custom field newsletters also.
 *
 * @RecipientHandler(
 *   id = "custom_multiple_newsletters",
 *   title = @Translation("Multiple newsletters recipient handler")
 * )
 */
class CustomMultipleNewslettersRecipientHandler extends RecipientHandlerAll {

  /**
   * {@inheritdoc}
   */
  protected function buildRecipientQuery() {
    // Method copy/pasted from RecipientHandlerAll::buildRecipientQuery().
    $select = $this->connection->select('simplenews_subscriber', 's');
    $select->innerJoin('simplenews_subscriber__subscriptions', 't', 's.id = t.entity_id');
    $select->addField('s', 'id', 'snid');
    $select->addField('t', 'subscriptions_target_id', 'newsletter_id');

    // The only difference is this condition:
    $newsletter_ids = $this->getNewsletterIds();
    $select->condition('t.subscriptions_target_id', $newsletter_ids, 'IN');

    $select->condition('t.subscriptions_status', SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED);
    $select->condition('s.status', SubscriberInterface::ACTIVE);

    return $select;
  }

  /**
   * Gets the list of newsletter ids to send the issue to.
   *
   * @return array
   *   The newsletter ids.
   */
  public function getNewsletterIds() {
    // Always include the newsletter from the simplenews field.
    $newsletter_ids = [$this->getNewsletterId()];
    // Get the values of the extra notifications field and add them.
    if ($this->issue->hasField('field_extra_notifications')) {
      $extra_newsletters = $this->issue->field_extra_notifications->getValue();
      if ($extra_newsletters) {
        foreach ($extra_newsletters as $newsletter) {
          if (!in_array($newsletter['target_id'], $newsletter_ids)) {
            $newsletter_ids[] = $newsletter['target_id'];
          }
        }
      }
    }
    return $newsletter_ids;
  }

  protected function getNewsletterId() {
    return $this->issue->get('simplenews_issue')->target_id;
  }

}

Thank you @jonathanshaw,

Very happy to hear you're willing to fund the feature, I have raised your proposal up with the team, and I'm happy to say that we agree to take on the test coverage for it. I now have created a ticket on our backlog for this porpuse. For now we'll will wait on writing the test coverage until we have a clear scenario from @AdamPS and/or a working patch.

Regarding my workaround I ended extending RecipientHandlerEntityBase, which has done the trick and is cleaner than my previous approach, so thanks for the suggestion there as well. Here it is in case anyone is curious:

namespace Drupal\my_module\Plugin\simplenews\RecipientHandler;

use Drupal\simplenews\Plugin\simplenews\RecipientHandler\RecipientHandlerEntityBase;
use Drupal\simplenews\SubscriberInterface;

/**
 * This handler sends a newsletter issue to custom field newsletters also.
 *
 * @RecipientHandler(
 *   id = "multiple_newsletters",
 *   title = @Translation("Multiple newsletters recipient handler")
 * )
 */
class MultipleNewslettersRecipientHandler extends RecipientHandlerEntityBase {

  /**
   * {@inheritdoc}
   */
  protected function buildEntityQuery() {
    return \Drupal::entityQuery('simplenews_subscriber')
      ->condition('status', SubscriberInterface::ACTIVE)
      ->condition('subscriptions', $this->getNewsletterIds(), 'IN')
      ->condition('subscriptions.status', SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED);
  }

  /**
   * Gets the list of newsletter ids to send the issue to.
   *
   * @return array
   *   The newsletter ids.
   */
  public function getNewsletterIds() {
    // Always include the newsletter from the simplenews field.
    $newsletter_ids = [$this->getNewsletterId()];
    // Get the values of the extra notifications field and add them.
    if ($this->issue->hasField('field_extra_notifications')) {
      $extra_newsletters = $this->issue->field_extra_notifications->getValue();
      if ($extra_newsletters) {
        foreach ($extra_newsletters as $newsletter) {
          if (!in_array($newsletter['target_id'], $newsletter_ids)) {
            $newsletter_ids[] = $newsletter['target_id'];
          }
        }
      }
    }
    return $newsletter_ids;
  }

  /**
   * {@inheritdoc}
   */
  protected function getNewsletterId() {
    return $this->issue->get('simplenews_issue')->target_id;
  }

}

Great work from the community here, each person contributing a part.

I expect to start work on this issue soon. The code from Manuel Garcia is a good start thank you. I will post a new patch soon, should be within 1 month.

Here is a patch using recipient handlers.

If the module goes in this direction then it means to reinforce the current assumption that simplenews_issue is single-valued. Therefore I included a simplification, removing a little bit of code for handling multiple values.

However as I re-read the comments I have a concern, see #38. The subscriber of one of the additional newsletters is going to get a message that by all appearances is for a newsletter he didn't subscribe to. Crucially, I think the unsubscribe links are likely to be broken. And it's hard to see how to fix that - maybe the subscriber is subscribed to both B and C, then which one do you unsubscribe from?

The unsubscribe link could be replaced with two (similar to MailChimp I think) "manage subscriptions" takes you to /newsletter/subscriptions and "unsubscribe" unsubscribes from all. This change would be desirable even without "multiple newsletters" - if a user subscribed to multiple lists originally, they will be frustrated to have to unsubscribe multiple times (legally GDPR requires that unsubscribe must be as easy as subscribe).

Thanks for the comments

1. I agree, done

2. The recipient handler plug-in doesn't have a separate method for defaults.

4. True, there is a general issue. However mostly when I create a newsletter based on a recipient handler then I set it to 'hidden' which removes the unsubscribe URL entirely. Normally with a RH there is no actual list that the subscriber is on or could be removed from. They are getting the mail because they are a member or they bought something or some other condition. It would useful to have #3037139: Add "subscribed reason" explanatory text to newsletter for this case. Other than that it works fine.

There is a specific problem here that is much worse than the general one. In this case, the subscriber is on a list so there ought to be a way for them to leave it. Plus we might have a link but it's broken. However I guess the broken link could be avoided with the 'hidden' trick as above.

I can see two possible solutions:

1) Add each subscriber to the spool with the 'newsletter_id' field set to one of the newsletters that they signed up to. So they will get the 'format' from that newsletter, which is probably mostly good: the email subject line will match something the subscriber agrees they signed up to. This would mean to derive from RecipientHandlerBase instead of RecipientHandlerEntityBase.

2) Go a step further and make this RH a generic "newsletter combiner" - even allowing combining of newsletters that are non-standard RHs themselves. I.e. it calls the addToSpool() function for each of the target newsletters in turn with some magic to remove duplicates.

I'm not sure off the top of my head which one is easier.

Yes #57 all makes sense. However don't forget this part:

Plus we might have a link but it's broken. However I guess the broken link could be avoided with the 'hidden' trick as above.

It's not so hard: any newsletter with this RH must be set to hidden. Plus as you already said, the newsletter footer should contain the manage-url token. So it's perfectly fine to use the RH if you understand some technical details.

However on the other hand it's not so easy:-) By default a newsletter allows any RH and is not hidden. There is no documentation for RHs. The manage token is not present by default and not documented. So I don't think this is ready for commit yet.

I have raised #3195323: Simplify and safeguard configuration of Recipient Handlers which I think we should commit before this one.

This needs tests. @Manuel Garcia if you would help with that it would be great.

I propose that we can check this into the demo module along with all the other RHs. I feel that's fine because it then seems more like an example idea rather a fully tested solution.

Back to "needs work" for the tests

We've been using the patch in #31 for about 2 years (of which around 8 months in production), which has been working well for us. The problem has been that we have not been able to update the module to 2.x.
I looked at that patch and tried to see what needed to be done for our setup to work with 2.x. Basically I just needed to make one minor change to be able to send the newsletter to recipients of the multiple newsletters that is chosen but without sending it twice if one address subscribes to two newsletters.

I will add a patch with the change I made. It looks like the basic idea of that’s in the patch in #60, but modified directly in RecipientHandlerAll.
How would I be able to use the code in #60?

The way we use this functionality is as follows:
We have a content type called Article. Not all articles are supposed to be sent as newletters, so we have a checkbox on it which makes an entity reference field with newsletters visible where it’s possible to select multiple newsletters.
When an Article is saved, if it should send a newsletter we programmatically create a Newsletter issue with the same node title as the Article and a custom body which has a link to the Article and some other text, and just puts the same newsletters in the Newsletter field that was selected on the Article.
The Newsletter issue content type has the Newsletter field (Simplenews issue) set to allow multiple values.

All our newsletters are set to just have the title of the Newsletter issue as the subject of the email. We have modified the twig output of the newsletters to include a custom unsubscribe link which goes to /newsletter/subscriptions so the recipient can manage all their subscriptions. So we don’t have any problems with the title or unsubscribe link being specific to a newsletter.

Thanks for the information!

I have tried RecipientHandlerMultiple and have found the following:
* When I modify the Newsletter field on the Newsletter Issue content type and export configuration the entire default_value setting for the field is emptied and when I view the edit page of the field I can no longer modify the Recipients setting. I have to manually set the handler to simplenews_multipe in configuration and import.
* In settingsForm I had to make the following changes
** Declare $options as an empty array (got an error before I had any newsletters otherwise)
** Change the default default value from NULL to []
* I can no longer have my simplenews_issue field set to allow multiple values, but I can change that since I programmatically create the Newsletter issues, so I can modify my creation to create the handler settings instead.

Has anyone else had problems with the configuration export of the field?

Update:
I have tested on simplytest.me with both the patch which adds the plugin to the demo module and without it, where I have enabled the demo module and when I save the Newsletter field on the Newsletter Issue content type all default value settings gets removed and it's not possible to get them back without manually adding it to config.

Very strange. I expect it would be tricky and non-BC to try and integrate PluginFormInterface at this stage.

There is WidgetInterface::massageFormValues() which could implemented in IssueWidget. Inspecting that using the debugger might give some insight on what code there would help.

I also noticed some mysterious code that I possibly wrote but can't recall now in IssueWidget::formElement() see the comment "Ensure that the extra properties are preserved." Maybe the checkboxes somehow don't trigger this correctly?

Basically I don't really know, and it seems more in need of Drupal core expert than a simplenews one.

I would suggest creating a separate issue if you want to continue the discussion - it seems like a general recipient handler problem.

I have a few questions about this implementation. It's great that we have this feature, but I'd like to suggest that we use delta for storing multiple newsletters, rather than a serialized value in simplenews_issue_handler_settings. This seems counter to normal Drupal best practice. Also when I select a newsletter that allows a simplenews_multiple RecipientHandler, the value in my database for that field is NULL.

See the attached screenshot.

the patch no longer applies to simplenews 4.x

Hi Jonathan(@jonathanshaw), how are you? I've been facing a similar issue. I implemented a custom handler with checkboxes, but when saving the node for the first time, the values aren't stored in the database. However, if I edit the node and save again, the checkboxes are saved correctly.

I haven't found a solution yet. I saw your comment #67, but what you mentioned didn't work for me.

Do you know any way to address this issue? I would really appreciate any help.

Best regards.

Patch 63 reroll

Patch 63 reroll

Is there a difference between $74 and #75?