vikunja/vikunja
6
43
Fork 73
Code Issues 150 Pull Requests 21 Packages Releases 32 Activity

#1416: Add relative Reminders #1427

Merged
konrad merged 21 commits from ce72/api:1416_reminders into main 2023-03-27 20:07:08 +00:00
Conversation 9 Commits 21 Files Changed 21 +265 -124
13 changed files with 265 additions and 124 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e0487516a3 - Show all commits

2
pkg/db/fixtures/task_reminders.yml
View File

@ -6,6 +6,8 @@
task_id: 27
reminder: 2018-12-01 01:13:44
created: 2018-12-01 01:12:04
relative_to: 'start_date'
relative_period: -3600
- id: 3
task_id: 2
reminder: 2018-12-01 01:13:44

3
pkg/db/fixtures/tasks.yml
View File

@ -244,7 +244,7 @@
created: 2018-12-01 01:12:04
updated: 2018-12-01 01:12:04
- id: 27
title: 'task #27 with reminders'
title: 'task #27 with reminders and start_date'
done: false
created_by_id: 1
list_id: 1
@ -252,6 +252,7 @@
bucket_id: 1
created: 2018-12-01 01:12:04
updated: 2018-12-01 01:12:04
start_date: 2018-11-30 22:25:24
- id: 28
title: 'task #28 with repeat after'
done: false

176
pkg/integrations/task_collection_test.go
View File

