Skip to content

fix: ui: workspace bumpers now honour template max_ttl #3532

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 15 commits into from
Aug 18, 2022
Merged

fix: ui: workspace bumpers now honour template max_ttl #3532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5eb9317
chore: WorkspacePage: invert workspace schedule bumper logic for read…
johnstcn Aug 17, 2022
c57fed2
fix: ui: workspace bumpers now honour template max_ttl
johnstcn Aug 17, 2022
7bdf392
extract to function
johnstcn Aug 17, 2022
35e5146
add tests for deadlineMax, deadlineMin
johnstcn Aug 17, 2022
0c24962
wip
johnstcn Aug 17, 2022
28220d5
move deadline-related stuff to util/workspace.ts
johnstcn Aug 18, 2022
c68b931
fix unit tests, working around dayjs plugin silliness
johnstcn Aug 18, 2022
494213f
fix dayjs plugin silliness the right way
johnstcn Aug 18, 2022
9c8d4d5
linter
johnstcn Aug 18, 2022
ca8b95f
yarn fumpt
johnstcn Aug 18, 2022
4aa6eaf
i deserve to be mocked
johnstcn Aug 18, 2022
ff50a83
move schedule-related util functions to util/schedule
johnstcn Aug 18, 2022
b8ada49
use FullScreenLoader if template is null
johnstcn Aug 18, 2022
f9832b1
extract [-+] disable logic to util/schedule, add unit tests
johnstcn Aug 18, 2022
e69ff7f
fix storybook
johnstcn Aug 18, 2022
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
12 changes: 12 additions & 0 deletions site/src/components/Workspace/Workspace.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { action } from "@storybook/addon-actions"
import { Story } from "@storybook/react"
import dayjs from "dayjs"
import { canExtendDeadline, canReduceDeadline } from "util/schedule"
import * as Mocks from "../../testHelpers/entities"
import { Workspace, WorkspaceErrors, WorkspaceProps } from "./Workspace"

Expand All @@ -24,6 +26,16 @@ Started.args = {
onDeadlinePlus: () => {
// do nothing, this is just for storybook
},
deadlineMinusEnabled: () => {
return canReduceDeadline(dayjs(Mocks.MockWorkspace.latest_build.deadline))
},
deadlinePlusEnabled: () => {
return canExtendDeadline(
dayjs(Mocks.MockWorkspace.latest_build.deadline),
Mocks.MockWorkspace,
Mocks.MockTemplate,
)
},
},
workspace: Mocks.MockWorkspace,
handleStart: action("start"),
Expand Down
4 changes: 4 additions & 0 deletions site/src/components/Workspace/Workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ export interface WorkspaceProps {
scheduleProps: {
onDeadlinePlus: () => void
onDeadlineMinus: () => void
deadlinePlusEnabled: () => boolean
deadlineMinusEnabled: () => boolean
}
handleStart: () => void
handleStop: () => void
Expand Down Expand Up @@ -81,6 +83,8 @@ export const Workspace: FC<WorkspaceProps> = ({
workspace={workspace}
onDeadlineMinus={scheduleProps.onDeadlineMinus}
onDeadlinePlus={scheduleProps.onDeadlinePlus}
deadlineMinusEnabled={scheduleProps.deadlineMinusEnabled}
deadlinePlusEnabled={scheduleProps.deadlinePlusEnabled}
canUpdateWorkspace={canUpdateWorkspace}
/>
<WorkspaceActions
Expand Down
95 changes: 1 addition & 94 deletions site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ import dayjs from "dayjs"
import utc from "dayjs/plugin/utc"
import * as TypesGen from "../../api/typesGenerated"
import * as Mocks from "../../testHelpers/entities"
import {
deadlineMinusDisabled,
deadlinePlusDisabled,
shouldDisplayPlusMinus,
} from "./WorkspaceScheduleButton"
import { shouldDisplayPlusMinus } from "./WorkspaceScheduleButton"

dayjs.extend(utc)
const now = dayjs()

describe("WorkspaceScheduleButton", () => {
describe("shouldDisplayPlusMinus", () => {
Expand All @@ -29,92 +24,4 @@ describe("WorkspaceScheduleButton", () => {
expect(shouldDisplayPlusMinus(workspace)).toBeTruthy()
})
})

describe("deadlineMinusDisabled", () => {
it("should be false if the deadline is more than 30 minutes in the future", () => {
// Given: a workspace with a deadline set to 31 minutes in the future
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(31, "minutes").utc().format(),
},
}

// Then: deadlineMinusDisabled should be falsy
expect(deadlineMinusDisabled(workspace, now)).toBeFalsy()
})

it("should be true if the deadline is 30 minutes or less in the future", () => {
// Given: a workspace with a deadline set to 30 minutes in the future
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(30, "minutes").utc().format(),
},
}

// Then: deadlineMinusDisabled should be truthy
expect(deadlineMinusDisabled(workspace, now)).toBeTruthy()
})

it("should be true if the deadline is in the past", () => {
// Given: a workspace with a deadline set to 1 minute in the past
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(-1, "minutes").utc().format(),
},
}

// Then: deadlineMinusDisabled should be truthy
expect(deadlineMinusDisabled(workspace, now)).toBeTruthy()
})
})

