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
12 changed files with 252 additions and 27 deletions

View File

@ -36,35 +36,35 @@
<tbody>
<tr>
<td><code>s</code></td>
<td>{{ $t('input.datemathHelp.units.seconds') }}</td>
<td>{{ $tc('time.seconds', 2) }}</td>
</tr>
<tr>
<td><code>m</code></td>
<td>{{ $t('input.datemathHelp.units.minutes') }}</td>
<td>{{ $tc('time.minutes', 2) }}</td>
</tr>
<tr>
<td><code>h</code></td>
<td>{{ $t('input.datemathHelp.units.hours') }}</td>
<td>{{ $tc('time.hours', 2) }}</td>
</tr>
<tr>
<td><code>H</code></td>
<td>{{ $t('input.datemathHelp.units.hours') }}</td>
<td>{{ $tc('time.hours', 2) }}</td>
</tr>
<tr>
<td><code>d</code></td>
<td>{{ $t('input.datemathHelp.units.days') }}</td>
<td>{{ $tc('time.days', 2) }}</td>
</tr>
<tr>
<td><code>w</code></td>
<td>{{ $t('input.datemathHelp.units.weeks') }}</td>
<td>{{ $tc('time.weeks', 2) }}</td>
</tr>
<tr>
<td><code>M</code></td>
<td>{{ $t('input.datemathHelp.units.months') }}</td>
<td>{{ $tc('time.months', 2) }}</td>
</tr>
<tr>
<td><code>y</code></td>
<td>{{ $t('input.datemathHelp.units.years') }}</td>
<td>{{ $tc('time.years', 2) }}</td>
</tr>
</tbody>
</table>

View File

@ -48,11 +48,11 @@
@change="updateData"
:disabled="disabled || undefined"
>
<option value="hours">{{ $t('task.repeat.hours') }}</option>
<option value="days">{{ $t('task.repeat.days') }}</option>
<option value="weeks">{{ $t('task.repeat.weeks') }}</option>
<option value="months">{{ $t('task.repeat.months') }}</option>
<option value="years">{{ $t('task.repeat.years') }}</option>
<option value="hours">{{ $tc('time.hours', 2) }}</option>
<option value="days">{{ $tc('time.days', 2) }}</option>
<option value="weeks">{{ $tc('time.weeks', 2) }}</option>
<option value="months">{{ $tc('time.months', 2) }}</option>
<option value="years">{{ $tc('time.years', 2) }}</option>
</select>
</div>
</div>

View File

@ -0,0 +1,63 @@
import {describe, it, expect, vi, afterEach, beforeEach} from 'vitest'
import {
AMOUNTS_IN_SECONDS,
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 * AMOUNTS_IN_SECONDS.minutes)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('minutes')
})
it('Should parse hours', () => {
const settings = parseSavedReminderAmount(5 * AMOUNTS_IN_SECONDS.hours)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('hours')
})
it('Should parse days', () => {
const settings = parseSavedReminderAmount(5 * AMOUNTS_IN_SECONDS.days)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('days')
})
it('Should parse months', () => {
const settings = parseSavedReminderAmount(5 * AMOUNTS_IN_SECONDS.months)
expect(settings.amount).toBe(5)
expect(settings.type).toBe('months')
})
})

View File

@ -0,0 +1,89 @@
const DEFAULT_REMINDER_KEY = 'defaultReminder'
export const AMOUNTS_IN_SECONDS: {
konrad marked this conversation as resolved
Review

Reuse type definition. E.g.

{ [type in SavedReminderSettings['type']]: number }
Reuse type definition. E.g. ```ts { [type in SavedReminderSettings['type']]: number } ```
Review

Done.

Done.
[type in SavedReminderSettings['type']]: number
} = {
minutes: 60,
hours: 60 * 60,
days: 60 * 60 * 24,
months: 60 * 60 * 24 * 30,
} as const
interface DefaultReminderSettings {
enabled: boolean,
konrad marked this conversation as resolved Outdated

Add as const

Add `as const`

Done.

Done.
amount: number,
}
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.
interface SavedReminderSettings {
enabled: boolean,
amount: number,
type: 'minutes' | 'hours' | 'days' | 'months',
}
function calculateDefaultReminderSeconds(type: SavedReminderSettings['type'], amount: number): number {
return amount * (AMOUNTS_IN_SECONDS[type] || 0)
}
konrad marked this conversation as resolved
Review

type would better be as keyof typeof AMOUNT_IN_SECONDS.

Maybe we should save the type as new type, e.g.

type IReminderDurationType = 'minutes' | 'hours' | 'days' | 'months'
`type` would better be as `keyof typeof AMOUNT_IN_SECONDS`. Maybe we should save the `type` as new type, e.g. ```ts type IReminderDurationType = 'minutes' | 'hours' | 'days' | 'months' ```
Review

I've changed it to use SavedReminderSettings['type'] instead.