@ -42,14 +42,14 @@ func TestTaskCollection(t *testing.T) {
assert.NoError(t, err)
// Not using assert.Equal to avoid having the tests break every time we add new fixtures
assert.Contains(t, rec.Body.String(), `task #1`)
assert.Contains(t, rec.Body.String(), `task #2`)
assert.Contains(t, rec.Body.String(), `task #3`)
assert.Contains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.Contains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.Contains(t, rec.Body.String(), `task #2 `)
assert.Contains(t, rec.Body.String(), `task #3 `)
assert.Contains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.Contains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
konrad marked this conversation as resolved
konrad commented 2023-03-08 13:14:44 +00:00
Review
Copy Link

Why the extra space?

Why the extra space?
ce72 commented 2023-03-08 14:57:45 +00:00
Review
Copy Link

Because you are basing assertions on comparing parts of json responses. Thus expected="task #2" matches "task #2" and "task #20"...
I can revert most of the changes though. Should I?

Because you are basing assertions on comparing parts of json responses. Thus expected="task #2" matches "task #2" and "task #20"... I can revert most of the changes though. Should I?
konrad commented 2023-03-08 14:58:21 +00:00
Review
Copy Link

Ah okay that makes sense. Should be fine leave it like this.

Ah okay that makes sense. Should be fine leave it like this.
assert.Contains(t, rec.Body.String(), `task #10`)
assert.Contains(t, rec.Body.String(), `task #11`)
assert.Contains(t, rec.Body.String(), `task #12`)
@ -75,14 +75,14 @@ func TestTaskCollection(t *testing.T) {
rec, err := testHandler.testReadAllWithUser(url.Values{"s": []string{"task #6"}}, urlParams)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.NotContains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.NotContains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.NotContains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.NotContains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.NotContains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.NotContains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -93,14 +93,14 @@ func TestTaskCollection(t *testing.T) {
rec, err := testHandler.testReadAllWithUser(url.Values{"s": []string{"tASk #6"}}, urlParams)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.NotContains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.NotContains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.NotContains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.NotContains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.NotContains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.NotContains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -190,14 +190,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.Contains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.Contains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -215,14 +215,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.NotContains(t, rec.Body.String(), `task #5`)
assert.NotContains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.NotContains(t, rec.Body.String(), `task #5 `)
assert.NotContains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -255,14 +255,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -291,14 +291,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.Contains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.Contains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -314,14 +314,14 @@ func TestTaskCollection(t *testing.T) {
assert.NoError(t, err)
// Not using assert.Equal to avoid having the tests break every time we add new fixtures
assert.Contains(t, rec.Body.String(), `task #1`)
assert.Contains(t, rec.Body.String(), `task #2`)
assert.Contains(t, rec.Body.String(), `task #3`)
assert.Contains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.Contains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.Contains(t, rec.Body.String(), `task #2 `)
assert.Contains(t, rec.Body.String(), `task #3 `)
assert.Contains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.Contains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.Contains(t, rec.Body.String(), `task #10`)
assert.Contains(t, rec.Body.String(), `task #11`)
assert.Contains(t, rec.Body.String(), `task #12`)
@ -347,14 +347,14 @@ func TestTaskCollection(t *testing.T) {
rec, err := testHandler.testReadAllWithUser(url.Values{"s": []string{"task #6"}}, nil)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.NotContains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.NotContains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.NotContains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.NotContains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.NotContains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.NotContains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -417,14 +417,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.Contains(t, rec.Body.String(), `task #5`)
assert.Contains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.Contains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.Contains(t, rec.Body.String(), `task #5 `)
assert.Contains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.Contains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)
@ -442,14 +442,14 @@ func TestTaskCollection(t *testing.T) {
)
assert.NoError(t, err)
assert.NotContains(t, rec.Body.String(), `task #1`)
assert.NotContains(t, rec.Body.String(), `task #2`)
assert.NotContains(t, rec.Body.String(), `task #3`)
assert.NotContains(t, rec.Body.String(), `task #4`)
assert.NotContains(t, rec.Body.String(), `task #5`)
assert.NotContains(t, rec.Body.String(), `task #6`)
assert.Contains(t, rec.Body.String(), `task #7`)
assert.NotContains(t, rec.Body.String(), `task #8`)
assert.Contains(t, rec.Body.String(), `task #9`)
assert.NotContains(t, rec.Body.String(), `task #2 `)
assert.NotContains(t, rec.Body.String(), `task #3 `)
assert.NotContains(t, rec.Body.String(), `task #4 `)
assert.NotContains(t, rec.Body.String(), `task #5 `)
assert.NotContains(t, rec.Body.String(), `task #6 `)
assert.Contains(t, rec.Body.String(), `task #7 `)
assert.NotContains(t, rec.Body.String(), `task #8 `)
assert.Contains(t, rec.Body.String(), `task #9 `)
assert.NotContains(t, rec.Body.String(), `task #10`)
assert.NotContains(t, rec.Body.String(), `task #11`)
assert.NotContains(t, rec.Body.String(), `task #12`)

10
pkg/integrations/task_test.go
View File

@ -117,24 +117,24 @@ func TestTask(t *testing.T) {
assert.NotContains(t, rec.Body.String(), `"reminder_dates":[1543626724,1543626824]`)
})
t.Run("Reminders", func(t *testing.T) {
rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "1"}, `{"reminders": [{"Reminder": "2020-02-10T10:00:00Z"},{"Reminder": "2020-02-11T10:00:00Z"}]}`)
rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "1"}, `{"reminders": [{"reminder": "2020-02-10T10:00:00Z"},{"reminder": "2020-02-11T10:00:00Z"}]}`)
assert.NoError(t, err)
assert.Contains(t, rec.Body.String(), `"reminders":[`)
assert.Contains(t, rec.Body.String(), `{"Reminder":"2020-02-10T10:00:00Z"}`)
assert.Contains(t, rec.Body.String(), `{"Reminder":"2020-02-11T10:00:00Z"}`)
assert.Contains(t, rec.Body.String(), `{"reminder":"2020-02-10T10:00:00Z"`)
assert.Contains(t, rec.Body.String(), `{"reminder":"2020-02-11T10:00:00Z"`)
assert.NotContains(t, rec.Body.String(), `"reminders":null`)
})
t.Run("Reminders unset to empty array", func(t *testing.T) {
rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "27"}, `{"reminders": []}`)
assert.NoError(t, err)
assert.Contains(t, rec.Body.String(), `"reminders":null`)
assert.NotContains(t, rec.Body.String(), `{"Reminder":"2020-02-10T10:00:00Z"}`)
assert.NotContains(t, rec.Body.String(), `{"Reminder":"2020-02-10T10:00:00Z"`)
})
t.Run("Reminders unset to null", func(t *testing.T) {
rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "27"}, `{"reminders": null}`)
assert.NoError(t, err)
assert.Contains(t, rec.Body.String(), `"reminder_dates":null`)
assert.NotContains(t, rec.Body.String(), `{"Reminder":"2020-02-10T10:00:00Z"}`)
assert.NotContains(t, rec.Body.String(), `{"Reminder":"2020-02-10T10:00:00Z"`)
})
t.Run("Repeat after", func(t *testing.T) {
rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "1"}, `{"repeat_after":3600}`)

