From 089be35b5d81f0a4ab8d0e0268b3bd8dbb118396 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Thu, 8 Mar 2018 17:39:36 +0100
Subject: [PATCH] Aggregation count: Do not return null categories

---
 NEWS.md                                    |  1 +
 lib/cartodb/models/dataview/aggregation.js | 10 +--
 test/acceptance/dataviews/aggregation.js   | 91 +++++++++++++++++++---
 3 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 712aacd9..593ad882 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -5,6 +5,7 @@ Released yyyy-mm-dd
  - Upgrades Windshaft to 4.5.3
  - Implemented middleware to authorize users via new Api Key system
  - Keep the old authorization system as fallback
+ - Aggregation widget: Remove NULL categories in 'count' aggregations too
 
 ## 5.3.1
 Released 2018-02-13
diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js
index 568d3cc2..7820ddbc 100644
--- a/lib/cartodb/models/dataview/aggregation.js
+++ b/lib/cartodb/models/dataview/aggregation.js
@@ -42,7 +42,7 @@ const rankedCategoriesQueryTpl = ctx => `
             ${ctx.aggregationFn} AS value,
             row_number() OVER (ORDER BY ${ctx.aggregationFn} desc) as rank
         FROM (${filteredQueryTpl(ctx)}) filtered_source
-        ${ctx.aggregationColumn !== null ? `WHERE ${ctx.aggregationColumn} IS NOT NULL` : ''}
+        WHERE ${ctx.aggregation === "count" ? `${ctx.column}` : `${ctx.aggregationColumn}`} IS NOT NULL
         GROUP BY ${ctx.column}
         ORDER BY 2 DESC
     )
@@ -279,7 +279,7 @@ module.exports = class Aggregation extends BaseDataview {
             max_val = 0,
             categories_count = 0
          } = result.rows[0] || {};
-        
+
         return {
             aggregation: this.aggregation,
             count: count,
@@ -290,10 +290,10 @@ module.exports = class Aggregation extends BaseDataview {
             max: max_val,
             categoriesCount: categories_count,
             categories: result.rows.map(({ category, value, agg }) => {
-                return { 
+                return {
                     category: agg ? 'Other' : category,
-                    value, 
-                    agg 
+                    value,
+                    agg
                 };
             })
         };
diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js
index e29d9ff4..204fcac7 100644
--- a/test/acceptance/dataviews/aggregation.js
+++ b/test/acceptance/dataviews/aggregation.js
@@ -70,12 +70,8 @@ describe('aggregations happy cases', function() {
     ].join(' UNION ALL ');
 
     operations.forEach(function (operation) {
-        var not = operation === 'count' ? ' not ' : ' ';
-        var description = 'should' +
-            not +
-            'handle NULL values in category and aggregation columns using "' +
-            operation +
-            '" as aggregation operation';
+        var description = 'should handle NULL values in category and aggregation columns using "' +
+            operation + '" as aggregation operation';
 
         it(description, function (done) {
             this.testClient = new TestClient(aggregationOperationMapConfig(operation, query, 'cat', 'val'));
@@ -96,12 +92,7 @@ describe('aggregations happy cases', function() {
                     }
                 });
 
-                if (operation === 'count') {
-                    assert.ok(hasNullCategory, 'aggregation has not a category NULL');
-                } else {
-                    assert.ok(!hasNullCategory, 'aggregation has category NULL');
-                }
-
+                assert.ok(!hasNullCategory, 'aggregation has category NULL');
                 done();
             });
         });
@@ -425,3 +416,79 @@ describe('aggregation dataview tuned by categories query param', function () {
         });
     });
 });
+
+
+
+describe('Count aggregation', function () {
+    const mapConfig = {
+        version: '1.5.0',
+        layers: [
+            {
+                type: "cartodb",
+                options: {
+                    source: {
+                        "id": "a0"
+                    },
+                    cartocss: "#points { marker-width: 10; marker-fill: red; }",
+                    cartocss_version: "2.3.0"
+                }
+            }
+        ],
+        dataviews: {
+            categories: {
+                source: {
+                    id: 'a0'
+                },
+                type: 'aggregation',
+                options: {
+                    column: 'cat',
+                    aggregation: 'count'
+                }
+            }
+        },
+        analyses: [
+            {
+                id: "a0",
+                type: "source",
+                params: {
+                    query: `
+                        SELECT
+                            null::geometry the_geom_webmercator,
+                            CASE
+                                WHEN x % 4 = 0 THEN 1
+                                WHEN x % 4 = 1 THEN 2
+                                WHEN x % 4 = 2 THEN 3
+                                ELSE null
+                            END AS val,
+                            CASE
+                                WHEN x % 4 = 0 THEN 'category_1'
+                                WHEN x % 4 = 1 THEN 'category_2'
+                                WHEN x % 4 = 2 THEN 'category_3'
+                                ELSE null
+                            END AS cat
+                        FROM generate_series(1, 1000) x
+                    `
+                }
+            }
+        ]
+    };
+
+    it(`should handle null values correctly when aggregationColumn isn't provided`, function (done) {
+        this.testClient = new TestClient(mapConfig, 1234);
+        this.testClient.getDataview('categories', { own_filter: 0, categories: 0 }, (err, dataview) => {
+            assert.ifError(err);
+            assert.equal(dataview.categories.length, 3);
+            this.testClient.drain(done);
+        });
+    });
+
+    it(`should handle null values correctly when aggregationColumn is provided`, function (done) {
+        mapConfig.dataviews.categories.options.aggregationColumn = 'val';
+        this.testClient = new TestClient(mapConfig, 1234);
+        this.testClient.getDataview('categories', { own_filter: 0, categories: 0 }, (err, dataview) => {
+            assert.ifError(err);
+            assert.equal(dataview.categories.length, 3);
+            this.testClient.drain(done);
+        });
+    });
+});
\ No newline at end of file