Skip to content

fix: UX issues in template settings form's default auto-stop field #5330

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

Merged
merged 8 commits into from
Dec 7, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions site/src/i18n/en/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import agent from "./agent.json"
import buildPage from "./buildPage.json"
import workspacesPage from "./workspacesPage.json"
import usersPage from "./usersPage.json"
import templateSettingsPage from "./templateSettingsPage.json"
import templateVersionPage from "./templateVersionPage.json"
import loginPage from "./loginPage.json"
import workspaceChangeVersionPage from "./workspaceChangeVersionPage.json"
Expand All @@ -23,6 +24,7 @@ export const en = {
buildPage,
workspacesPage,
usersPage,
templateSettingsPage,
templateVersionPage,
loginPage,
workspaceChangeVersionPage,
Expand Down
14 changes: 1 addition & 13 deletions site/src/i18n/en/templatePage.json
Original file line number Diff line number Diff line change
@@ -1,16 +1,4 @@
{
"deleteSuccess": "Template successfully deleted.",
"createdVersion": "created the version",
"templateSettings": {
"title": "Template settings",
"dangerZone": {
"dangerZoneHeader": "Danger Zone",
"deleteTemplateHeader": "Delete this template",
"deleteTemplateCaption": "Do you want to permanently delete this template?",
"deleteCta": "Delete Template"
}
},
"displayNameLabel": "Display name",
"allowUserCancelWorkspaceJobsLabel": "Allow users to cancel in-progress workspace jobs.",
"allowUserCancelWorkspaceJobsNotice": "Depending on your template, canceling builds may leave workspaces in an unhealthy state. This option isn't recommended for most use cases."
"createdVersion": "created the version"
}
24 changes: 24 additions & 0 deletions site/src/i18n/en/templateSettingsPage.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"title": "Template settings",
"nameLabel": "Name",
"displayNameLabel": "Display name",
"descriptionLabel": "Description",
"descriptionMaxError": "Please enter a description that is less than or equal to 128 characters.",
"defaultTtlLabel": "Auto-stop default",
"iconLabel": "Icon",
"formAriaLabel": "Template settings form",
"selectEmoji": "Select emoji",
"ttlMaxError": "Please enter a limit that is less than or equal to 168 hours (7 days).",
"ttlMinError": "Default time until auto-stop must not be less than 0.",
"ttlHelperText_zero": "Workspaces created from this template will run until stopped manually.",
"ttlHelperText_one": "Workspaces created from this template will default to stopping after {{count}} hour.",
"ttlHelperText_other": "Workspaces created from this template will default to stopping after {{count}} hours.",
"allowUserCancelWorkspaceJobsLabel": "Allow users to cancel in-progress workspace jobs.",
"allowUserCancelWorkspaceJobsNotice": "Depending on your template, canceling builds may leave workspaces in an unhealthy state. This option isn't recommended for most use cases.",
"dangerZone": {
"dangerZoneHeader": "Danger Zone",
"deleteTemplateHeader": "Delete this template",
"deleteTemplateCaption": "Do you want to permanently delete this template?",
"deleteCta": "Delete Template"
}
}
87 changes: 43 additions & 44 deletions site/src/pages/TemplateSettingsPage/TemplateSettingsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,43 +24,44 @@ import {
import * as Yup from "yup"
import i18next from "i18next"
import { useTranslation } from "react-i18next"
import { Maybe } from "components/Conditionals/Maybe"

export const Language = {
nameLabel: "Name",
descriptionLabel: "Description",
defaultTtlLabel: "Auto-stop default",
iconLabel: "Icon",
formAriaLabel: "Template settings form",
selectEmoji: "Select emoji",
ttlMaxError:
"Please enter a limit that is less than or equal to 168 hours (7 days).",
descriptionMaxError:
"Please enter a description that is less than or equal to 128 characters.",
ttlHelperText: (ttl: number): string =>
`Workspaces created from this template will default to stopping after ${ttl} hours.`,
const TTLHelperText = ({ ttl }: { ttl?: number }) => {
const { t } = useTranslation("templateSettingsPage")
const count = typeof ttl !== "number" ? 0 : ttl
return (
// no helper text if ttl is negative - error will show once field is considered touched
<Maybe condition={count >= 0}>
<span>{t("ttlHelperText", { count })}</span>
</Maybe>
)
}

const MAX_DESCRIPTION_CHAR_LIMIT = 128
const MAX_TTL_DAYS = 7
const MS_HOUR_CONVERSION = 3600000

export const validationSchema = Yup.object({
name: nameValidator(Language.nameLabel),
display_name: templateDisplayNameValidator(
i18next.t("displayNameLabel", {
ns: "templatePage",
}),
),
description: Yup.string().max(
MAX_DESCRIPTION_CHAR_LIMIT,
Language.descriptionMaxError,
),
default_ttl_ms: Yup.number()
.integer()
.min(0)
.max(24 * MAX_TTL_DAYS /* 7 days in hours */, Language.ttlMaxError),
allow_user_cancel_workspace_jobs: Yup.boolean(),
})
export const getValidationSchema = (): Yup.AnyObjectSchema =>
Yup.object({
name: nameValidator(i18next.t("nameLabel", { ns: "templateSettingsPage" })),
display_name: templateDisplayNameValidator(
i18next.t("displayNameLabel", {
ns: "templateSettingsPage",
}),
),
description: Yup.string().max(
MAX_DESCRIPTION_CHAR_LIMIT,
i18next.t("descriptionMaxError", { ns: "templateSettingsPage" }),
),
default_ttl_ms: Yup.number()
.integer()
.min(0, i18next.t("ttlMinError", { ns: "templateSettingsPage" }))
.max(
24 * MAX_TTL_DAYS /* 7 days in hours */,
i18next.t("ttlMaxError", { ns: "templateSettingsPage" }),
),
allow_user_cancel_workspace_jobs: Yup.boolean(),
})

export interface TemplateSettingsForm {
template: Template
Expand All @@ -81,6 +82,7 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
initialTouched,
}) => {
const [isEmojiPickerOpen, setIsEmojiPickerOpen] = useState(false)
const validationSchema = getValidationSchema()
const form: FormikContextType<UpdateTemplateMeta> =
useFormik<UpdateTemplateMeta>({
initialValues: {
Expand Down Expand Up @@ -110,18 +112,18 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
const hasIcon = form.values.icon && form.values.icon !== ""
const emojiButtonRef = useRef<HTMLButtonElement>(null)

const { t } = useTranslation("templatePage")
const { t } = useTranslation("templateSettingsPage")

return (
<form onSubmit={form.handleSubmit} aria-label={Language.formAriaLabel}>
<form onSubmit={form.handleSubmit} aria-label={t("formAriaLabel")}>
<Stack>
<TextField
{...getFieldHelpers("name")}
disabled={isSubmitting}
onChange={onChangeTrimmed(form)}
autoFocus
fullWidth
label={Language.nameLabel}
label={t("nameLabel")}
variant="outlined"
/>

Expand All @@ -138,7 +140,7 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
multiline
disabled={isSubmitting}
fullWidth
label={Language.descriptionLabel}
label={t("descriptionLabel")}
variant="outlined"
rows={2}
/>
Expand All @@ -148,7 +150,7 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
{...getFieldHelpers("icon")}
disabled={isSubmitting}
fullWidth
label={Language.iconLabel}
label={t("iconLabel")}
variant="outlined"
InputProps={{
endAdornment: hasIcon ? (
Expand Down Expand Up @@ -177,7 +179,7 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
setIsEmojiPickerOpen((v) => !v)
}}
>
{Language.selectEmoji}
{t("selectEmoji")}
</Button>

<Popover
Expand All @@ -204,20 +206,17 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
</div>

<TextField
{...getFieldHelpers("default_ttl_ms")}
{...getFieldHelpers(
"default_ttl_ms",
<TTLHelperText ttl={form.values.default_ttl_ms} />,
)}
disabled={isSubmitting}
fullWidth
inputProps={{ min: 0, step: 1 }}
label={Language.defaultTtlLabel}
label={t("defaultTtlLabel")}
variant="outlined"
type="number"
/>
{/* If a value for default_ttl_ms has been entered and
there are no validation errors for that field, display helper text.
We do not use the MUI helper-text prop because it overrides the validation error */}
{form.values.default_ttl_ms && !form.errors.default_ttl_ms && (
<span>{Language.ttlHelperText(form.values.default_ttl_ms)}</span>
)}

<Box display="flex">
<div>
Expand Down
53 changes: 29 additions & 24 deletions site/src/pages/TemplateSettingsPage/TemplateSettingsPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ import { UpdateTemplateMeta } from "api/typesGenerated"
import { Language as FooterFormLanguage } from "components/FormFooter/FormFooter"
import { MockTemplate } from "../../testHelpers/entities"
import { renderWithAuth } from "../../testHelpers/renderHelpers"
import {
Language as FormLanguage,
validationSchema,
} from "./TemplateSettingsForm"
import { getValidationSchema } from "./TemplateSettingsForm"
import { TemplateSettingsPage } from "./TemplateSettingsPage"
import i18next from "i18next"

const { t } = i18next

const renderTemplateSettingsPage = async () => {
const renderResult = renderWithAuth(<TemplateSettingsPage />, {
route: `/templates/${MockTemplate.name}/settings`,
path: `/templates/:templateId/settings`,
})
// Wait the form to be rendered
await screen.findAllByLabelText(FormLanguage.nameLabel)
const label = t("nameLabel", { ns: "templateSettingsPage" })
await screen.findAllByLabelText(label)
return renderResult
}

Expand All @@ -39,28 +39,29 @@ const fillAndSubmitForm = async ({
icon,
allow_user_cancel_workspace_jobs,
}: Required<UpdateTemplateMeta>) => {
const nameField = await screen.findByLabelText(FormLanguage.nameLabel)
const label = t("nameLabel", { ns: "templateSettingsPage" })
const nameField = await screen.findByLabelText(label)
await userEvent.clear(nameField)
await userEvent.type(nameField, name)

const { t } = i18next
const displayNameLabel = t("displayNameLabel", { ns: "templatePage" })
const displayNameLabel = t("displayNameLabel", { ns: "templateSettingsPage" })

const displayNameField = await screen.findByLabelText(displayNameLabel)
await userEvent.clear(displayNameField)
await userEvent.type(displayNameField, display_name)

const descriptionField = await screen.findByLabelText(
FormLanguage.descriptionLabel,
)
const descriptionLabel = t("descriptionLabel", { ns: "templateSettingsPage" })
const descriptionField = await screen.findByLabelText(descriptionLabel)
await userEvent.clear(descriptionField)
await userEvent.type(descriptionField, description)

const iconField = await screen.findByLabelText(FormLanguage.iconLabel)
const iconLabel = t("iconLabel", { ns: "templateSettingsPage" })
const iconField = await screen.findByLabelText(iconLabel)
await userEvent.clear(iconField)
await userEvent.type(iconField, icon)

const maxTtlField = await screen.findByLabelText(FormLanguage.defaultTtlLabel)
const defaultTtlLabel = t("defaultTtlLabel", { ns: "templateSettingsPage" })
const maxTtlField = await screen.findByLabelText(defaultTtlLabel)
await userEvent.clear(maxTtlField)
await userEvent.type(maxTtlField, default_ttl_ms.toString())

Expand All @@ -79,8 +80,8 @@ const fillAndSubmitForm = async ({
describe("TemplateSettingsPage", () => {
it("renders", async () => {
const { t } = i18next
const pageTitle = t("templateSettings.title", {
ns: "templatePage",
const pageTitle = t("title", {
ns: "templateSettingsPage",
})
await renderTemplateSettingsPage()
const element = await screen.findByText(pageTitle)
Expand All @@ -90,8 +91,8 @@ describe("TemplateSettingsPage", () => {
it("allows an admin to delete a template", async () => {
const { t } = i18next
await renderTemplateSettingsPage()
const deleteCta = t("templateSettings.dangerZone.deleteCta", {
ns: "templatePage",
const deleteCta = t("dangerZone.deleteCta", {
ns: "templateSettingsPage",
})
const deleteButton = await screen.findByText(deleteCta)
expect(deleteButton).toBeDefined()
Expand Down Expand Up @@ -137,7 +138,7 @@ describe("TemplateSettingsPage", () => {
...validFormValues,
default_ttl_ms: 24 * 7,
}
const validate = () => validationSchema.validateSync(values)
const validate = () => getValidationSchema().validateSync(values)
expect(validate).not.toThrowError()
})

Expand All @@ -146,7 +147,7 @@ describe("TemplateSettingsPage", () => {
...validFormValues,
default_ttl_ms: 0,
}
const validate = () => validationSchema.validateSync(values)
const validate = () => getValidationSchema().validateSync(values)
expect(validate).not.toThrowError()
})

Expand All @@ -155,8 +156,10 @@ describe("TemplateSettingsPage", () => {
...validFormValues,
default_ttl_ms: 24 * 7 + 1,
}
const validate = () => validationSchema.validateSync(values)
expect(validate).toThrowError(FormLanguage.ttlMaxError)
const validate = () => getValidationSchema().validateSync(values)
expect(validate).toThrowError(
t("ttlMaxError", { ns: "templateSettingsPage" }),
)
})

it("allows a description of 128 chars", () => {
Expand All @@ -165,7 +168,7 @@ describe("TemplateSettingsPage", () => {
description:
"Nam quis nulla. Integer malesuada. In in enim a arcu imperdiet malesuada. Sed vel lectus. Donec odio urna, tempus molestie, port",
}
const validate = () => validationSchema.validateSync(values)
const validate = () => getValidationSchema().validateSync(values)
expect(validate).not.toThrowError()
})

Expand All @@ -175,7 +178,9 @@ describe("TemplateSettingsPage", () => {
description:
"Nam quis nulla. Integer malesuada. In in enim a arcu imperdiet malesuada. Sed vel lectus. Donec odio urna, tempus molestie, port a",
}
const validate = () => validationSchema.validateSync(values)
expect(validate).toThrowError(FormLanguage.descriptionMaxError)
const validate = () => getValidationSchema().validateSync(values)
expect(validate).toThrowError(
t("descriptionMaxError", { ns: "templateSettingsPage" }),
)
})
})
8 changes: 3 additions & 5 deletions site/src/pages/TemplateSettingsPage/TemplateSettingsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,15 @@ import { useMachine } from "@xstate/react"
import { useOrganizationId } from "hooks/useOrganizationId"
import { FC } from "react"
import { Helmet } from "react-helmet-async"
import { useTranslation } from "react-i18next"
import { useNavigate, useParams } from "react-router-dom"
import { pageTitle } from "util/page"
import { templateSettingsMachine } from "xServices/templateSettings/templateSettingsXService"
import { TemplateSettingsPageView } from "./TemplateSettingsPageView"

const Language = {
title: "Template Settings",
}

export const TemplateSettingsPage: FC = () => {
const { template: templateName } = useParams() as { template: string }
const { t } = useTranslation("templateSettingsPage")
const navigate = useNavigate()
const organizationId = useOrganizationId()
const [state, send] = useMachine(templateSettingsMachine, {
Expand All @@ -34,7 +32,7 @@ export const TemplateSettingsPage: FC = () => {
return (
<>
<Helmet>
<title>{pageTitle(Language.title)}</title>
<title>{pageTitle(t("title"))}</title>
</Helmet>
<TemplateSettingsPageView
isSubmitting={state.hasTag("submitting")}
Expand Down
Loading