From a52e86a69b2835551ae3eee73d7d3a51eeed47c6 Mon Sep 17 00:00:00 2001
From: Bruce Martin <bruce@chanzuckerberg.com>
Date: Wed, 13 Nov 2019 09:40:37 -0800
Subject: [PATCH] improve category and label name validation (#1034)

---
 .../src/components/categorical/categorical.js | 103 ++++++++++++------
 client/src/components/categorical/category.js |  83 ++++++++++----
 .../util/stateManager/annotationsHelpers.js   |   5 +
 3 files changed, 136 insertions(+), 55 deletions(-)

diff --git a/client/src/components/categorical/categorical.js b/client/src/components/categorical/categorical.js
index 9f9d2941..d9948a5d 100644
--- a/client/src/components/categorical/categorical.js
+++ b/client/src/components/categorical/categorical.js
@@ -14,6 +14,7 @@ import { Select } from "@blueprintjs/select";
 import { connect } from "react-redux";
 import * as globals from "../../globals";
 import Category from "./category";
+import { AnnotationsHelpers } from "../../util/stateManager";
 
 @connect(state => ({
   categoricalSelection: state.categoricalSelection,
@@ -50,13 +51,57 @@ class Categories extends React.Component {
   };
 
   handleDisableAnnoMode = () => {
-    this.setState({ createAnnoModeActive: false });
+    this.setState({
+      createAnnoModeActive: false,
+      categoryToDuplicate: null,
+      newCategoryText: ""
+    });
   };
 
   handleModalDuplicateCategorySelection = d => {
     this.setState({ categoryToDuplicate: d });
   };
 
+  categoryNameError = name => {
+    /*
+    return false if this is a LEGAL/acceptable category name,
+    or return an error type.
+    */
+    const { categoricalSelection } = this.props;
+    const allCategoryNames = Object.keys(categoricalSelection);
+
+    if (allCategoryNames.indexOf(name) !== -1) {
+      return "duplicate";
+    }
+
+    if (!AnnotationsHelpers.isLegalAnnotationName(name)) {
+      return "characters";
+    }
+
+    return false;
+  };
+
+  categoryNameErrorMessage = name => {
+    const err = this.categoryNameError(name);
+    if (err == "duplicate") {
+      return (
+        <span>
+          <span style={{ fontStyle: "italic" }}>{name}</span> already exists -
+          no duplicates allowed
+        </span>
+      );
+    }
+    if (err == "characters") {
+      return (
+        <span>
+          <span style={{ fontStyle: "italic" }}>{name}</span> contains illegal
+          characters. Hint: use alpha-numeric and underscore
+        </span>
+      );
+    }
+    return err;
+  };
+
   render() {
     const {
       createAnnoModeActive,
@@ -81,30 +126,26 @@ class Categories extends React.Component {
       >
         {/* READ ONLY CATEGORICAL FIELDS */}
         {/* this is duplicative but flat, could be abstracted */}
-        {_.map(
-          allCategoryNames,
-          catName =>
-            !schema.annotations.obsByName[catName].writable ? (
-              <Category
-                key={catName}
-                metadataField={catName}
-                createAnnoModeActive={createAnnoModeActive}
-                isUserAnno={false}
-              />
-            ) : null
+        {_.map(allCategoryNames, catName =>
+          !schema.annotations.obsByName[catName].writable ? (
+            <Category
+              key={catName}
+              metadataField={catName}
+              createAnnoModeActive={createAnnoModeActive}
+              isUserAnno={false}
+            />
+          ) : null
         )}
         {/* WRITEABLE FIELDS */}
-        {_.map(
-          allCategoryNames,
-          catName =>
-            schema.annotations.obsByName[catName].writable ? (
-              <Category
-                key={catName}
-                metadataField={catName}
-                createAnnoModeActive={createAnnoModeActive}
-                isUserAnno
-              />
-            ) : null
+        {_.map(allCategoryNames, catName =>
+          schema.annotations.obsByName[catName].writable ? (
+            <Category
+              key={catName}
+              metadataField={catName}
+              createAnnoModeActive={createAnnoModeActive}
+              isUserAnno
+            />
+          ) : null
         )}
         {writableCategoriesEnabled ? (
           <div>
@@ -124,8 +165,10 @@ class Categories extends React.Component {
                     <p>New, unique category name:</p>
                     <InputGroup
                       autoFocus
+                      value={newCategoryText}
                       intent={
-                        allCategoryNames.indexOf(newCategoryText) !== -1
+                        newCategoryText &&
+                        this.categoryNameError(newCategoryText)
                           ? "warning"
                           : "none"
                       }
@@ -138,16 +181,14 @@ class Categories extends React.Component {
                       style={{
                         marginTop: 7,
                         visibility:
-                          allCategoryNames.indexOf(newCategoryText) !== -1
+                          newCategoryText &&
+                          this.categoryNameError(newCategoryText)
                             ? "visible"
                             : "hidden",
                         color: Colors.ORANGE3
                       }}
                     >
-                      <span style={{ fontStyle: "italic" }}>
-                        {newCategoryText}
-                      </span>{" "}
-                      already exists
+                      {this.categoryNameErrorMessage(newCategoryText)}
                     </p>
                   </div>
 
@@ -186,9 +227,7 @@ class Categories extends React.Component {
                     </Tooltip>
                     <Button
                       onClick={this.handleCreateUserAnno}
-                      disabled={
-                        allCategoryNames.indexOf(newCategoryText) !== -1
-                      }
+                      disabled={this.categoryNameError(newCategoryText)}
                       intent="primary"
                       type="submit"
                     >
diff --git a/client/src/components/categorical/category.js b/client/src/components/categorical/category.js
index 84476ee0..078fc384 100644
--- a/client/src/components/categorical/category.js
+++ b/client/src/components/categorical/category.js
@@ -21,6 +21,7 @@ import {
 import * as globals from "../../globals";
 import Value from "./value";
 import sortedCategoryValues from "./util";
+import { AnnotationsHelpers } from "../../util/stateManager";
 
 @connect(state => ({
   colorAccessor: state.colors.colorAccessor,
@@ -82,18 +83,14 @@ class Category extends React.Component {
     dispatch({
       type: "annotation: disable add new label mode"
     });
+    this.setState({
+      newLabelText: ""
+    });
   };
 
   handleAddNewLabelToCategory = () => {
     const { dispatch, metadataField } = this.props;
     const { newLabelText } = this.state;
-    /*
-    XXX TODO - temporary code generates random label string. Remove
-    when the label creation UI is implemented.
-
-    const { newLabelText } = this.state;
-    */
-    // const newLabelText = `label${Math.random()}`;
     dispatch({
       type: "annotation: add new label to category",
       metadataField,
@@ -146,6 +143,48 @@ class Category extends React.Component {
     });
   };
 
+  labelNameError = name => {
+    /*
+    return false if this is a LEGAL/acceptable category name,
+    or return an error type.
+    */
+    const { metadataField, universe } = this.props;
+    const obsByName = universe.schema.annotations.obsByName;
+
+    if (obsByName[metadataField].categories.indexOf(name) !== -1) {
+      return "duplicate";
+    }
+
+    if (!AnnotationsHelpers.isLegalAnnotationName(name)) {
+      return "characters";
+    }
+
+    return false;
+  };
+
+  labelNameErrorMessage = name => {
+    const { metadataField } = this.props;
+    const err = this.labelNameError(name);
+    if (err == "duplicate") {
+      return (
+        <span>
+          <span style={{ fontStyle: "italic" }}>{name}</span> already exists
+          already exists within{" "}
+          <span style={{ fontStyle: "italic" }}>{metadataField}</span>{" "}
+        </span>
+      );
+    }
+    if (err == "characters") {
+      return (
+        <span>
+          <span style={{ fontStyle: "italic" }}>{name}</span> contains illegal
+          characters. Hint: use alpha-numeric and underscore
+        </span>
+      );
+    }
+    return err;
+  };
+
   toggleAll() {
     const { dispatch, metadataField } = this.props;
     dispatch({
@@ -346,10 +385,12 @@ class Category extends React.Component {
                         <p>New, unique label name:</p>
                         <InputGroup
                           autoFocus
+                          value={newLabelText}
                           intent={
-                            universe.schema.annotations.obsByName[
-                              metadataField
-                            ].categories.indexOf(newLabelText) !== -1
+                            // universe.schema.annotations.obsByName[
+                            //   metadataField
+                            // ].categories.indexOf(newLabelText) !== -1
+                            newLabelText && this.labelNameError(newLabelText)
                               ? "warning"
                               : "none"
                           }
@@ -362,21 +403,16 @@ class Category extends React.Component {
                           style={{
                             marginTop: 7,
                             visibility:
-                              universe.schema.annotations.obsByName[
-                                metadataField
-                              ].categories.indexOf(newLabelText) !== -1
+                              // universe.schema.annotations.obsByName[
+                              //   metadataField
+                              // ].categories.indexOf(newLabelText) !== -1
+                              newLabelText && this.labelNameError(newLabelText)
                                 ? "visible"
                                 : "hidden",
                             color: Colors.ORANGE3
                           }}
                         >
-                          <span style={{ fontStyle: "italic" }}>
-                            {newLabelText}
-                          </span>{" "}
-                          already exists within{" "}
-                          <span style={{ fontStyle: "italic" }}>
-                            {metadataField}
-                          </span>
+                          {this.labelNameErrorMessage(newLabelText)}
                         </p>
                       </div>
                     </div>
@@ -390,9 +426,10 @@ class Category extends React.Component {
                         <Button
                           disabled={
                             newLabelText.length === 0 ||
-                            universe.schema.annotations.obsByName[
-                              metadataField
-                            ].categories.indexOf(newLabelText) !== -1
+                            // universe.schema.annotations.obsByName[
+                            //   metadataField
+                            // ].categories.indexOf(newLabelText) !== -1
+                            this.labelNameError(newLabelText)
                           }
                           onClick={this.handleAddNewLabelToCategory}
                           intent="primary"
diff --git a/client/src/util/stateManager/annotationsHelpers.js b/client/src/util/stateManager/annotationsHelpers.js
index 53b2c796..4310781c 100644
--- a/client/src/util/stateManager/annotationsHelpers.js
+++ b/client/src/util/stateManager/annotationsHelpers.js
@@ -182,3 +182,8 @@ export function createWritableAnnotationDimensions(world, crossfilter) {
 	}, crossfilter);
 	return crossfilter;
 }
+
+const legalNames = /^\w+$/;
+export function isLegalAnnotationName(name) {
+	return legalNames.test(name);
+}