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

johnstcn · 2022-08-17T15:35:54Z

chore: WorkspacePage: invert workspace schedule bumper logic for readibility
fix: ui: workspace bumpers now honour template max_ttl
chore: refactor workspace schedule bumper logic mostly into util/schedule.ts and unit test separately

Verification Steps:

Create and start a workspace from a template that has a max_ttl of less than 24 hours and has autostop enabled.
Repeatedly click the ➕ icon by the schedule -> it should not allow extending the workspace deadline past template max_ttl
Repeatedly click the ➖ icon by the schedule -> it should not allow decreasing the workspace deadline before now plus 30 minutes

johnstcn · 2022-08-18T14:08:33Z

site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx

-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
-}
-


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

presleyp · 2022-08-18T15:57:18Z

site/src/pages/WorkspacePage/WorkspacePage.tsx

        {checkPermissionsError && <ErrorSummary error={checkPermissionsError} />}
      </div>
    )
  } else if (!workspace) {
    return <FullScreenLoader />
+  } else if (!template) {
+    return <div className={styles.error}>Loading template</div>


I think this should be treated as a loading state like !workspace. I'm assuming that if fetching it failed, we'll have an error.

That's fair. Although I'd really like to be able to add information to FullScreenLoader on what's currently happening (e.g. Reticulating Splines)

We could add a prop to FullScreenLoader that can take that info?

That sounds like a good idea. I'll open a separate issue for that.

presleyp · 2022-08-18T16:42:32Z

site/src/pages/WorkspacePage/WorkspacePage.test.tsx

@@ -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)


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?

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

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.

presleyp · 2022-08-18T16:47:21Z

site/src/pages/WorkspacePage/WorkspacePage.tsx

@@ -85,7 +91,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)),


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.

Yeah, I was wondering that but didn't want to over-complicate the machine. :-)

presleyp · 2022-08-18T16:53:39Z

site/src/util/workspace.ts

@@ -1,10 +1,14 @@
 import { Theme } from "@material-ui/core/styles"
 import dayjs from "dayjs"
+import duration from "dayjs/plugin/duration"


Love that you put util functions in their own spot and tested them! I noticed when I was working on the schedule form that there are a lot of different places for schedule-related util functions. Any thoughts on that? I don't want to make you figure it all out in this PR but I might at least call this util file something about the schedule instead of just the workspace.

IMO I think it makes sense to keep it in the workspace util file, as workspaces and schedules are tightly coupled. If we eventually move schedules into more of a separate entity of their own, then maybe I could see it making more sense.

EDIT: Silly me, I forgot about util/schedule.ts. Moving things in there.

presleyp

Great work! The loading state is the only thing I feel strongly about.

…ibility

johnstcn self-assigned this Aug 17, 2022

johnstcn commented Aug 18, 2022

View reviewed changes

johnstcn marked this pull request as ready for review August 18, 2022 15:05

johnstcn requested a review from a team as a code owner August 18, 2022 15:05

johnstcn changed the title ~~WIP: fix: ui: workspace bumpers now honour template max_ttl~~ fix: ui: workspace bumpers now honour template max_ttl Aug 18, 2022

presleyp reviewed Aug 18, 2022

View reviewed changes

johnstcn mentioned this pull request Aug 18, 2022

Add option message prop to FullScreenLoader #3554

Closed

presleyp reviewed Aug 18, 2022

View reviewed changes

presleyp approved these changes Aug 18, 2022

View reviewed changes

johnstcn mentioned this pull request Aug 18, 2022

Workspace schedule bumpers are broken #2764

Closed

johnstcn added 15 commits August 18, 2022 22:17

chore: WorkspacePage: invert workspace schedule bumper logic for read…

5eb9317

…ibility

fix: ui: workspace bumpers now honour template max_ttl

c57fed2

extract to function

7bdf392

add tests for deadlineMax, deadlineMin

35e5146

wip

0c24962

move deadline-related stuff to util/workspace.ts

28220d5

fix unit tests, working around dayjs plugin silliness

c68b931

fix dayjs plugin silliness the right way

494213f

linter

9c8d4d5

yarn fumpt

ca8b95f

i deserve to be mocked

4aa6eaf

move schedule-related util functions to util/schedule

ff50a83

use FullScreenLoader if template is null

b8ada49

extract [-+] disable logic to util/schedule, add unit tests

f9832b1

fix storybook

e69ff7f

johnstcn force-pushed the cj/gh-2764/fix-workspace-bumpers branch from 3a16557 to e69ff7f Compare August 18, 2022 22:17

johnstcn merged commit 04c5f92 into main Aug 18, 2022

johnstcn deleted the cj/gh-2764/fix-workspace-bumpers branch August 18, 2022 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johnstcn commented Aug 17, 2022 •

edited

Loading

johnstcn Aug 18, 2022 •

edited

Loading

presleyp Aug 18, 2022

johnstcn Aug 18, 2022

presleyp Aug 18, 2022

johnstcn Aug 18, 2022

presleyp Aug 18, 2022

johnstcn Aug 18, 2022

johnstcn Aug 18, 2022

presleyp Aug 18, 2022

johnstcn Aug 18, 2022

presleyp Aug 18, 2022

johnstcn Aug 18, 2022 •

edited

Loading

presleyp left a comment

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

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

Conversation

johnstcn commented Aug 17, 2022 • edited Loading

johnstcn Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

presleyp left a comment

Choose a reason for hiding this comment

johnstcn commented Aug 17, 2022 •

edited

Loading

johnstcn Aug 18, 2022 •

edited

Loading

johnstcn Aug 18, 2022 •

edited

Loading