[Bugfix] Issues with table filtering (apache#4073)

* Fixing some issues with table filtering * Added some comments
michellethomas · May 23, 2018 · 46896f0 · 46896f0
1 parent dd54812
commit 46896f0
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 29 deletions.
diff --git a/superset/assets/javascripts/chart/Chart.jsx b/superset/assets/javascripts/chart/Chart.jsx
@@ -106,8 +106,8 @@ class Chart extends React.PureComponent {
     this.props.clearFilter();
   }
 
-  removeFilter(col, vals) {
-    this.props.removeFilter(col, vals);
+  removeFilter(col, vals, refresh = true) {
+    this.props.removeFilter(col, vals, refresh);
   }
 
   clearError() {

diff --git a/superset/assets/javascripts/dashboard/actions.js b/superset/assets/javascripts/dashboard/actions.js
@@ -13,8 +13,8 @@ export function clearFilter(sliceId) {
 }
 
 export const REMOVE_FILTER = 'REMOVE_FILTER';
-export function removeFilter(sliceId, col, vals) {
-  return { type: REMOVE_FILTER, sliceId, col, vals };
+export function removeFilter(sliceId, col, vals, refresh = true) {
+  return { type: REMOVE_FILTER, sliceId, col, vals, refresh };
 }
 
 export const UPDATE_DASHBOARD_LAYOUT = 'UPDATE_DASHBOARD_LAYOUT';

diff --git a/superset/assets/javascripts/dashboard/reducers.js b/superset/assets/javascripts/dashboard/reducers.js
@@ -138,27 +138,26 @@ export const dashboard = function (state = {}, action) {
         return state;
       }
 
-      let filters;
+      let filters = state.filters;
       const { sliceId, col, vals, merge, refresh } = action;
       const filterKeys = ['__from', '__to', '__time_col',
         '__time_grain', '__time_origin', '__granularity'];
       if (filterKeys.indexOf(col) >= 0 ||
         selectedSlice.formData.groupby.indexOf(col) !== -1) {
-        if (!(sliceId in state.filters)) {
-          filters = { ...state.filters, [sliceId]: {} };
-        }
-
         let newFilter = {};
-        if (state.filters[sliceId] && !(col in state.filters[sliceId]) || !merge) {
-          newFilter = { ...state.filters[sliceId], [col]: vals };
+        if (!(sliceId in filters)) {
+          // Straight up set the filters if none existed for the slice
+          newFilter = { [col]: vals };
+        } else if (filters[sliceId] && !(col in filters[sliceId]) || !merge) {
+          newFilter = { ...filters[sliceId], [col]: vals };
           // d3.merge pass in array of arrays while some value form filter components
           // from and to filter box require string to be process and return
-        } else if (state.filters[sliceId][col] instanceof Array) {
-          newFilter[col] = d3.merge([state.filters[sliceId][col], vals]);
+        } else if (filters[sliceId][col] instanceof Array) {
+          newFilter[col] = d3.merge([filters[sliceId][col], vals]);
         } else {
-          newFilter[col] = d3.merge([[state.filters[sliceId][col]], vals])[0] || '';
+          newFilter[col] = d3.merge([[filters[sliceId][col]], vals])[0] || '';
         }
-        filters = { ...state.filters, [sliceId]: newFilter };
+        filters = { ...filters, [sliceId]: newFilter };
       }
       return { ...state, filters, refresh };
     },
@@ -168,21 +167,18 @@ export const dashboard = function (state = {}, action) {
       return { ...state, filter: newFilters, refresh: true };
     },
     [actions.REMOVE_FILTER]() {
-      const newFilters = { ...state.filters };
-      const { sliceId, col, vals } = action;
-
-      if (sliceId in state.filters) {
-        if (col in state.filters[sliceId]) {
-          const a = [];
-          newFilters[sliceId][col].forEach(function (v) {
-            if (vals.indexOf(v) < 0) {
-              a.push(v);
-            }
-          });
-          newFilters[sliceId][col] = a;
-        }
+      const { sliceId, col, vals, refresh } = action;
+      const excluded = new Set(vals);
+      const valFilter = val => !excluded.has(val);
+
+      let filters = state.filters;
+      // Have to be careful not to modify the dashboard state so that
+      // the render actually triggers
+      if (sliceId in state.filters && col in state.filters[sliceId]) {
+        const newFilter = filters[sliceId][col].filter(valFilter);
+        filters = { ...filters, [sliceId]: newFilter };
       }
-      return { ...state, filter: newFilters, refresh: true };
+      return { ...state, filters, refresh };
     },
 
     // slice reducer

diff --git a/superset/assets/visualizations/table.js b/superset/assets/visualizations/table.js
@@ -66,6 +66,7 @@ function tableVis(slice, payload) {
       return d;
     });
 
+  const filters = slice.getFilters();
   table.append('tbody')
     .selectAll('tr')
     .data(data.records)
@@ -119,6 +120,12 @@ function tableVis(slice, payload) {
     .attr('data-sort', function (d) {
       return (d.isMetric) ? d.val : null;
     })
+    // Check if the dashboard currently has a filter for each row
+    .classed('filtered', d =>
+      filters &&
+      filters[d.col] &&
+      filters[d.col].indexOf(d.val) >= 0,
+    )
     .on('click', function (d) {
       if (!d.isMetric && fd.table_filter) {
         const td = d3.select(this);