Merge pull request #2964 from github/robertbrignull/enable-add-multiple-models

Enable/disable the add/remove model buttons at the right times

Author: robertbrignull

extensions/ql-vscode/src/model-editor/shared/multiple-modeled-methods.ts

@@ -0,0 +1,20 @@
+import { ModeledMethod } from "../modeled-method";
+
+export function canAddNewModeledMethod(
+  modeledMethods: ModeledMethod[],
+): boolean {
+  // Disallow adding methods when there are no modeled methods or where there is a single unmodeled method.
+  // In both of these cases the UI will already be showing the user inputs they can use for modeling.
+  return (
+    modeledMethods.length > 1 ||
+    (modeledMethods.length === 1 && modeledMethods[0].type !== "none")
+  );
+}
+
+export function canRemoveModeledMethod(
+  modeledMethods: ModeledMethod[],
+): boolean {
+  // Don't allow removing the last modeled method. In this case the user is intended to
+  // set the type to "none" instead.
+  return modeledMethods.length > 1;
+}

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

@@ -2,6 +2,10 @@ import * as React from "react";
 import { useCallback, useMemo, useState } from "react";
 import { Method } from "../../model-editor/method";
 import { ModeledMethod } from "../../model-editor/modeled-method";
+import {
+  canAddNewModeledMethod,
+  canRemoveModeledMethod,
+} from "../../model-editor/shared/multiple-modeled-methods";
 import { styled } from "styled-components";
 import { MethodModelingInputs } from "./MethodModelingInputs";
 import { VSCodeButton } from "@vscode/webview-ui-toolkit/react";
