Merge pull request #2997 from github/robertbrignull/model-table-validation

Show validation results in the model editor

Author: robertbrignull

extensions/ql-vscode/src/stories/model-editor/MethodRow.stories.tsx

@@ -153,3 +153,28 @@ MultipleModelings.args = {
   methodCanBeModeled: true,
   viewState,
 };
+
+export const ValidationError = Template.bind({});
+ValidationError.args = {
+  method,
+  modeledMethods: [
+    { ...modeledMethod, type: "source" },
+    { ...modeledMethod, type: "source" },
+  ],
+  methodCanBeModeled: true,
+  viewState,
+};
+
+export const MultipleValidationErrors = Template.bind({});
+MultipleValidationErrors.args = {
+  method,
+  modeledMethods: [
+    { ...modeledMethod, type: "source" },
+    { ...modeledMethod, type: "source" },
+    { ...modeledMethod, type: "sink" },
+    { ...modeledMethod, type: "sink" },
+    { ...modeledMethod, type: "neutral", kind: "source" },
+  ],
+  methodCanBeModeled: true,
+  viewState,
+};

extensions/ql-vscode/src/view/method-modeling/ModeledMethodAlert.tsx

@@ -6,12 +6,12 @@ import { useCallback } from "react";
 
 type Props = {
   error: ModeledMethodValidationError;
-  setSelectedIndex: (index: number) => void;
+  setSelectedIndex?: (index: number) => void;
 };
 
 export const ModeledMethodAlert = ({ error, setSelectedIndex }: Props) => {
   const handleClick = useCallback(() => {
-    setSelectedIndex(error.index);
+    setSelectedIndex?.(error.index);
   }, [error.index, setSelectedIndex]);
 
   return (
@@ -22,7 +22,11 @@ export const ModeledMethodAlert = ({ error, setSelectedIndex }: Props) => {
       message={
         <>
           {error.message}{" "}
-          <TextButton onClick={handleClick}>{error.actionText}</TextButton>
+          {setSelectedIndex ? (
+            <TextButton onClick={handleClick}>{error.actionText}</TextButton>
+          ) : (
+            error.actionText
+          )}
         </>
       }
     />

extensions/ql-vscode/src/view/model-editor/MethodRow.tsx

@@ -31,6 +31,8 @@ import { ModelEditorViewState } from "../../model-editor/shared/view-state";
 import { Codicon } from "../common";
 import { canAddNewModeledMethod } from "../../model-editor/shared/multiple-modeled-methods";
 import { DataGridCell, DataGridRow } from "../common/DataGrid";
+import { validateModeledMethods } from "../../model-editor/shared/validation";
+import { ModeledMethodAlert } from "../method-modeling/ModeledMethodAlert";
 
 const ApiOrMethodRow = styled.div`
   min-height: calc(var(--input-height) * 1px);
@@ -111,6 +113,11 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
       [modeledMethodsProp, method, viewState],
     );
 
+    const validationErrors = useMemo(
+      () => validateModeledMethods(modeledMethods),
+      [modeledMethods],
+    );
+
     const modeledMethodChangedHandlers = useMemo(
       () =>
         modeledMethods.map((_, index) => (modeledMethod: ModeledMethod) => {
@@ -163,7 +170,9 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
         ref={ref}
         focused={revealedMethodSignature === method.signature}
       >
-        <DataGridCell gridRow={`span ${modeledMethods.length}`}>
+        <DataGridCell
+          gridRow={`span ${modeledMethods.length + validationErrors.length}`}
+        >
           <ApiOrMethodRow>
             <ModelingStatusIndicator status={modelingStatus} />
             <MethodClassifications method={method} />
@@ -200,61 +209,69 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
             )}
           </>
         )}
-        {!props.modelingInProgress &&
-          modeledMethods.map((modeledMethod, index) => (
-            <Fragment key={index}>
-              <DataGridCell>
-                <ModelTypeDropdown
-                  method={method}
-                  modeledMethod={modeledMethod}
-                  onChange={modeledMethodChangedHandlers[index]}
-                />
-              </DataGridCell>
-              <DataGridCell>
-                <ModelInputDropdown
-                  method={method}
-                  modeledMethod={modeledMethod}
-                  onChange={modeledMethodChangedHandlers[index]}
-                />
-              </DataGridCell>
-              <DataGridCell>
-                <ModelOutputDropdown
-                  method={method}
-                  modeledMethod={modeledMethod}
-                  onChange={modeledMethodChangedHandlers[index]}
-                />
-              </DataGridCell>
-              <DataGridCell>
-                <ModelKindDropdown
-                  method={method}
-                  modeledMethod={modeledMethod}
-                  onChange={modeledMethodChangedHandlers[index]}
-                />
-              </DataGridCell>
-              {viewState.showMultipleModels && (
+        {!props.modelingInProgress && (
+          <>
+            {modeledMethods.map((modeledMethod, index) => (
+              <Fragment key={index}>
+                <DataGridCell>
+                  <ModelTypeDropdown
+                    method={method}
+                    modeledMethod={modeledMethod}
+                    onChange={modeledMethodChangedHandlers[index]}
+                  />
+                </DataGridCell>
+                <DataGridCell>
+                  <ModelInputDropdown
+                    method={method}
+                    modeledMethod={modeledMethod}
+                    onChange={modeledMethodChangedHandlers[index]}
+                  />
+                </DataGridCell>
                 <DataGridCell>
-                  {index === modeledMethods.length - 1 ? (
-                    <CodiconRow
-                      appearance="icon"
-                      aria-label="Add new model"
-                      onClick={handleAddModelClick}
-                      disabled={addModelButtonDisabled}
-                    >
-                      <Codicon name="add" />
-                    </CodiconRow>
-                  ) : (
-                    <CodiconRow
-                      appearance="icon"
-                      aria-label="Remove model"
-                      onClick={removeModelClickedHandlers[index]}
-                    >
-                      <Codicon name="trash" />
-                    </CodiconRow>
-                  )}
+                  <ModelOutputDropdown
+                    method={method}
+                    modeledMethod={modeledMethod}
+                    onChange={modeledMethodChangedHandlers[index]}
+                  />
                 </DataGridCell>
-              )}
-            </Fragment>
-          ))}
+                <DataGridCell>
+                  <ModelKindDropdown
+                    method={method}
+                    modeledMethod={modeledMethod}
+                    onChange={modeledMethodChangedHandlers[index]}
+                  />
+                </DataGridCell>
+                {viewState.showMultipleModels && (
+                  <DataGridCell>
+                    {index === modeledMethods.length - 1 ? (
+                      <CodiconRow
+                        appearance="icon"
+                        aria-label="Add new model"
+                        onClick={handleAddModelClick}
+                        disabled={addModelButtonDisabled}
+                      >
+                        <Codicon name="add" />
+                      </CodiconRow>
+                    ) : (
+                      <CodiconRow
+                        appearance="icon"
+                        aria-label="Remove model"
+                        onClick={removeModelClickedHandlers[index]}
+                      >
+                        <Codicon name="trash" />
+                      </CodiconRow>
+                    )}
+                  </DataGridCell>
+                )}
+              </Fragment>
+            ))}
+            {validationErrors.map((error, index) => (
+              <DataGridCell gridColumn="span 5" key={index}>
+                <ModeledMethodAlert error={error} />
+              </DataGridCell>
+            ))}
+          </>
+        )}
       </DataGridRow>
     );
   },

extensions/ql-vscode/src/view/model-editor/__tests__/MethodRow.spec.tsx

@@ -438,4 +438,62 @@ describe(MethodRow.name, () => {
       { ...modeledMethod, type: "summary" },
     ]);
   });
+
+  it("does not display validation errors when everything is valid", () => {
+    render({
+      modeledMethods: [
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "sink" },
+      ],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    expect(screen.queryByRole("alert")).not.toBeInTheDocument();
+  });
+
+  it("displays a single validation error", () => {
+    render({
+      modeledMethods: [
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "source" },
+      ],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    expect(screen.getByRole("alert")).toBeInTheDocument();
+    expect(
+      screen.getByText("Error: Duplicated classification"),
+    ).toBeInTheDocument();
+    expect(
+      screen.queryByText("Error: Conflicting classification"),
+    ).not.toBeInTheDocument();
+  });
+
+  it("displays multiple validation errors", () => {
+    render({
+      modeledMethods: [
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "neutral", kind: "source" },
+      ],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    expect(screen.getAllByRole("alert").length).toBe(2);
+    expect(
+      screen.getByText("Error: Duplicated classification"),
+    ).toBeInTheDocument();
+    expect(
+      screen.getByText("Error: Conflicting classification"),
+    ).toBeInTheDocument();
+  });
 });