describe("deadlinePlusDisabled", () => {
it("should be false if the deadline is less than 24 hours in the future", () => {
// Given: a workspace with a deadline set to 23 hours in the future
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(23, "hours").utc().format(),
},
}

// Then: deadlinePlusDisabled should be falsy
expect(deadlinePlusDisabled(workspace, now)).toBeFalsy()
})

it("should be true if the deadline is 24 hours or more in the future", () => {
// Given: a workspace with a deadline set to 25 hours in the future
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(25, "hours").utc().format(),
},
}

// Then: deadlinePlusDisabled should be truthy
expect(deadlinePlusDisabled(workspace, now)).toBeTruthy()
})

it("should be false if the deadline is in the past", () => {
// Given: a workspace with a deadline set to 1 minute in the past
const workspace: TypesGen.Workspace = {
...Mocks.MockWorkspace,
latest_build: {
...Mocks.MockWorkspaceBuild,
deadline: now.add(-1, "minute").utc().format(),
},
}

// Then: deadlinePlusDisabled should be falsy
expect(deadlinePlusDisabled(workspace, now)).toBeFalsy()
})
})
})
18 changes: 6 additions & 12 deletions site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,27 +40,21 @@ export const shouldDisplayPlusMinus = (workspace: Workspace): boolean => {
return deadline.year() > 1
}

export const deadlineMinusDisabled = (workspace: Workspace, now: dayjs.Dayjs): boolean => {
const delta = dayjs(workspace.latest_build.deadline).diff(now)
return delta <= 30 * 60 * 1000 // 30 minutes
}

export const deadlinePlusDisabled = (workspace: Workspace, now: dayjs.Dayjs): boolean => {
const delta = dayjs(workspace.latest_build.deadline).diff(now)
return delta >= 24 * 60 * 60 * 1000 // 24 hours
}

Comment on lines -43 to -52
Copy link
Member Author

@johnstcn johnstcn Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

review: moved to util/workspace.ts
EDIT: actually moved to util/schedule.ts which is a more sensible place for it

export interface WorkspaceScheduleButtonProps {
workspace: Workspace
onDeadlinePlus: () => void
onDeadlineMinus: () => void
deadlineMinusEnabled: () => boolean
deadlinePlusEnabled: () => boolean
canUpdateWorkspace: boolean
}