@@ -171,18 +175,15 @@ export const MultipleModeledMethodsPanel = ({
             appearance="icon"
             aria-label="Delete modeling"
             onClick={handleRemoveClick}
-            disabled={modeledMethods.length < 2}
+            disabled={!canRemoveModeledMethod(modeledMethods)}
           >
             <Codicon name="trash" />
           </VSCodeButton>
           <VSCodeButton
             appearance="icon"
             aria-label="Add modeling"
             onClick={handleAddClick}
-            disabled={
-              modeledMethods.length === 0 ||
-              (modeledMethods.length === 1 && modeledMethods[0].type === "none")
-            }
+            disabled={!canAddNewModeledMethod(modeledMethods)}
           >
             <Codicon name="add" />
           </VSCodeButton>

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

@@ -24,6 +24,7 @@ import { ModelInputDropdown } from "./ModelInputDropdown";
 import { ModelOutputDropdown } from "./ModelOutputDropdown";
 import { ModelEditorViewState } from "../../model-editor/shared/view-state";
 import { Codicon } from "../common";
+import { canAddNewModeledMethod } from "../../model-editor/shared/multiple-modeled-methods";
 
 const MultiModelColumn = styled(VSCodeDataGridCell)`
   display: flex;
@@ -132,6 +133,8 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
 
     const modelingStatus = getModelingStatus(modeledMethods, methodIsUnsaved);
 
+    const addModelButtonDisabled = !canAddNewModeledMethod(modeledMethods);
+
     return (
       <DataGridRow
         data-testid="modelable-method-row"
@@ -219,15 +222,26 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
             </MultiModelColumn>
             {viewState.showMultipleModels && (
               <MultiModelColumn gridColumn={6}>
-                {modeledMethods.map((_, index) => (
-                  <CodiconRow key={index} appearance="icon" disabled={false}>
-                    {index === modeledMethods.length - 1 ? (
-                      <Codicon name="add" label="Add new model" />
-                    ) : (
-                      <Codicon name="trash" label="Remove model" />
-                    )}
-                  </CodiconRow>
-                ))}
+                {modeledMethods.map((_, index) =>
+                  index === modeledMethods.length - 1 ? (
+                    <CodiconRow
+                      key={index}
+                      appearance="icon"
+                      aria-label="Add new model"
+                      disabled={addModelButtonDisabled}
+                    >
+                      <Codicon name="add" />
+                    </CodiconRow>
+                  ) : (
+                    <CodiconRow
+                      key={index}
+                      appearance="icon"
+                      aria-label="Remove model"
+                    >
+                      <Codicon name="trash" />
+                    </CodiconRow>
+                  ),
+                )}
               </MultiModelColumn>
             )}
           </>

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

@@ -251,4 +251,111 @@ describe(MethodRow.name, () => {
     expect(screen.queryByRole("combobox")).not.toBeInTheDocument();
     expect(screen.getByText("Method already modeled")).toBeInTheDocument();
   });
+
+  it("doesn't show add/remove buttons when multiple methods feature flag is disabled", async () => {
+    render({
+      modeledMethods: [modeledMethod],
+      viewState: {
+        ...viewState,
+        showMultipleModels: false,
+      },
+    });
+
+    expect(screen.queryByLabelText("Add new model")).not.toBeInTheDocument();
+    expect(screen.queryByLabelText("Remove model")).not.toBeInTheDocument();
+  });
+
+  it("shows disabled button add new model when there are no modeled methods", async () => {
+    render({
+      modeledMethods: [],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    const addButton = screen.queryByLabelText("Add new model");
+    expect(addButton).toBeInTheDocument();
+    expect(addButton?.getElementsByTagName("input")[0]).toBeDisabled();
+
+    expect(screen.queryByLabelText("Remove model")).not.toBeInTheDocument();
+  });
+
+  it("disabled button to add new model when there is one unmodeled method", async () => {
+    render({
+      modeledMethods: [{ ...modeledMethod, type: "none" }],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    const addButton = screen.queryByLabelText("Add new model");
+    expect(addButton).toBeInTheDocument();
+    expect(addButton?.getElementsByTagName("input")[0]).toBeDisabled();
+
+    expect(screen.queryByLabelText("Remove model")).not.toBeInTheDocument();
+  });
+
+  it("enabled button to add new model when there is one modeled method", async () => {
+    render({
+      modeledMethods: [modeledMethod],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    const addButton = screen.queryByLabelText("Add new model");
+    expect(addButton).toBeInTheDocument();
+    expect(addButton?.getElementsByTagName("input")[0]).toBeEnabled();
+
+    expect(screen.queryByLabelText("Remove model")).not.toBeInTheDocument();
+  });
+
+  it("enabled add/remove model buttons when there are multiple modeled methods", async () => {
+    render({
+      modeledMethods: [
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "none" },
+      ],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    const addButton = screen.queryByLabelText("Add new model");
+    expect(addButton).toBeInTheDocument();
+    expect(addButton?.getElementsByTagName("input")[0]).toBeEnabled();
+
+    const removeButton = screen.queryByLabelText("Remove model");
+    expect(removeButton).toBeInTheDocument();
+    expect(removeButton?.getElementsByTagName("input")[0]).toBeEnabled();
+  });
+
+  it("shows add model button on last row and remove model button on all other rows", async () => {
+    render({
+      modeledMethods: [
+        { ...modeledMethod, type: "source" },
+        { ...modeledMethod, type: "sink" },
+        { ...modeledMethod, type: "summary" },
+        { ...modeledMethod, type: "none" },
+      ],
+      viewState: {
+        ...viewState,
+        showMultipleModels: true,
+      },
+    });
+
+    const addButtons = screen.queryAllByLabelText("Add new model");
+    expect(addButtons.length).toBe(1);
+    expect(addButtons[0]?.getElementsByTagName("input")[0]).toBeEnabled();
+
+    const removeButtons = screen.queryAllByLabelText("Remove model");
+    expect(removeButtons.length).toBe(3);
+    for (const removeButton of removeButtons) {
+      expect(removeButton?.getElementsByTagName("input")[0]).toBeEnabled();
+    }
+  });
 });