Movatterモバイル変換

refactor: resource strings in WorkspaceSchedule#1702

Merged

greyscaled commented

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx OutdatedShow resolvedHide resolved

greyscaled commented

site/src/xServices/workspaceSchedule/workspaceScheduleXService.tsShow resolvedHide resolved

greyscaled requested a review fromjohnstcn

May 24, 2022 06:05

johnstcn reviewed

site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx OutdatedShow resolvedHide resolved

johnstcn reviewed

site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx OutdatedShow resolvedHide resolved

johnstcn reviewed

site/src/xServices/workspaceSchedule/workspaceScheduleXService.tsShow resolvedHide resolved

johnstcn reviewed

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

Looking good so far! Comments below.

kylecarbs reviewed

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but skipping a thorough review until in a ready state 😄!

What do you think about Schedule being an option in the Settings pages? It could be helpful to the user for all editable options to be in one spot!

Copy link

ContributorAuthor

greyscaled commentedMay 24, 2022

What do you think about Schedule being an option in the Settings pages? It could be helpful to the user for all editable options to be in one spot!

In the short term, I don't want to do that for the reasons of:

we don't yet have the settings page or a design or really any thoughts of how it will look
the schedule form is complex and is represented as a full-page form for that reason right now

I do think it's the right call in the long(er) term, longer here is relative - could even be as early as next sprint (week). I don't want to pause getting it in here as our current workspace design has a sidebar that links out to the schedule page, and it fits in really nicely.

If we decide we have cycles to design the settings page, I'd absolutely believe this could be a candidate.

Copy link

Member

kylecarbs commentedMay 24, 2022

Makes complete sense to me! Appreciate hearing the thought process 💡

johnstcn mentioned this pull request

Codersdk/ts: migrate workspace ttl to ttl_ms#1707

Closed

Copy link

Member

ammario commentedMay 24, 2022

@vapurrmaid could you share what it looks like please?

Copy link

Member

johnstcn commentedMay 24, 2022

@ammario you can check theStorybook UI review

greyscaled self-assigned this

greyscaled mentioned this pull request

Add retry to frontend API calls#912

Closed

2 tasks

greyscaled added6 commits

May 24, 2022 14:53

Merge branch 'main' into vapurrmaid/gh-1455/part-3/page

30dff81

remove promise

ecc6792

Merge origin/main

c0f28c3

refactor to map + add loading/disabled

d61a332

time validation

f60f59b

more tests

406d465

greyscaled commented

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsx OutdatedShow resolvedHide resolved

greyscaled commented

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx OutdatedShow resolvedHide resolved

Update site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx

a6dff9d

Copy link

Member

ammario commentedMay 25, 2022•
edited
Loading

@johnstcn thanks for sharing the Storybook.

Why did we omit the timezone field?
Why was "runtime" selected instead of "TTL"?
Why is runtime in minutes instead of hours?

Copy link

ContributorAuthor

greyscaled commentedMay 25, 2022

@johnstcn thanks for sharing the Storybook.
Why did we omit the timezone field?
Why was "runtime" selected instead of "TTL"?
Why is runtime in minutes instead of hours?

There are no good reasons other than the initial draft, followed by iteration.

I'll change from runtime to TTL.

I'll change from minutes to hours.

I'll add a timezone field

Copy link

Member

ammario commentedMay 25, 2022

Sounds good@vapurrmaid. I think the UI is clean and professional and I'm excited to give it a swing!

greyscaled commented

site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx

Copy link

ContributorAuthor

greyscaled left a comment•
edited
Loading

Choose a reason for hiding this comment

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

This PR is unfortunately large (though a lot of the code is test code, thankfully).

I am addingREVIEW comments to most diffs to add further explanation/context.

I'm happy to do a synchronous review in Discord if it's warranted.

Sorry that this one got large, I like to find smaller pieces, but this is just how things are as CE deadline approaches. Many thanks in advance 💖 .

Edit: Also, if you're newer to cron strings,https://crontab.guru/#30_09_*_*_1-5 is the way to go

