Skip to content

feat: allow users to duplicate workspaces by parameters #10362

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
merged 24 commits into from
Nov 3, 2023
Merged

feat: allow users to duplicate workspaces by parameters #10362

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9e4f999
chore: add queries for workspace build info
Parkreiner Oct 20, 2023
112bc95
refactor: clean up logic for CreateWorkspacePage to support multiple …
Parkreiner Oct 20, 2023
15fdfbf
chore: add custom workspace duplication hook
Parkreiner Oct 20, 2023
d007b86
chore: integrate mode into CreateWorkspacePageView
Parkreiner Oct 20, 2023
294156e
fix: add mode to CreateWorkspacePageView stories
Parkreiner Oct 20, 2023
4554895
refactor: extract workspace duplication outside CreateWorkspacePage file
Parkreiner Oct 20, 2023
25bacf2
chore: integrate useWorkspaceDuplication into WorkspaceActions
Parkreiner Oct 20, 2023
0947031
chore: delete unnecessary function
Parkreiner Oct 20, 2023
1d4d4d7
Merge branch 'main' into mes/workspace-clone-feat
Parkreiner Oct 31, 2023
d71acf6
refactor: swap useReducer for useState
Parkreiner Oct 31, 2023
c0a8c56
fix: swap warning alert for info alert
Parkreiner Oct 31, 2023
0b3e954
refactor: move info alert message
Parkreiner Oct 31, 2023
7a763a9
refactor: simplify UI logic for mode alerts
Parkreiner Oct 31, 2023
da488fa
fix: prevent dismissed Alerts from affecting layouts
Parkreiner Oct 31, 2023
5c7242f
fix: remove unnecessary prop binding
Parkreiner Oct 31, 2023
98d1b1b
docs: reword comment for clarity
Parkreiner Oct 31, 2023
aeacda5
chore: update msw build params to return multiple params
Parkreiner Oct 31, 2023
230a4f1
chore: rename duplicationReady to isDuplicationReady
Parkreiner Oct 31, 2023
75b1839
chore: expose root component for testing/re-rendering
Parkreiner Nov 1, 2023
7cf446f
chore: get tests in place (still have act warnings)
Parkreiner Nov 1, 2023
bf21656
refactor: move stuff around for clarity
Parkreiner Nov 1, 2023
38ba3b2
chore: finish tests
Parkreiner Nov 1, 2023
923d080
chore: revamp tests
Parkreiner Nov 3, 2023
8b3d4dd
chore: merge main into branch
Parkreiner Nov 3, 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
Prev Previous commit
Next Next commit
chore: integrate mode into CreateWorkspacePageView
  • Loading branch information
