Skip to content
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.

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

#3831 Add validation support for sub pipelines #3832

Merged
merged 9 commits into from Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/blocks/renderers/documentView/DocumentView.tsx
Expand Up @@ -21,7 +21,7 @@ import ReactShadowRoot from "react-shadow-root";
import BootstrapStylesheet from "@/blocks/renderers/BootstrapStylesheet";
import { DocumentViewProps } from "./DocumentViewProps";
import DocumentContext from "@/components/documentBuilder/render/DocumentContext";
import { joinElementName } from "@/components/documentBuilder/utils";
import { joinPathParts } from "@/utils";

const DocumentView: React.FC<DocumentViewProps> = ({ body, options, meta }) => {
// Track style loading to avoid a FOUC
Expand Down Expand Up @@ -52,7 +52,7 @@ const DocumentView: React.FC<DocumentViewProps> = ({ body, options, meta }) => {
body.map((documentElement, index) => {
const { Component, props } = buildDocumentBranch(
documentElement,
{ staticId: joinElementName("body", "children"), branches: [] }
{ staticId: joinPathParts("body", "children"), branches: [] }
);
return <Component key={index} {...props} />;
})}
Expand Down
5 changes: 4 additions & 1 deletion src/blocks/renderers/markdown.ts
Expand Up @@ -19,11 +19,14 @@ import { Renderer } from "@/types";
import { propertiesToSchema } from "@/validators/generic";
import { BlockArg, SafeHTML } from "@/core";
import safeMarkdown from "@/utils/safeMarkdown";
import { validateRegistryId } from "@/types/helpers";

export class MarkdownRenderer extends Renderer {
static BLOCK_ID = validateRegistryId("@pixiebrix/markdown");

constructor() {
super(
"@pixiebrix/markdown",
MarkdownRenderer.BLOCK_ID,
"Render Markdown",
"Render Markdown to sanitized HTML"
);
Expand Down
4 changes: 2 additions & 2 deletions src/components/documentBuilder/documentTree.tsx
Expand Up @@ -30,7 +30,7 @@ import {
import ButtonElement from "@/components/documentBuilder/render/ButtonElement";
import ListElement from "@/components/documentBuilder/render/ListElement";
import { BusinessError } from "@/errors/businessErrors";
import { joinElementName } from "@/components/documentBuilder/utils";
import { joinPathParts } from "@/utils";

const headerComponents = {
header_1: "h1",
Expand Down Expand Up @@ -181,7 +181,7 @@ export const buildDocumentBranch: BuildDocumentBranch = (root, tracePath) => {
if (root.children?.length > 0) {
componentDefinition.props.children = root.children.map((child, index) => {
const { Component, props } = buildDocumentBranch(child, {
staticId: joinElementName(staticId, root.type, "children"),
staticId: joinPathParts(staticId, root.type, "children"),
branches: [...branches, { staticId, index }],
});
return <Component key={index} {...props} />;
Expand Down
6 changes: 3 additions & 3 deletions src/components/documentBuilder/hooks/useMoveWithinParent.ts
Expand Up @@ -21,7 +21,7 @@ import { actions } from "@/pageEditor/slices/editorSlice";
import { useField } from "formik";
import { DocumentElement } from "@/components/documentBuilder/documentBuilderTypes";
import getElementCollectionName from "@/components/documentBuilder/edit/getElementCollectionName";
import { joinElementName } from "@/components/documentBuilder/utils";
import { joinPathParts } from "@/utils";

type MoveWithinParent = {
canMoveUp: boolean;
Expand All @@ -38,7 +38,7 @@ function useMoveWithinParent(documentBodyName: string): MoveWithinParent {
const { collectionName, elementIndex } =
getElementCollectionName(activeElement);

const fullCollectionName = joinElementName(documentBodyName, collectionName);
const fullCollectionName = joinPathParts(documentBodyName, collectionName);

const [{ value: elementsCollection }, , { setValue: setElementsCollection }] =
useField<DocumentElement[]>(fullCollectionName);
Expand All @@ -58,7 +58,7 @@ function useMoveWithinParent(documentBodyName: string): MoveWithinParent {
/* eslint-enable security/detect-object-injection */

setElementsCollection(newElementsCollection);
setActiveElement(joinElementName(collectionName, toIndex));
setActiveElement(joinPathParts(collectionName, toIndex));
};

return {
Expand Down
6 changes: 3 additions & 3 deletions src/components/documentBuilder/outline/outlineHelpers.ts
Expand Up @@ -22,7 +22,7 @@ import {
import { TreeExpandedState } from "@/components/jsonTree/JsonTree";
import { ItemId, TreeData } from "@atlaskit/tree";
import { TreeItem } from "@atlaskit/tree/types";
import { joinElementName } from "@/components/documentBuilder/utils";
import { joinPathParts } from "@/utils";
import { PARENT_ELEMENT_TYPES } from "@/components/documentBuilder/allowedElementTypes";

type ElementArgs = {
Expand All @@ -39,7 +39,7 @@ function getChildren(
return [
{
element: element.config.element.__value__,
elementName: joinElementName(
elementName: joinPathParts(
elementName,
"config",
"element",
Expand All @@ -53,7 +53,7 @@ function getChildren(

return children.map((child, index) => ({
element: child,
elementName: joinElementName(elementName, "children", String(index)),
elementName: joinPathParts(elementName, "children", String(index)),
}));
}

Expand Down
5 changes: 2 additions & 3 deletions src/components/documentBuilder/render/ListElement.tsx
Expand Up @@ -30,10 +30,9 @@ import { produce } from "immer";
import ErrorBoundary from "@/components/ErrorBoundary";
import { getErrorMessage } from "@/errors/errorHelpers";
import { runMapArgs } from "@/contentScript/messenger/api";
import { isNullOrBlank } from "@/utils";
import { isNullOrBlank, joinPathParts } from "@/utils";
import apiVersionOptions from "@/runtime/apiVersionOptions";
import { whoAmI } from "@/background/messenger/api";
import { joinElementName } from "@/components/documentBuilder/utils";

type DocumentListProps = {
array: UnknownObject[];
Expand Down Expand Up @@ -130,7 +129,7 @@ const ListElementInternal: React.FC<DocumentListProps> = ({
const { Component, props } = buildDocumentBranch(
documentElement as DocumentElement,
{
staticId: joinElementName(staticId, "list", "children"),
staticId: joinPathParts(staticId, "list", "children"),
branches: [...branches, { staticId, index }],
}
);
Expand Down
9 changes: 0 additions & 9 deletions src/components/documentBuilder/utils.ts
Expand Up @@ -18,15 +18,6 @@
import { DynamicPath } from "@/components/documentBuilder/documentBuilderTypes";
import { Branch } from "@/blocks/types";

/**
* Join parts of a document builder element name, ignoring null/blank parts.
* @param nameParts the parts of the name
*/
export function joinElementName(...nameParts: Array<string | number>): string {
// Don't use lodash.compact and lodash.isEmpty since they treat 0 as falsy
return nameParts.filter((x) => x != null && x !== "").join(".");
}

export function mapPathToTraceBranches(tracePath: DynamicPath): Branch[] {
return tracePath.branches.map(({ staticId, index }) => ({
key: staticId,
Expand Down
4 changes: 2 additions & 2 deletions src/pageEditor/extensionPoints/pipelineMapping.ts
Expand Up @@ -32,7 +32,7 @@ export function normalizePipelineForEditor(
): BlockPipeline {
return produce(pipeline, (pipeline: WritableDraft<BlockPipeline>) => {
traversePipeline({
blockPipeline: pipeline,
pipeline,
visitBlock({ blockConfig }) {
blockConfig.instanceId = uuidv4();
},
Expand All @@ -55,7 +55,7 @@ export function normalizePipelineForEditor(
export function omitEditorMetadata(pipeline: BlockPipeline): BlockPipeline {
return produce(pipeline, (pipeline: WritableDraft<BlockPipeline>) => {
traversePipeline({
blockPipeline: pipeline,
pipeline,
visitBlock({ blockConfig }) {
delete blockConfig.instanceId;
},
Expand Down
10 changes: 7 additions & 3 deletions src/pageEditor/hooks/usePipelineErrors.ts
Expand Up @@ -30,30 +30,34 @@ import validateStringTemplates from "@/pageEditor/validation/validateStringTempl
import { PIPELINE_BLOCKS_FIELD_NAME } from "@/pageEditor/consts";
import { FormState } from "@/pageEditor/pageEditorTypes";
import useAllBlocks from "./useAllBlocks";
import { selectPipelineMap } from "@/pageEditor/slices/editorSelectors";

function usePipelineErrors() {
const [allBlocks] = useAllBlocks();
const traceErrors = useSelector(selectTraceErrors);
const formikContext = useFormikContext<FormState>();
const extensionPointType = formikContext.values.type;
const pipelineMap = useSelector(selectPipelineMap);

const validatePipelineBlocks = useCallback(
(pipeline: BlockPipeline): void | FormikErrorTree => {
const formikErrors: FormikErrorTree = {};

// TODO move this to the OutputKey field level
validateOutputKey(formikErrors, pipeline, allBlocks);
validateRenderers(formikErrors, pipeline, allBlocks, extensionPointType);
// TODO move this to the TextField level
validateStringTemplates(formikErrors, pipeline);
applyTraceErrors(formikErrors, traceErrors, pipeline);
applyTraceErrors(formikErrors, traceErrors, pipelineMap);

return isEmpty(formikErrors) ? undefined : formikErrors;
},
[allBlocks, extensionPointType, traceErrors]
[allBlocks, extensionPointType, pipelineMap, traceErrors]
);

useField<BlockPipeline>({
name: PIPELINE_BLOCKS_FIELD_NAME,
// @ts-expect-error working with nested errors
// @ts-expect-error -- validatePipelineBlocks can return an object b/c we're working with nested errors
validate: validatePipelineBlocks,
});

Expand Down
62 changes: 49 additions & 13 deletions src/pageEditor/panes/EditorPane.test.tsx
Expand Up @@ -40,6 +40,8 @@ import {
import { PipelineExpression } from "@/runtime/mapArgs";
import { act } from "react-dom/test-utils";
import { OutputKey } from "@/core";
import { fireTextInput, selectSchemaFieldType } from "@/testUtils/formHelpers";
import { PIPELINE_BLOCKS_FIELD_NAME } from "@/pageEditor/consts";

jest.setTimeout(30_000); // This test is flaky with the default timeout of 5000 ms

Expand All @@ -50,25 +52,12 @@ const forEachBlock = new ForEach();
const immediateUserEvent = userEvent.setup({ delay: null });

beforeAll(async () => {
jest.useFakeTimers();

registerDefaultWidgets();
blockRegistry.clear();
blockRegistry.register(echoBlock, teapotBlock, jqBlock, forEachBlock);
await blockRegistry.allTyped();
});

afterAll(() => {
jest.useRealTimers();
});

beforeEach(() => {
jest.clearAllTimers();
});
afterEach(() => {
jest.runOnlyPendingTimers();
});

const getPlainFormState = (): FormState =>
formStateFactory(undefined, [
blockConfigFactory({
Expand Down Expand Up @@ -143,6 +132,20 @@ describe("renders", () => {
});

describe("can add a node", () => {
beforeAll(() => {
jest.useFakeTimers();
});
afterAll(() => {
jest.useRealTimers();
});

beforeEach(() => {
jest.clearAllTimers();
});
afterEach(() => {
jest.runOnlyPendingTimers();
});

async function addABlock(addButton: Element, blockName: string) {
await immediateUserEvent.click(addButton);

Expand Down Expand Up @@ -319,3 +322,36 @@ describe("can remove a node", () => {
expect(nodes[2]).toHaveTextContent(/for-each loop/i);
});
});

describe("validation", () => {
test("validates string templates", async () => {
const formState = getFormStateWithSubPipelines();
const subEchoNode = (
formState.extension.blockPipeline[1].config.body as PipelineExpression
).__value__[0];
const rendered = render(<EditorPane />, {
setupRedux(dispatch) {
dispatch(editorActions.addElement(formState));
dispatch(editorActions.selectElement(formState.uuid));
dispatch(editorActions.setElementActiveNodeId(subEchoNode.instanceId));
},
});

await waitForEffect();

await selectSchemaFieldType(
`${PIPELINE_BLOCKS_FIELD_NAME}.1.config.body.__value__.0.config.message`,
"var"
);

// By some reason, the validation doesn't fire with userEvent.type
fireTextInput(rendered.getByLabelText("message"), "{{!");
await waitForEffect();

expect(
screen.getByText(
"Invalid text template. Read more about text templates: https://docs.pixiebrix.com/nunjucks-templates"
)
).toBeInTheDocument();
});
});
44 changes: 43 additions & 1 deletion src/pageEditor/panes/__snapshots__/EditorPane.test.tsx.snap
Expand Up @@ -1443,7 +1443,49 @@ exports[`renders an extension with sub pipeline 1`] = `
>
<ul
style="padding: 0px; margin: 0px; list-style: none;"
/>
>
<li
style="padding-top: 0.25em; padding-right: 0px; margin-left: 0.875em; word-wrap: break-word; padding-left: 1.25em; text-indent: -0.5em; word-break: break-all; white-space: pre-wrap;"
>
<label
style="display: inline-block; color: rgb(66, 122, 245); margin: 0px 0.5em 0px 0px;"
>
<div>
<span>
@input
</span>
:
<a
aria-label="copy path"
class="copyPath p-0 btn btn-text"
href="#"
role="button"
>
<svg
aria-hidden="true"
class="svg-inline--fa fa-copy fa-w-14 "
data-icon="copy"
data-prefix="fas"
focusable="false"
role="img"
viewBox="0 0 448 512"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M320 448v40c0 13.255-10.745 24-24 24H24c-13.255 0-24-10.745-24-24V120c0-13.255 10.745-24 24-24h72v296c0 30.879 25.121 56 56 56h168zm0-344V0H152c-13.255 0-24 10.745-24 24v368c0 13.255 10.745 24 24 24h272c13.255 0 24-10.745 24-24V128H344c-13.2 0-24-10.8-24-24zm120.971-31.029L375.029 7.029A24 24 0 0 0 358.059 0H352v96h96v-6.059a24 24 0 0 0-7.029-16.97z"
fill="currentColor"
/>
</svg>
</a>
</div>
</label>
<span
style="color: rgb(241, 89, 80);"
>
undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it supposed to print undefined in the UI like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, in the test. this is the @input of the extension reported to the Data Panel. normally it is the HTML document meta data, but in the test there's no document and we get undefined

</span>
</li>
</ul>
</li>
</ul>
</div>
Expand Down
6 changes: 3 additions & 3 deletions src/pageEditor/tabs/editTab/editHelpers.ts
Expand Up @@ -76,11 +76,11 @@ export async function generateFreshOutputKey(
return freshIdentifier(type as SafeString, outputKeys) as OutputKey;
}

export function getPipelineMap(blockPipeline: BlockPipeline) {
export function getPipelineMap(pipeline: BlockPipeline) {
const pipelineMap: PipelineMap = {};
traversePipeline({
blockPipeline,
blockPipelinePath: PIPELINE_BLOCKS_FIELD_NAME,
pipeline,
pipelinePath: PIPELINE_BLOCKS_FIELD_NAME,
visitBlock({
blockConfig,
index,
Expand Down