44
pkg/migration/20230307171848.go Normal file
View File

@ -0,0 +1,44 @@
// Vikunja is a to-do list application to facilitate your life.
// Copyright 2018-2021 Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public Licensee as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public Licensee for more details.
//
// You should have received a copy of the GNU Affero General Public Licensee
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package migration
import (
"src.techknowlogick.com/xormigrate"
"xorm.io/xorm"
)
type taskReminders20230307171848 struct {
RelativePeriod int64 `xorm:"bigint null" json:"relative_period"`
RelativeTo string `xorm:"varchar(50) null" json:"relative_to,omitempty"`
}
ce72 marked this conversation as resolved Outdated
ce72 commented 2023-03-07 17:04:28 +00:00
Outdated
Review
Copy Link

Just json:"reminder" was added to this line. Do we need to include that in the migration script?

Just `json:"reminder"` was added to this line. Do we need to include that in the migration script?
konrad commented 2023-03-07 17:14:41 +00:00
Outdated
Review
Copy Link

No, the json declarations are usually ignored.

No, the json declarations are usually ignored.
👍 1
func (taskReminders20230307171848) TableName() string {
return "task_reminders"
}
func init() {
migrations = append(migrations, &xormigrate.Migration{
ID: "20230307171848",
Description: "Add relative period to task reminders",
Migrate: func(tx *xorm.Engine) error {
return tx.Sync2(taskReminders20230307171848{})
},
Rollback: func(tx *xorm.Engine) error {
return nil
},
})
}

15
pkg/models/task_collection_test.go
View File

@ -480,7 +480,7 @@ func TestTaskCollection_ReadAll(t *testing.T) {
}
task27 := &Task{
ID: 27,
Title: "task #27 with reminders",
Title: "task #27 with reminders and start_date",
Identifier: "test1-12",
Index: 12,
CreatedByID: 1,
@ -497,12 +497,15 @@ func TestTaskCollection_ReadAll(t *testing.T) {
Created: time.Unix(1543626724, 0).In(loc),
},
{
ID: 2,
TaskID: 27,
Reminder: time.Unix(1543626824, 0).In(loc),
Created: time.Unix(1543626724, 0).In(loc),
ID: 2,
TaskID: 27,
Reminder: time.Unix(1543626824, 0).In(loc),
Created: time.Unix(1543626724, 0).In(loc),
RelativePeriod: -3600,
RelativeTo: "start_date",
},
},
StartDate: time.Unix(1543616724, 0).In(loc),
ListID: 1,
BucketID: 1,
RelatedTasks: map[RelationKind][]*Task{},
@ -1268,7 +1271,7 @@ func TestTaskCollection_ReadAll(t *testing.T) {
return
}
t.Errorf("Test %s, Task.ReadAll() = %v, want %v, \ndiff: %v", tt.name, got, tt.want, diff)
t.Errorf("Test %s, Task.ReadAll() = %v, \nwant %v, \ndiff: %v", tt.name, got, tt.want, diff)
}
})
}

