[GitHub] mistercrunch closed pull request #6578: Improve false negative on AlteredSliceTag

[GitBox <gi...@apache.org>]

mistercrunch closed pull request #6578: Improve false negative on AlteredSliceTag
URL: https://github.com/apache/incubator-superset/pull/6578
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx b/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
index 399b5a3aad..d5520819ee 100644
--- a/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
+++ b/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
@@ -284,4 +284,28 @@ describe('AlteredSliceTag', () => {
       expect(wrapper.instance().formatValue(filters, 'adhoc_filters')).toBe(expected);
     });
   });
+  describe('isEqualish', () => {
+    it('considers null, undefined, {} and [] as equal', () => {
+      const inst = wrapper.instance();
+      expect(inst.isEqualish(null, undefined)).toBe(true);
+      expect(inst.isEqualish(null, [])).toBe(true);
+      expect(inst.isEqualish(null, {})).toBe(true);
+      expect(inst.isEqualish(undefined, {})).toBe(true);
+    });
+    it('considers empty strings are the same as null', () => {
+      const inst = wrapper.instance();
+      expect(inst.isEqualish(undefined, '')).toBe(true);
+      expect(inst.isEqualish(null, '')).toBe(true);
+    });
+    it('considers deeply equal objects as equal', () => {
+      const inst = wrapper.instance();
+      expect(inst.isEqualish('', '')).toBe(true);
+      expect(inst.isEqualish({ a: 1, b: 2, c: 3 }, { a: 1, b: 2, c: 3 })).toBe(true);
+      // Out of order
+      expect(inst.isEqualish({ a: 1, b: 2, c: 3 }, { b: 2, a: 1, c: 3 })).toBe(true);
+
+      // Actually  not equal
+      expect(inst.isEqualish({ a: 1, b: 2, z: 9 }, { a: 1, b: 2, c: 3 })).toBe(false);
+    });
+  });
 });
diff --git a/superset/assets/src/components/AlteredSliceTag.jsx b/superset/assets/src/components/AlteredSliceTag.jsx
index bfde6f62bf..ff2bc61665 100644
--- a/superset/assets/src/components/AlteredSliceTag.jsx
+++ b/superset/assets/src/components/AlteredSliceTag.jsx
@@ -12,6 +12,24 @@ const propTypes = {
   currentFormData: PropTypes.object.isRequired,
 };
 
+function alterForComparison(value) {
+  // Considering `[]`, `{}`, `null` and `undefined` as identical
+  // for this purpose
+  if (value === undefined || value === null || value === '') {
+    return null;
+  } else if (typeof value === 'object') {
+    if (Array.isArray(value) && value.length === 0) {
+      return null;
+    }
+    const keys = Object.keys(value);
+    if (keys && keys.length === 0) {
+      return null;
+    }
+  }
+  return value;
+}
+
+
 export default class AlteredSliceTag extends React.Component {
 
   constructor(props) {
@@ -45,13 +63,17 @@ export default class AlteredSliceTag extends React.Component {
       if (['filters', 'having', 'having_filters', 'where'].includes(fdKey)) {
         continue;
       }
-      if (!isEqual(ofd[fdKey], cfd[fdKey])) {
+      if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
         diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
       }
     }
     return diffs;
   }
 
+  isEqualish(val1, val2) {
+    return isEqual(alterForComparison(val1), alterForComparison(val2));
+  }
+
   formatValue(value, key) {
     // Format display value based on the control type
     // or the value type


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org