Segment selector: max out at 5-6, instead of throwing many errors in reporting interface

config/global.ini.php

@@ -873,7 +873,7 @@
 num_days_before_tracking_code_reminder = 5
 
 ; The maximum number of segments that can be compared simultaneously.
-data_comparison_segment_limit = 5
+data_comparison_segment_limit = 6
 
 ; The maximum number of periods that can be compared simultaneously.
 data_comparison_period_limit = 5

core/Config.php

@@ -188,6 +188,7 @@ public function getClientSideOptions()
             'action_title_category_delimiter' => $general['action_title_category_delimiter'],
             'are_ads_enabled' => Advertising::isAdsEnabledInConfig($general),
             'autocomplete_min_sites' => $general['autocomplete_min_sites'],
+            'data_comparison_segment_limit' => $general['data_comparison_segment_limit'],
             'datatable_export_range_as_day' => $general['datatable_export_range_as_day'],
             'datatable_row_limits' => $this->getDatatableRowLimits(),
             'enable_general_settings_admin' => Controller::isGeneralSettingsAdminEnabled(),

plugins/API/Filter/DataComparisonFilter.php

@@ -144,7 +144,7 @@ public function __construct($request, ?Report $report = null)
         $this->segmentName = $this->getSegmentNameFromReport($report);
 
         $this->compareSegments = self::getCompareSegments();
-        if (count($this->compareSegments) > $this->segmentCompareLimit + 1) {
+        if (count($this->compareSegments) > $this->segmentCompareLimit) {
             throw new BadRequestException(Piwik::translate('General_MaximumNumberOfSegmentsComparedIs', [$this->segmentCompareLimit]));
         }
 

plugins/SegmentEditor/SegmentEditor.php

@@ -384,6 +384,7 @@ public function getClientSideTranslationKeys(&$translationKeys)
         $translationKeys[] = 'SegmentEditor_ThisSegmentIsSelectedAndCannotBeCompared';
         $translationKeys[] = 'SegmentEditor_CompareThisSegment';
         $translationKeys[] = 'Live_VisitsLog';
+        $translationKeys[] = 'General_MaximumNumberOfSegmentsComparedIs';
     }
 
     public static function getAllSegmentsForSite($idSite)

plugins/SegmentEditor/javascripts/Segmentation.js

@@ -86,19 +86,41 @@ Segmentation = (function($) {
               content: title,
             });
         };