Edit: If you're wondering why we don't use a cron library for some of our manipulation, it's because we have a custom implementation right now and do not want to expand it to full blast just yet.

Edit: This PR grew way larger than I had intended. I'm happy to address all comments, but my default response to things like refactors, improvements, nice-to-haves will be "iteration", as in follow-up efforts.

@johnstcn and I already have various follow-up efforts, like changing thettl from nanosecond precision. Some of the messy hours to nanosecond time conversion will get cleaned up then.

See also@ammario and I's awesome comment thread and a consensus here:#1701 (comment) that we want to give the user a diff-like preview in this form while editing it. That may not land in CE (I'll certainly try), as there are other priorities like notifications and extension that must come first.

site/src/AppRouter.tsx

		<WorkspaceSchedulePage />
		</RequireAuth>
		}
		/>

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

A route/workspaces/:workspace/schedule was added to view the form

		<Link className={styles.scheduleAction}>{Language.editScheduleLink}</Link>
		<Link className={styles.scheduleAction} component={RouterLink} to={`/workspaces/${workspace.id}/schedule`}>
		{Language.editScheduleLink}
		</Link>

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review: The dashboard link to edit a workspace schedule now works!

site/src/components/WorkspaceStats/WorkspaceScheduleForm.stories.tsx

		action("onSubmit")
		return Promise.resolve()
		},
		onSubmit: () => action("onSubmit"),

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

You'll see this show up in theWorkspaceScheduleForm component as well: I un-promisifiedonSubmit. I did this because the model goes to a new state once the promises resolve and the API calls are success. We use that state to navigate away from the page.

In other words, there is a bit of a 'fire-n-forget' from Formik's point of view, but since we also pass inisLoading to the form, which stems from the model, it still works out OK.

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsx

		{form.errors.monday && <FormHelperText>{Language.errorNoDayOfWeek}</FormHelperText>}
		</FormControl>

		<TextField
		{...formHelpers("ttl", Language.ttlHelperText)}
		inputProps={{ min: 0, step: 30 }}
		disabled={form.isSubmitting \|\| isLoading}
		inputProps={{ min: 0, step: 1 }}

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

We changed from minutes to hours

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsx

		{ value: form.values.thursday, name: "thursday", label: Language.dayThursdayLabel },
		{ value: form.values.friday, name: "friday", label: Language.dayFridayLabel },
		{ value: form.values.saturday, name: "saturday", label: Language.daySaturdayLabel },
		]

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

I refactored to use amap

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsx

Comment on lines +94 to +107

		startTime: Yup.string()
		.ensure()
		.test("is-time-string", Language.errorTime, (value) => {
		if (value === "") {
		return true
		} else if (!/^[0-9][0-9]:[0-9][0-9]$/.test(value)) {
		return false
		} else {
		const parts = value.split(":")
		const HH = Number(parts[0])
		const mm = Number(parts[1])
		return HH >= 0 && HH <= 23 && mm >= 0 && mm <= 59
		}
		}),

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

