Skip to content
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

Jump to bottom

Make settings reset button only reset current level #3855

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Make settings reset button only reset current level #3855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b70ffc
Update test to expect new behavior (failing)
franknoirot Sep 10, 2024
979f349
Update behavior to match new test expectations
franknoirot Sep 10, 2024
4cd570b
Make reset button more clear
franknoirot Sep 10, 2024
e54d44e
Fix eslint issue
franknoirot Sep 10, 2024
08dff7d
Fix up separate test that relied on old reset logic
franknoirot Sep 10, 2024
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
113 changes: 66 additions & 47 deletions e2e/playwright/testing-settings.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
tearDown,
executorInputPath,
} from './test-utils'
import { SaveSettingsPayload } from 'lib/settings/settingsTypes'
import { SaveSettingsPayload, SettingsLevel } from 'lib/settings/settingsTypes'
import { TEST_SETTINGS_KEY, TEST_SETTINGS_CORRUPTED } from './storageStates'
import * as TOML from '@iarna/toml'

Expand Down Expand Up @@ -154,29 +154,33 @@ test.describe('Testing settings', () => {

test('Project and user settings can be reset', async ({ page }) => {
const u = await getUtils(page)
await page.setViewportSize({ width: 1200, height: 500 })
await u.waitForAuthSkipAppStart()
await page
.getByRole('button', { name: 'Start Sketch' })
.waitFor({ state: 'visible' })
await test.step(`Setup`, async () => {
await page.setViewportSize({ width: 1200, height: 500 })
await u.waitForAuthSkipAppStart()
})

// Selectors and constants
const projectSettingsTab = page.getByRole('radio', { name: 'Project' })
const userSettingsTab = page.getByRole('radio', { name: 'User' })
const resetButton = page.getByRole('button', {
name: 'Restore default settings',
})
const resetButton = (level: SettingsLevel) =>
page.getByRole('button', {
name: `Reset ${level}-level settings`,
})
const themeColorSetting = page.locator('#themeColor').getByRole('slider')
const settingValues = {
default: '259',
user: '120',
project: '50',
}
const resetToast = (level: SettingsLevel) =>
page.getByText(`${level}-level settings were reset`)

// Open the settings modal with lower-right button
await page.getByRole('link', { name: 'Settings' }).last().click()
await expect(
page.getByRole('heading', { name: 'Settings', exact: true })
).toBeVisible()
await test.step(`Open the settings modal`, async () => {
await page.getByRole('link', { name: 'Settings' }).last().click()
await expect(
page.getByRole('heading', { name: 'Settings', exact: true })
).toBeVisible()
})

await test.step('Set up theme color', async () => {
// Verify we're looking at the project-level settings,
Expand All @@ -195,37 +199,40 @@ test.describe('Testing settings', () => {

await test.step('Reset project settings', async () => {
// Click the reset settings button.
await resetButton.click()
await resetButton('project').click()

await expect(page.getByText('Settings restored to default')).toBeVisible()
await expect(
page.getByText('Settings restored to default')
).not.toBeVisible()
await expect(resetToast('project')).toBeVisible()
await expect(resetToast('project')).not.toBeVisible()

// Verify it is now set to the inherited user value
await expect(themeColorSetting).toHaveValue(settingValues.default)
await expect(themeColorSetting).toHaveValue(settingValues.user)

// Check that the user setting also rolled back
await userSettingsTab.click()
await expect(themeColorSetting).toHaveValue(settingValues.default)
await projectSettingsTab.click()
await test.step(`Check that the user settings did not change`, async () => {
await userSettingsTab.click()
await expect(themeColorSetting).toHaveValue(settingValues.user)
})

// Set project-level value to 50 again to test the user-level reset
await themeColorSetting.fill(settingValues.project)
await userSettingsTab.click()
await test.step(`Set project-level again to test the user-level reset`, async () => {
await projectSettingsTab.click()
await themeColorSetting.fill(settingValues.project)
await userSettingsTab.click()
})
})

await test.step('Reset user settings', async () => {
// Change the setting and click the reset settings button.
await themeColorSetting.fill(settingValues.user)
await resetButton.click()
// Click the reset settings button.
await resetButton('user').click()

await expect(resetToast('user')).toBeVisible()
await expect(resetToast('user')).not.toBeVisible()

// Verify it is now set to the default value
await expect(themeColorSetting).toHaveValue(settingValues.default)

// Check that the project setting also changed
await projectSettingsTab.click()
await expect(themeColorSetting).toHaveValue(settingValues.default)
await test.step(`Check that the project settings did not change`, async () => {
await projectSettingsTab.click()
await expect(themeColorSetting).toHaveValue(settingValues.project)
})
})
})

Expand Down Expand Up @@ -429,25 +436,37 @@ test.describe('Testing settings', () => {

test('Changing modeling default unit', async ({ page }) => {
const u = await getUtils(page)
await page.setViewportSize({ width: 1200, height: 500 })
await u.waitForAuthSkipAppStart()
await page
.getByRole('button', { name: 'Start Sketch' })
.waitFor({ state: 'visible' })
await test.step(`Test setup`, async () => {
await page.setViewportSize({ width: 1200, height: 500 })
await u.waitForAuthSkipAppStart()
await page
.getByRole('button', { name: 'Start Sketch' })
.waitFor({ state: 'visible' })
})

// Selectors and constants
const userSettingsTab = page.getByRole('radio', { name: 'User' })
const projectSettingsTab = page.getByRole('radio', { name: 'Project' })
const defaultUnitSection = page.getByText(
'default unitRoll back default unitRoll back to match'
)
const defaultUnitRollbackButton = page.getByRole('button', {
name: 'Roll back default unit',
})

// Open the settings modal with lower-right button
await page.getByRole('link', { name: 'Settings' }).last().click()
await expect(
page.getByRole('heading', { name: 'Settings', exact: true })
).toBeVisible()
await test.step(`Open the settings modal`, async () => {
await page.getByRole('link', { name: 'Settings' }).last().click()
await expect(
page.getByRole('heading', { name: 'Settings', exact: true })
).toBeVisible()
})

const resetButton = page.getByRole('button', {
name: 'Restore default settings',
await test.step(`Reset unit setting`, async () => {
await userSettingsTab.click()
await defaultUnitSection.hover()
await defaultUnitRollbackButton.click()
await projectSettingsTab.click()
})
// Default unit should be mm
await resetButton.click()

await test.step('Change modeling default unit within project tab', async () => {
const changeUnitOfMeasureInProjectTab = async (unitOfMeasure: string) => {
Expand Down
14 changes: 7 additions & 7 deletions src/components/Settings/AllSettingsFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { useLocation, useNavigate } from 'react-router-dom'
import { isDesktop } from 'lib/isDesktop'
import { ActionButton } from 'components/ActionButton'
import { SettingsFieldInput } from './SettingsFieldInput'
import { getInitialDefaultDir } from 'lib/desktop'
import toast from 'react-hot-toast'
import { APP_VERSION } from 'routes/Settings'
import { PATHS } from 'lib/paths'
Expand Down Expand Up @@ -214,22 +213,23 @@ export const AllSettingsFields = forwardRef(
)}
<ActionButton
Element="button"
onClick={toSync(async () => {
const defaultDirectory = await getInitialDefaultDir()
onClick={() => {
send({
type: 'Reset settings',
defaultDirectory,
level: searchParamTab,
})
toast.success('Settings restored to default')
}, reportRejection)}
toast.success(
`Your ${searchParamTab}-level settings were reset`
)
}}
iconStart={{
icon: 'refresh',
size: 'sm',
className: 'p-1 text-chalkboard-10',
bgClassName: 'bg-destroy-70',
}}
>
Restore default settings
Reset {searchParamTab}-level settings
</ActionButton>
</div>
</SettingsSection>
Expand Down
31 changes: 19 additions & 12 deletions src/machines/settingsMachine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import {
SettingsPaths,
WildcardSetEvent,
} from 'lib/settings/settingsTypes'
import {
configurationToSettingsPayload,
projectConfigurationToSettingsPayload,
setSettingsAtLevel,
} from 'lib/settings/settingsUtils'

export const settingsMachine = setup({
types: {
Expand All @@ -24,7 +29,10 @@ export const settingsMachine = setup({
type: 'set.modeling.units'
data: { level: SettingsLevel; value: BaseUnit }
}
| { type: 'Reset settings'; defaultDirectory: string }
| {
type: 'Reset settings'
level: SettingsLevel
}
| { type: 'Set all settings'; settings: typeof settings },
},
actions: {
Expand All @@ -37,17 +45,16 @@ export const settingsMachine = setup({
setClientSideSceneUnits: () => {},
persistSettings: () => {},
resetSettings: assign(({ context, event }) => {
if (!('defaultDirectory' in event)) return {}
// Reset everything except onboarding status,
// which should be preserved
const newSettings = createSettings()
if (context.app.onboardingStatus.user) {
newSettings.app.onboardingStatus.user =
context.app.onboardingStatus.user
}
// We instead pass in the default directory since it's asynchronous
// to re-initialize, and that can be done by the caller.
newSettings.app.projectDirectory.default = event.defaultDirectory
if (!('level' in event)) return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a neat one. Why not if (!event.level) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the same in this case! I could totally do that, and it's probably a hair better.

In other type narrowing for these XState events we'll have data objects and have to narrow based on the keys within the data object, that may or may not exist. TS will yell at you if you try to type narrow in that instance with something like (!event.data?.innerKey), so you have to narrow with something like (!('data' in event && event.data.innerKey)). So I've just matched that idiom.

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand all data in XState (5) is strongly typed, so I'd be surprised if we ever need in but please ping me so I can collect some examples for the ol' brain vault :D


// Create a new, blank payload
const newPayload =
event.level === 'user'
? configurationToSettingsPayload({})
: projectConfigurationToSettingsPayload({})

// Reset the settings at that level
const newSettings = setSettingsAtLevel(context, event.level, newPayload)

return newSettings
}),
Expand Down
Loading