Merge pull request #789 from theseyi/fix-reviewable-fields

removes hasRecentSuggestions computed property in favor of diffing s…

wherehows-web/app/components/dataset-compliance-row.js

@@ -2,6 +2,7 @@ import Ember from 'ember';
 import DatasetTableRow from 'wherehows-web/components/dataset-table-row';
 import {
   fieldIdentifierTypeIds,
+  fieldIdentifierOptions,
   defaultFieldDataTypeClassification,
   isMixedId,
   isCustomId,
@@ -16,8 +17,9 @@ import {
   highConfidenceSuggestions,
   accumulateFieldSuggestions
 } from 'wherehows-web/utils/datasets/compliance-suggestions';
+import { hasEnumerableKeys } from 'wherehows-web/utils/object';
 
-const { computed, get, getProperties } = Ember;
+const { computed, get, getProperties, getWithDefault } = Ember;
 
 /**
  * Extracts the suggestions for identifierType, logicalType suggestions, and confidence from a list of predictions
@@ -81,12 +83,24 @@ export default DatasetTableRow.extend({
    * @return {string | void} the current value if a suggestion & current value exists or undefined
    */
   getCurrentValueBeforeSuggestion(fieldProp) {
-    const prediction = get(this, 'prediction') || {};
-    const suggested = prediction[fieldProp];
-    const { label: current } = get(this, fieldProp) || {};
+    if (hasEnumerableKeys(get(this, 'prediction'))) {
+      /**
+       * Current value on policy prior to the suggested value
+       * @type {string}
+       */
+      const value = get(this, `field.${fieldProp}`);
 
-    if (suggested !== 'undefined' && current) {
-      return current;
+      /**
+       * Convenience function to get `label` attribute on the display properties object
+       * @param {Array<{value: string, label: string}>} valueObjects
+       */
+      const getLabel = (valueObjects = []) =>
+        Array.isArray(valueObjects) && (valueObjects.findBy('value', value) || {}).label;
+
+      return {
+        identifierType: getLabel(fieldIdentifierOptions),
+        logicalType: getLabel(get(this, 'fieldFormats'))
+      }[fieldProp];
     }
   },
 
@@ -181,16 +195,17 @@ export default DatasetTableRow.extend({
 
   /**
    * Extracts the field suggestions into a cached computed property, if a suggestion exists
-   * @type {Ember.computed}
+   * @type {Ember.ComputedProperty}
+   * @return {void | {identifierType: string, logicalType: string, confidence: number}}
    */
-  prediction: computed('field.suggestion', 'field.suggestionAuthority', 'hasRecentSuggestions', function() {
-    const { field = {}, hasRecentSuggestions } = getProperties(this, 'field', 'hasRecentSuggestions');
+  prediction: computed('field.suggestion', 'field.suggestionAuthority', function() {
+    const field = getWithDefault(this, 'field', {});
     // If a suggestionAuthority property exists on the field, then the user has already either accepted or ignored
     // the suggestion for this field. It's value should not be take into account on re-renders
     // this line takes that into account and substitutes an empty suggestion
     const { suggestion } = field.hasOwnProperty('suggestionAuthority') ? {} : field;
 
-    if (suggestion && hasRecentSuggestions) {
+    if (suggestion) {
       const { identifierTypePrediction, logicalTypePrediction } = suggestion;
       // The order of the array supplied to getFieldSuggestions is importance to it's order of operations
       // the last element in the array takes highest precedence: think Object.assign

wherehows-web/app/components/dataset-compliance.js

@@ -4,6 +4,7 @@ import {
   classifiers,
   datasetClassifiers,
   fieldIdentifierTypes,
+  fieldIdentifierOptions,
   fieldIdentifierTypeIds,
   idLogicalTypes,
   nonIdFieldLogicalTypes,
@@ -12,8 +13,7 @@ import {
   logicalTypesForIds,
   logicalTypesForGeneric,
   hasPredefinedFieldFormat,
-  getDefaultLogicalType,
-  lastSeenSuggestionInterval
+  getDefaultLogicalType
 } from 'wherehows-web/constants';
 import {
   isPolicyExpectedShape,
@@ -85,8 +85,6 @@ const datasetClassifiersKeys = Object.keys(datasetClassifiers);
 const policyComplianceEntitiesKey = 'complianceInfo.complianceEntities';
 
 assert('`fieldIdentifierTypes` contains an object with a key `none`', typeof fieldIdentifierTypes.none === 'object');
-const fieldIdentifierTypeKeysBarNone = Object.keys(fieldIdentifierTypes).filter(k => k !== 'none');
-const fieldDisplayKeys = ['none', '_', ...fieldIdentifierTypeKeysBarNone];
 
 /**
  * Returns a list of changeSet fields that requires user attention
@@ -94,23 +92,6 @@ const fieldDisplayKeys = ['none', '_', ...fieldIdentifierTypeKeysBarNone];
  */
 const changeSetFieldsRequiringReview = arrayFilter(fieldChangeSetRequiresReview);
 
-/**
- * A list of field identifier types mapped to label, value options for select display
- * @type {any[]|Array.<{value: String, label: String}>}
- */
-const fieldIdentifierOptions = fieldDisplayKeys.map(fieldIdentifierType => {
-  const divider = '──────────';
-  const { value = fieldIdentifierType, displayAs: label = divider } = fieldIdentifierTypes[fieldIdentifierType] || {};
-
-  // Adds a divider for a value of _
-  // Visually this separates ID from none fieldIdentifierTypes
-  return {
-    value,
-    label,
-    isDisabled: fieldIdentifierType === '_'
-  };
-});
-
 export default Component.extend({
   sortColumnWithName: 'identifierField',
   filterBy: 'identifierField',
@@ -322,27 +303,6 @@ export default Component.extend({
     return getWithDefault(this, 'complianceInfo.modifiedTime', 0);
   }),
 
-  /**
-   * Checks that suggested values postdate the last save date or that suggestions exist
-   * @type {boolean}
-   */
-  hasRecentSuggestions: computed('policyModificationTimeInEpoch', 'complianceSuggestion', function() {
-    const { policyModificationTimeInEpoch, complianceSuggestion = {} } = getProperties(this, [
-      'policyModificationTimeInEpoch',
-      'complianceSuggestion'
-    ]);
-    const { lastModified: suggestionsLastModified, complianceSuggestions = [] } = complianceSuggestion;
-
-    // If modification dates exist, check that the suggestions are considered 'unseen' since the last time the policy was saved
-    // and we have at least 1 suggestion, otherwise check that the count of suggestions is at least 1
-    if (policyModificationTimeInEpoch && suggestionsLastModified) {
-      const suggestionIsUnseen = suggestionsLastModified - policyModificationTimeInEpoch >= lastSeenSuggestionInterval;
-      return complianceSuggestions.length && suggestionIsUnseen;
-    }
-
-    return !!complianceSuggestions.length;
-  }),
-
   /**
    * @type {Boolean} cached boolean flag indicating that fields do contain a `kafka type`
    *    tracking header.
@@ -428,9 +388,10 @@ export default Component.extend({
    *
    * @param {Array<object>} columnFieldProps
    * @param {Array<object>} complianceEntities
+   * @param {policyModificationTime}
    * @return {object}
    */
-  mapColumnIdFieldsToCurrentPrivacyPolicy(columnFieldProps, complianceEntities) {
+  mapColumnIdFieldsToCurrentPrivacyPolicy(columnFieldProps, complianceEntities, { policyModificationTime }) {
     const getKeysOnField = (keys = [], fieldName, source = []) => {
       const sourceField = source.find(({ identifierField }) => identifierField === fieldName) || {};
       let ret = {};
@@ -456,6 +417,7 @@ export default Component.extend({
           identifierField,
           dataType,
           ...currentPrivacyAttrs,
+          policyModificationTime,
           privacyPolicyExists: hasEnumerableKeys(currentPrivacyAttrs),
           isDirty: false
         }
@@ -479,7 +441,9 @@ export default Component.extend({
       // Dataset fields that currently have a compliance policy
       const currentComplianceEntities = get(this, policyComplianceEntitiesKey) || [];
 
-      return this.mapColumnIdFieldsToCurrentPrivacyPolicy(columnFieldProps, currentComplianceEntities);
+      return this.mapColumnIdFieldsToCurrentPrivacyPolicy(columnFieldProps, currentComplianceEntities, {
+        policyModificationTime: getWithDefault(this, 'complianceInfo.modifiedTime', 0)
+      });
     }
   ),
 
@@ -526,7 +490,9 @@ export default Component.extend({
    */
   identifierFieldToSuggestion: computed('complianceSuggestion', function() {
     const identifierFieldToSuggestion = {};
-    const complianceSuggestions = getWithDefault(this, 'complianceSuggestion.complianceSuggestions', []);
+    const complianceSuggestion = get(this, 'complianceSuggestion') || {};
+    const { lastModified: suggestionsModificationTime, complianceSuggestions = [] } = complianceSuggestion;
+
     // If the compliance suggestions array contains suggestions the create reduced map,
     // otherwise, ignore
     if (complianceSuggestions.length) {
@@ -535,7 +501,8 @@ export default Component.extend({
           ...identifierFieldToSuggestion,
           [fieldName]: {
             identifierTypePrediction,
-            logicalTypePrediction
+            logicalTypePrediction,
+            suggestionsModificationTime
           }
         }),
         identifierFieldToSuggestion

wherehows-web/app/constants/dataset-compliance.ts

@@ -1,3 +1,4 @@
+import { fieldIdentifierTypes } from 'wherehows-web/constants/metadata-acquisition';
 /**
  * Defines a map of values for the compliance policy on a dataset
  * @type {object}
@@ -17,4 +18,37 @@ const compliancePolicyStrings = {
   }
 };
 
-export { compliancePolicyStrings };
+/**
+ * List of identifier type keys without the none object value
+ * @type {Array<string>}
+ */
+const fieldIdentifierTypeKeysBarNone: Array<string> = Object.keys(fieldIdentifierTypes).filter(k => k !== 'none');
+
+/**
+ * Keys for the field display options
+ * @type {Array<string>}
+ */
+const fieldDisplayKeys: Array<string> = ['none', '_', ...fieldIdentifierTypeKeysBarNone];
+
+/**
+ * A list of field identifier types mapped to label, value options for select display
+ * @type {Array<{value: string, label: string, isDisabled: boolean}>}
+ */
+const fieldIdentifierOptions: Array<{
+  value: string;
+  label: string;
+  isDisabled: boolean;
+}> = fieldDisplayKeys.map(fieldIdentifierType => {
+  const divider = '──────────';
+  const { value = fieldIdentifierType, displayAs: label = divider } = fieldIdentifierTypes[fieldIdentifierType] || {};
+
+  // Adds a divider for a value of _
+  // Visually this separates ID from none fieldIdentifierTypes
+  return {
+    value,
+    label,
+    isDisabled: fieldIdentifierType === '_'
+  };
+});
+
+export { compliancePolicyStrings, fieldIdentifierOptions };

wherehows-web/app/templates/datasets/dataset-compliance/-dataset-compliance-entities.hbs

@@ -111,8 +111,7 @@
         {{#body.row
           field=field
           isNewComplianceInfo=isNewComplianceInfo
-          class=(if (and hasRecentSuggestions field.suggestion) "dataset-compliance-fields__has-suggestions")
-          hasRecentSuggestions=hasRecentSuggestions
+          class=(if field.suggestion "dataset-compliance-fields__has-suggestions")
           onFieldLogicalTypeChange=(action 'onFieldLogicalTypeChange')
           onFieldClassificationChange=(action 'onFieldClassificationChange')
           onSuggestionIntent=(action 'onFieldSuggestionIntentChange')

wherehows-web/app/utils/datasets/compliance-policy.js

@@ -1,5 +1,6 @@
 import Ember from 'ember';
 import { datasetClassifiers } from 'wherehows-web/constants/dataset-classification';
+import { lastSeenSuggestionInterval } from 'wherehows-web/constants/metadata-acquisition';
 
 const { assert, Logger: { warn } } = Ember;
 
@@ -110,6 +111,16 @@ const isPolicyExpectedShape = (candidatePolicy = {}) => {
   return false;
 };
 
+/**
+ * Checks if the compliance suggestion has a date that is equal or exceeds the policy mod time by at least the
+ * ms time in lastSeenSuggestionInterval
+ * @param {number} [policyModificationTime = 0] timestamp for the policy modification date
+ * @param {number} suggestionModificationTime timestamp for the suggestion modification date
+ * @return {boolean}
+ */
+const isRecentSuggestion = (policyModificationTime = 0, suggestionModificationTime) =>
+  !!suggestionModificationTime && suggestionModificationTime - policyModificationTime >= lastSeenSuggestionInterval;
+
 /**
  * Checks if a compliance policy changeSet field requires user attention: if a suggestion
  * is available  but the user has not indicated intent or a policy for the field does not currently exist remotely
@@ -143,6 +154,7 @@ const mergeMappedColumnFieldsWithSuggestions = (mappedColumnFields = {}, fieldSu
       identifierType,
       logicalType,
       securityClassification,
+      policyModificationTime,
       privacyPolicyExists,
       isDirty
     } = mappedColumnFields[fieldName];
@@ -158,8 +170,9 @@ const mergeMappedColumnFieldsWithSuggestions = (mappedColumnFields = {}, fieldSu
       classification: securityClassification
     };
 
-    // If a suggestion exists for this field add the suggestion attribute to the field properties
-    if (suggestion) {
+    // If a suggestion exists for this field add the suggestion attribute to the field properties / changeSet
+    // Check if suggestion isRecent before augmenting, otherwise, suggestion will not be considered on changeSet
+    if (suggestion && isRecentSuggestion(policyModificationTime, suggestion.suggestionsModificationTime)) {
       return { ...field, suggestion };
     }
 
@@ -170,5 +183,6 @@ export {
   createInitialComplianceInfo,
   isPolicyExpectedShape,
   fieldChangeSetRequiresReview,
-  mergeMappedColumnFieldsWithSuggestions
+  mergeMappedColumnFieldsWithSuggestions,
+  isRecentSuggestion
 };

wherehows-web/tests/helpers/datasets/compliance-policy/recent-suggestions-constants.ts

@@ -0,0 +1,80 @@
+interface IMockTimeStamp {
+  policyModificationTime: number;
+  suggestionModificationTime: number;
+  __assertMsg__: string;
+  __isRecent__: boolean;
+}
+
+const sevenDays: number = 7 * 24 * 60 * 60 * 1000;
+
+const isRecent = ({ policyModificationTime, suggestionModificationTime }: IMockTimeStamp) =>
+  !!suggestionModificationTime && suggestionModificationTime - policyModificationTime >= sevenDays;
+
+const mockTimeStamps = [
+  {
+    policyModificationTime: new Date('2017-10-08 06:58:53.0').getTime(),
+    suggestionModificationTime: new Date('2017-10-15 06:58:53.0').getTime(),
+    __assertMsg__: 'Suggestion time is exactly 7 days after policy modification date',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: new Date('2017-10-08 06:58:53.0').getTime(),
+    suggestionModificationTime: new Date('2017-10-15 06:57:53.0').getTime(),
+    __assertMsg__: 'Suggestion time is slightly less than 7 days after policy modification date',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: new Date('2017-10-08 06:58:53.0').getTime(),
+    suggestionModificationTime: new Date('2017-10-14 06:58:53.0').getTime(),
+    __assertMsg__: 'Suggestion time is 6 days after policy modification date',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: new Date('2017-10-08 06:58:53.0').getTime(),
+    suggestionModificationTime: new Date('2016-10-15 06:58:53.0').getTime(),
+    __assertMsg__: 'Suggestion time is before policy modification date',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: new Date('2017-10-08 06:58:53.0').getTime(),
+    suggestionModificationTime: new Date('2017-11-15 06:58:53.0').getTime(),
+    __assertMsg__: 'Suggestion time is greater than minimum interval since policy modification date',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: 0,
+    suggestionModificationTime: new Date('2017-11-15 06:58:53.0').getTime(),
+    __assertMsg__: 'Policy modification time does not exist',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: new Date('2017-11-15 06:58:53.0').getTime(),
+    suggestionModificationTime: 0,
+    __assertMsg__: 'Suggestion modification time does not exist',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  },
+  {
+    policyModificationTime: 0,
+    suggestionModificationTime: 0,
+    __assertMsg__: 'Suggestion and policy modification time does not exist',
+    get __isRecent__(): boolean {
+      return isRecent(this);
+    }
+  }
+];
+
+export { mockTimeStamps };

wherehows-web/tests/unit/utils/datasets/compliance-policy-test.js

@@ -1,9 +1,11 @@
 import {
   createInitialComplianceInfo,
-  fieldChangeSetRequiresReview
+  fieldChangeSetRequiresReview,
+  isRecentSuggestion
 } from 'wherehows-web/utils/datasets/compliance-policy';
 
 import { mockFieldChangeSets } from 'wherehows-web/tests/helpers/datasets/compliance-policy/field-changeset-constants';
+import { mockTimeStamps } from 'wherehows-web/tests/helpers/datasets/compliance-policy/recent-suggestions-constants';
 import { module, test } from 'qunit';
 
 module('Unit | Utility | datasets/compliance policy');
@@ -32,10 +34,24 @@ test('Compliance utility function fieldChangeSetRequiresReview exists', function
   assert.ok(typeof fieldChangeSetRequiresReview() === 'boolean', 'fieldChangeSetRequiresReview returns a boolean');
 });
 
-test('Compliance utility function fieldChangeSetRequiresReview exists', function(assert) {
+test('fieldChangeSetRequiresReview correctly determines if a fieldChangeSet requires review', function(assert) {
   assert.expect(mockFieldChangeSets.length);
 
   mockFieldChangeSets.forEach(changeSet =>
     assert.ok(fieldChangeSetRequiresReview(changeSet) === changeSet.__requiresReview__, changeSet.__msg__)
   );
 });
+
+test('isRecentSuggestion exists', function(assert) {
+  assert.expect(1);
+  assert.ok(typeof isRecentSuggestion === 'function', 'isRecentSuggestion is a function');
+});
+
+test('isRecentSuggestion correctly determines if a suggestion is recent or not', function(assert) {
+  assert.expect(mockTimeStamps.length);
+
+  mockTimeStamps.forEach(({ policyModificationTime, suggestionModificationTime, __isRecent__, __assertMsg__ }) => {
+    const recent = isRecentSuggestion(policyModificationTime, suggestionModificationTime);
+    assert.ok(recent === __isRecent__, `${__assertMsg__} isRecent? ${recent}`);
+  });
+});