+        // We will listen to changes in the Segment Comparison Store
+        // so we can mark compared segments properly. This will now include deletion of compared segments.
+        piwik.on('piwikComparisonsChanged', function () {
+          self.markComparedSegments();
+        });
 
         segmentation.prototype.markComparedSegments = function() {
             var comparisonService = window.CoreHome.ComparisonsStoreInstance;
             var comparedSegments = comparisonService.getSegmentComparisons().map(function (comparison) {
                 return comparison.params.segment;
             });
-
             $('div.segmentList ul li[data-definition]', this.target).removeClass('comparedSegment').filter(function () {
                 var definition = $(this).attr('data-definition');
                 return comparedSegments.indexOf(definition) !== -1 || comparedSegments.indexOf(decodeURIComponent(definition)) !== -1;
             }).each(function () {
                 $(this).addClass('comparedSegment');
             });
+            self.checkIfComparedSegmentsHasReachedLimit();
+        };
+        segmentation.prototype.checkIfComparedSegmentsHasReachedLimit = function() {
+            const limit = piwik.config.data_comparison_segment_limit;
+            const comparisonService = window.CoreHome.ComparisonsStoreInstance;
+            const comparedSegmentsLength = comparisonService.getSegmentComparisons().length;
+            $('div.segmentList ul li[data-definition] span.compareSegment').each(function() {
+              if (comparedSegmentsLength >= limit) {
+                $(this).addClass('no-click');
+                $(this).parent().attr('title', _pk_translate('General_MaximumNumberOfSegmentsComparedIs', [limit]));
+              } else {
+                $(this).removeClass('no-click');
+                var idSegment = $(this).parent().attr('data-idsegment');
+                const title = getSegmentName(getSegmentFromId(idSegment));
+                $(this).parent().attr('title', title);
+              }
+            });
+            return false;
         };
 
         segmentation.prototype.markCurrentSegment = function(){
@@ -408,20 +430,21 @@ Segmentation = (function($) {
             self.target.on('click', '.compareSegment', function (e) {
                 e.stopPropagation();
                 e.preventDefault();
-
                 var comparisonService = window.CoreHome.ComparisonsStoreInstance;
                 comparisonService.addSegmentComparison({
                     segment: $(e.target).closest('li').data('definition'),
                 });
 
-                self.markComparedSegments();
-
+                setTimeout(function () {
+                  self.markComparedSegments();
+                });
                 closeAllOpenLists();
             });
 
-            self.target.on("click", ".segmentList li", function (e) {
-                if ($(e.currentTarget).hasClass("grayed") !== true) {
-                    var segmentDefinition = $(this).data("definition");
+            self.target.on("click", ".segmentList li span.segname", function (e) {
+                let parentLi = $(this).parent();
+                if (parentLi.hasClass("grayed") !== true) {
+                    var segmentDefinition = $(parentLi).data("definition");
 
                     if (!piwikHelper.isReportingPage()) {
                         // we update segment on location change success

plugins/SegmentEditor/stylesheets/segmentation.less

@@ -298,6 +298,9 @@ div.scrollable {
             margin-right: 24px;
         }
     }
+    span.compareSegment.no-click {
+        pointer-events: none;
+    }
 
     li.segmentSelected, li.comparedSegment {
         span.compareSegment {

plugins/SegmentEditor/tests/UI/SegmentSelectorEditor_spec.js

@@ -354,4 +354,51 @@ describe("SegmentSelectorEditorTest", function () {
         await page.click('.segmentationContainer .title');
         expect(await page.screenshotSelector(selectorsToCapture)).to.matchImage('enabled_create_realtime_segments_saved');
     });
+
+    it('should not allow comparing segments more than the limit set', async function() {
+      const maxSegments = 3;
+      testEnvironment.overrideConfig('General', 'data_comparison_segment_limit', maxSegments);
+      testEnvironment.save();
+      await page.goto(url);
+      await page.waitForNetworkIdle();
+      // We grab the max limit message
+      const maxLimitMessage = await page.evaluate(
+        (limit) => _pk_translate('General_MaximumNumberOfSegmentsComparedIs', [limit]),
+      maxSegments);
+
+      // We check that the title attribute is still not the max limit message
+      let title = await page.$eval('.segmentationContainer .segmentList li:last-child', (el) => el.getAttribute('title'));
+      expect(title).to.not.equal(maxLimitMessage);
+
+      await page.waitForSelector('.segmentationContainer');
+      // We check that the number of <li> elements is greater than the limit we set
+      const liElemLength = await page.$$eval('.segmentListContainer .segmentList li', (e) => e.length);
+      expect(liElemLength).to.be.greaterThan(maxSegments);
+
+      // We check that the number of segments compared is 1 at the start
+      let comparedCount = await page.$$eval(
+        '.segmentListContainer .segmentList li.comparedSegment',
+        (nodes) => nodes.length,
+      );
+      expect(comparedCount).to.equal(1);
+
+      // We want to click all the segments so that we can check that it stops at the limit
+      for (let i=0; i<liElemLength; i++) {
+        await page.click('.segmentationContainer .title');
+        const segments = await page.$$('.segmentListContainer .segmentList li span.compareSegment');
+        await segments[i].click();
+        await page.waitForTimeout(50);
+      }
+
+      // We check that the number of segments compared is now equal to the limit we set
+      comparedCount = await page.$$eval(
+        '.segmentListContainer .segmentList li.comparedSegment',
+        (nodes) => nodes.length,
+      );
+      expect(comparedCount).to.equal(maxSegments);
+
+      // Last we check that the title attribute has changed to reflect the limit has been reached
+      title = await page.$eval('.segmentationContainer .segmentList li:last-child', (el) => el.getAttribute('title'));
+      expect(title).to.equal(maxLimitMessage);
+    });
 });