From db1288aec35aef7426710debcf8318b0b21ca313 Mon Sep 17 00:00:00 2001 From: Alexander Streed Date: Sun, 24 Nov 2024 14:35:28 -0600 Subject: [PATCH] [UI v2] Edit variables (#16035) --- ui-v2/src/components/ui/button/button.tsx | 21 ++- .../components/variables/data-table/cells.tsx | 14 +- .../variables/data-table/data-table.tsx | 15 +- ui-v2/src/components/variables/hooks.ts | 86 ++++++++++ ui-v2/src/components/variables/page.tsx | 34 +++- ...ariable-dialog.tsx => variable-dialog.tsx} | 122 ++++++------- ui-v2/tests/mocks/handlers.ts | 4 + ui-v2/tests/variables/variables.test.tsx | 161 +++++++++++++++++- 8 files changed, 385 insertions(+), 72 deletions(-) create mode 100644 ui-v2/src/components/variables/hooks.ts rename ui-v2/src/components/variables/{add-variable-dialog.tsx => variable-dialog.tsx} (58%) diff --git a/ui-v2/src/components/ui/button/button.tsx b/ui-v2/src/components/ui/button/button.tsx index b7842f2b9d4e..76ae8a7d8b05 100644 --- a/ui-v2/src/components/ui/button/button.tsx +++ b/ui-v2/src/components/ui/button/button.tsx @@ -4,22 +4,39 @@ import * as React from "react"; import { cn } from "@/lib/utils"; import { buttonVariants } from "./styles"; +import { Loader2 } from "lucide-react"; export interface ButtonProps extends React.ButtonHTMLAttributes, VariantProps { asChild?: boolean; + loading?: boolean; } const Button = React.forwardRef( - ({ className, variant, size, asChild = false, ...props }, ref) => { + ( + { + className, + variant, + size, + asChild = false, + loading = false, + children, + ...props + }, + ref, + ) => { const Comp = asChild ? Slot : "button"; + return ( + > + {loading ? : children} + ); }, ); diff --git a/ui-v2/src/components/variables/data-table/cells.tsx b/ui-v2/src/components/variables/data-table/cells.tsx index 937185a86ce4..d25ecf5fa065 100644 --- a/ui-v2/src/components/variables/data-table/cells.tsx +++ b/ui-v2/src/components/variables/data-table/cells.tsx @@ -13,9 +13,14 @@ import type { CellContext } from "@tanstack/react-table"; import type { components } from "@/api/prefect"; import { useToast } from "@/hooks/use-toast"; -export const ActionsCell = ({ - row, -}: CellContext) => { +type ActionsCellProps = CellContext< + components["schemas"]["Variable"], + unknown +> & { + onVariableEdit: (variable: components["schemas"]["Variable"]) => void; +}; + +export const ActionsCell = ({ row, onVariableEdit }: ActionsCellProps) => { const id = row.original.id; const queryClient = useQueryClient(); const { mutate: deleteVariable } = useMutation({ @@ -81,6 +86,9 @@ export const ActionsCell = ({ > Copy Value + onVariableEdit(row.original)}> + Edit + Delete diff --git a/ui-v2/src/components/variables/data-table/data-table.tsx b/ui-v2/src/components/variables/data-table/data-table.tsx index a0780f93d945..fe522e959c8b 100644 --- a/ui-v2/src/components/variables/data-table/data-table.tsx +++ b/ui-v2/src/components/variables/data-table/data-table.tsx @@ -10,7 +10,7 @@ import { import { DataTable } from "@/components/ui/data-table"; import { Badge } from "@/components/ui/badge"; import { ActionsCell } from "./cells"; -import { useCallback } from "react"; +import { useCallback, useMemo } from "react"; import { SearchInput } from "@/components/ui/input"; import { TagsInput } from "@/components/ui/tags-input"; import { @@ -24,7 +24,9 @@ import type React from "react"; const columnHelper = createColumnHelper(); -const columns = [ +const createColumns = ( + onVariableEdit: (variable: components["schemas"]["Variable"]) => void, +) => [ columnHelper.accessor("name", { header: "Name", }), @@ -74,7 +76,7 @@ const columns = [ }), columnHelper.display({ id: "actions", - cell: ActionsCell, + cell: (props) => , }), ]; @@ -87,6 +89,7 @@ type VariablesDataTableProps = { onColumnFiltersChange: OnChangeFn; sorting: components["schemas"]["VariableSort"]; onSortingChange: (sortKey: components["schemas"]["VariableSort"]) => void; + onVariableEdit: (variable: components["schemas"]["Variable"]) => void; }; export const VariablesDataTable = ({ @@ -98,7 +101,13 @@ export const VariablesDataTable = ({ onColumnFiltersChange, sorting, onSortingChange, + onVariableEdit, }: VariablesDataTableProps) => { + const columns = useMemo( + () => createColumns(onVariableEdit), + [onVariableEdit], + ); + const nameSearchValue = columnFilters.find((filter) => filter.id === "name") ?.value as string; const tagsSearchValue = columnFilters.find((filter) => filter.id === "tags") diff --git a/ui-v2/src/components/variables/hooks.ts b/ui-v2/src/components/variables/hooks.ts new file mode 100644 index 000000000000..bc2f72b263ef --- /dev/null +++ b/ui-v2/src/components/variables/hooks.ts @@ -0,0 +1,86 @@ +import type { components } from "@/api/prefect"; +import { getQueryService } from "@/api/service"; +import { useToast } from "@/hooks/use-toast"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; + +type UseCreateVariableProps = { + onSuccess: () => void; + onError: (error: Error) => void; +}; + +export const useCreateVariable = ({ + onSuccess, + onError, +}: UseCreateVariableProps) => { + const queryClient = useQueryClient(); + const { toast } = useToast(); + + return useMutation({ + mutationFn: (variable: components["schemas"]["VariableCreate"]) => { + return getQueryService().POST("/variables/", { + body: variable, + }); + }, + onSettled: async () => { + return await Promise.all([ + queryClient.invalidateQueries({ + predicate: (query) => query.queryKey[0] === "variables", + }), + queryClient.invalidateQueries({ + predicate: (query) => query.queryKey[0] === "total-variable-count", + }), + ]); + }, + onSuccess: () => { + toast({ + title: "Variable created", + }); + onSuccess(); + }, + onError, + }); +}; + +type UseUpdateVariableProps = { + onSuccess: () => void; + onError: (error: Error) => void; +}; + +type VariableUpdateWithId = components["schemas"]["VariableUpdate"] & { + id: string; +}; + +export const useUpdateVariable = ({ + onSuccess, + onError, +}: UseUpdateVariableProps) => { + const queryClient = useQueryClient(); + const { toast } = useToast(); + + return useMutation({ + mutationFn: (variable: VariableUpdateWithId) => { + const { id, ...body } = variable; + return getQueryService().PATCH("/variables/{id}", { + params: { path: { id } }, + body, + }); + }, + onSettled: async () => { + return await Promise.all([ + queryClient.invalidateQueries({ + predicate: (query) => query.queryKey[0] === "variables", + }), + queryClient.invalidateQueries({ + predicate: (query) => query.queryKey[0] === "total-variable-count", + }), + ]); + }, + onSuccess: () => { + toast({ + title: "Variable updated", + }); + onSuccess(); + }, + onError, + }); +}; diff --git a/ui-v2/src/components/variables/page.tsx b/ui-v2/src/components/variables/page.tsx index cdd2cb25cb24..abdf0d9320a0 100644 --- a/ui-v2/src/components/variables/page.tsx +++ b/ui-v2/src/components/variables/page.tsx @@ -5,10 +5,13 @@ import { BreadcrumbList, } from "@/components/ui/breadcrumb"; import { Button } from "@/components/ui/button"; -import { AddVariableDialog } from "@/components/variables/add-variable-dialog"; +import { + VariableDialog, + type VariableDialogProps, +} from "@/components/variables/variable-dialog"; import { VariablesEmptyState } from "@/components/variables/empty-state"; import { PlusIcon } from "lucide-react"; -import { useState } from "react"; +import { useCallback, useState } from "react"; import { VariablesDataTable } from "./data-table"; import type { ColumnFiltersState, @@ -40,15 +43,33 @@ export const VariablesPage = ({ onSortingChange, }: VariablesPageProps) => { const [addVariableDialogOpen, setAddVariableDialogOpen] = useState(false); - const onAddVariableClick = () => { + const [variableToEdit, setVariableToEdit] = useState< + VariableDialogProps["existingVariable"] | undefined + >(undefined); + + const onAddVariableClick = useCallback(() => { + setVariableToEdit(undefined); setAddVariableDialogOpen(true); - }; + }, []); + + const handleVariableEdit = useCallback( + (variable: components["schemas"]["Variable"]) => { + setVariableToEdit(variable); + setAddVariableDialogOpen(true); + }, + [], + ); + + const handleVariableDialogOpenChange = useCallback((open: boolean) => { + setAddVariableDialogOpen(open); + }, []); return ( <> -
@@ -80,6 +101,7 @@ export const VariablesPage = ({ onColumnFiltersChange={onColumnFiltersChange} sorting={sorting} onSortingChange={onSortingChange} + onVariableEdit={handleVariableEdit} /> )}
diff --git a/ui-v2/src/components/variables/add-variable-dialog.tsx b/ui-v2/src/components/variables/variable-dialog.tsx similarity index 58% rename from ui-v2/src/components/variables/add-variable-dialog.tsx rename to ui-v2/src/components/variables/variable-dialog.tsx index 2abccd2226e6..922cc94d29a9 100644 --- a/ui-v2/src/components/variables/add-variable-dialog.tsx +++ b/ui-v2/src/components/variables/variable-dialog.tsx @@ -9,7 +9,6 @@ import { DialogTrigger, } from "@/components/ui/dialog"; import { zodResolver } from "@hookform/resolvers/zod"; -import { useMutation } from "@tanstack/react-query"; import { useForm } from "react-hook-form"; import { z } from "zod"; import { @@ -21,14 +20,12 @@ import { FormMessage, } from "../ui/form"; import { Input } from "../ui/input"; -import { getQueryService } from "@/api/service"; import type { components } from "@/api/prefect"; import type { JSONValue } from "@/lib/types"; -import { Loader2 } from "lucide-react"; import { TagsInput } from "../ui/tags-input"; -import { useToast } from "@/hooks/use-toast"; -import { queryClient } from "@/router"; import { JsonInput } from "@/components/ui/json-input"; +import { useEffect, useMemo } from "react"; +import { useCreateVariable, useUpdateVariable } from "./hooks"; const formSchema = z.object({ name: z.string().min(2, { message: "Name must be at least 2 characters" }), @@ -40,51 +37,61 @@ const formSchema = z.object({ .optional(), }); -type AddVariableDialogProps = { +export type VariableDialogProps = { onOpenChange: (open: boolean) => void; open: boolean; + existingVariable?: components["schemas"]["Variable"]; }; -export const AddVariableDialog = ({ +const VARIABLE_FORM_DEFAULT_VALUES = { + name: "", + value: "", + tags: [], +}; + +export const VariableDialog = ({ onOpenChange, open, -}: AddVariableDialogProps) => { - const defaultValues = { - name: "", - value: "", - tags: [], - }; + existingVariable, +}: VariableDialogProps) => { const form = useForm>({ resolver: zodResolver(formSchema), - defaultValues, + defaultValues: VARIABLE_FORM_DEFAULT_VALUES, }); - const { toast } = useToast(); + const initialValues = useMemo(() => { + if (!existingVariable) return undefined; + return { + name: existingVariable.name, + value: JSON.stringify(existingVariable.value), + tags: existingVariable.tags, + }; + }, [existingVariable]); - const queryService = getQueryService(); - const { mutate: createVariable, isPending } = useMutation({ - mutationFn: (variable: components["schemas"]["VariableCreate"]) => { - return queryService.POST("/variables/", { - body: variable, - }); + useEffect(() => { + // Ensure we start with the initial values when the dialog opens + if (open) { + form.reset(initialValues ?? VARIABLE_FORM_DEFAULT_VALUES); + } + }, [initialValues, form, open]); + + const { mutate: createVariable, isPending: isCreating } = useCreateVariable({ + onSuccess: () => { + onOpenChange(false); }, - onSettled: async () => { - return await Promise.all([ - queryClient.invalidateQueries({ - predicate: (query) => query.queryKey[0] === "variables", - }), - queryClient.invalidateQueries({ - queryKey: ["total-variable-count"], - }), - ]); + onError: (error) => { + const message = error.message || "Unknown error while creating variable."; + form.setError("root", { + message, + }); }, + }); + + const { mutate: updateVariable, isPending: isUpdating } = useUpdateVariable({ onSuccess: () => { - toast({ - title: "Variable created", - }); - onClose(); + onOpenChange(false); }, onError: (error) => { - const message = error.message || "Unknown error while creating variable."; + const message = error.message || "Unknown error while updating variable."; form.setError("root", { message, }); @@ -94,31 +101,36 @@ export const AddVariableDialog = ({ const onSubmit = (values: z.infer) => { try { const value = JSON.parse(values.value) as JSONValue; - createVariable({ - name: values.name, - value, - tags: values.tags, - }); + if (existingVariable?.id) { + updateVariable({ + id: existingVariable.id, + name: values.name, + value, + tags: values.tags, + }); + } else { + createVariable({ + name: values.name, + value, + tags: values.tags, + }); + } } catch { form.setError("value", { message: "Value must be valid JSON" }); } }; - - const onClose = () => { - form.reset(); - onOpenChange(false); - }; + const dialogTitle = existingVariable ? "Edit Variable" : "New Variable"; + const dialogDescription = existingVariable + ? "Edit the variable by changing the name, value, or tags." + : "Add a new variable by providing a name, value, and optional tags. Values can be any valid JSON value."; return ( - + - New Variable + {dialogTitle} - - Add a new variable by providing a name, value, and optional tags. - Values can be any valid JSON value. - + {dialogDescription}
void form.handleSubmit(onSubmit)(e)} @@ -168,12 +180,8 @@ export const AddVariableDialog = ({ -
diff --git a/ui-v2/tests/mocks/handlers.ts b/ui-v2/tests/mocks/handlers.ts index 09f8cf184e26..77895e554cfc 100644 --- a/ui-v2/tests/mocks/handlers.ts +++ b/ui-v2/tests/mocks/handlers.ts @@ -12,6 +12,10 @@ const variablesHandlers = [ http.post("http://localhost:4200/api/variables/count", () => { return HttpResponse.json(0); }), + + http.patch("http://localhost:4200/api/variables/:id", () => { + return new HttpResponse(null, { status: 204 }); + }), ]; export const handlers = [ diff --git a/ui-v2/tests/variables/variables.test.tsx b/ui-v2/tests/variables/variables.test.tsx index e70eb88a9ff4..23b2f83d2001 100644 --- a/ui-v2/tests/variables/variables.test.tsx +++ b/ui-v2/tests/variables/variables.test.tsx @@ -1,5 +1,11 @@ import "./mocks"; -import { render, screen } from "@testing-library/react"; +import { + getByLabelText, + getByTestId, + getByText, + render, + screen, +} from "@testing-library/react"; import { VariablesPage } from "@/components/variables/page"; import userEvent from "@testing-library/user-event"; import { @@ -247,6 +253,159 @@ describe("Variables page", () => { }); }); + describe("Edit variable dialog", () => { + it("should allow editing a variable", async () => { + const user = userEvent.setup(); + const queryClient = new QueryClient(); + + const variables = [ + { + id: "1", + name: "my-variable", + value: 123, + created: "2021-01-01T00:00:00Z", + updated: "2021-01-01T00:00:00Z", + tags: ["tag1"], + }, + ]; + + render( + + + + , + ); + + await user.click(screen.getByRole("button", { expanded: false })); + await user.click(screen.getByText("Edit")); + expect(screen.getByText("Edit Variable")).toBeVisible(); + + const dialog = screen.getByRole("dialog"); + expect(getByLabelText(dialog, "Name")).toHaveValue("my-variable"); + expect(getByTestId(dialog, "mock-json-input")).toHaveValue("123"); + expect(getByText(dialog, "tag1")).toBeVisible(); + + await user.type(getByLabelText(dialog, "Name"), "new_name"); + await user.click(screen.getByRole("button", { name: "Save" })); + expect(screen.getByText("Variable updated")).toBeVisible(); + }); + + it("should show an error when API call fails with detail", async () => { + server.use( + http.patch("http://localhost:4200/api/variables/:id", () => { + return HttpResponse.json( + { detail: "Failed to update variable. Here's some detail..." }, + { status: 500 }, + ); + }), + ); + const user = userEvent.setup(); + const queryClient = new QueryClient(); + + const variables = [ + { + id: "1", + name: "my-variable", + value: 123, + created: "2021-01-01T00:00:00Z", + updated: "2021-01-01T00:00:00Z", + tags: ["tag1"], + }, + ]; + + render( + + + + , + ); + + await user.click(screen.getByRole("button", { expanded: false })); + await user.click(screen.getByText("Edit")); + expect(screen.getByText("Edit Variable")).toBeVisible(); + + const dialog = screen.getByRole("dialog"); + expect(getByLabelText(dialog, "Name")).toHaveValue("my-variable"); + + await user.type(getByLabelText(dialog, "Name"), "new_name"); + await user.click(screen.getByRole("button", { name: "Save" })); + expect( + screen.getByText("Failed to update variable. Here's some detail..."), + ).toBeVisible(); + }); + }); + + it("should show an error when API call fails without detail", async () => { + server.use( + http.patch("http://localhost:4200/api/variables/:id", () => { + return HttpResponse.json( + { error: "Internal server error" }, + { status: 500 }, + ); + }), + ); + const user = userEvent.setup(); + const queryClient = new QueryClient(); + + const variables = [ + { + id: "1", + name: "my-variable", + value: 123, + created: "2021-01-01T00:00:00Z", + updated: "2021-01-01T00:00:00Z", + tags: ["tag1"], + }, + ]; + + render( + + + + , + ); + + await user.click(screen.getByRole("button", { expanded: false })); + await user.click(screen.getByText("Edit")); + + const dialog = screen.getByRole("dialog"); + expect(getByLabelText(dialog, "Name")).toHaveValue("my-variable"); + + await user.type(getByLabelText(dialog, "Name"), "new_name"); + await user.click(screen.getByRole("button", { name: "Save" })); + expect(screen.getByText("Unknown error", { exact: false })).toBeVisible(); + }); + describe("Variables table", () => { beforeAll(() => { // Need to mock PointerEvent for the selects to work