WIP: feat: add default reminder for tasks with a due date #2360

Draft
konrad wants to merge 17 commits from feature/add-default-reminder into main
4 changed files with 111 additions and 5 deletions
Showing only changes of commit 5aafbd9a72 - Show all commits

View File

@ -0,0 +1,62 @@
import {describe, it, expect, vi, afterEach, beforeEach} from 'vitest'
import {
getDefaultReminderSettings,
getSavedReminderSettings,
parseSavedReminderAmount,
saveDefaultReminder,
} from '@/helpers/defaultReminder'
import * as exports from '@/helpers/defaultReminder'
describe('Default Reminder Save', () => {
it('Should save a default reminder with minutes', () => {
const spy = vi.spyOn(window.localStorage, 'setItem')
saveDefaultReminder(true, 'minutes', 5)
expect(spy).toHaveBeenCalledWith('defaultReminder', '{"enabled":true,"amount":300}')
})
it('Should save a default reminder with hours', () => {
const spy = vi.spyOn(window.localStorage, 'setItem')
saveDefaultReminder(true, 'hours', 5)
expect(spy).toHaveBeenCalledWith('defaultReminder', '{"enabled":true,"amount":18000}')
})
it('Should save a default reminder with days', () => {
const spy = vi.spyOn(window.localStorage, 'setItem')
saveDefaultReminder(true, 'days', 5)
expect(spy).toHaveBeenCalledWith('defaultReminder', '{"enabled":true,"amount":432000}')
})
it('Should save a default reminder with months', () => {
const spy = vi.spyOn(window.localStorage, 'setItem')
saveDefaultReminder(true, 'months', 5)
expect(spy).toHaveBeenCalledWith('defaultReminder', '{"enabled":true,"amount":12960000}')
})
})
describe('Default Reminder Load', () => {
it('Should parse minutes', () => {
const settings = parseSavedReminderAmount(5 * 60)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('minutes')
})
it('Should parse hours', () => {
const settings = parseSavedReminderAmount(5 * 60 * 60)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('hours')
})
it('Should parse days', () => {
const settings = parseSavedReminderAmount(5 * 60 * 60 * 24)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('days')
})
it('Should parse months', () => {
const settings = parseSavedReminderAmount(5 * 60 * 60 * 24 * 30)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('months')
})
})

View File

@ -5,6 +5,12 @@ interface DefaultReminderSettings {
amount: number,
}
interface SavedReminderSettings {
enabled: boolean,
amount?: number,
type?: 'minutes' | 'hours' | 'days' | 'months',
}
konrad marked this conversation as resolved Outdated

Add as const

Add `as const`

Done.

Done.
function calculateDefaultReminderSeconds(type: string, amount: number): number {
switch (type) {
case 'minutes':
konrad marked this conversation as resolved Outdated

We should save the factors in array in a mapping constant. This way the constant can be reused for the test.

We should save the factors in array in a mapping constant. This way the constant can be reused for the test.

Done.

Done.
@ -37,10 +43,47 @@ export function getDefaultReminderAmount(): number | null {
}
export function getDefaultReminderSettings(): DefaultReminderSettings | null {
const s: string | null = localStorage.getItem(DEFAULT_REMINDER_KEY)
const s: string | null = window.localStorage.getItem(DEFAULT_REMINDER_KEY)
if (s === null) {
return null
}
return JSON.parse(s)
}
export function parseSavedReminderAmount(amountSeconds: number): SavedReminderSettings {

Why don't we save the type as well? Wouldn't that make thinks simpler?

Why don't we save the type as well? Wouldn't that make thinks simpler?

I guess I just thought about having the seconds ready to do the calculation, because that's what we need at the end.

I guess I just thought about having the seconds ready to do the calculation, because that's what we need at the end.

Wouldn't it be easier to keep the format until it's actually displayed?

Wouldn't it be easier to keep the format until it's actually displayed?

It might.

It might.
const amountMinutes = amountSeconds / 60
const settings: SavedReminderSettings = {
enabled: true, // We're assuming the caller to have checked this properly
amount: amountMinutes,
type: 'minutes',
}
if ((amountMinutes / 60 / 24) % 30 === 0) {
settings.amount = amountMinutes / 60 / 24 / 30
settings.type = 'months'
} else if ((amountMinutes / 60) % 24 === 0) {
settings.amount = amountMinutes / 60 / 24
settings.type = 'days'
} else if (amountMinutes % 60 === 0) {
settings.amount = amountMinutes / 60
settings.type = 'hours'
}
return settings
}
export function getSavedReminderSettings(): SavedReminderSettings | null {
const s = getDefaultReminderSettings()
if (s === null) {
return null
}
if (!s.enabled) {
return {
enabled: false,
}
}
return parseSavedReminderAmount(s.amount)
}

View File

@ -219,7 +219,7 @@ import {AuthenticatedHTTPFactory} from '@/http-common'
import {useColorScheme} from '@/composables/useColorScheme'
import {useTitle} from '@/composables/useTitle'
import {objectIsEmpty} from '@/helpers/objectIsEmpty'
import {getDefaultReminderSettings, saveDefaultReminder} from '@/helpers/defaultReminder'
import {getDefaultReminderSettings, getSavedReminderSettings, saveDefaultReminder} from '@/helpers/defaultReminder'
const {t} = useI18n({useScope: 'global'})
useTitle(() => `${t('user.settings.general.title')} - ${t('user.settings.title')}`)
@ -318,10 +318,10 @@ async function updateSettings() {
})
}
const reminderSettings = getDefaultReminderSettings()
const reminderSettings = getSavedReminderSettings()
const defaultReminderEnabled = ref<boolean>(reminderSettings?.enabled || false)
// TODO: re-populate amount and type
const defaultReminderAmount = ref(1)
const defaultReminderAmountType = ref('days')
const defaultReminderAmount = ref(reminderSettings?.amount || 1)
konrad marked this conversation as resolved
Review

Shouldn't the fallback be the same as the default in the model?

Shouldn't the fallback be the same as the default in the model?
Review

Actually not, because this is what gets shown in the input when setting this for the first time. I felt like putting a 0 here would feel weired.

Actually not, because this is what gets shown in the input when setting this for the first time. I felt like putting a `0` here would feel weired.
const defaultReminderAmountType = ref(reminderSettings?.type || 'days')
</script>

View File

@ -32,6 +32,7 @@ export default defineConfig({
// https://vitest.dev/config/
test: {
environment: 'happy-dom',
mockReset: true,
},
css: {
preprocessorOptions: {