Merge pull request #600 from theseyi/dupes

fixes issue with dupes in saved compliance entities list: ensures tha…

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

@@ -6,7 +6,8 @@ import {
   fieldIdentifierTypes,
   idLogicalTypes,
   nonIdFieldLogicalTypes,
-  defaultFieldDataTypeClassification
+  defaultFieldDataTypeClassification,
+  compliancePolicyStrings
 } from 'wherehows-web/constants';
 import { isPolicyExpectedShape } from 'wherehows-web/utils/datasets/functions';
 
@@ -23,25 +24,14 @@ const {
   String: { htmlSafe }
 } = Ember;
 
-// TODO: DSS-6671 Extract to constants module
+const { complianceDataException, missingTypes, successUpdating, failedUpdating, helpText } = compliancePolicyStrings;
-const missingTypes = 'Looks like some fields may contain privacy data but do not have a specified `Field Format`?';
+
-const successUpdating = 'Your changes have been successfully saved!';
-const failedUpdating = 'Oops! We are having trouble updating this dataset at the moment.';
 const hiddenTrackingFieldsMsg = htmlSafe(
   '<p>Some fields in this dataset have been hidden from the table(s) below. ' +
     "These are tracking fields for which we've been able to predetermine the compliance classification.</p>" +
     '<p>For example: <code>header.memberId</code>, <code>requestHeader</code>. ' +
     'Hopefully, this saves you some scrolling!</p>'
 );
-const helpText = {
-  classification:
-    'The default value is taken from go/dht and should be good enough in most cases. ' +
-      'You can optionally override it if required by house security.',
-  subjectOwner:
-    'Member ID (Subject Owner): Choose this option if the member specified here is not the owner of this ' +
-      'record, e.g. the profile ID viewed, or recipient ID of a message. See wiki for more explanation. ' +
-      'Mixed ID: Choose this option if the field is a URN containing a mixture of member, organization, or group ID.'
-};
 
 /**
  * Takes a string, returns a formatted string. Niche , single use case
@@ -85,10 +75,10 @@ const policyFieldClassificationKey = 'complianceInfo.fieldClassification';
 const policyComplianceEntitiesKey = 'complianceInfo.complianceEntities';
 /**
  * Duplicate check using every to short-circuit iteration
- * @param {Array} names = [] the list to check for dupes
+ * @param {Array} list = [] the list to check for dupes
  * @return {Boolean} true is unique, false otherwise
  */
-const fieldNamesAreUnique = (names = []) => names.every((name, index) => names.indexOf(name) === index);
+const listIsUnique = (list = []) => new Set(list).size === list.length;
 
 assert('`fieldIdentifierTypes` contains an object with a key `none`', typeof fieldIdentifierTypes.none === 'object');
 const fieldIdentifierTypeKeysBarNone = Object.keys(fieldIdentifierTypes).filter(k => k !== 'none');
