Problem/Motivation

If the site uses a custom recipient handler the reported counts of subscribers will be wrong.

Proposed Solution

  1. Fix using new function SpoolStorage::countSubscriptions($node) that calls RecipientHandlerInterface::count().
  2. Remove simplenews_count_subscriptions as the arguments don't provide enough information to count with a custom recipient handler.
  3. There is some existing caching that we should keep for good performance. The default recipient handler can implement a cache of newsletter_id => count. For other recipient handlers, the count may depend on values in the issue entity in which case they cannot do the same caching.
CommentFileSizeAuthor
#17 simplenews.recipient-count.3053650-interdiff-14-17.txt1.89 KBadamps
#17 simplenews.recipient-count.3053650-17.patch9.32 KBadamps
#14 simplenews.recipient-count.3053650-interdiff-12-14.txt2.71 KBadamps
#14 simplenews.recipient-count.3053650-14.patch8.91 KBadamps
#12 simplenews.recipient-count.3053650-interdiff-10-12.txt594 bytesadamps
#12 simplenews.recipient-count.3053650-12.patch7.38 KBadamps
#10 simplenews.recipient-count.3053650-10.patch7.38 KBadamps
#9 simplenews.recipient-count.3053650-6.patch7.38 KBadamps
#6 simplenews.recipient-count.3053650-6.patch7.38 KBadamps

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented

Hmm it's tricky in NodeTabForm. The count depends on the choice of recipient handler and settings, so would need to be updated by Ajax.

It seems that data is being collected in the wrong place. Newsletter ID is set when editing the node. Recipient handler feels exactly like newsletter ID but is set on the newsletter page.

adamps’s picture

adamps commented

I have raised #3055850: Wrong location of recipient handler UI for the related issue.

adamps’s picture

adamps commented

NB

1) There is some existing caching that we should keep for good performance. The default recipient handler can implement a cache of newsletter_id => count. For other recipient handlers, the count may depend on values in the issue entity in which case they cannot do the same caching.

2) Remove simplenews_count_subscriptions or refactor it to call the default recipient handler.

adamps’s picture

adamps commented
Issue summary: View changes
adamps’s picture

adamps commented
Status: Active » Needs review
StatusFileSize
new simplenews.recipient-count.3053650-6.patch7.38 KB
adamps’s picture

adamps commented

This patch applies on top of #193100: Creating Playlists using swftools_prepare_playlist_data. However review comments are still welcome.

adamps’s picture

adamps commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
adamps’s picture

adamps commented
StatusFileSize
new simplenews.recipient-count.3053650-6.patch7.38 KB
adamps’s picture

adamps commented
StatusFileSize
new simplenews.recipient-count.3053650-10.patch7.38 KB

Status: Needs review » Needs work

The last submitted patch, 10: simplenews.recipient-count.3053650-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new simplenews.recipient-count.3053650-12.patch7.38 KB
new simplenews.recipient-count.3053650-interdiff-10-12.txt594 bytes

Status: Needs review » Needs work

The last submitted patch, 12: simplenews.recipient-count.3053650-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new simplenews.recipient-count.3053650-14.patch8.91 KB
new simplenews.recipient-count.3053650-interdiff-12-14.txt2.71 KB
adamps’s picture

adamps commented
Issue tags: +Plan to commit
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented 
  1. +++ b/modules/simplenews_demo/src/Plugin/simplenews/RecipientHandler/RecipientHandlerNewUsers.php
@@ -23,4 +23,11 @@ class RecipientHandlerNewUsers extends RecipientHandlerEntityBase {
+  protected function cacheCount() {
+    return TRUE;
+  }

    Wouldn't this more naturally be a property rather than a method? Or do you imagine situations where its value is determined dynamically rather than by the developer? 

  2. +++ b/modules/simplenews_demo/src/Plugin/simplenews/RecipientHandler/RecipientHandlerSiteMail.php
@@ -26,7 +26,7 @@ class RecipientHandlerSiteMail extends RecipientHandlerBase {
+  public function doCount() {

+++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php
@@ -54,6 +54,47 @@ abstract class RecipientHandlerBase extends PluginBase implements RecipientHandl
+  abstract protected function doCount();

    doCount should be protected not public?

adamps’s picture

adamps commented
StatusFileSize
new simplenews.recipient-count.3053650-17.patch9.32 KB
new simplenews.recipient-count.3053650-interdiff-14-17.txt1.89 KB

1. Personally I prefer a function because it is more flexible and more rigorous (it prevents accidental overwriting of a property value). I guess it's perhaps a matter of opinion. Seeing as I have written and tested it this way it doesn't really seem worth changing.

2. Done (*3)

  • AdamPS committed 2b59253 on 8.x-2.x 
    Issue #3053650 by AdamPS: Subscriber counts ignore...
adamps’s picture

adamps commented
Status: Needs review » Fixed
Issue tags: -Plan to commit

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.