Skip to content

Show validation results in the model editor #2997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
robertbrignull merged 5 commits into from
Oct 19, 2023
Merged

Show validation results in the model editor #2997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1606829
Make setSelectedIndex optional
robertbrignull Oct 18, 2023
c69a310
Add <> around inputs
robertbrignull Oct 19, 2023
0af77d0
Display validation errors in model editor
robertbrignull Oct 19, 2023
553435d
Add stories for validation errors in MethodRow
robertbrignull Oct 19, 2023
31d654d
Add tests of showing validation errors
robertbrignull Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions extensions/ql-vscode/src/stories/model-editor/MethodRow.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
10 changes: 7 additions & 3 deletions extensions/ql-vscode/src/view/method-modeling/ModeledMethodAlert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ import { useCallback } from "react";

type Props = {
error: ModeledMethodValidationError;
setSelectedIndex: (index: number) => void;
setSelectedIndex?: (index: number) => void;
Copy link
Copy Markdown
Member

@koesie10 koesie10 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we were able to highlight the model on which the error is located, but this can still be added in a follow-up PR if this is something that comes out of a design review.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a separate issue for this so we don't forget it.

};

export const ModeledMethodAlert = ({ error, setSelectedIndex }: Props) => {
const handleClick = useCallback(() => {
setSelectedIndex(error.index);
setSelectedIndex?.(error.index);
}, [error.index, setSelectedIndex]);

return (
Expand All @@ -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
)}
</>
}
/>
Expand Down
125 changes: 71 additions & 54 deletions extensions/ql-vscode/src/view/model-editor/MethodRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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} />
Expand Down Expand Up @@ -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>
);
},
Expand Down
58 changes: 58 additions & 0 deletions extensions/ql-vscode/src/view/model-editor/__tests__/MethodRow.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});