@Parkreiner
Parkreiner committed Oct 20, 2023
commit d007b865e78be9422e3d133c9bac562827633f2a
1 change: 1 addition & 0 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ const CreateWorkspacePage: FC = () => {
<Loader />
) : (
<CreateWorkspacePageView
mode={mode}
defaultName={defaultName}
defaultOwner={me}
defaultBuildParameters={defaultBuildParameters}
Expand Down
279 changes: 161 additions & 118 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import TextField from "@mui/material/TextField";
import * as TypesGen from "api/typesGenerated";
import { UserAutocomplete } from "components/UserAutocomplete/UserAutocomplete";
import { FormikContextType, useFormik } from "formik";
import { FC, useEffect, useState } from "react";
import { FC, useEffect, useReducer, useState } from "react";
import {
getFormHelpers,
nameValidator,
Expand All @@ -29,11 +29,18 @@ import {
import { ExternalAuth } from "./ExternalAuth";
import { ErrorAlert } from "components/Alert/ErrorAlert";
import { Stack } from "components/Stack/Stack";
import { type ExternalAuthPollingState } from "./CreateWorkspacePage";
import {
CreateWorkspaceMode,
type ExternalAuthPollingState,
} from "./CreateWorkspacePage";
import { useSearchParams } from "react-router-dom";
import { CreateWSPermissions } from "./permissions";
import { useTheme } from "@emotion/react";
import { Margins } from "components/Margins/Margins";
import { Alert } from "components/Alert/Alert";

export interface CreateWorkspacePageViewProps {
mode: CreateWorkspaceMode;
error: unknown;
defaultName: string;
defaultOwner: TypesGen.User;
Expand All @@ -54,6 +61,7 @@ export interface CreateWorkspacePageViewProps {
}

export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
mode,
error,
defaultName,
defaultOwner,
Expand Down Expand Up @@ -112,133 +120,143 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
);

return (
<FullPageHorizontalForm title="New workspace" onCancel={onCancel}>
<HorizontalForm onSubmit={form.handleSubmit}>
{Boolean(error) && <ErrorAlert error={error} />}
{/* General info */}
<FormSection
title="General"
description="The template and name of your new workspace."
>
<FormFields>
<SelectedTemplate template={template} />
{versionId && versionId !== template.active_version_id && (
<Stack spacing={1} className={styles.hasDescription}>
<TextField
disabled
fullWidth
value={versionId}
label="Version ID"
/>
<span className={styles.description}>
This parameter has been preset, and cannot be modified.
</span>
</Stack>
)}
<TextField
{...getFieldHelpers("name")}
disabled={creatingWorkspace}
onChange={onChangeTrimmed(form)}
autoFocus
fullWidth
label="Workspace Name"
/>
</FormFields>
</FormSection>
<>
{mode === "duplicate" && <DuplicateWarningMessage />}
Copy link
Member Author

@Parkreiner Parkreiner Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The only thing that changes with this chunk of code is that a React fragment got added, so I could squeeze in the DuplicateWarningMessage component. Everything else is the same – it just changed by an indentation level.

Copy link
Member

Choose a reason for hiding this comment

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

you could also put it next to the {Boolean(error) ... and that might look nice


{permissions.createWorkspaceForUser && (
<FullPageHorizontalForm title="New workspace" onCancel={onCancel}>
<HorizontalForm onSubmit={form.handleSubmit}>
{Boolean(error) && <ErrorAlert error={error} />}
{/* General info */}
<FormSection
title="Workspace Owner"
description="Only admins can create workspace for other users."
title="General"
description="The template and name of your new workspace."
>
<FormFields>
<UserAutocomplete
value={owner}
onChange={(user) => {
setOwner(user ?? defaultOwner);
}}
label="Owner"
size="medium"
<SelectedTemplate template={template} />
{versionId && versionId !== template.active_version_id && (
<Stack spacing={1} className={styles.hasDescription}>
<TextField
disabled
fullWidth
value={versionId}
label="Version ID"
/>
<span className={styles.description}>
This parameter has been preset, and cannot be modified.
</span>
</Stack>
)}
<TextField
{...getFieldHelpers("name")}
disabled={creatingWorkspace}
onChange={onChangeTrimmed(form)}
autoFocus
fullWidth
label="Workspace Name"
/>
</FormFields>
</FormSection>
)}

{externalAuth && externalAuth.length > 0 && (
<FormSection
title="External Authentication"
description="This template requires authentication to external services."
>
<FormFields>
{externalAuth.map((auth) => (
<ExternalAuth
key={auth.id}
authenticateURL={auth.authenticate_url}
authenticated={auth.authenticated}
externalAuthPollingState={externalAuthPollingState}
startPollingExternalAuth={startPollingExternalAuth}
displayName={auth.display_name}
displayIcon={auth.display_icon}
error={externalAuthErrors[auth.id]}
{permissions.createWorkspaceForUser && (
<FormSection
title="Workspace Owner"
description="Only admins can create workspace for other users."
>
<FormFields>
<UserAutocomplete
value={owner}
onChange={(user) => {
setOwner(user ?? defaultOwner);
}}
label="Owner"
size="medium"
/>
))}
</FormFields>
</FormSection>
)}
</FormFields>
</FormSection>
)}

{parameters && (
<>
<MutableTemplateParametersSection
templateParameters={parameters}
getInputProps={(parameter, index) => {
return {
...getFieldHelpers(
"rich_parameter_values[" + index + "].value",
),
onChange: async (value) => {
await form.setFieldValue("rich_parameter_values." + index, {
name: parameter.name,
value: value,
});
},
disabled:
disabledParamsList?.includes(
parameter.name.toLowerCase().replace(/ /g, "_"),
) || creatingWorkspace,
};
}}
/>
<ImmutableTemplateParametersSection
templateParameters={parameters}
classes={{ root: styles.warningSection }}
getInputProps={(parameter, index) => {
return {
...getFieldHelpers(
"rich_parameter_values[" + index + "].value",
),
onChange: async (value) => {
await form.setFieldValue("rich_parameter_values." + index, {
name: parameter.name,
value: value,
});
},
disabled:
disabledParamsList?.includes(
parameter.name.toLowerCase().replace(/ /g, "_"),
) || creatingWorkspace,
};
}}
/>
</>
)}
{externalAuth && externalAuth.length > 0 && (
<FormSection
title="External Authentication"
description="This template requires authentication to external services."
>
<FormFields>
{externalAuth.map((auth) => (
<ExternalAuth
key={auth.id}
authenticateURL={auth.authenticate_url}
authenticated={auth.authenticated}
externalAuthPollingState={externalAuthPollingState}
startPollingExternalAuth={startPollingExternalAuth}
displayName={auth.display_name}
displayIcon={auth.display_icon}
error={externalAuthErrors[auth.id]}
/>
))}
</FormFields>
</FormSection>
)}

<FormFooter
onCancel={onCancel}
isLoading={creatingWorkspace}
submitLabel="Create Workspace"
/>
</HorizontalForm>
</FullPageHorizontalForm>
{parameters && (
<>
<MutableTemplateParametersSection
templateParameters={parameters}
getInputProps={(parameter, index) => {
return {
...getFieldHelpers(
"rich_parameter_values[" + index + "].value",
),
onChange: async (value) => {
await form.setFieldValue(
"rich_parameter_values." + index,
{
name: parameter.name,
value: value,
},
);
},
disabled:
disabledParamsList?.includes(
parameter.name.toLowerCase().replace(/ /g, "_"),
) || creatingWorkspace,
};
}}
/>
<ImmutableTemplateParametersSection
templateParameters={parameters}
classes={{ root: styles.warningSection }}
getInputProps={(parameter, index) => {
return {
...getFieldHelpers(
"rich_parameter_values[" + index + "].value",
),
onChange: async (value) => {
await form.setFieldValue(
"rich_parameter_values." + index,
{
name: parameter.name,
value: value,
},
);
},
disabled:
disabledParamsList?.includes(
parameter.name.toLowerCase().replace(/ /g, "_"),
) || creatingWorkspace,
};
}}
/>
</>
)}

<FormFooter
onCancel={onCancel}
isLoading={creatingWorkspace}
submitLabel="Create Workspace"
/>
</HorizontalForm>
</FullPageHorizontalForm>
</>
);
};

Expand Down Expand Up @@ -279,6 +297,31 @@ const useExternalAuthVerification = (
};
};

function DuplicateWarningMessage() {
const [isDismissed, dismiss] = useReducer(() => true, false);
Copy link
Member

Choose a reason for hiding this comment

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

this is clever, but maybe too clever. 😅 it took me a second to realize what this does, and others who are less familiar with useReducer could probably be quite confused by this. can we just use useState instead?

Copy link
Member Author

@Parkreiner Parkreiner Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's fair, and I figured I'd get some pushback on that.

I use the action-less reducer pattern a lot in my personal code, where I want to make my expected state transitions super, super explicit (in this case, making it clear that state can only ever transition from false to true, and shouldn't be undone). It helps that useReducer has function overloads to augment the type of the dispatch if it is action-less

But as far as readability/communication, it's probably a bit much/too convoluted for a simple use case like this. I'll change this

const theme = useTheme();

if (isDismissed) {
return null;
}

// Setup looks a little hokey (having an Alert already fully configured to
// listen to dismissals, on top of more dismissal state), but relying solely
// on the Alert API wouldn't get rid of the div and horizontal margin helper
// after the dismiss happens. Not using CSS margins because those can be a
// style maintenance nightmare over time
return (
<div css={{ paddingTop: theme.spacing(6) }}>
<Margins size="medium">
<Alert severity="warning" dismissible onDismiss={dismiss}>
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be an "info". Nothing actually went wrong, the user isn't doing anything sketchy. It's good context to provide, but I think warnings should result from users doing something worth complaining about, not from following the "blessed path"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agreed. Will get that fixed up

Duplicating a workspace only copies its parameters. No state from the
old workspace is copied over.
</Alert>
</Margins>
</div>
);
}

const useStyles = makeStyles((theme) => ({
hasDescription: {
paddingBottom: theme.spacing(2),
Expand Down