I've changed it to use `SavedReminderSettings['type']` instead.
export function saveDefaultReminder(enabled: boolean, type: SavedReminderSettings['type'], amount: number) {
const defaultReminderSeconds = calculateDefaultReminderSeconds(type, amount)
localStorage.setItem(DEFAULT_REMINDER_KEY, JSON.stringify(<DefaultReminderSettings>{
enabled,
amount: defaultReminderSeconds,
}))
}
export function getDefaultReminderAmount(): number | null {
const settings = getDefaultReminderSettings()
return settings?.enabled
? settings.amount
dpschen marked this conversation as resolved
Review

picky: don't create one-use-variable

picky: don't create one-use-variable
Review

But here I'm using it twice?

But here I'm using it twice?
Review

Beeing too picky is also stupid… you are right!

Beeing too picky is also stupid… you are right!
: null
}
export function getDefaultReminderSettings(): DefaultReminderSettings | null {
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 {
const amountMinutes = amountSeconds / 60
const settings: 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.
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,
type: 'minutes',
amount: 0,
}
}
return parseSavedReminderAmount(s.amount)
}

View File

@ -87,7 +87,11 @@
"language": "Language",
"defaultList": "Default List",
"timezone": "Time Zone",
"overdueTasksRemindersTime": "Overdue tasks reminder email time"
"overdueTasksRemindersTime": "Overdue tasks reminder email time",
"defaultReminder": "Set a default task reminder",
"defaultReminderHint": "If enabled, Vikunja will automatically create a reminder for a task if you set a due date and the task does not have any reminders yet.",
"defaultReminderAmount": "Default task reminder amount",
"defaultReminderAmountBefore": "before the due date of a task"
},
"totp": {
"title": "Two Factor Authentication",
@ -547,19 +551,16 @@
"fromto": "{from} to {to}",
"ranges": {
"today": "Today",
"thisWeek": "This Week",
"restOfThisWeek": "The Rest of This Week",
"nextWeek": "Next Week",
"next7Days": "Next 7 Days",
"lastWeek": "Last Week",
"thisMonth": "This Month",
"restOfThisMonth": "The Rest of This Month",
"nextMonth": "Next Month",
"next30Days": "Next 30 Days",
"lastMonth": "Last Month",
"thisYear": "This Year",
"restOfThisYear": "The Rest of This Year"
}
@ -576,15 +577,6 @@
"roundDay": "Round down to the nearest day",
"supportedUnits": "Supported time units are:",
"someExamples": "Some examples of time expressions:",
"units": {
"seconds": "Seconds",
"minutes": "Minutes",
"hours": "Hours",
"days": "Days",
"weeks": "Weeks",
"months": "Months",
"years": "Years"
},
"examples": {
"now": "Right now",
"in24h": "In 24h",
@ -596,6 +588,15 @@
}
}
},
"time": {
konrad marked this conversation as resolved Outdated

Use vue-i18n pluralisation feature.
Or didn't you use that to be more because of crowdin?

Use [vue-i18n pluralisation](https://vue-i18n.intlify.dev/guide/essentials/pluralization.html) feature. Or didn't you use that to be more because of crowdin?

I actually just forgot about that feature :) will change it.

I actually just forgot about that feature :) will change it.

Done.