We had no tests or validation onstartTime previously. See related tests inWorkspaceScheduleForm.test.tsx.

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsx

		@@ -45,6 +62,7 @@ export interface WorkspaceScheduleFormValues {
		saturday: boolean

		startTime: string
		timezone: string

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

The form now has a newtimezone input, with validation and tests.

site/src/util/formUtils.tsx OutdatedShow resolvedHide resolved

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx OutdatedShow resolvedHide resolved

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.test.tsx OutdatedShow resolvedHide resolved

greyscaled added2 commits

May 26, 2022 02:03

Apply suggestions from code review

5bacfb1

Update site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tes…

5b3adc3

…t.tsx

greyscaled requested review fromjohnstcn,ammario andkylecarbs

May 26, 2022 06:04

fix ttl initialization

81e7e05

greyscaled commented

site/src/xServices/workspaceSchedule/workspaceScheduleXService.ts

Comment on lines +1 to +4

		/**
		* @fileoverview workspaceSchedule is an xstate machine backing a form to CRUD
		* an individual workspace's schedule.
		*/

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

Here's a GIF of me stepping through the machine states/possibilities. Please LMK if there are any questions.

To attempt and get ahead, I added a final statesubmitSuccess that's essentially a dupe of theidle state, but the page is using it to know tonavigate.

Copy link

Member

Kira-PilotMay 26, 2022

Choose a reason for hiding this comment

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

This was so helpful; thank you!

greyscaled commented

site/src/util/schedule.ts

		* The 0th index is Sunday
		* The 6th index is Saturday
		*/
		export type WeeklyFlag = [boolean, boolean, boolean, boolean, boolean, boolean, boolean]

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

This type is very helpful and needed because we get some static analysis on the size of the arrays. The semantic meaning is like a bit mask where in the array, each index corresponds to a day and whether that day is "set" (true) or unset "false".

So in other words, it goes from Sunday -> Saturday.

This sort of thing is easier to understand than bit masks, and maps very cleanly to setting values for the checkbox group.

greyscaled commented

site/src/util/schedule.ts

		}

		return result
		}

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

This is a "private" method (only meant to be used indowToWeeklyFlag and thus is not exported.

I'd review this before I'd reviewdowToWeeklyFlag.

The algorithm does the following:

takes a number (ex: 1)
or number range (ex: 1-5)

and returns the associatedWeeklyFlag array (see:https://github.com/coder/coder/pull/1701/files#r882360151).

I have a method that focuses just on number or range, because in the parent functiondowToWeeklyFlag, you can have comma-separated values like this:

1,3,5
1-2,4-5

So this function is called in a loop on each comma-separated group.

greyscaled commented

site/src/util/schedule.ts

		}

		return results
		}

Copy link

ContributorAuthor

greyscaledMay 26, 2022

Choose a reason for hiding this comment

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

Review:

Before reading this function, I'd readhttps://github.com/coder/coder/pull/1701/files#r882361592

This builds uponprocessRangeOrNum by calling it in a loop after splitting the dow cron string by comma.

greyscaled commented

site/src/util/schedule.test.tsShow resolvedHide resolved

Update site/src/util/schedule.test.ts

531df3e

johnstcn approved these changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

Looking good!

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx OutdatedShow resolvedHide resolved

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsxShow resolvedHide resolved

Kira-Pilot reviewed

site/src/components/WorkspaceStats/WorkspaceScheduleForm.tsxShow resolvedHide resolved

Kira-Pilot reviewed

site/src/util/schedule.ts OutdatedShow resolvedHide resolved

Fix typo

7ae590a

Co-authored-by: Kira Pilot <kira@coder.com>

kylecarbs approved these changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

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

Very thorough comments and tests!

site/src/util/schedule.tsShow resolvedHide resolved

Copy link

Member

Kira-Pilot commentedMay 26, 2022

I noticed that I can blissfully keyboard shortcut my way through the form until I get to the Days of Week checklist; then I must use a mouse. Accessibility probably isn't a huge priority rn, but would be nice to revisit this at some time.

Fantastic work! Also great job leaving comments/screenshots throughout the PR - that made it much easier to review.

Kira-Pilot approved these changes

Copy link

ContributorAuthor

greyscaled commentedMay 26, 2022

keyboard shortcut my way through the form until I get to the Days of Week checklist; then I must use a mouse.

Interesting, I wonder why that is - I will look into this and see if I can figure it out. If I get stuck spinning my wheels, I'll log a ticket and move on.

The code I was using was adapted from:https://v4.mui.com/components/checkboxes/#checkboxes-with-formgroup which means their example must not be accessible either (odd).

Either way, thank you for finding this, that's a wonderful thing to test/review!!!!

greyscaled added4 commits

May 26, 2022 15:45

import ReactNode directly

262d9e3

guess timezone

5d22197

fix test

eda8ad8

lint

f59f056

greyscaled merged commit7c59ec4 intomain