export const WorkspaceScheduleButton: React.FC<WorkspaceScheduleButtonProps> = ({
workspace,
onDeadlinePlus,
onDeadlineMinus,
deadlinePlusEnabled,
deadlineMinusEnabled,
canUpdateWorkspace,
}) => {
const anchorRef = useRef<HTMLButtonElement>(null)
Expand All @@ -81,7 +75,7 @@ export const WorkspaceScheduleButton: React.FC<WorkspaceScheduleButtonProps> = (
<IconButton
className={styles.iconButton}
size="small"
disabled={deadlineMinusDisabled(workspace, dayjs())}
disabled={!deadlineMinusEnabled()}
onClick={onDeadlineMinus}
>
<Tooltip title={Language.editDeadlineMinus}>
Expand All @@ -91,7 +85,7 @@ export const WorkspaceScheduleButton: React.FC<WorkspaceScheduleButtonProps> = (
<IconButton
className={styles.iconButton}
size="small"
disabled={deadlinePlusDisabled(workspace, dayjs())}
disabled={!deadlinePlusEnabled()}
onClick={onDeadlinePlus}
>
<Tooltip title={Language.editDeadlinePlus}>
Expand Down
8 changes: 6 additions & 2 deletions site/src/pages/WorkspacePage/WorkspacePage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ import { WorkspacePage } from "./WorkspacePage"

// It renders the workspace page and waits for it be loaded
const renderWorkspacePage = async () => {
const getTemplateMock = jest.spyOn(api, "getTemplate").mockResolvedValueOnce(MockTemplate)
renderWithAuth(<WorkspacePage />, {
route: `/@${MockWorkspace.owner_name}/${MockWorkspace.name}`,
path: "/@:username/:workspace",
})
await screen.findByText(MockWorkspace.name)
expect(getTemplateMock).toBeCalled()
}

/**
Expand All @@ -50,10 +52,10 @@ const testButton = async (label: string, actionMock: jest.SpyInstance) => {
expect(actionMock).toBeCalled()
}

const testStatus = async (mock: Workspace, label: string) => {
const testStatus = async (ws: Workspace, label: string) => {
server.use(
rest.get(`/api/v2/users/:username/workspace/:workspaceName`, (req, res, ctx) => {
return res(ctx.status(200), ctx.json(mock))
return res(ctx.status(200), ctx.json(ws))
}),
)
await renderWorkspacePage()
Expand Down Expand Up @@ -181,6 +183,7 @@ describe("Workspace Page", () => {

describe("Resources", () => {
it("shows the status of each agent in each resource", async () => {
const getTemplateMock = jest.spyOn(api, "getTemplate").mockResolvedValueOnce(MockTemplate)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally better to test things that are apparent to the user than internals like whether a function was called, but sometimes it's necessary. Is this one of the cases where testing user-apparent effects is too hard?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure -- I'll try seeing if I can make some assertions about child components here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it seems like testing properties of child components is considered somewhat of an anti-pattern. I'll factor some more stuff out to a util file and test it separately.

renderWithAuth(<WorkspacePage />, {
route: `/@${MockWorkspace.owner_name}/${MockWorkspace.name}`,
path: "/@:username/:workspace",
Expand All @@ -197,6 +200,7 @@ describe("Workspace Page", () => {
DisplayAgentStatusLanguage[MockWorkspaceAgentDisconnected.status],
)
expect(agent2Status.length).toEqual(2)
expect(getTemplateMock).toBeCalled()
})
})
})
31 changes: 16 additions & 15 deletions site/src/pages/WorkspacePage/WorkspacePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { FullScreenLoader } from "../../components/Loader/FullScreenLoader"
import { Workspace, WorkspaceErrors } from "../../components/Workspace/Workspace"
import { firstOrItem } from "../../util/array"
import { pageTitle } from "../../util/page"
import { canExtendDeadline, canReduceDeadline, maxDeadline, minDeadline } from "../../util/schedule"
import { getFaviconByStatus } from "../../util/workspace"
import { selectUser } from "../../xServices/auth/authSelectors"
import { XServiceContext } from "../../xServices/StateContext"
Expand All @@ -35,6 +36,8 @@ export const WorkspacePage: React.FC = () => {
const {
workspace,
getWorkspaceError,
template,
refreshTemplateError,
resources,
getResourcesError,
builds,
Expand Down Expand Up @@ -63,12 +66,16 @@ export const WorkspacePage: React.FC = () => {
return (
<div className={styles.error}>
{getWorkspaceError && <ErrorSummary error={getWorkspaceError} />}
{refreshTemplateError && <ErrorSummary error={refreshTemplateError} />}
{checkPermissionsError && <ErrorSummary error={checkPermissionsError} />}
</div>
)
} else if (!workspace) {
return <FullScreenLoader />
} else if (!template) {
return <FullScreenLoader />
} else {
const deadline = dayjs(workspace.latest_build.deadline).utc()
const favicon = getFaviconByStatus(workspace.latest_build)
return (
<>
Expand All @@ -85,7 +92,7 @@ export const WorkspacePage: React.FC = () => {
bannerSend({
type: "UPDATE_DEADLINE",
workspaceId: workspace.id,
newDeadline: dayjs(workspace.latest_build.deadline).utc().add(4, "hours"),
newDeadline: dayjs.min(deadline.add(4, "hours"), maxDeadline(workspace, template)),
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to change this here, but I'm wondering if we want to try to push logic like this into XState. Instead of one UPDATE_DEADLINE event, we could have multiple events that do calculations within them. The idea would be to try to get more logic out of components. Maybe a topic for Frontend Variety.

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, I was wondering that but didn't want to over-complicate the machine. :-)

})
},
}}
Expand All @@ -94,22 +101,22 @@ export const WorkspacePage: React.FC = () => {
bannerSend({
type: "UPDATE_DEADLINE",
workspaceId: workspace.id,
newDeadline: boundedDeadline(
dayjs(workspace.latest_build.deadline).utc().add(-1, "hours"),
dayjs(),
),
newDeadline: dayjs.max(deadline.add(-1, "hours"), minDeadline()),
})
},
onDeadlinePlus: () => {
bannerSend({
type: "UPDATE_DEADLINE",
workspaceId: workspace.id,
newDeadline: boundedDeadline(
dayjs(workspace.latest_build.deadline).utc().add(1, "hours"),
dayjs(),
),
newDeadline: dayjs.min(deadline.add(1, "hours"), maxDeadline(workspace, template)),
})
},
deadlineMinusEnabled: () => {
return canReduceDeadline(deadline)
},
deadlinePlusEnabled: () => {
return canExtendDeadline(deadline, workspace, template)
},
}}
workspace={workspace}
handleStart={() => workspaceSend("START")}
Expand Down Expand Up @@ -139,12 +146,6 @@ export const WorkspacePage: React.FC = () => {
}
}

export const boundedDeadline = (newDeadline: dayjs.Dayjs, now: dayjs.Dayjs): dayjs.Dayjs => {
const minDeadline = now.add(30, "minutes")
const maxDeadline = now.add(24, "hours")
return dayjs.min(dayjs.max(minDeadline, newDeadline), maxDeadline)
}

const useStyles = makeStyles((theme) => ({
error: {
margin: theme.spacing(2),
Expand Down
4 changes: 2 additions & 2 deletions site/src/testHelpers/entities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ export const MockTemplate: TypesGen.Template = {
active_version_id: MockTemplateVersion.id,
workspace_owner_count: 1,
description: "This is a test description.",
max_ttl_ms: 604800000,
min_autostart_interval_ms: 3600000,
max_ttl_ms: 24 * 60 * 60 * 1000,
min_autostart_interval_ms: 60 * 60 * 1000,
created_by_id: "test-creator-id",
created_by_name: "test_creator",
}
Expand Down
Loading