Done.
"seconds": "Second | Seconds",
"minutes": "Minute | Minutes",
"hours": "Hour | Hours",
"days": "Day | Days",
"weeks": "Week | Weeks",
"months": "Month | Months",
"years": "Year | Years"
},
"task": {
"task": "Task",
"new": "Create a new task",

View File

@ -12,4 +12,6 @@ export interface IUserSettings extends IAbstract {
weekStart: 0 | 1 | 2 | 3 | 4 | 5 | 6
timezone: string
language: string
defaultReminder: boolean
defaultReminderAmount: number // The amount of seconds a reminder should be set before a given due date
}

View File

@ -2,6 +2,7 @@ import AbstractModel from './abstractModel'
import type {IUserSettings} from '@/modelTypes/IUserSettings'
import {getCurrentLanguage} from '@/i18n'
import {getDefaultReminderAmount} from '@/helpers/defaultReminder'
export default class UserSettingsModel extends AbstractModel<IUserSettings> implements IUserSettings {
name = ''
@ -13,6 +14,8 @@ export default class UserSettingsModel extends AbstractModel<IUserSettings> impl
weekStart = 0 as IUserSettings['weekStart']
timezone = ''
language = getCurrentLanguage()
konrad marked this conversation as resolved Outdated

se here getDefaultReminderAmount

se here `getDefaultReminderAmount`

Done.

Done.
defaultReminder = false
defaultReminderAmount = getDefaultReminderAmount() || 0
constructor(data: Partial<IUserSettings> = {}) {
super()

View File

@ -6,6 +6,7 @@ import LabelService from './label'
import {formatISO} from 'date-fns'
import {colorFromHex} from '@/helpers/color/colorFromHex'
import {getDefaultReminderAmount} from '@/helpers/defaultReminder'
const parseDate = date => {
if (date) {
@ -39,7 +40,7 @@ export default class TaskService extends AbstractService<ITask> {
}
processModel(updatedModel) {
const model = { ...updatedModel }
const model = {...updatedModel}
model.title = model.title?.trim()
@ -68,6 +69,15 @@ export default class TaskService extends AbstractService<ITask> {
})
}
if (model.dueDate !== null && model.reminderDates.length === 0) {
const defaultReminder = getDefaultReminderAmount()
if (defaultReminder !== null) {
const dueDate = +new Date(model.dueDate)
const reminderDate = new Date(dueDate - (defaultReminder * 1000))
model.reminderDates.push(formatISO(reminderDate))
}
}

Below seems to be the same calculation as in the new calculateDefaultReminderSeconds function. We should probably combine these.

Below seems to be the same calculation as in the new `calculateDefaultReminderSeconds` function. We should probably combine these.

Looks very similar, yes.

Looks very similar, yes.
// Make the repeating amount to seconds
let repeatAfterSeconds = 0
if (model.repeatAfter !== null && (model.repeatAfter.amount !== null || model.repeatAfter.amount !== 0)) {

View File

@ -56,6 +56,7 @@
}
}
.field.has-addons .select select,
konrad marked this conversation as resolved Outdated

Combine with selector below.

Maybe better: use class for specific case in General

Combine with selector below. Maybe better: use class for specific case in General

I've combined them.

I've combined them.
.field.has-addons .control .select select {
height: 100%;
}

View File

@ -697,6 +697,9 @@ export default defineComponent({
}
this.task = await this.$store.dispatch('tasks/update', task)
// Show new fields set from the api or a newly set default reminder
this.$nextTick(() => this.setActiveFields())
Review

I don't understand why the nextTick is necessary here.

I don't understand why the nextTick is necessary here.
Review

The comment is wrong but the tick is required to make sure the maybe newly set reminder would be shown in the task detail view as well. I'll clarify the comment.

The comment is wrong but the tick is required to make sure the maybe newly set reminder would be shown in the task detail view as well. I'll clarify the comment.
if (!showNotification) {
return

View File

@ -18,6 +18,50 @@
</label>
<list-search v-model="defaultList"/>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="defaultReminderEnabled"/>
{{ $t('user.settings.general.defaultReminder') }}
</label>
<p class="is-size-7">
{{ $t('user.settings.general.defaultReminderHint') }}
</p>
</div>
<div class="field" v-if="defaultReminderEnabled">
<label class="label" for="defaultReminderAmount">
{{ $t('user.settings.general.defaultReminderAmount') }}
</label>
<div class="field has-addons is-align-items-center">
<div class="control">
<input
@keyup.enter="updateSettings"
class="input"
id="defaultReminderAmount"
type="number"
min="0"
v-model="defaultReminderAmount"/>
</div>
<div class="control select">
<select v-model="defaultReminderAmountType">
<option value="minutes">
{{ $tc('time.minutes', defaultReminderAmount) }}
konrad marked this conversation as resolved Outdated

In case we don't use vue-i18n pluralisation feature we shouldn't use local messages that can't be precompiled (see: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization).

{{ defaultReminderAmount === 1 ? $t('time.minute') : $t('time.minutes') }}
In case we don't use vue-i18n pluralisation feature we shouldn't use local messages that can't be precompiled (see: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization). ```ts {{ defaultReminderAmount === 1 ? $t('time.minute') : $t('time.minutes') }} ```
</option>
<option value="hours">
{{ $tc('time.hours', defaultReminderAmount) }}
</option>
<option value="days">
{{ $tc('time.days', defaultReminderAmount) }}
</option>
<option value="months">
{{ $tc('time.months', defaultReminderAmount) }}
</option>
</select>
</div>
<p class="pl-2">
{{ $t('user.settings.general.defaultReminderAmountBefore') }}
</p>
</div>
</div>
<div class="field">
<label class="checkbox">
<input type="checkbox" v-model="settings.overdueTasksRemindersEnabled"/>
@ -175,6 +219,7 @@ import {AuthenticatedHTTPFactory} from '@/http-common'
import {useColorScheme} from '@/composables/useColorScheme'
import {useTitle} from '@/composables/useTitle'
import {objectIsEmpty} from '@/helpers/objectIsEmpty'
import {getSavedReminderSettings, saveDefaultReminder} from '@/helpers/defaultReminder'
const {t} = useI18n({useScope: 'global'})
useTitle(() => `${t('user.settings.general.title')} - ${t('user.settings.title')}`)
@ -266,9 +311,16 @@ watch(
async function updateSettings() {
localStorage.setItem(playSoundWhenDoneKey, playSoundWhenDone.value ? 'true' : 'false')
setQuickAddMagicMode(quickAddMagicMode.value)
saveDefaultReminder(defaultReminderEnabled.value, defaultReminderAmountType.value, defaultReminderAmount.value)
await authStore.saveUserSettings({
settings: {...settings.value},
})
}
const reminderSettings = getSavedReminderSettings()
const defaultReminderEnabled = ref<boolean>(reminderSettings?.enabled || false)
const defaultReminderAmount = ref(reminderSettings?.amount || 1)
const defaultReminderAmountType = ref(reminderSettings?.type || 'days')
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.
</script>

View File

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