Mass Mailer: filter custom segments by their email list

A custom segment (cst:<id>) was never handled by getSqlBySegment(), so
it matched every account with a profile. Now that the reports page
sends the chosen segment, each custom list showed site-wide numbers,
and a campaign sent to one reached every member.

Read the segment's email_list (commas, semicolons or white space between
emails) and keep only the accounts with one of those emails and a
person-type profile, as the level segments do. A missing or empty list
matches no one. The SQL fragments live in BxMassMailerDb.

Split getReportsData() into one helper per report plus the time axis
and datasets (Sonar S3776, cognitive complexity 61). Its output is the
same for every report and segment checked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

modules/boonex/massmailer/classes/BxMassMailerDb.php

@@ -68,7 +68,41 @@ class BxMassMailerDb extends BxBaseModGeneralDb
         $sQuery = $this->prepare ("SELECT `" . $CNF['FIELD_ID'] . "`, `" . $CNF['FIELD_TITLE'] . "` FROM `" . $CNF['TABLE_SEGMENTS'] . "`");
         return $this->getPairs($sQuery, $CNF['FIELD_ID'], $CNF['FIELD_TITLE']);
     }
-    
+
+    /**
+     * Emails in a custom segment's list, separated by commas, semicolons or white space.
+     */
+    public function getSegmentEmails($iSegmentId)
+    {
+        $CNF = &$this->_oConfig->CNF;
+        $sEmails = $this->getOne("SELECT `" . $CNF['FIELD_EMAIL_LIST'] . "` FROM `" . $CNF['TABLE_SEGMENTS'] . "` WHERE `" . $CNF['FIELD_ID'] . "` = :id", array('id' => $iSegmentId));
+
+        $aEmails = $sEmails ? preg_split('/[\s,;]+/u', $sEmails, -1, PREG_SPLIT_NO_EMPTY) : false;
+        if (!$aEmails)
+            return array();
+
+        return array_values(array_unique(preg_grep('/@/', $aEmails)));
+    }
+
+    /**
+     * Account terms: only the accounts with one of these emails, none for an empty list.
+     */
+    public function getTermsByEmails($aEmails)
+    {
+        if (empty($aEmails))
+            return " AND 0 ";
+
+        return " AND `ta`.`email` IN (" . $this->implode_escape($aEmails) . ") ";
+    }
+
+    /**
+     * Account terms: only the profiles of these types.
+     */
+    public function getTermsByProfileTypes($aTypes)
+    {
+        return " AND `tp`.`type` IN (" . $this->implode_escape($aTypes) . ")";
+    }
+
     public function getAccountsByTerms($sTerms = "")
     {
         $sSql = "SELECT `ta`.`email`, `tp`.`id` AS `profile_id`, `ta`.`id` as `account_id` FROM `sys_accounts` AS `ta` " . $sTerms . "  WHERE `ta`.`receive_news` <> 0 AND `ta`.`email_confirmed` = 1";

modules/boonex/massmailer/classes/BxMassMailerModule.php

@@ -70,142 +70,43 @@ class BxMassMailerModule extends BxBaseModGeneralModule
      */
     public function getReportsData($sReportName, $sReportType, $sDateFrom, $sDateTo, $sSegment)
     {
-        $aColors = array('#3366CC', '#DC3912', '#FF9900', '#109618', '#990099');
         $iDateFrom = strtotime($sDateFrom);
         $iDateTo = strtotime($sDateTo);
-        $sType = "bar";
         $bIsTimeX = false;
-        $iMinValueY = 0;
-        $iMaxValueY = 0;
-        $aValues = array('labels' => array(), 'values' => array(array('legend' => '', 'data' => array())), 'links' => array(), 'strings' => array(0 => _t('_bx_massmailer_txt_item'), 1 => _t('_bx_massmailer_txt_value')));
-        
+
         switch ($sReportName) {
             case "SUBSCRIBERS_INFO":
-                $sType = "line";
                 $bIsTimeX = true;
-                $aValues['strings'][0] = _t('_bx_massmailer_txt_date');
-                $aValues['strings'][1] = '';
-                $sSql = $this->getSqlBySegment($sSegment);
-                $aData = $this->_oDb->getAccountsByTermsStat($iDateFrom, $iDateTo, $sSql);
-                $aData2 = $this->_oDb->getAccountsByTermsStatUnsubscribe($iDateFrom, $iDateTo, $sSql);
-                $iValuePrev = $this->_oDb->getAccountsByTermsStatInitValue($iDateFrom, $sSql);
-                $iValuePrev2 = $this->_oDb->getAccountsByTermsStatInitValueUnsubscribe($iDateFrom, $sSql);
-                $aTmpDates = array();
-                $aTmpDates2 = array();
-                $aValues['values'][0]['legend'] = _t('_bx_massmailer_txt_subscribed');
-                $aValues['values'][1] = array('legend' => _t('_bx_massmailer_txt_unsubscribed') , 'data' => array());
-                foreach ($aData as $aValue) {
-                    $aTmpDates[$aValue['period']] = $aValue['count'];
-                }
-                foreach ($aData2 as $aValue) {
-                    $aTmpDates2[$aValue['period']] = $aValue['count'];
-                }
-                for ($i = $iDateFrom; $i < $iDateTo ; $i = $i + 86400 ){
-                    $sX = date('Y-m-d', $i);
-                    
-                    if (!array_key_exists($sX, $aTmpDates)){
-                        array_push($aValues['values'][0]['data'], array('x' => $sX, 'y' => $sReportType == 'content_total' ? $iValuePrev : 0));
-                        
-                    }
-                    else{
-                        array_push($aValues['values'][0]['data'], array('x' => $sX, 'y' => $sReportType == 'content_total' ? $iValuePrev : $aTmpDates[$sX]));
-                        $iValuePrev += $aTmpDates[$sX];
-                    }
-                    
-                    if (!array_key_exists($sX, $aTmpDates2)){
-                        array_push($aValues['values'][1]['data'], array('x' => $sX, 'y' => $sReportType == 'content_total' ? $iValuePrev2 : 0));
-                        
-                    }
-                    else{
-                        array_push($aValues['values'][1]['data'], array('x' => $sX, 'y' => $sReportType == 'content_total' ? $iValuePrev2 : $aTmpDates2[$sX]));
-                        $iValuePrev2 += $aTmpDates2[$sX];
-                    }
-                    
-                    if ($sReportType == 'content_total'){
-                        if ($iValuePrev > $iMaxValueY)
-                            $iMaxValueY = $iValuePrev;
-                        if ($iValuePrev2 > $iMaxValueY)
-                            $iMaxValueY = $iValuePrev2;
-                    }
-                    else{
-                        if (array_key_exists($sX, $aTmpDates) && $aTmpDates[$sX] > $iMaxValueY)
-                            $iMaxValueY = $aTmpDates[$sX];
-                        if (array_key_exists($sX, $aTmpDates2) && $aTmpDates2[$sX] > $iMaxValueY)
-                            $iMaxValueY = $aTmpDates2[$sX];
-                    }
-                }
+                $aReport = $this->getReportSubscribers($sReportType == 'content_total', $iDateFrom, $iDateTo, $sSegment);
                 break;
-            
+
             case "CAMPAIGN_REPORT":
-                $aDataStat = $this->_oDb->getStatByCampaignId($sReportType);
-                // One category for all bars: a category axis without labels places no bars.
-                $aValues['labels'] = array('');
-                $aValues['values'][0]['legend'] = _t('_bx_massmailer_txt_sent_total_title', $aDataStat['total']);
-                array_push($aValues['values'][0]['data'], $aDataStat['total']);
-                $aValues['values'][1] = array('legend' => _t('_bx_massmailer_txt_opened_total_title', $aDataStat['seen']) , 'data' => array($aDataStat['seen']));
-                $aValues['values'][2] = array('legend' => _t('_bx_massmailer_txt_unopened_total_title', $aDataStat['total'] - $aDataStat['seen']), 'data' => array($aDataStat['total'] - $aDataStat['seen']));
-                $aValues['values'][3] = array('legend' => _t('_bx_massmailer_txt_clicked_total_title', $aDataStat['clicked']), 'data' => array($aDataStat['clicked']));
-                $aValues['values'][4] = array('legend' => _t('_bx_massmailer_txt_unsubscribed_total_title', $aDataStat['unsubscribed']), 'data' => array($aDataStat['unsubscribed']));
-                $iMaxValueY = $aDataStat['total'];
+                $aReport = $this->getReportCampaign($sReportType);
                 break;
+
+            default:
+                $aReport = array(
+                    'labels' => array(),
+                    'values' => array(array('legend' => '', 'data' => array())),
+                    'strings' => array(_t('_bx_massmailer_txt_item'), _t('_bx_massmailer_txt_value')),
+                    'max' => 0
+                );
         }
-        
-        $iMaxValueY = ceil($iMaxValueY * 1.1);
-        
-        $aDataForChartXAxes = array();
-        if ($bIsTimeX){
-            $sUnit = 'day';
-            $iInterval = ($iDateTo - $iDateFrom) / 86400;
-            if ($iInterval > 50)
-                $sUnit = 'week';
-            if ($iInterval > 100)
-                $sUnit = 'month';
-            
-            $aDataForChartXAxes = array(
-                'type' => 'time',
-                'time' => array(
-                    'tooltipFormat' => 'DD.MM.YYYY',
-                    'unit' => $sUnit,
-                 ),
-                'display' => true,
-                'ticks' => array(
-                    'display' => true,
-                    'autoSkip' => true,
-                )
-            );
-        }
-        else{
-            $aDataForChartXAxes = array(
-                'ticks' => array(
-                    'autoSkip' => false
-                ),
-                'display' => true
-            );
-        }
-        
-        $aDataForChartDatasets = array();
-        for($i = 0; $i < count($aValues['values']); $i++){
-            $aDataForChartDatasets[] = array(
-                'label' => $aValues['values'][$i]['legend'],
-                'fill' => false,
-                'backgroundColor' => $aColors[$i],
-                'borderColor' => $aColors[$i],
-                'borderWidth' => 1,
-                'data' => $aValues['values'][$i]['data']
-            );
-        }
-        
-        $aDataForChart = array(
-            'type' => $sType,
+
+        $iMinValueY = 0;
+        $iMaxValueY = ceil($aReport['max'] * 1.1);
+
+        return array(
+            'type' => $bIsTimeX ? 'line' : 'bar',
             'data' => array(
-                'labels' => $aValues['labels'],
-                'datasets' => $aDataForChartDatasets
+                'labels' => $aReport['labels'],
+                'datasets' => $this->getReportDatasets($aReport['values'])
             ),
             'options' => array(
                 'plugins' => array(
                     'legend' => array(
                         'position' => 'bottom',
-                        'display' => count($aValues['values']) == 1 ? false : true
+                        'display' => count($aReport['values']) > 1
                     )
                 ),
                 'scales' => array(
@@ -217,14 +118,118 @@ class BxMassMailerModule extends BxBaseModGeneralModule
                             'autoSkip' => true
                         )
                     ),
-                    'x' => $aDataForChartXAxes
+                    'x' => $bIsTimeX ? $this->getReportTimeAxis($iDateFrom, $iDateTo) : array('ticks' => array('autoSkip' => false), 'display' => true)
                 )
             ),
-            'links' => $aValues['links'],
-            'strings' => $aValues['strings'],
+            'links' => array(),
+            'strings' => $aReport['strings'],
         );
-        
-        return $aDataForChart;
+    }
+
+    /**
+     * Subscribed and unsubscribed accounts of a segment, a point per day.
+     */
+    private function getReportSubscribers($bTotal, $iDateFrom, $iDateTo, $sSegment)
+    {
+        $sSql = $this->getSqlBySegment($sSegment);
+        $iMaxValueY = 0;
+
+        $aSubscribed = $this->getReportSeries($this->_oDb->getAccountsByTermsStat($iDateFrom, $iDateTo, $sSql), (int)$this->_oDb->getAccountsByTermsStatInitValue($iDateFrom, $sSql), $bTotal, $iDateFrom, $iDateTo, $iMaxValueY);
+        $aUnsubscribed = $this->getReportSeries($this->_oDb->getAccountsByTermsStatUnsubscribe($iDateFrom, $iDateTo, $sSql), (int)$this->_oDb->getAccountsByTermsStatInitValueUnsubscribe($iDateFrom, $sSql), $bTotal, $iDateFrom, $iDateTo, $iMaxValueY);
+
+        return array(
+            'labels' => array(),
+            'values' => array(
+                array('legend' => _t('_bx_massmailer_txt_subscribed'), 'data' => $aSubscribed),
+                array('legend' => _t('_bx_massmailer_txt_unsubscribed'), 'data' => $aUnsubscribed)
+            ),
+            'strings' => array(_t('_bx_massmailer_txt_date'), ''),
+            'max' => $iMaxValueY
+        );
+    }
+
+    /**
+     * Points from the per-day counts in $aRows: the running total before the day when $bTotal is set,
+     * the day's count otherwise. $iMaxValueY is raised to the largest total or count.
+     */
+    private function getReportSeries($aRows, $iTotal, $bTotal, $iDateFrom, $iDateTo, &$iMaxValueY)
+    {
+        $aCounts = array_column($aRows, 'count', 'period');
+
+        $aPoints = array();
+        for ($i = $iDateFrom; $i < $iDateTo; $i += 86400) {
+            $sX = date('Y-m-d', $i);
+            $iCount = isset($aCounts[$sX]) ? (int)$aCounts[$sX] : 0;
+
+            $aPoints[] = array('x' => $sX, 'y' => $bTotal ? $iTotal : $iCount);
+            $iTotal += $iCount;
+
+            $iMaxValueY = max($iMaxValueY, $bTotal ? $iTotal : $iCount);
+        }
+
+        return $aPoints;
+    }
+
+    private function getReportCampaign($iCampaignId)
+    {
+        $aStat = $this->_oDb->getStatByCampaignId($iCampaignId);
+        $iUnopened = $aStat['total'] - $aStat['seen'];
+
+        return array(
+            // One category for all bars: a category axis without labels places no bars.
+            'labels' => array(''),
+            'values' => array(
+                array('legend' => _t('_bx_massmailer_txt_sent_total_title', $aStat['total']), 'data' => array($aStat['total'])),
+                array('legend' => _t('_bx_massmailer_txt_opened_total_title', $aStat['seen']), 'data' => array($aStat['seen'])),
+                array('legend' => _t('_bx_massmailer_txt_unopened_total_title', $iUnopened), 'data' => array($iUnopened)),
+                array('legend' => _t('_bx_massmailer_txt_clicked_total_title', $aStat['clicked']), 'data' => array($aStat['clicked'])),
+                array('legend' => _t('_bx_massmailer_txt_unsubscribed_total_title', $aStat['unsubscribed']), 'data' => array($aStat['unsubscribed']))
+            ),
+            'strings' => array(_t('_bx_massmailer_txt_item'), _t('_bx_massmailer_txt_value')),
+            'max' => $aStat['total']
+        );
+    }
+
+    private function getReportTimeAxis($iDateFrom, $iDateTo)
+    {
+        $iDays = ($iDateTo - $iDateFrom) / 86400;
+
+        $sUnit = 'day';
+        if ($iDays > 100)
+            $sUnit = 'month';
+        else if ($iDays > 50)
+            $sUnit = 'week';
+
+        return array(
+            'type' => 'time',
+            'time' => array(
+                'tooltipFormat' => 'DD.MM.YYYY',
+                'unit' => $sUnit,
+            ),
+            'display' => true,
+            'ticks' => array(
+                'display' => true,
+                'autoSkip' => true,
+            )
+        );
+    }
+
+    private function getReportDatasets($aValues)
+    {
+        $aColors = array('#3366CC', '#DC3912', '#FF9900', '#109618', '#990099');
+
+        $aDatasets = array();
+        foreach ($aValues as $i => $aValue)
+            $aDatasets[] = array(
+                'label' => $aValue['legend'],
+                'fill' => false,
+                'backgroundColor' => $aColors[$i],
+                'borderColor' => $aColors[$i],
+                'borderWidth' => 1,
+                'data' => $aValue['data']
+            );
+
+        return $aDatasets;
     }
   
     /**
@@ -616,6 +621,7 @@ class BxMassMailerModule extends BxBaseModGeneralModule
     {
         $sRv = '';
         $sJoin = ' INNER ';
+        $aMatches = array();
         if (strpos($sSegment, 'lvl:') !== false){
             $sLvl = str_replace('lvl:', '', $sSegment);
             switch ($sLvl) {
@@ -639,7 +645,12 @@ class BxMassMailerModule extends BxBaseModGeneralModule
                     $sRv = $this->_oDb->prepareAsString(" AND `tp`.`id` IN (SELECT `IDMember` FROM `sys_acl_levels_members` WHERE IDLevel = ?) ", $sLvl);
                     break;
             }
-            $sRv .= " AND `tp`.`type` IN (" . $this->_oDb->implode_escape($this->getProfileModules()) . ")";
+            $sRv .= $this->_oDb->getTermsByProfileTypes($this->getProfileModules());
+        }
+        else if (preg_match('/^cst:(\d+)$/', $sSegment, $aMatches)){
+            //--- A custom segment is a list of emails: only the accounts with one of them.
+            $sRv = $this->_oDb->getTermsByEmails($this->_oDb->getSegmentEmails((int)$aMatches[1]));
+            $sRv .= $this->_oDb->getTermsByProfileTypes($this->getProfileModules());
         }
         return $sJoin . " JOIN `sys_profiles` AS `tp` ON `tp`.`account_id`=`ta`.`id` " . $sRv;
     }

tests/units/modules/boonex/massmailer/classes/BxMassMailerModuleTest.php

@@ -7,6 +7,7 @@
 class BxMassMailerModuleTest extends BxDolTestCase
 {
     protected $_oModule;
+    protected $_aSegmentIds = [];
 
     protected function setUp(): void
     {
@@ -16,6 +17,44 @@ class BxMassMailerModuleTest extends BxDolTestCase
             $this->markTestSkipped('bx_massmailer module is not installed.');
     }
 
+    protected function tearDown(): void
+    {
+        foreach ($this->_aSegmentIds as $iId)
+            $this->_oModule->_oDb->query("DELETE FROM `" . $this->_oModule->_oConfig->CNF['TABLE_SEGMENTS'] . "` WHERE `id` = :id", ['id' => $iId]);
+
+        $this->_aSegmentIds = [];
+    }
+
+    public function testCustomSegmentEmailsAreSplitOnCommasSemicolonsAndSpaces()
+    {
+        $iId = $this->addSegment(" a@example.com,b@example.com;\n c@example.com\ta@example.com  not-an-email 123 ");
+
+        $this->assertSame(['a@example.com', 'b@example.com', 'c@example.com'], $this->_oModule->_oDb->getSegmentEmails($iId));
+        $this->assertSame([], $this->_oModule->_oDb->getSegmentEmails($this->addSegment('')));
+    }
+
+    public function testCustomSegmentKeepsOnlyListedAccounts()
+    {
+        //--- The accounts "All" reaches through a profile, cut down to the listed emails.
+        $aAll = array_values(array_filter($this->accounts('lvl:0'), fn($a) => !empty($a['profile_id'])));
+        $aListed = array_slice(array_values(array_unique(array_column($aAll, 'email'))), 0, 2);
+        $iId = $this->addSegment(implode(', ', array_merge($aListed, ['nobody@example.invalid'])));
+
+        $aExpected = array_values(array_filter($aAll, fn($a) => in_array($a['email'], $aListed)));
+        $this->assertEqualsCanonicalizing($aExpected, $this->accounts('cst:' . $iId));
+    }
+
+    public function testMissingCustomSegmentReachesNobody()
+    {
+        $sSegment = 'cst:2147483000';
+        $this->assertSame([], $this->accounts($sSegment));
+
+        //--- The report used to count every account with a profile for a custom segment.
+        $a = $this->_oModule->getReportsData('SUBSCRIBERS_INFO', 'content_total', '2026-01-01', '2026-01-31', $sSegment);
+        foreach ($a['data']['datasets'] as $aDataset)
+            $this->assertSame([0], array_values(array_unique(array_column($aDataset['data'], 'y'))));
+    }
+
     public function testCampaignReportHasOneCategoryForItsBars()
     {
         $a = $this->_oModule->getReportsData('CAMPAIGN_REPORT', 0, '-', '-', '-');
@@ -47,6 +86,18 @@ class BxMassMailerModuleTest extends BxDolTestCase
         $this->assertChartJs4Options($a['options']);
     }
 
+    private function accounts(string $sSegment): array
+    {
+        return $this->_oModule->_oDb->getAccountsByTerms($this->bxCallProtected($this->_oModule, 'getSqlBySegment', $sSegment));
+    }
+
+    private function addSegment(string $sEmails): int
+    {
+        $this->_oModule->_oDb->query("INSERT INTO `" . $this->_oModule->_oConfig->CNF['TABLE_SEGMENTS'] . "` (`title`, `info`, `email_list`) VALUES ('Test segment', '', :emails)", ['emails' => $sEmails]);
+
+        return $this->_aSegmentIds[] = (int)$this->_oModule->_oDb->lastId();
+    }
+
     private function assertChartJs4Options(array $aOptions): void
     {
         $this->assertArrayNotHasKey('legend', $aOptions);