Overhaul recurrence system #2642
Open
BeatLink
wants to merge 19 commits from
BeatLink/vikunja:main
into main
pull from: BeatLink/vikunja:main
merge into: vikunja:main
vikunja:main
vikunja:renovate/dev-dependencies
vikunja:renovate/pnpm-9.x
vikunja:feature/hasAttachments-as-store-computed
vikunja:fix/resetEmptyTitleError
vikunja:renovate/intlify-unplugin-vue-i18n-6.x
vikunja:renovate/express-4.x
vikunja:renovate/github.com-go-testfixtures-testfixtures-v3-3.x
vikunja:renovate/vuemoji-picker-0.x
vikunja:renovate/golang.org-x-crypto-0.x
vikunja:renovate/github.com-labstack-echo-jwt-v4-4.x
vikunja:renovate/golang.org-x-term-0.x
vikunja:renovate/golang.org-x-image-0.x
vikunja:renovate/golang.org-x-text-0.x
vikunja:renovate/golang.org-x-sys-0.x
vikunja:renovate/lowlight-3.x
vikunja:renovate/golang.org-x-sync-0.x
vikunja:renovate/pinia-2.x
vikunja:renovate/axios-1.x
vikunja:renovate/node-22.x
vikunja:renovate/tailwindcss-3.x
vikunja:renovate/go-1.x
vikunja:renovate/github.com-getsentry-sentry-go-0.x
vikunja:renovate/sentry-javascript-monorepo
vikunja:feat/improve-ProjectSettingsDelete
vikunja:feature/inline-dynamic-routes
vikunja:feat/add-vite-plugin-vue-devtools
vikunja:fix/remove-postcss-easings-types
vikunja:renovate/dompurify-3.x
vikunja:renovate/sortablejs-1.x
vikunja:renovate/vue-i18n-10.x
vikunja:renovate/major-vueuse
vikunja:renovate/major-dev-dependencies
vikunja:renovate/tiptap
vikunja:renovate/vue-router-4.x
vikunja:renovate/golangci-golangci-lint-1.x
vikunja:renovate/github.com-stretchr-testify-1.x
vikunja:release/0.24
vikunja:renovate/vueuse
vikunja:renovate/font-awesome
vikunja:renovate/github.com-gabriel-vasile-mimetype-1.x
vikunja:fix/saved-filter-search
vikunja:renovate/goreleaser-nfpm-2.x
vikunja:renovate/vue-monorepo
vikunja:feat/remove-postcss-easings
vikunja:feat/move-useProjectBackground-to-composables
vikunja:renovate/intlify-unplugin-vue-i18n-5.x
vikunja:fix/reactive-ancestor-projects
vikunja:renovate/golang.org-x-oauth2-0.x
vikunja:renovate/postgres-17.x
vikunja:renovate/github.com-wneessen-go-mail-0.x
vikunja:feature/use-modern-compiler-for-sass-files-as-well
vikunja:feat/webp-and-avif-attachment-previews
vikunja:fix/tsconfig22
vikunja:feature/only-build-sourcemaps-for-sentry
vikunja:fix/tiptap-editor-reactive-destructuring
vikunja:feature/migrate-back-to-bulma
vikunja:fix/sass-add-missing-list-import
vikunja:feature/use-sass-embedded
vikunja:renovate/github.com-threedotslabs-watermill-1.x
vikunja:feature/sticky-demo-bar
vikunja:fix/remove-defineProps
vikunja:renovate/workbox-monorepo
vikunja:renovate/github.com-swaggo-swag-1.x
vikunja:renovate/node-20.x
vikunja:renovate/kyvg-vue3-notification-3.x
vikunja:fix/gantt-view-switch
vikunja:renovate/github.com-redis-go-redis-v9-9.x
vikunja:renovate/github.com-yuin-goldmark-1.x
vikunja:renovate/github.com-prometheus-client_golang-1.x
vikunja:renovate/github.com-mattn-go-sqlite3-1.x
vikunja:feature/typesense-position-join
vikunja:feature/focus-visible
vikunja:renovate/date-fns-4.x
vikunja:renovate/flexsearch-0.x
vikunja:renovate/dario.cat-mergo-1.x
vikunja:renovate/github.com-typesense-typesense-go-2.x
vikunja:renovate/vue-i18n-9.x
vikunja:renovate/dayjs-1.x
vikunja:dependencies/golangci-lint
vikunja:renovate/github.com-coreos-go-oidc-v3-3.x
vikunja:renovate/github.com-microcosm-cc-bluemonday-1.x
vikunja:renovate/github.com-arran4-golang-ical-0.x
vikunja:renovate/ufo-1.x
vikunja:renovate/github-hotkey-3.x
vikunja:feature/better-filter-syntax
vikunja:fix/tiptap-task-list
vikunja:feature/zod-schema
vikunja:renovate/github.com-golang-jwt-jwt-v4-5.x
vikunja:feature/hide-forbidden-related-tasks
vikunja:renovate/golang-1.x
vikunja:release/0.20
vikunja:release/0.17
vikunja:release/0.16
vikunja:release/0.15
vikunja:release/0.14
vikunja:release/0.13
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
No description provided.
Delete Branch "BeatLink/vikunja:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR reimplements the recurrence system.
All current repeat fields are replaced with a field called 'repeats' and a boolean field called 'repeats_from_completion'. The repeats field stores the RRULE recurrence string as implemented in the ICalendar standards.
The current repeats widget will be redesigned to generate and store this RRULE string to the database. When a task is marked as complete, the rrule-go library developed here https://github.com/teambition/rrule-go will take the RRULE string and generate the next recurrence string either from the due, start or end dates (in that order if present) or the completion date, determined by the repeats_from_completion.
Advantages of this implementation are that all repeat functionality is handled by the rrule-go library, significantly simplifying the code in vikunja itself. Also as the repeat system is now conforming to the ICalendar standards, the caldav repeat implementation is now fully functional.
WIP: Overhaul recurrence systemto Overhaul recurrence systemSorry for the late review, seems like Gitea does not send emails anymore.
This already looks pretty good! There are still a bunch of things to do though. I really like the idea of letting the caldav library do the heavy lifting.
@ -0,0 +10,4 @@
class="input max-width"
type="number"
min="1"
max="9999"
Why the max limit?
@ -0,0 +203,4 @@
</div>
</div>
</div>
<!-- Debugging Elements - Set to false in production -->
Please remove this - use the vue dev extension for things like this.
I think it’s fine to keep this, but I would add a const instead of setting the false value inline. This has the added advantage, that when you read the component starting from the script block you will still find out that there is some debugging flag.
Even better would be if we could remove this on compilation depending on the flag, but I’ve never done this, so not sure how that would work.
@ -0,0 +239,4 @@
const rrule_input = ref('')
const task = ref<ITask>(new TaskModel())
const interval = ref('DAILY')
Can you add a typescript enum for this?
Enums are weird. But I agree that we should define the possible values. I would just use a string union instead.
@ -0,0 +282,4 @@
})
const repeatFromCompletionDate = ref('false')
function getRRULEstring() {
Can this be a computed?
Especially because there is a call of
new Date
(that would change for this seems wise.@ -0,0 +284,4 @@
function getRRULEstring() {
let string = ''
string += `FREQ=${interval.value}`
Won't this produce an invalid string when the interval is not set?
@ -0,0 +315,4 @@
}
if (stopType.value == 'date'){
// Having the stoptype as a date with an empty date breaks things (such as when the user clicks the Clear button. So we force a date here)
if (!stopDate.value) {
Can we fix the breaking of things instead of relying on workarounds?
@ -0,0 +320,4 @@
}
const date = new Date(stopDate.value)
// Typescript doesn't have replaceAll. Sorry :(
string += `;UNTIL=${date.toISOString().replace('-', '').replace('-', '').replace(':', '').replace(':', '').replace('.000', '')}`
Typescript is a superset of JS, so you can still use replaceAll.
@ -0,0 +331,4 @@
return string
}
function loadRRULEstring(){
Can you move this out of the component and add unit tests for it?
Seems like a function that can easily be isolated and only receive inputs and outputs.
@ -0,0 +409,4 @@
function saveRRULEString(){
Can you debounce this? Here or in the parent component, where it gets saved.
Should be here.
@ -214,3 +214,3 @@
switch (results[2]) {
case 'biennially':
type = REPEAT_TYPES.Years
type = 'YEARLY'
Please use const here
@ -536,3 +536,3 @@
variant="secondary"
icon="history"
@click="setFieldActive('repeatAfter')"
@click="() => { AddDefaultRepeats(); setFieldActive('repeats')}"
Why do we need to add the default repeats?
Also: please use camel case - lowercase first letter
@ -149,6 +148,7 @@ func ParseTaskFromVTODO(content string) (vTask *models.Task, err error) {
DueDate: caldavTimeToTimestamp(task["DUE"]),
Updated: caldavTimeToTimestamp(task["DTSTAMP"]),
StartDate: caldavTimeToTimestamp(task["DTSTART"]),
Repeats: task["RRULE"].Value,
Please validate this before saving.
@ -116,3 +116,3 @@
rec, err := testHandler.testReadAllWithUser(url.Values{"sort_by": []string{"priority"}}, urlParams)
require.NoError(t, err)
assert.Contains(t, rec.Body.String(), `{"id":33,"title":"task #33 with percent done","description":"","done":false,"done_at":"0001-01-01T00:00:00Z","due_date":"0001-01-01T00:00:00Z","reminders":null,"project_id":1,"repeat_after":0,"repeat_mode":0,"priority":0,"start_date":"0001-01-01T00:00:00Z","end_date":"0001-01-01T00:00:00Z","assignees":null,"labels":null,"hex_color":"","percent_done":0.5,"identifier":"test1-17","index":17,"related_tasks":{},"attachments":null,"cover_image_attachment_id":0,"is_favorite":false,"created":"2018-12-01T01:12:04Z","updated":"2018-12-01T01:12:04Z","bucket_id":0,"position":0,"reactions":null,"created_by":{"id":1,"name":"","username":"user1","created":"2018-12-01T15:13:12Z","updated":"2018-12-02T15:13:12Z"}}]`)
assert.Contains(t, rec.Body.String(), `{"id":33,"title":"task #33 with percent done","description":"","done":false,"done_at":"0001-01-01T00:00:00Z","due_date":"0001-01-01T00:00:00Z","reminders":null,"project_id":1,"repeats":'',"priority":0,"start_date":"0001-01-01T00:00:00Z","end_date":"0001-01-01T00:00:00Z","assignees":null,"labels":null,"hex_color":"","percent_done":0.5,"identifier":"test1-17","index":17,"related_tasks":{},"attachments":null,"cover_image_attachment_id":0,"is_favorite":false,"created":"2018-12-01T01:12:04Z","updated":"2018-12-01T01:12:04Z","bucket_id":0,"position":0,"reactions":null,"created_by":{"id":1,"name":"","username":"user1","created":"2018-12-01T15:13:12Z","updated":"2018-12-02T15:13:12Z"}}]`)
This should be
"repeats":""
@ -121,3 +121,2 @@
require.NoError(t, err)
assert.Contains(t, rec.Body.String(), `"repeat_after":3600`)
assert.NotContains(t, rec.Body.String(), `"repeat_after":0`)
assert.Contains(t, rec.Body.String(), `"repeats":'FREQ=HOURLY;INTERVAL=1'`)
Same here and everywhere, please use
"
.@ -0,0 +28,4 @@
}
func (Task20240819201553) TableName() string {
return "Task"
This should be
tasks
- passingTask
will create a new tableTask
which has nothing to do with thetasks
table, where all tasks are stored. This made me unable to test this.@ -0,0 +36,4 @@
ID: "20240819201553",
Description: "Adds the Repeats text field to support the new recurrence system",
Migrate: func(tx *xorm.Engine) error {
return tx.Sync2(Task20240819201553{})
Please change this migration so that it migrates all old migration data over to the new system and drops the old columns.
@ -494,3 +494,3 @@
ProjectID: 1,
RelatedTasks: map[RelationKind][]*Task{},
RepeatAfter: 3600,
Repeats: 'FREQ=HOURLY;INTERVAL=1',
Please change this in the fixtures as well.
And
'
will not work, please use"
.@ -72,2 +63,2 @@
// Can have three possible values which will trigger when the task is marked as done: 0 = repeats after the amount specified in repeat_after, 1 = repeats all dates each months (ignoring repeat_after), 3 = repeats from the current date rather than the last set date.
RepeatMode TaskRepeatMode `xorm:"not null default 0" json:"repeat_mode"`
// The RRULE string that configures how the task repeats.
Repeats string `xorm:"TEXT not null 'repeats' default ''" json:"repeats"`
Why not make it nullable?
@ -165,3 +158,2 @@
func (t *Task) isRepeating() bool {
return t.RepeatAfter > 0 ||
t.RepeatMode == TaskRepeatModeMonth
return len(t.Repeats) > 0
Please do a string comparison instead of len
@ -1180,3 +1171,2 @@
func setTaskDatesDefault(oldTask, newTask *Task) {
if oldTask.RepeatAfter == 0 {
func updateRecurrence(oldTask, newTask *Task){
Can you add unit tests for this?
@ -1302,0 +1179,4 @@
If the due date is not present, the start date is used and the end date is updated relative to that.
If the start or due dates aren't present, the end date is updated.
*/
rruleManager, _ := rrule.StrToRRule(oldTask.Repeats)
Please validate the recurrence string and return a new error, when it is invalid.
@ -1316,0 +1211,4 @@
will be based on the reference time, not the current time
*/
now = time.Now()
Can you rename this to something like
referenceDate
or similar? Since it's not really now after you change it.@ -1321,3 +1236,1 @@
diff := oldTask.DueDate.Sub(oldTask.EndDate)
newTask.EndDate = newTask.DueDate.Add(-diff)
}
if !oldTask.DueDate.IsZero() {
Unsure here, but what if the task was marked done and the dates edited in the same go? Won't that be overridden? That's probably an edge case, and probably one we don't handle properly with the current system anyways.
@ -355,3 +355,3 @@
switch t.Recurrence.Pattern.Type {
case "daily":
task.RepeatAfter = t.Recurrence.Pattern.Interval * 60 * 60 * 24
task.Repeats = fmt.Sprintf("FREQ=DAILY;INTERVAL=%d", t.Recurrence.Pattern.Interval)
Don't the other migrators need changes too?
Please fix the lint and unit tests.
I also didn’t receive mail notifications :/
While I wasn’t able to get the UI to work, this looks like a great PR.
Thank you for your work! 🥳
I’d like to review the PR again after I can test it locally to get a better feeling for the UX
@ -0,0 +1,452 @@
<template>
<div class="pb-4 pt-4">
<!--Repeat Interval -->
Picky:
{{ /* comment */ }}
<!—
@ -0,0 +36,4 @@
</div>
<!--Weekdays -->
<div
v-if="interval=='WEEKLY'"
Use
===
@ -0,0 +57,4 @@
</div>
<!--Month Days -->
<div
v-if="interval=='MONTHLY'"
Use
===
@ -0,0 +63,4 @@
<label class="label">{{ $t('task.repeat.dayOfMonth') }}</label>
<div class="control max-width">
<div class="select max-width">
<select
I didn’t see the UI yet, but just from reading this it feels like this should be an input radio since there are so few options to select from. This also would reduce the amount of clicks needed to change. What do you think?
@ -0,0 +79,4 @@
</div>
<div class="field has-addons max-width">
<div
v-if="monthType=='weekday'"
Use
===
@ -0,0 +90,4 @@
>
<option
v-for="(value, key, index) of monthWeekdayOrdinalOpts"
:key="index"
Since the value is unique already I would use it here instead of the index. Under certain circumstances this can improve cues rendering. I don’t think that’s the case here, but it’s a good pattern to use unique values if we already have them available
@ -0,0 +99,4 @@
</div>
</div>
<div
v-if="monthType=='weekday'"
Use
===
@ -0,0 +119,4 @@
</div>
</div>
<div
v-if="monthType=='dayOfMonth'"
Use
===
@ -0,0 +145,4 @@
@change="saveRRULEString()"
>
<option
v-for="(value, key, index) of stopTypeOpts"
topTypeOptions
=> unnecessary abbreviations@ -0,0 +147,4 @@
<option
v-for="(value, key, index) of stopTypeOpts"
:key="index"
:value="key"
Please use value as key
@ -0,0 +155,4 @@
</div>
</div>
<div
v-if="stopType=='date'"
Use
===
@ -0,0 +166,4 @@
>
</div>
<div
v-if="stopType=='number'"
Use
===
@ -0,0 +186,4 @@
<div class="field has-addons max-width">
<div class="control max-width">
<div class="select max-width">
<select
From the resulting value this seems like it could be a checkbox instead. Didn’t check the label value, so not sure if that makes sense (I‘m doing the review on my phone so I‘m afraid to jump and loose context 😅).
@ -0,0 +241,4 @@
const task = ref<ITask>(new TaskModel())
const interval = ref('DAILY')
const intervalNumber = ref(1)
const weeks = ref({
Please save state separate from label. The label should be a computed, so that in theory - even while unrealistic - we can change the language while having the component open.
@ -0,0 +242,4 @@
const interval = ref('DAILY')
const intervalNumber = ref(1)
const weeks = ref({
'SU': {selectText: t('task.repeat.sunday'), checked: false},
Remove ticks around keys here, since they are not necessary
@ -0,0 +250,4 @@
'FR': {selectText: t('task.repeat.friday'), checked: false},
'SA': {selectText: t('task.repeat.saturday'), checked: false},
})
const monthType = ref('weekday')
Define possible values via string union
@ -0,0 +257,4 @@
const stopType = ref('none')
const stopDate = ref(new Date().toString())
const stopNumber = ref(1)
const intervalOpts = ref({
This should be a computed as well. Please change the other labels accordingly.
selectTextPlural
doesn’t need to exist because in a computed we can use vue i18n pluralisation. See https://vue-i18n.intlify.dev/guide/essentials/pluralization.html@ -0,0 +267,4 @@
'YEARLY': {selectText: t('task.repeat.year'), selectTextPlural: t('task.repeat.years')},
})
const monthWeekdayOrdinalOpts = ref({
For all the option configs: please write out
opts
asoptions
@ -0,0 +292,4 @@
if (interval.value == 'WEEKLY'){
const enabledDays = []
for (const [key, value] of Object.entries(weeks.value)){
if (value.checked){
Space missing After
(
@ -0,0 +300,4 @@
string += `;BYDAY=${enabledDays.join(',')}`
}
}
if (interval.value == 'MONTHLY') {
Please use
===
for this and the next comparisons.@ -0,0 +307,4 @@
if (monthType.value == 'dayOfMonth'){
if (monthDayNumber.value < 1) {
monthDayNumber.value = 1
} else if (monthDayNumber.value > 31) {
This seems unnecessary. We already have the max attribute in the input. Seems like further validation should happen on server side (?)
Yeah validation on the server should definitely happen.
@ -0,0 +348,4 @@
'COUNT': '',
}
if (string){
Return early
@ -0,0 +368,4 @@
}
}
} else if (optionsMap['FREQ'] == 'MONTHLY'){
console.log(optionsMap)
Remove this and the following logs
@ -0,0 +423,4 @@
(value: ITask) => {
task.value = value
if (value.repeats) {
loadRRULEstring()
Seems to me like the whole repeating configuration values might profit from being written as computed getter and setter. The
loadRRulestring
would be used in the getter.@ -0,0 +432,4 @@
deep: true,
},
)
Remove empty line
@ -0,0 +436,4 @@
</script>
<style lang="scss" scoped>
p {
Please use a class for this.
@ -934,2 +928,2 @@
"each": "Each",
"specifyAmount": "Specify an amount…",
"taskRepeatsEvery": "Task Repeats Every",
"onWeekdays": "On Weekdays",
Unsure: isn’t week day written as two words?
@ -936,0 +938,4 @@
"fourthOrd": "Fourth",
"fifthOrd": "Fifth",
"lastOrd": "Last",
"second": "Second",
Unsure: use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DisplayNames
How would that work to get the translation for seconds?
I think we have that in a few other places as well, we should check if we can do this all at once? That would be a separate PR
@ -942,0 +961,4 @@
"saturday": "Saturday",
"never": "Never",
"onDate": "On Date",
"afterNumber": "After # of Repeats"
#
might profit from dynamic value@ -7,13 +7,8 @@ import type {IAttachment} from '@/modelTypes/IAttachment'
import type {IProject} from '@/modelTypes/IProject'
import type {ISubscription} from '@/modelTypes/ISubscription'
import type {IBucket} from '@/modelTypes/IBucket'
Please re-add empty lines in this file. They were for separation.
@ -72,3 +52,1 @@
repeatAfter: number | IRepeatAfter = 0
repeatFromCurrentDate = false
repeatMode: IRepeatMode = TASK_REPEAT_MODES.REPEAT_MODE_DEFAULT
repeats: string
Wouldn’t it make sense to already parse the string here?
@ -756,3 +755,3 @@
| 'relatedTasks'
| 'reminders'
| 'repeatAfter'
| 'repeats'
I think we should rename this to
recurrences
and the component accordingly toRecurrences.vue
. Seems to fit better. I was never really happy with therepeatAfter
. What do you think @konrad?I think in caldav it's called recurrence as well, sounds like a good idea. Should be renamed in the api as well though.
@ -927,2 +925,2 @@
task.value.repeatAfter.amount = 0
task.value.repeatMode = TASK_REPEAT_MODES.REPEAT_MODE_DEFAULT
async function AddDefaultRepeats() {
task.value.repeats = 'FREQ=DAILY;INTERVAL=1'
Now I‘m more convinced: this should be already a parsed object here and only parsed and transformed in the model / when sending to api.
@ -164,3 +164,3 @@
Title: "Task 7",
DueDate: testtimeTime,
RepeatAfter: 60 * 60 * 24 * 7, // The amount of seconds in a week
Repeats: "FREQ=WEEKLY;INTERVAL=1"
I love RRules 🥳
@ -2470,7 +2470,7 @@ const docTemplate = `{
},
{
"type": "string",
"description": "The sorting parameter. You can pass this multiple times to get the tasks ordered by multiple different parametes, along with ` + "`" + `order_by` + "`" + `. Possible values to sort by are ` + "`" + `id` + "`" + `, ` + "`" + `title` + "`" + `, ` + "`" + `description` + "`" + `, ` + "`" + `done` + "`" + `, ` + "`" + `done_at` + "`" + `, ` + "`" + `due_date` + "`" + `, ` + "`" + `created_by_id` + "`" + `, ` + "`" + `project_id` + "`" + `, ` + "`" + `repeat_after` + "`" + `, ` + "`" + `priority` + "`" + `, ` + "`" + `start_date` + "`" + `, ` + "`" + `end_date` + "`" + `, ` + "`" + `hex_color` + "`" + `, ` + "`" + `percent_done` + "`" + `, ` + "`" + `uid` + "`" + `, ` + "`" + `created` + "`" + `, ` + "`" + `updated` + "`" + `. Default is ` + "`" + `id` + "`" + `.",
I might be mistaken, but from a Quick Look it seems like there is now no documentation regarding the RRule type of the new repeats.
We don’t have to reinvent the wheel and can link to existing documentation. We should also document what parts are supported and what parts are not (I got the impression that we can only handle a subset of the features.
@ -3321,3 +3287,3 @@
the tasks ordered by multiple different parametes, along with `order_by`.
Possible values to sort by are `id`, `title`, `description`, `done`, `done_at`,
`due_date`, `created_by_id`, `project_id`, `repeat_after`, `priority`, `start_date`,
`due_date`, `created_by_id`, `project_id`, `repeat_s`, `priority`, `start_date`,
typo?
huh! The swagger docs are automatically generated, this looks like a manual edit?
Hey @BeatLink are you still interested in this?
Hi. Unfortunately, I've switched to another todo app. The feature is fully implemented however, minus the needed changes. If someone wants to pick up where I left off, have at it.
Checkout
From your project repository, check out a new branch and test the changes.