16
pkg/models/task_reminder.go
View File

@ -33,13 +33,27 @@ import (
"code.vikunja.io/api/pkg/user"
)
// ReminderRelation represents the task attribute which the period based reminder relates to
type ReminderRelation string
// All valid ReminderRelations
const (
DueDate ReminderRelation = `due_date`
StartDate ReminderRelation = `start_date`
EndDate ReminderRelation = `end_date`
)
// TaskReminder holds a reminder on a task
type TaskReminder struct {
ID int64 `xorm:"bigint autoincr not null unique pk" json:"-"`
TaskID int64 `xorm:"bigint not null INDEX" json:"-"`
// The absolute time when the user wants to be reminded of the task.
Reminder time.Time `xorm:"DATETIME not null INDEX 'reminder'"`
Reminder time.Time `xorm:"DATETIME not null INDEX 'reminder'" json:"reminder"`
Created time.Time `xorm:"created not null" json:"-"`
// A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date, positive after.
RelativePeriod int64 `xorm:"bigint null" json:"relative_period"`
// The name of the date field to which the relative period refers to.
RelativeTo ReminderRelation `xorm:"varchar(50) null" json:"relative_to"`
}
// TableName returns a pretty table name

41
pkg/models/tasks.go
View File

@ -973,14 +973,12 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
}
}
// Update the reminders
//
// Deprecated: the if clause can be removed if ReminderDates will be removed
// Deprecated: the if clause can be removed when ReminderDates will be removed
if t.ReminderDates != nil {
if err := t.updateRemindersFromReminderDates(t.ReminderDates); err != nil {
return err
}
t.updateRemindersFromReminderDates(t.ReminderDates)
}
// Update the reminders
if err := t.updateReminders(s, t.Reminders); err != nil {
ce72 marked this conversation as resolved Outdated
konrad commented 2023-03-08 13:19:47 +00:00
Outdated
Review
Copy Link

What about passing only the task? (As the method is called on a task we can avoid the many parameters that way)

What about passing only the task? (As the method is called on a task we can avoid the many parameters that way)
ce72 commented 2023-03-08 14:58:05 +00:00
Outdated
Review
Copy Link

ok

ok
return err
}
@ -1068,12 +1066,11 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
return err
}
// Deprecated: This statement can be removed if ReminderDates will be removed
// Deprecated: This statement can be removed when ReminderDates will be removed
if t.ReminderDates != nil {
if err := t.updateRemindersFromReminderDates(t.ReminderDates); err != nil {
return err
}
t.updateRemindersFromReminderDates(t.ReminderDates)
}
// Update the reminders
if err := ot.updateReminders(s, t.Reminders); err != nil {
ce72 marked this conversation as resolved Outdated
konrad commented 2023-03-08 13:22:43 +00:00
Outdated
Review
Copy Link

How does the other function override the new reminders if both are sent?

Can we maybe move the logic into updateReminders alltogether? I feel like this is now duplicated in too many places.

How does the other function override the new reminders if both are sent? Can we maybe move the logic into `updateReminders` alltogether? I feel like this is now duplicated in too many places.
ce72 commented 2023-03-08 15:06:39 +00:00
Outdated
Review
Copy Link

ok

ok
return err
@ -1505,18 +1502,24 @@ func updateDone(oldTask *Task, newTask *Task) {
}
// Deprecated: will be removed when ReminderDates are removed from Task.
// For now the method just creates Taskeminder Objects from the ReminderDates
func (t *Task) updateRemindersFromReminderDates(reminders []time.Time) (err error) {
// For now the method just creates Taskeminder objects from the ReminderDates
func (t *Task) updateRemindersFromReminderDates(reminderDates []time.Time) {
// if the client still sends old reminder_dates, then these will overwrite the absolute triggers
// of the Reminders, sent by the client. We assume that clients still sending old reminder_dates do not
// understand the new reminders.
for _, reminder := range reminders {
t.Reminders = append(t.Reminders, &TaskReminder{TaskID: t.ID, Reminder: reminder})
// remove absolute triggers in Reminders
updatedReminders := make([]*TaskReminder, 0)
for _, reminder := range t.Reminders {
if reminder.RelativeTo != "" {
updatedReminders = append(updatedReminders, reminder)
}
}
t.ReminderDates = reminders
if len(reminders) == 0 {
t.ReminderDates = nil
// append absolute triggers from ReminderDates
for _, reminderDate := range reminderDates {
updatedReminders = append(updatedReminders, &TaskReminder{TaskID: t.ID, Reminder: reminderDate})
}
konrad marked this conversation as resolved Outdated
konrad commented 2023-03-26 15:40:12 +00:00
Outdated
Review
Copy Link

I think we can get rid of all the else clauses and move them before the switch statements so that the logic inside the switch will override the empty reminder time.

I think we can get rid of all the `else` clauses and move them before the `switch` statements so that the logic inside the switch will override the empty reminder time.
konrad commented 2023-03-26 15:42:51 +00:00
Outdated
Review
Copy Link

Also, what should happen in case the reminder date is an empty time.Time? Isn't that an invalid case and should be filtered out? Or better, fail with an error message so that the user knows why it ignores the reminder?

Also, what should happen in case the reminder date is an empty `time.Time`? Isn't that an invalid case and should be filtered out? Or better, fail with an error message so that the user knows why it ignores the reminder?
ce72 commented 2023-03-26 18:17:52 +00:00
Outdated
Review
Copy Link

I think we can get rid of all the else clauses and move them before the switch statements so that the logic inside the switch will override the empty reminder time.

Ok. Not sure if it's better, but ok.

Also, what should happen in case the reminder date is an empty time.Time? Isn't that an invalid case and should be filtered out? Or better, fail with an error message so that the user knows why it ignores the reminder?

The current implementation permits users to enter a reminder relative to due date and set the due date after that. I don't think we should ban this use case and I don't think the api should be too restrictive here.
An empty reminder date is still valid (actually it has fired at 01.01.0001) and it may be updated to a current value, if the due_date is entered later. In any case the situation is always visible at the frontend, because inactive reminders are rendered in red. So I would strongly vote against an additional validation.

> I think we can get rid of all the `else` clauses and move them before the `switch` statements so that the logic inside the switch will override the empty reminder time. Ok. Not sure if it's better, but ok. > Also, what should happen in case the reminder date is an empty `time.Time`? Isn't that an invalid case and should be filtered out? Or better, fail with an error message so that the user knows why it ignores the reminder? The current implementation permits users to enter a reminder relative to due date and set the due date after that. I don't think we should ban this use case and I don't think the api should be too restrictive here. An empty reminder date is still valid (actually it has fired at 01.01.0001) and it may be updated to a current value, if the due_date is entered later. In any case the situation is always visible at the frontend, because inactive reminders are rendered in red. So I would strongly vote against an additional validation.
konrad commented 2023-03-27 07:49:08 +00:00
Outdated
Review
Copy Link

The current implementation permits users to enter a reminder relative to due date and set the due date after that.

Okay, that makes a lot of sense. I didn't think of that. If we make this clear in the frontend it is fine to leave it the way it currently is.

> The current implementation permits users to enter a reminder relative to due date and set the due date after that. Okay, that makes a lot of sense. I didn't think of that. If we make this clear in the frontend it is fine to leave it the way it currently is.
return
t.Reminders = updatedReminders
}
// Removes all old reminders and adds the new ones. This is a lot easier and less buggy than

7
pkg/modules/migration/ticktick/ticktick.go
View File

@ -147,8 +147,11 @@ func convertTickTickToVikunja(tasks []*tickTickTask) (result []*models.Namespace
}
if !t.DueDate.IsZero() && t.Reminder > 0 {
task.Task.ReminderDates = []time.Time{
t.DueDate.Add(t.Reminder * -1),
task.Task.Reminders = []*models.TaskReminder{
{
RelativeTo: models.DueDate,
RelativePeriod: int64((t.Reminder * -1).Seconds()),
},
}
}

6
pkg/modules/migration/ticktick/ticktick_test.go
View File

@ -101,7 +101,8 @@ func TestConvertTicktickTasksToVikunja(t *testing.T) {
{Title: "label1"},
{Title: "label2"},
})
//assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].ReminderDates, tickTickTasks[0].) // TODO
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].Reminders[0].RelativeTo, models.ReminderRelation("due_date"))
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].Reminders[0].RelativePeriod, int64(-24*3600))
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].Position, tickTickTasks[0].Order)
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].Done, false)
@ -127,7 +128,8 @@ func TestConvertTicktickTasksToVikunja(t *testing.T) {
{Title: "label2"},
{Title: "other label"},
})
//assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[0].ReminderDates, tickTickTasks[0].) // TODO
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[2].Reminders[0].RelativeTo, models.ReminderRelation("due_date"))
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[2].Reminders[0].RelativePeriod, int64(-24*3600))
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[2].Position, tickTickTasks[2].Order)
assert.Equal(t, vikunjaTasks[0].Lists[0].Tasks[2].Done, false)

