feat: Add switches for auto-start and auto-stop #3358

presleyp · 2022-08-02T14:57:18Z

Refactors scheduling util functions to separate auto-start from auto-stop since they are now independently enable-able
Changes default behavior: if the API gives no schedule, assume that's meaningful and present features as disabled. But if the user disables and re-enables, repopulate the data from the API.
Puts auto-start and auto-stop in sections
Adds a switch to disable each section

I don't love the useEffect but I'm not sure it's worth refactoring to put more logic in the xservice - open to input.
I'd like to have integration tests but they run into routing problems.

Testing

Given I am on the schedule page
When I turn a switch on,
Then the controls in that section become enabled
When I turn a switch off,
Then the controls in that section become disabled (but its values aren't cleared).

Given I save the form with one or both sections disabled,
When I go back to the form from the Workspace page,
Then I see the previously disabled section is still disabled, and its values are cleared (empty, unchecked, or 0).
When I enable Auto-start,
Then I see the default values (9:30 AM, my current timezone, M-F) selected.
When I enable Auto-stop,
Then I see the default value (8) in the input.

Given I enable both sections, give non-default inputs, and save the form,
When I go back to the form from the Workspace page,
Then I see the sections are still enabled and still show the values I selected.
When I disable the sections and then enable them again,
Then I see the values I selected (not the default values).

johnstcn

Looks good to me from the backend perspective!

Kira-Pilot · 2022-08-02T16:51:26Z

What do you think about restricting the user from adding '0' here? When a user does that, saves, and then toggles the switch, the default comes back anyway. I wonder if it would cause less confusion to validate against this case in the same way we do with start time.

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx

Kira-Pilot

Left some comments but overall I think this looks great!

presleyp · 2022-08-08T18:47:23Z

@Kira-Pilot fyi on the border - other uses of Section didn't have a border either, so I just added it in the Section component.

presleyp · 2022-08-08T19:10:09Z

@Kira-Pilot @ammario I changed auto-stop to disallow 0 if it's enabled, and I'm thinking maybe auto-start should work the same way. Currently it's like, you can't have no days checked if you have a start time, but now that there's a switch I figure the logic should be, you have to have a time and at least one day if the switch is on. Does that sound right to you?

ammario · 2022-08-09T15:31:15Z

When I toggle the Auto-start or Auto-stop buttons, the old form inputs disappear, which surprises me.
I'm seeing a validation error when disabling auto off:

presleyp · 2022-08-09T15:51:09Z

@ammario I'm working on the validation one, and I've found a resetting bug but I'm not sure it's the same one you're describing. I intend for this behavior to happen:

Given Auto-start is enabled and I've entered days and a time,
When I disable Auto-start,
Then the days and time are cleared.

But I consider this a bug:

Given Auto-start and Auto-stop are enabled and I've entered a number of hours until shutdown,
When I disable Auto-start (not Auto-stop),
Then the number of hours until shutdown is reset to whatever value was pulled from the backend before I set it.

If you think the former case is a problem, let me know!

ammario · 2022-08-09T16:07:31Z

@ammario I'm working on the validation one, and I've found a resetting bug but I'm not sure it's the same one you're describing. I intend for this behavior to happen:

Given Auto-start is enabled and I've entered days and a time, When I disable Auto-start, Then the days and time are cleared.

The behavior doesn't surprise me. It's when you enable Auto-start again and then the days and time are reset to system default that surprises me. I would expect the days and time to be set to what they were right before I disabled Auto-start.

But I consider this a bug:

Given Auto-start and Auto-stop are enabled and I've entered a number of hours until shutdown, When I disable Auto-start (not Auto-stop), Then the number of hours until shutdown is reset to whatever value was pulled from the backend before I set it.

If you think the former case is a problem, let me know!

Yes I agree this is a surprising behavior.

presleyp · 2022-08-09T18:05:05Z

@ammario okay, it sounds like we mostly agree but there may be one place we're not on the same page: say my Auto-start is set to Mondays at 9am. I go to my schedule to add Tuesdays. But before submitting, I turn Auto-start off, and then back on again. This PR will reset your form to Mondays. I think you're saying it should show Mondays and Tuesdays.

My argument for Mondays is that that's your current actual schedule, so when you come back from nothing, you should start there. But I see the argument that you should just go back in time to before you disabled Auto-start. I think implementing it to go back in time to Mondays and Tuesdays will be harder than resetting to Mondays. How strongly do you feel about it?

ammario · 2022-08-09T22:29:39Z

I don't feel that strongly about. This PR is an improvement on what exists today so it's mergeable as far as I'm concerned :)

presleyp · 2022-08-10T14:48:21Z

@ammario @Kira-Pilot I'm close to putting up changes that will persist form state while a section is disabled (easier than I expected!) and fix the bug where one section affects the other. But I'm realizing having the form state persisted might be confusing to the user. The values are still visible when I disable, and so for instance the stop section says "Your workspace will shut down after n hours" instead of "Your workspace will not automatically shut down." But if I submit the form with Auto-stop disabled, my workspace will not automatically shut down. What do you think, confusing or fine?

Kira-Pilot · 2022-08-10T16:52:51Z

@ammario @Kira-Pilot What do you think, confusing or fine?

I think that is potentially confusing. Can we just clear the validation when they disable? Something like
formik.setTouched({}, false) ?

presleyp · 2022-08-10T20:04:17Z

@Kira-Pilot removed helper text when a section is disabled so that the grey text isn't lying lol

This reverts commit a6271ca.

presleyp · 2022-08-10T20:12:20Z

@Kira-Pilot just kidding, that made the helper text override the error text so I reverted it. I'll make a ticket for figuring out the UX.

Kira-Pilot · 2022-08-10T21:10:02Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

+  const handleToggleAutoStart = async (e: ChangeEvent) => {
+    form.handleChange(e)
+    // if enabling from empty values, fill with defaults
+    if (!form.values.autoStartEnabled && !form.values.startTime) {


are we checking !form.values.startTime here to ensure the user didn't fill something out previously?

Yes, so we don't overwrite your values with defaults.

Kira-Pilot · 2022-08-10T21:16:30Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

+        await form.setFieldValue(name, defaults[name as keyof AutoStartSchedule])
+      })
+      await form.setFieldValue("startTime", defaults.startTime)
+      await form.setFieldValue("timezone", defaults.timezone)


We might be able to setValues here and handle these setters in an object: https://formik.org/docs/api/formik#setvalues-fields-reactsetstateaction-field-string-any--shouldvalidate-boolean--void

Yay, thanks!

Kira-Pilot · 2022-08-10T21:19:03Z

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

  startTime: string
  timezone: string
+
+  autoStopEnabled: boolean


Sorry, I know we chatted about this yesterday, but why are we including autoStartEnabled and autoStopEnabled as form values instead of stateful variables? The API doesn't have a concept of these values, right?

Right. I just thought it was a little cleaner this way:

validation can go back to being an object instead of a function

all fields in the form are handled by the form

now that I'm persisting pre-submission form state, the formToRequest functions need to refer to the enabled values to decide whether to send the backend falsey values

And the formToRequest functions just create values to return rather than passing everything through, so there wasn't a real downside to having extra data in the form.

Sounds good, thanks!

Kira-Pilot · 2022-08-10T21:24:06Z

Two small UI things that do not have to be addressed in this PR:
I noticed a flicker on input border when fields are disabled:

Sometimes "Days of Week" gets a blue highlight:

These are not blockers!! Just noting for posterity.

Kira-Pilot · 2022-08-10T21:24:40Z

Nice job! This was complicated. I left some questions but as far as I'm concerned, this is good to be merged up. Looks great when I run it locally. Thanks for seeing it through!

presleyp added 9 commits July 29, 2022 19:59

Add elements

54bf130

Add Loading story

ecc1137

Make form show empty values when manual

6705076

Make form depend on switches

91259c4

Fix style

70228bd

Format

52b26bf

Update unit tests

b224258

Tweaks

4f68bb1

Update storybook

ff48adf

presleyp marked this pull request as ready for review August 2, 2022 14:58

presleyp requested a review from a team as a code owner August 2, 2022 14:58

presleyp requested review from johnstcn and a team and removed request for a team August 2, 2022 15:22

johnstcn approved these changes Aug 2, 2022

View reviewed changes

Kira-Pilot reviewed Aug 2, 2022

View reviewed changes

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx Outdated Show resolved Hide resolved

Kira-Pilot reviewed Aug 2, 2022

View reviewed changes

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx Outdated Show resolved Hide resolved

Kira-Pilot reviewed Aug 2, 2022

View reviewed changes

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx Show resolved Hide resolved

Kira-Pilot reviewed Aug 2, 2022

View reviewed changes

site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx Outdated Show resolved Hide resolved

Kira-Pilot approved these changes Aug 2, 2022

View reviewed changes

ammario self-requested a review August 4, 2022 16:16

presleyp added 3 commits August 8, 2022 18:21

Move util files

bbaee7d

Pull out more util functions

f251efa

Pull out strings

eab6e5f

presleyp added 3 commits August 8, 2022 18:47

Add border to section

e92a2c1

Make min ttl 1

a35bf20

Format

750f08b

presleyp added 2 commits August 8, 2022 19:59

Fix import

3dfa41b

Fix validation for falsey values

848732a

Format and fix tests

bfbabe1

presleyp added 3 commits August 10, 2022 19:44

Put switches in form, persist form state

d1420de

Fix bug

0577ea9

Remove helper text when disabled

a6271ca

presleyp added 2 commits August 10, 2022 20:11

Fix storybook

af4e8e4

Revert "Remove helper text when disabled"

e8ae5ba

This reverts commit a6271ca.

Format

4993c3d

presleyp mentioned this pull request Aug 10, 2022

Improve UX of disabled auto-start/stop #3463

Closed

Kira-Pilot reviewed Aug 10, 2022

View reviewed changes

presleyp added 2 commits August 10, 2022 21:41

Use nicer function to set values

a17dc2e

Format

bf22caa

presleyp enabled auto-merge (squash) August 10, 2022 21:54

presleyp merged commit 5d40b1f into main Aug 10, 2022

presleyp deleted the disable-auto-start/presleyp/3051 branch August 10, 2022 22:03

johnstcn mentioned this pull request Aug 15, 2022

Workspace schedule form: 8h is displayed for workspaces with "Shutdown: Manual" when open schedule for edit #2237

Closed

feat: Add switches for auto-start and auto-stop #3358

feat: Add switches for auto-start and auto-stop #3358

Uh oh!

Conversation

presleyp commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot commented Aug 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

presleyp commented Aug 8, 2022

Uh oh!

presleyp commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ammario commented Aug 9, 2022

Uh oh!

presleyp commented Aug 9, 2022

Uh oh!

ammario commented Aug 9, 2022

Uh oh!

presleyp commented Aug 9, 2022

Uh oh!

ammario commented Aug 9, 2022

Uh oh!

presleyp commented Aug 10, 2022

Uh oh!

Kira-Pilot commented Aug 10, 2022

Uh oh!

presleyp commented Aug 10, 2022

Uh oh!

presleyp commented Aug 10, 2022

Uh oh!

Kira-Pilot Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

presleyp Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

presleyp Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

presleyp Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot commented Aug 10, 2022

Uh oh!

Kira-Pilot commented Aug 10, 2022

Uh oh!

Uh oh!

presleyp commented Aug 2, 2022 •

edited

Loading

presleyp commented Aug 8, 2022 •

edited

Loading