@@ -146,8 +136,17 @@ export default Component.extend({
   fieldIdentifierOptions,
   hiddenTrackingFields: hiddenTrackingFieldsMsg,
   classNames: ['compliance-container'],
-  isEditing: false,
   classNameBindings: ['isEditing:compliance-container--edit-mode'],
+  /**
+   * Flag indicating that the component is in edit mode
+   * @type {String}
+   */
+  isEditing: false,
+  /**
+   * Flag indicating that the component is currently saving / attempting to save the privacy policy
+   * @type {String}
+   */
+  isSaving: false,
 
   didReceiveAttrs() {
     this._super(...arguments);
@@ -164,7 +163,8 @@ export default Component.extend({
   validateAttrs() {
     const fieldNames = getWithDefault(this, 'schemaFieldNamesMappedToDataTypes', []).mapBy('fieldName');
 
-    if (fieldNamesAreUnique(fieldNames.sort())) {
+    // identifier field names from the column api should be unique
+    if (listIsUnique(fieldNames.sort())) {
       return set(this, '_hasBadData', false);
     }
 
@@ -196,18 +196,20 @@ export default Component.extend({
    *    tracking header.
    *    Used to indicate to viewer that these fields are hidden.
    */
-  containsHiddenTrackingFields: computed('complianceDataFieldsSansHiddenTracking.length', function() {
+  containsHiddenTrackingFields: computed('truncatedSchemaFields.length', function() {
-    // If their is a diff in complianceDataFields and complianceDataFieldsSansHiddenTracking,
+    // If their is a diff in complianceDataFields and truncatedSchemaFields,
     //   then we have hidden tracking fields
-    return get(this, 'complianceDataFieldsSansHiddenTracking.length') !== get(this, 'complianceDataFields.length');
+    return get(this, 'truncatedSchemaFields.length') !== get(this, 'complianceDataFields.length');
   }),
 
   /**
    * @type {Array.<Object>} Filters the mapped compliance data fields without `kafka type`
    *   tracking headers
    */
-  complianceDataFieldsSansHiddenTracking: computed('complianceDataFields.[]', function() {
+  truncatedSchemaFields: computed('schemaFieldNamesMappedToDataTypes', function() {
-    return get(this, 'complianceDataFields').filter(({ identifierField }) => !isTrackingHeaderField(identifierField));
+    return getWithDefault(this, 'schemaFieldNamesMappedToDataTypes', []).filter(
+      ({ fieldName }) => !isTrackingHeaderField(fieldName)
+    );
   }),
 
   /**
@@ -222,6 +224,28 @@ export default Component.extend({
       .every(({ value }) => [true, false].includes(value));
   }),
 
+  /**
+   * Determines if the save feature is allowed for the current dataset, otherwise e.g. interface should be disabled
+   * @type {Ember.computed}
+   */
+  isSavingDisabled: computed('isDatasetFullyClassified', 'isSaving', function() {
+    const { isDatasetFullyClassified, isSaving } = getProperties(this, ['isDatasetFullyClassified', 'isSaving']);
+
+    return !isDatasetFullyClassified || isSaving;
+  }),
+
+  /**
+   * Checks to ensure the the number of fields added to compliance entities is less than or equal
+   * to what is available on the dataset schema
+   * @return {boolean}
+   */
+  isSchemaFieldLengthGreaterThanComplianceEntities() {
+    const { length: columnFieldsLength } = getWithDefault(this, 'schemaFieldNamesMappedToDataTypes', []);
+    const { length: complianceListLength } = get(this, policyComplianceEntitiesKey);
+
+    return columnFieldsLength >= complianceListLength;
+  },
+
   /**
    * Computed property that is dependent on all the keys in the datasetClassification map
    *   Returns a new map of datasetClassificationKey: String-> Object.<Boolean|undefined,String>
@@ -250,7 +274,7 @@ export default Component.extend({
     `${policyComplianceEntitiesKey}.@each.identifierType`,
     `${policyComplianceEntitiesKey}.[]`,
     policyFieldClassificationKey,
-    'schemaFieldNamesMappedToDataTypes',
+    'truncatedSchemaFields',
     function() {
       /**
        * Retrieves an attribute on the `policyComplianceEntitiesKey` where the identifierField is the same as the
@@ -261,7 +285,17 @@ export default Component.extend({
        */
       const getAttributeOnField = (attribute, fieldName) => {
         const complianceEntities = get(this, policyComplianceEntitiesKey) || [];
-        const sourceField = complianceEntities.find(({ identifierField }) => identifierField === fieldName);
+        // const sourceField = complianceEntities.find(({ identifierField }) => identifierField === fieldName);
+        let sourceField;
+
+        // For long records: >500 elements, the find operation is consistently less performant than a for-loop:
+        // trading elegance for efficiency here
+        for (let i = 0; i < complianceEntities.length; i++) {
+          if (complianceEntities[i]['identifierField'] === fieldName) {
+            sourceField = complianceEntities[i];
+            break;
+          }
+        }
         return sourceField ? sourceField[attribute] : null;
       };
 
@@ -277,11 +311,7 @@ export default Component.extend({
 
       // Set default or if already in policy, retrieve current values from
       //   privacyCompliancePolicy.complianceEntities
-      return getWithDefault(
+      return getWithDefault(this, 'truncatedSchemaFields', []).map(({ fieldName: identifierField, dataType }) => {
-        this,
-        'schemaFieldNamesMappedToDataTypes',
-        []
-      ).map(({ fieldName: identifierField, dataType }) => {
         const [identifierType, isSubject, logicalType] = getAttributesOnField(
           ['identifierType', 'isSubject', 'logicalType'],
           identifierField
@@ -372,7 +402,7 @@ export default Component.extend({
    * @param { Boolean} [isSaving = false] optional flag indicating when the user intends to persist / save
    */
   whenRequestCompletes(request, { successMessage, isSaving = false } = {}) {
-    Promise.resolve(request)
+    return Promise.resolve(request)
       .then(({ status = 'error' }) => {
         return status === 'ok'
           ? setProperties(this, {
@@ -421,10 +451,11 @@ export default Component.extend({
    */
   confirmUnformattedFields() {
     // Current list of compliance entities on policy
-    const complianceList = get(this, policyComplianceEntitiesKey);
+    const complianceEntities = get(this, policyComplianceEntitiesKey);
-    // All candidate fields that can be on policy
+    // All candidate fields that can be on policy, excluding tracking type fields
-    const datasetFields = get(this, 'complianceDataFieldsSansHiddenTracking');
+    const datasetFields = get(this, 'complianceDataFields');
-    // Fields that do not have a logicalType and not identifierType or identifierType is `fieldIdentifierTypes.none`
+    const fieldsCurrentlyInComplianceList = complianceEntities.mapBy('identifierField');
+    // Fields that do not have a logicalType, and no identifierType or identifierType is `fieldIdentifierTypes.none`
     const unformattedFields = datasetFields.filter(
       ({ identifierType, logicalType }) =>
         !logicalType && (fieldIdentifierTypes.none.value === identifierType || !identifierType)
@@ -433,6 +464,17 @@ export default Component.extend({
 
     // If there are unformatted fields, require confirmation from user
     if (!isEmpty(unformattedFields)) {
+      // Ensure that the unformatted fields to be added to the entities are not already present
+      // in the previous compliance entities list
+      const unformattedComplianceEntities = unformattedFields
+        .filter(({ identifierField }) => !fieldsCurrentlyInComplianceList.includes(identifierField))
+        .map(({ identifierField }) => ({
+          identifierField,
+          identifierType: fieldIdentifierTypes.none.value,
+          isSubject: null,
+          logicalType: void 0
+        }));
+
       isConfirmed = confirm(
         `There are ${unformattedFields.length} non-ID fields that have no field format specified. ` +
           `Are you sure they don't contain any of the following PII?\n\n` +
@@ -441,21 +483,43 @@ export default Component.extend({
 
       // If the user confirms that this is ok, apply the unformatted fields on the current compliance list
       //   to be saved
-      isConfirmed &&
+      isConfirmed && complianceEntities.setObjects([...complianceEntities, ...unformattedComplianceEntities]);
-        complianceList.setObjects([
-          ...complianceList,
-          ...unformattedFields.map(({ identifierField }) => ({
-            identifierField,
-            identifierType: fieldIdentifierTypes.none.value,
-            isSubject: null,
-            logicalType: void 0
-          }))
-        ]);
     }
 
     return isConfirmed;
   },
 
+  /**
+   * Ensures the fields in the updated list of compliance entities meet the criteria
+   * checked in the function. If criteria is not met, an the returned promise is settled
+   * in a rejected state, otherwise fulfilled
+   * @method
+   * @return {any | Promise<any>}
+   */
+  validateFields() {
+    const complianceEntities = get(this, policyComplianceEntitiesKey);
+    const idFieldsHaveValidLogicalType = this.checkEachEntityByLogicalType(
+      complianceEntities.filter(({ identifierType }) => fieldIdentifierTypeIds.includes(identifierType)),
+      [...genericLogicalTypes, ...idLogicalTypes]
+    );
+    const fieldIdentifiersAreUnique = listIsUnique(complianceEntities.mapBy('identifierField'));
+    const schemaFieldLengthGreaterThanComplianceEntities = this.isSchemaFieldLengthGreaterThanComplianceEntities();
+
+    //
+    if (!fieldIdentifiersAreUnique || !schemaFieldLengthGreaterThanComplianceEntities) {
+      return Promise.reject(new Error(complianceDataException));
+    }
+
+    if (!idFieldsHaveValidLogicalType) {
+      return Promise.reject(
+        setProperties(this, {
+          _message: missingTypes,
+          _alertType: 'danger'
+        })
+      );
+    }
+  },
+
   actions: {
     /**
      * Handle the user intent to place this compliance component in edit mode
@@ -634,26 +698,31 @@ export default Component.extend({
     /**
      * If all validity checks are passed, invoke onSave action on controller
      */
-    saveCompliance() {
+    async saveCompliance() {
-      const complianceList = get(this, policyComplianceEntitiesKey);
+      const setSaveFlag = (flag = false) => set(this, 'isSaving', flag);
-      const idFieldsHaveValidLogicalType = this.checkEachEntityByLogicalType(
+      // If fields are confirmed as unique we can proceed with saving compliance entities
-        complianceList.filter(({ identifierType }) => fieldIdentifierTypeIds.includes(identifierType)),
-        [...genericLogicalTypes, ...idLogicalTypes]
-      );
       const saveConfirmed = this.confirmUnformattedFields();
 
       // If user provides confirmation for unformatted fields or there are none,
-      // then check if id type fields have a specified logical type
+      // then validate fields against expectations
-      // otherwise we should inform the user of missing field formats for id fields, which are not allowed
+      // otherwise inform user of validation exception
       if (saveConfirmed) {
-        if (idFieldsHaveValidLogicalType) {
+        try {
-          return this.whenRequestCompletes(get(this, 'onSave')(), { isSaving: true });
+          const isSaving = true;
-        }
+          const onSave = get(this, 'onSave');
+          setSaveFlag(isSaving);
+          await this.validateFields();
 
-        setProperties(this, {
+          return await this.whenRequestCompletes(onSave(), { isSaving });
-          _message: missingTypes,
+        } catch (e) {
-          _alertType: 'danger'
+          // Flag this dataset's data as problematic
-        });
+          if (e instanceof Error && e.message === complianceDataException) {
+            set(this, '_hasBadData', true);
+            window.scrollTo(0, 0);
+          }
+        } finally {
+          setSaveFlag();
+        }
       }
     },
 

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

@@ -0,0 +1,17 @@
+/**
+ * Defines a map of values for the compliance policy on a dataset
+ * @type {Object}
+ */
+const compliancePolicyStrings = {
+  complianceDataException: 'Unexpected discrepancy in compliance data',
+  missingTypes: 'Looks like some fields may contain privacy data but do not have a specified `Field Format`?',
+  successUpdating: 'Your changes have been successfully saved!',
+  failedUpdating: 'Oops! We are having trouble updating this dataset at the moment.',
+  helpText: {
+    classification:
+      'The default value is taken from go/dht and should be good enough in most cases. ' +
+        'You can optionally override it if required by house security.'
+  }
+};
+
+export { compliancePolicyStrings };

wherehows-web/app/constants/index.js

@@ -1,2 +1,3 @@
 export * from 'wherehows-web/constants/metadata-acquisition';
 export * from 'wherehows-web/constants/dataset-classification';
+export * from 'wherehows-web/constants/dataset-compliance';

wherehows-web/app/styles/components/notifications/_action-notifications.scss

@@ -1,7 +1,11 @@
 .post-action-notification {
   margin-bottom: 0;
-  display: inline-block;
   border-radius: 2px;
   padding: 5px;
   vertical-align: top;
 }
+
+/// Defines styles for a notification bar in an action bar
+.action-footer-notification {
+  display: inline-block;
+}

wherehows-web/app/templates/components/dataset-compliance.hbs

@@ -15,11 +15,12 @@
     <section class="action-bar">
       {{#if isEditing}}
         <div class="container action-bar__content">
-          <button class="nacho-button nacho-button--large-inverse action-bar__item"
+          <button
+            class="nacho-button nacho-button--large-inverse action-bar__item"
             title={{unless isDatasetFullyClassified
                            "Ensure you have provided a yes/no value for all dataset tags"
                            "Save"}}
-                    disabled={{not isDatasetFullyClassified}}
+              disabled={{isSavingDisabled}}
             {{action "saveCompliance"}}>
             Save
           </button>
@@ -32,7 +33,7 @@
           </button>
 
           {{#if _message}}
-            <div class="alert alert-{{_alertType}} post-action-notification" role="alert">
+            <div class="alert alert-{{_alertType}} post-action-notification action-footer-notification" role="alert">
               {{_message}}
             </div>
           {{/if}}
@@ -51,7 +52,9 @@
     </div>
 
     {{#if isEditing}}
+      {{#if (not _hasBadData)}}
         {{json-upload receiveJsonFile=(action "onComplianceJsonUpload") class="secondary-actions__action"}}
+      {{/if}}
     {{else}}
       <button
         {{action "onEdit"}}
@@ -165,7 +168,7 @@
 
   {{#dataset-table
     class="nacho-table--stripped"
-    fields=complianceDataFieldsSansHiddenTracking
+    fields=complianceDataFields
     sortColumnWithName=sortColumnWithName
     filterBy=filterBy
     sortDirection=sortDirection