25
pkg/swagger/docs.go
View File

@ -8309,6 +8309,19 @@ const docTemplate = `{
"RelationKindCopiedTo"
]
},
"models.ReminderRelation": {
"type": "string",
"enum": [
"due_date",
"start_date",
"end_date"
],
"x-enum-varnames": [
"DueDate",
"StartDate",
"EndDate"
]
},
"models.Right": {
"type": "integer",
"enum": [
@ -8707,6 +8720,18 @@ const docTemplate = `{
"models.TaskReminder": {
"type": "object",
"properties": {
"relative_period": {
"description": "A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date, positive after.",
"type": "integer"
},
"relative_to": {
"description": "The name of the date field to which the relative period refers to.",
"allOf": [
{
"$ref": "#/definitions/models.ReminderRelation"
}
]
},
"reminder": {
"description": "The absolute time when the user wants to be reminded of the task.",
"type": "string"

25
pkg/swagger/swagger.json
View File

@ -8301,6 +8301,19 @@
"RelationKindCopiedTo"
]
},
"models.ReminderRelation": {
"type": "string",
"enum": [
"due_date",
"start_date",
"end_date"
],
"x-enum-varnames": [
"DueDate",
"StartDate",
"EndDate"
]
},
"models.Right": {
"type": "integer",
"enum": [
@ -8699,6 +8712,18 @@
"models.TaskReminder": {
"type": "object",
"properties": {
"relative_period": {
"description": "A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date, positive after.",
"type": "integer"
},
"relative_to": {
"description": "The name of the date field to which the relative period refers to.",
"allOf": [
{
"$ref": "#/definitions/models.ReminderRelation"
}
]
},
"reminder": {
"description": "The absolute time when the user wants to be reminded of the task.",
"type": "string"

19
pkg/swagger/swagger.yaml
View File

@ -602,6 +602,16 @@ definitions:
- RelationKindFollows
- RelationKindCopiedFrom
- RelationKindCopiedTo
models.ReminderRelation:
enum:
- due_date
- start_date
- end_date
type: string
x-enum-varnames:
- DueDate
- StartDate
- EndDate
models.Right:
enum:
- 0
@ -904,6 +914,15 @@ definitions:
type: object
models.TaskReminder:
properties:
relative_period:
description: A period in seconds relative to another date argument. Negative
values mean the reminder triggers before the date, positive after.
type: integer
relative_to:
allOf:
- $ref: '#/definitions/models.ReminderRelation'
description: The name of the date field to which the relative period refers
to.
reminder:
description: The absolute time when the user wants to be reminded of the task.
type: string