Skip to content

fix: create and read workspace page #1294

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 13 commits into from
May 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion site/src/AppRouter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { TemplatesPage } from "./pages/TemplatesPages/TemplatesPage"
import { TerminalPage } from "./pages/TerminalPage/TerminalPage"
import { CreateUserPage } from "./pages/UsersPage/CreateUserPage/CreateUserPage"
import { UsersPage } from "./pages/UsersPage/UsersPage"
import { WorkspacePage } from "./pages/WorkspacesPage/WorkspacesPage"
import { WorkspacePage } from "./pages/WorkspacePage/WorkspacePage"

export const AppRouter: React.FC = () => (
<Routes>
Expand Down
19 changes: 17 additions & 2 deletions site/src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const provisioners: Types.Provisioner[] = [

export namespace Workspace {
export const create = async (request: Types.CreateWorkspaceRequest): Promise<Types.Workspace> => {
const response = await fetch(`/api/v2/users/me/workspaces`, {
const response = await fetch(`/api/v2/organizations/${request.organization_id}/workspaces`, {
method: "POST",
headers: {
"Content-Type": "application/json",
Expand Down Expand Up @@ -80,12 +80,27 @@ export const getUsers = async (): Promise<TypesGen.User[]> => {
return response.data
}

export const getOrganization = async (organizationId: string): Promise<Types.Organization> => {
const response = await axios.get<Types.Organization>(`/api/v2/organizations/${organizationId}`)
return response.data
}

export const getOrganizations = async (): Promise<Types.Organization[]> => {
const response = await axios.get<Types.Organization[]>("/api/v2/users/me/organizations")
return response.data
}

export const getWorkspace = async (
export const getTemplate = async (templateId: string): Promise<Types.Template> => {
const response = await axios.get<Types.Template>(`/api/v2/templates/${templateId}`)
return response.data
}

export const getWorkspace = async (workspaceId: string): Promise<Types.Workspace> => {
const response = await axios.get<Types.Workspace>(`/api/v2/workspaces/${workspaceId}`)
return response.data
}

export const getWorkspaceByOwnerAndName = async (
organizationID: string,
username = "me",
workspaceName: string,
Expand Down
1 change: 1 addition & 0 deletions site/src/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export interface CreateTemplateRequest {
export interface CreateWorkspaceRequest {
name: string
template_id: string
organization_id: string
}

export interface WorkspaceBuild {
Expand Down
11 changes: 9 additions & 2 deletions site/src/forms/CreateWorkspaceForm.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { render, screen } from "@testing-library/react"
import React from "react"
import { MockTemplate, MockWorkspace } from "../testHelpers"
import { MockOrganization, MockTemplate, MockWorkspace } from "../testHelpers"
import { CreateWorkspaceForm } from "./CreateWorkspaceForm"

describe("CreateWorkspaceForm", () => {
Expand All @@ -10,7 +10,14 @@ describe("CreateWorkspaceForm", () => {
const onCancel = () => Promise.resolve()

// When
render(<CreateWorkspaceForm template={MockTemplate} onSubmit={onSubmit} onCancel={onCancel} />)
render(
<CreateWorkspaceForm
template={MockTemplate}
onSubmit={onSubmit}
onCancel={onCancel}
organization_id={MockOrganization.id}
/>,
)

// Then
// Simple smoke test to verify form renders
Expand Down
9 changes: 8 additions & 1 deletion site/src/forms/CreateWorkspaceForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,19 @@ export interface CreateWorkspaceForm {
template: Template
onSubmit: (request: CreateWorkspaceRequest) => Promise<Workspace>
onCancel: () => void
organization_id: string
Copy link
Member

Choose a reason for hiding this comment

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

Should this be organizationID to match other camelCase fields in this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it snakecased because it gets piped directly out of and into api calls, but I agree the inconsistency isn't good. I guess I'll ask about this at FE V, we have some other spots like this.

}

const validationSchema = Yup.object({
name: Yup.string().required("Name is required"),
})

export const CreateWorkspaceForm: React.FC<CreateWorkspaceForm> = ({ template, onSubmit, onCancel }) => {
export const CreateWorkspaceForm: React.FC<CreateWorkspaceForm> = ({
template,
onSubmit,
onCancel,
organization_id,
}) => {
const styles = useStyles()

const form: FormikContextType<{ name: string }> = useFormik<{ name: string }>({
Expand All @@ -34,6 +40,7 @@ export const CreateWorkspaceForm: React.FC<CreateWorkspaceForm> = ({ template, o
return onSubmit({
template_id: template.id,
name: name,
organization_id,
})
},
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { makeStyles } from "@material-ui/core/styles"
import React, { useCallback } from "react"
import { useSelector } from "@xstate/react"
import React, { useCallback, useContext } from "react"
import { useNavigate, useParams } from "react-router-dom"
import useSWR from "swr"
import * as API from "../../../../api"
Expand All @@ -8,12 +9,17 @@ import { ErrorSummary } from "../../../../components/ErrorSummary/ErrorSummary"
import { FullScreenLoader } from "../../../../components/Loader/FullScreenLoader"
import { CreateWorkspaceForm } from "../../../../forms/CreateWorkspaceForm"
import { unsafeSWRArgument } from "../../../../util"
import { selectOrgId } from "../../../../xServices/auth/authSelectors"
import { XServiceContext } from "../../../../xServices/StateContext"

export const CreateWorkspacePage: React.FC = () => {
const { organization: organizationName, template: templateName } = useParams()
const navigate = useNavigate()
const styles = useStyles()

const xServices = useContext(XServiceContext)
const myOrgId = useSelector(xServices.authXService, selectOrgId)

const { data: organizationInfo, error: organizationError } = useSWR<Types.Organization, Error>(
() => `/api/v2/users/me/organizations/${organizationName}`,
)
Expand Down Expand Up @@ -44,9 +50,13 @@ export const CreateWorkspacePage: React.FC = () => {
return <FullScreenLoader />
}

if (!myOrgId) {
return <ErrorSummary error={Error("no organization id")} />
}

return (
<div className={styles.root}>
<CreateWorkspaceForm onCancel={onCancel} onSubmit={onSubmit} template={template} />
<CreateWorkspaceForm onCancel={onCancel} onSubmit={onSubmit} template={template} organization_id={myOrgId} />
</div>
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ export const TemplatePage: React.FC = () => {

// This just grabs all workspaces... and then later filters them to match the
// current template.
const { data: workspaces, error: workspacesError } = useSWR<Workspace[], Error>(() => `/api/v2/users/me/workspaces`)

const { data: workspaces, error: workspacesError } = useSWR<Workspace[], Error>(
() => `/api/v2/organizations/${unsafeSWRArgument(organizationInfo).id}/workspaces`,
)

if (organizationError) {
return <ErrorSummary error={organizationError} />
Expand Down
14 changes: 14 additions & 0 deletions site/src/pages/WorkspacePage/WorkspacePage.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { screen } from "@testing-library/react"
import React from "react"
import { MockTemplate, MockWorkspace, renderWithAuth } from "../../testHelpers"
import { WorkspacePage } from "./WorkspacePage"

describe("Workspace Page", () => {
it("shows a workspace", async () => {
renderWithAuth(<WorkspacePage />, { route: `/workspaces/${MockWorkspace.id}`, path: "/workspaces/:workspace" })
const workspaceName = await screen.findByText(MockWorkspace.name)
const templateName = await screen.findByText(MockTemplate.name)
expect(workspaceName).toBeDefined()
expect(templateName).toBeDefined()
})
})
42 changes: 42 additions & 0 deletions site/src/pages/WorkspacePage/WorkspacePage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { useActor } from "@xstate/react"
import React, { useContext, useEffect } from "react"
import { useParams } from "react-router-dom"
import { ErrorSummary } from "../../components/ErrorSummary/ErrorSummary"
import { FullScreenLoader } from "../../components/Loader/FullScreenLoader"
import { Margins } from "../../components/Margins/Margins"
import { Stack } from "../../components/Stack/Stack"
import { Workspace } from "../../components/Workspace/Workspace"
import { firstOrItem } from "../../util/array"
import { XServiceContext } from "../../xServices/StateContext"

export const WorkspacePage: React.FC = () => {
const { workspace: workspaceQueryParam } = useParams()
const workspaceId = firstOrItem(workspaceQueryParam, null)

const xServices = useContext(XServiceContext)
const [workspaceState, workspaceSend] = useActor(xServices.workspaceXService)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this uses a global XState service. Would useMachine be more appropriate here since the machine should only be active for the lifecycle of this page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking it would be active for longer but maybe that's not actually desired since it's for a specific workspace. I can make it local for now and useMachine and we can hoist it later if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Neato neato

Copy link
Contributor Author

@presleyp presleyp May 5, 2022

Choose a reason for hiding this comment

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

@kylecarbs Actually I think we'll want it to outlive the component because we're using separate pages for things like the create/edit form and the parts of the timeline. Does that sound right?

const { workspace, template, organization, getWorkspaceError, getTemplateError, getOrganizationError } =
workspaceState.context

/**
* Get workspace, template, and organization on mount and whenever workspaceId changes.
* workspaceSend should not change.
*/
useEffect(() => {
workspaceId && workspaceSend({ type: "GET_WORKSPACE", workspaceId })
}, [workspaceId, workspaceSend])

if (workspaceState.matches("error")) {
return <ErrorSummary error={getWorkspaceError || getTemplateError || getOrganizationError} />
} else if (!workspace || !template || !organization) {
return <FullScreenLoader />
} else {
return (
<Margins>
<Stack spacing={4}>
<Workspace organization={organization} template={template} workspace={workspace} />
</Stack>
</Margins>
)
}
}
54 changes: 0 additions & 54 deletions site/src/pages/WorkspacesPage/WorkspacesPage.tsx

This file was deleted.

13 changes: 11 additions & 2 deletions site/src/testHelpers/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,22 @@ export const render = (component: React.ReactElement): RenderResult => {

type RenderWithAuthResult = RenderResult & { user: typeof MockUser }

export function renderWithAuth(ui: JSX.Element, { route = "/" }: { route?: string } = {}): RenderWithAuthResult {
/**
*
* @param ui The component to render and test
* @param options Can contain `route`, the URL to use, such as /users/user1, and `path`,
* such as /users/:userid. When there are no parameters, they are the same and you can just supply `route`.
*/
export function renderWithAuth(
ui: JSX.Element,
{ route = "/", path }: { route?: string; path?: string } = {},
): RenderWithAuthResult {
const renderResult = wrappedRender(
<MemoryRouter initialEntries={[route]}>
<XServiceProvider>
<ThemeProvider theme={dark}>
<Routes>
<Route path={route} element={<RequireAuth>{ui}</RequireAuth>} />
<Route path={path ?? route} element={<RequireAuth>{ui}</RequireAuth>} />
</Routes>
</ThemeProvider>
</XServiceProvider>
Expand Down
3 changes: 3 additions & 0 deletions site/src/xServices/StateContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import { ActorRefFrom } from "xstate"
import { authMachine } from "./auth/authXService"
import { buildInfoMachine } from "./buildInfo/buildInfoXService"
import { usersMachine } from "./users/usersXService"
import { workspaceMachine } from "./workspace/workspaceXService"

interface XServiceContextType {
authXService: ActorRefFrom<typeof authMachine>
buildInfoXService: ActorRefFrom<typeof buildInfoMachine>
usersXService: ActorRefFrom<typeof usersMachine>
workspaceXService: ActorRefFrom<typeof workspaceMachine>
}

/**
Expand All @@ -34,6 +36,7 @@ export const XServiceProvider: React.FC = ({ children }) => {
authXService: useInterpret(authMachine),
buildInfoXService: useInterpret(buildInfoMachine),
usersXService: useInterpret(() => usersMachine.withConfig({ actions: { redirectToUsersPage } })),
workspaceXService: useInterpret(workspaceMachine),
}}
>
{children}
Expand Down
2 changes: 1 addition & 1 deletion site/src/xServices/terminal/terminalXService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export const terminalMachine =
if (!context.organizations || !context.workspaceName) {
throw new Error("organizations or workspace not set")
}
return API.getWorkspace(context.organizations[0].id, context.username, context.workspaceName)
return API.getWorkspaceByOwnerAndName(context.organizations[0].id, context.username, context.workspaceName)
},
getWorkspaceAgent: async (context) => {
if (!context.workspace || !context.workspaceName) {
Expand Down
Loading