opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-24 15:04:27 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: handle blank dynamic parameter values consistently (#26122)

Browse Source 
There was inconsistency with what the form showed and what actually was
sent to the backend. I opted to make it so that explicitly blank values
are always sent rather than have blank values silently changing to the
default value.
This commit is contained in:
Asher
2026-06-16 15:05:27 -08:00
committed by GitHub
parent 53a6459ecd
commit 744c00f45c
7 changed files with 268 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx
+2 -1
View File
@@ -698,7 +698,8 @@ export const getInitialParameterValues = (
);
const useAutofill =
autofillParam?.value && isValidParameterOption(parameter, autofillParam);
autofillParam?.value !== undefined &&
isValidParameterOption(parameter, autofillParam);
return {
name: parameter.name,
site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx
+49 -34
View File
@@ -8,7 +8,9 @@ import {
MockDynamicParametersResponse,
MockDynamicParametersResponseWithError,
MockPermissions,
MockPreviewParameter,
MockPreviewParameter1,
MockPreviewParameter2,
MockPreviewParameter7,
MockSliderParameter,
MockTemplate,
MockTemplateVersion,
@@ -18,6 +20,7 @@ import {
MockValidationParameter,
MockWorkspace,
} from "#/testHelpers/entities";
import { checkParameters, editParameters } from "#/testHelpers/parameters";
import {
renderWithAuth,
waitForLoaderToBeRemoved,
@@ -253,11 +256,12 @@ describe("CreateWorkspacePage", () => {
it("does not clobber user values", async () => {
const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => {
publisher.publishOpen(new Event("open"));
// The initial message always has the default values.
publisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [MockPreviewParameter],
parameters: [MockPreviewParameter1, MockPreviewParameter7],
diagnostics: [],
}),
}),
@@ -267,26 +271,32 @@ describe("CreateWorkspacePage", () => {
renderCreateWorkspacePage();
await waitForLoaderToBeRemoved();
const form = screen.getByTestId("form");
const input = await within(form).findByRole("textbox", {
name: /parameter 1/i,
});
await userEvent.clear(input);
await userEvent.type(input, "hi there hello");
// Blank out one field and fill out another.
const editedParameters = [
// Put the blank one first to ensure we are preserving blank values and
// not just including it the first time due to the change handler.
{
name: MockPreviewParameter1.name,
value: "",
},
{
name: MockPreviewParameter7.name,
value: "not-blank",
},
];
editParameters(...editedParameters);
await waitFor(() => {
expect(
within(form).getByDisplayValue("hi there hello"),
).toBeInTheDocument();
});
// Simulate a stale response.
// Respond with different values.
await act(async () => {
mockPublisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: 2,
parameters: [MockPreviewParameter, MockValidationParameter],
parameters: [
MockPreviewParameter1,
MockPreviewParameter2, // new field
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
@@ -294,43 +304,53 @@ describe("CreateWorkspacePage", () => {
});
// Should have the new field, but keep the existing user-filled values.
await waitFor(() => {
expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
expect(
within(form).getByDisplayValue("hi there hello"),
).toBeInTheDocument();
});
await checkParameters(...editedParameters, MockPreviewParameter2);
});
it("does not clobber auto-filled values", async () => {
const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => {
publisher.publishOpen(new Event("open"));
// The initial message always has the default values.
publisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [MockPreviewParameter, MockSliderParameter],
parameters: [MockPreviewParameter1, MockPreviewParameter7],
diagnostics: [],
}),
}),
);
});
// Blank out one field and fill out another.
const editedParameters = [
{
name: MockPreviewParameter1.name,
value: "",
},
{
name: MockPreviewParameter7.name,
value: "not-blank",
},
];
const query = editedParameters
.map((param) => `param.${param.name}=${param.value}`)
.join("&");
renderCreateWorkspacePage(
`/templates/${MockTemplate.name}/workspace?param.cpu_count=44&param.parameter1=auto`,
`/templates/${MockTemplate.name}/workspace?${query}`,
);
await waitForLoaderToBeRemoved();
// Simulate a stale response.
// Respond with different values.
await act(async () => {
mockPublisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: 2,
parameters: [
MockPreviewParameter,
MockSliderParameter,
MockValidationParameter,
MockPreviewParameter1,
MockPreviewParameter2, // new field
MockPreviewParameter7,
],
diagnostics: [],
}),
@@ -339,12 +359,7 @@ describe("CreateWorkspacePage", () => {
});
// Should have the new field, but keep the existing auto-filled values.
const form = screen.getByTestId("form");
await waitFor(() => {
expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
expect(within(form).getByDisplayValue("44")).toBeInTheDocument();
expect(within(form).getByDisplayValue("auto")).toBeInTheDocument();
});
await checkParameters(...editedParameters, MockPreviewParameter2);
});
});
site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx
+1 -1
View File
@@ -189,7 +189,7 @@ const CreateWorkspacePage: FC = () => {
const initialParamsToSend: Record<string, string> = {};
for (const param of initialFormValues) {
if (param.name && param.value) {
if (param.name && param.value !== undefined) {
initialParamsToSend[param.name] = param.value;
}
}
site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx
+152 -10
View File
@@ -7,13 +7,20 @@ import {
MockPreviewParameter1,
MockPreviewParameter2,
MockPreviewParameter4,
MockPreviewParameter7,
MockTemplateVersionParameter1,
MockTemplateVersionParameter4,
MockTemplateVersionParameter7,
MockWorkspace,
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter4,
MockWorkspaceBuildParameter7,
} from "#/testHelpers/entities";
import { checkParameters } from "#/testHelpers/parameters";
import {
checkParameters,
editParameters,
isBuildParameter,
} from "#/testHelpers/parameters";
import {
renderWithWorkspaceSettingsLayout,
waitForLoaderToBeRemoved,
@@ -49,7 +56,11 @@ describe("WorkspaceParametersPageExperimental", () => {
vi.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValueOnce([
MockTemplateVersionParameter1, // a mutable string
MockTemplateVersionParameter4, // an immutable string
MockTemplateVersionParameter7, // optional string
]);
vi.spyOn(API, "postWorkspaceBuild").mockRejectedValueOnce(
new Error("not implemented"),
);
});
afterEach(() => {
@@ -69,7 +80,11 @@ describe("WorkspaceParametersPageExperimental", () => {
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [MockPreviewParameter1, MockPreviewParameter4],
parameters: [
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
@@ -90,12 +105,13 @@ describe("WorkspaceParametersPageExperimental", () => {
const buildParameters = [
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter4,
MockWorkspaceBuildParameter7,
];
await act(async () => {
resolve(buildParameters);
});
// The client's init message should include the build values.
// The client's init message should include all the build values.
await waitFor(() => {
expect(mockPublisher.clientSentData).toHaveLength(1);
expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual(
@@ -125,6 +141,10 @@ describe("WorkspaceParametersPageExperimental", () => {
...MockPreviewParameter4,
value: { valid: true, value: MockWorkspaceBuildParameter4.value },
},
{
...MockPreviewParameter7,
value: { valid: true, value: MockWorkspaceBuildParameter7.value },
},
],
diagnostics: [],
}),
@@ -136,6 +156,7 @@ describe("WorkspaceParametersPageExperimental", () => {
await checkParameters(
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter4,
MockWorkspaceBuildParameter7,
);
// The submit button should be enabled.
@@ -158,7 +179,11 @@ describe("WorkspaceParametersPageExperimental", () => {
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [MockPreviewParameter1, MockPreviewParameter4],
parameters: [
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
@@ -183,7 +208,11 @@ describe("WorkspaceParametersPageExperimental", () => {
// Since there are no build values, the page is rendered with defaults and
// the client does not need to send anything.
await waitForLoaderToBeRemoved();
await checkParameters(MockPreviewParameter1, MockPreviewParameter4);
await checkParameters(
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
);
expect(mockPublisher.clientSentData).toHaveLength(0);
// The submit button should be enabled.
@@ -194,11 +223,16 @@ describe("WorkspaceParametersPageExperimental", () => {
await waitFor(() => expect(submitButton).toBeEnabled());
});
it("does not clobber touched parameters", async () => {
vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([
it("does not clobber build parameters", async () => {
const buildParameters = [
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter4,
]);
MockWorkspaceBuildParameter7,
];
vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce(
buildParameters,
);
const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => {
publisher.publishOpen(new Event("open"));
@@ -207,7 +241,11 @@ describe("WorkspaceParametersPageExperimental", () => {
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [MockPreviewParameter1, MockPreviewParameter4],
parameters: [
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
@@ -219,6 +257,14 @@ describe("WorkspaceParametersPageExperimental", () => {
// Wait for the client's init message then respond with different values.
await waitFor(() => {
expect(mockPublisher.clientSentData).toHaveLength(1);
expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual(
expect.objectContaining({
id: 0,
inputs: Object.fromEntries(
buildParameters.map((p) => [p.name, p.value]),
),
}),
);
});
mockPublisher.publishMessage(
@@ -227,8 +273,9 @@ describe("WorkspaceParametersPageExperimental", () => {
id: 0,
parameters: [
MockPreviewParameter1,
MockPreviewParameter2,
MockPreviewParameter2, // new field
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
@@ -241,6 +288,101 @@ describe("WorkspaceParametersPageExperimental", () => {
await checkParameters(
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter4,
MockWorkspaceBuildParameter7,
MockPreviewParameter2,
);
// However the submit button should be disabled because the state
// mismatches.
const form = screen.getByTestId("form");
const submitButton = within(form).getByRole("button", {
name: /update and restart/i,
});
await waitFor(() => expect(submitButton).toBeDisabled());
});
it("does not clobber edited parameters", async () => {
vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([]);
const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => {
publisher.publishOpen(new Event("open"));
// The initial message always has the default values.
publisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: -1,
parameters: [
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
);
});
renderWorkspaceParametersPageExperimental();
// Page should render with the default values.
await waitForLoaderToBeRemoved();
await checkParameters(
MockPreviewParameter1,
MockPreviewParameter4,
MockPreviewParameter7,
);
// Blank out one field and fill out another.
const editedParameters = [
// Put the blank one first to ensure we are preserving blank values and
// not just including it the first time due to the change handler.
{
name: MockPreviewParameter1.name,
value: "",
},
{
name: MockPreviewParameter7.name,
value: "not-blank",
},
];
editParameters(...editedParameters);
// The client should now send all parameters.
await waitFor(() => {
expect(mockPublisher.clientSentData).toHaveLength(1);
expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual(
expect.objectContaining({
id: 0,
inputs: Object.fromEntries(
[...editedParameters, MockPreviewParameter4].map((p) => [
p.name,
isBuildParameter(p) ? p.value : p.value.value,
]),
),
}),
);
});
// Respond with different values.
mockPublisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify({
id: 0,
parameters: [
MockPreviewParameter1,
MockPreviewParameter2, // new field
MockPreviewParameter4,
MockPreviewParameter7,
],
diagnostics: [],
}),
}),
);
// The form should keep the user's values but include the new field.
await checkParameters(
...editedParameters,
MockPreviewParameter4,
MockPreviewParameter2,
);
site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx
+1 -1
View File
@@ -81,7 +81,7 @@ export const WorkspaceParametersPageViewExperimental: FC<
const formInputs: Record<string, string> = {};
const formParameters = form.values.rich_parameter_values ?? [];
for (const param of formParameters) {
if (param?.name && param?.value) {
if (param?.name && param?.value !== undefined) {
formInputs[param.name] = param.value;
}
}
site/src/testHelpers/entities.ts
+41
View File
@@ -1947,6 +1947,25 @@ export const MockTemplateVersionParameter6: TypesGen.TemplateVersionParameter =
ephemeral: true,
};
// Not required and the default is a blank string.
export const MockTemplateVersionParameter7: TypesGen.TemplateVersionParameter =
{
name: "seventh_parameter",
type: "string",
form_type: "input",
description: "This is seventh parameter",
description_plaintext: "Markdown: This is seventh parameter",
default_value: "",
mutable: true,
icon: "/icon/folder.svg",
options: [],
validation_min: 1,
validation_max: 10,
validation_monotonic: "decreasing",
required: false,
ephemeral: false,
};
export const MockTemplateVersionVariable1: TypesGen.TemplateVersionVariable = {
name: "first_variable",
description: "This is first variable.",
@@ -3424,6 +3443,12 @@ export const MockWorkspaceBuildParameter5: TypesGen.WorkspaceBuildParameter = {
value: "5",
};
// Has a blank value.
export const MockWorkspaceBuildParameter7: TypesGen.WorkspaceBuildParameter = {
name: MockTemplateVersionParameter7.name,
value: "",
};
export const MockPreviewParameter: TypesGen.PreviewParameter = {
name: "parameter1",
display_name: "Parameter 1",
@@ -3483,6 +3508,22 @@ export const MockPreviewParameter4: TypesGen.PreviewParameter = {
mutable: false,
};
// A text parameter that is mutable, not required, and has a blank value. Maps
// to MockTemplateVersionParameter7.
export const MockPreviewParameter7: TypesGen.PreviewParameter = {
...MockPreviewParameter,
name: MockTemplateVersionParameter7.name,
display_name: MockTemplateVersionParameter7.name,
default_value: {
valid: true,
value: MockTemplateVersionParameter7.default_value,
},
value: { valid: true, value: MockTemplateVersionParameter7.default_value },
required: MockTemplateVersionParameter7.required,
mutable: MockTemplateVersionParameter7.mutable,
ephemeral: MockTemplateVersionParameter7.ephemeral,
};
export const MockDropdownParameter: TypesGen.PreviewParameter = {
...MockPreviewParameter,
name: "instance_type",
site/src/testHelpers/parameters.ts
+22 -1
View File
@@ -1,9 +1,10 @@
import { screen, waitFor, within } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import type * as TypesGen from "#/api/typesGenerated";
type Parameter = TypesGen.WorkspaceBuildParameter | TypesGen.PreviewParameter;
function isBuildParameter(
export function isBuildParameter(
parameter: Parameter,
): parameter is TypesGen.WorkspaceBuildParameter {
return typeof parameter.value === "string";
@@ -26,3 +27,23 @@ export async function checkParameters(...parameters: Parameter[]) {
}
});
}
// editParameters edits each parameter so it has the provided value. Requires
// that the form and parameters all have test IDs (`form` and
// `parameter-field-$name`).
export async function editParameters(...parameters: Parameter[]) {
const form = screen.getByTestId("form");
for (const parameter of parameters) {
const field = within(form).getByTestId(`parameter-field-${parameter.name}`);
const input = await within(field).findByRole("textbox", {
name: new RegExp(parameter.name, "i"),
});
await userEvent.clear(input);
const value = isBuildParameter(parameter)
? parameter.value
: parameter.value.value;
if (value !== "") {
await userEvent.type(input, value);
}
}
}