vikunja
/
vikunja
6
43
Fork 72
Code Issues 150 Pull Requests 19 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 +125 -50
7 changed files with 125 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e5cec4bd16 - Show all commits

6
pkg/models/task_reminder.go
View File

@ -38,9 +38,9 @@ type ReminderRelation string
// All valid ReminderRelations
const (
DueDate ReminderRelation = `due_date`
StartDate ReminderRelation = `start_date`
EndDate ReminderRelation = `end_date`
ReminderDueDate ReminderRelation = `due_date`
ReminderStartDate ReminderRelation = `start_date`
ReminderEndDate ReminderRelation = `end_date`
)
// TaskReminder holds a reminder on a task

30
pkg/models/tasks.go
View File

@ -979,7 +979,7 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
}
// Update the reminders
if err := t.updateReminders(s, t.Reminders); err != nil {
if err := t.updateReminders(s, t.Reminders, t.DueDate, t.StartDate, t.EndDate); 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
}
@ -1072,7 +1072,7 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
}
// Update the reminders
if err := ot.updateReminders(s, t.Reminders); err != nil {
if err := ot.updateReminders(s, t.Reminders, t.DueDate, t.StartDate, t.EndDate); 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
}
@ -1525,8 +1525,8 @@ func (t *Task) updateRemindersFromReminderDates(reminderDates []time.Time) {
// Removes all old reminders and adds the new ones. This is a lot easier and less buggy than
// trying to figure out which reminders changed and then only re-add those needed. And since it does
ce72 marked this conversation as resolved Outdated
konrad commented 2023-03-08 13:24:03 +00:00
Outdated
Review
Copy Link

We should add a validation so that it's not possible to create a relative reminder without telling it what it's relative to (probably not here).

We should add a validation so that it's not possible to create a relative reminder without telling it what it's relative to (probably not here).
ce72 commented 2023-03-08 15:09:28 +00:00
Outdated
Review
Copy Link

That would not break anything. There would just be a useless half-of-a-reminder (which maybe could be edited later). Where should we add a validation, if?

That would not break anything. There would just be a useless half-of-a-reminder (which maybe could be edited later). Where should we add a validation, if?
konrad commented 2023-03-10 15:54:17 +00:00
Outdated
Review
Copy Link

Since there's no dedicated endpoint just for reminders, I think right here is a good place for a validation. I think updateRelativeReminderDates is called when creating or updating a task?

Since there's no dedicated endpoint just for reminders, I think right here is a good place for a validation. I think `updateRelativeReminderDates` is called when creating or updating a task?
ce72 commented 2023-03-13 06:22:15 +00:00
Outdated
Review
Copy Link

ok

ok
// not make a performance difference we'll just do that.
// The parameter is a slice with unix dates which holds the new reminders.
func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder) (err error) {
// The parameter is a slice which holds the new reminders.
func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder, dueDate time.Time, startDate time.Time, endDate time.Time) (err error) {
_, err = s.
Where("task_id = ?", t.ID).
@ -1535,6 +1535,8 @@ func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder) (err
return
}
setRelativeReminderDates(reminders, dueDate, startDate, endDate)
// Resolve duplicates and sort them
reminderMap := make(map[int64]TaskReminder, len(reminders))
for _, reminder := range reminders {
@ -1564,6 +1566,26 @@ func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder) (err
return
}
func setRelativeReminderDates(reminders []*TaskReminder, dueDate time.Time, startDate time.Time, endDate time.Time) {
for _, reminder := range reminders {
relativeDuration := time.Duration(reminder.RelativePeriod) * time.Second
switch reminder.RelativeTo {
case ReminderDueDate:
if !dueDate.IsZero() {
reminder.Reminder = dueDate.Add(relativeDuration)
}
case ReminderStartDate:
if !startDate.IsZero() {
reminder.Reminder = startDate.Add(relativeDuration)
}
case ReminderEndDate:
if !endDate.IsZero() {
reminder.Reminder = endDate.Add(relativeDuration)
konrad marked this conversation as resolved Outdated
konrad commented 2023-03-08 13:26:12 +00:00
Outdated
Review
Copy Link

Why sort them here? Just for the updated task response?

Why sort them here? Just for the updated task response?
ce72 commented 2023-03-08 14:59:36 +00:00
Outdated
Review
Copy Link

Yes. It simplifies the tests too.

Yes. It simplifies the tests too.
👍 1
}
}
}
}
func updateTaskLastUpdated(s *xorm.Session, task *Task) error {
_, err := s.ID(task.ID).Cols("updated").Update(task)
return err

119
pkg/models/tasks_test.go
View File

@ -70,6 +70,44 @@ func TestTask_Create(t *testing.T) {
events.AssertDispatched(t, &TaskCreatedEvent{})
})
t.Run("with reminders", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
defer s.Close()
task := &Task{
Title: "Lorem",
Description: "Lorem Ipsum Dolor",
ListID: 1,
DueDate: time.Unix(1550000000, 0),
StartDate: time.Unix(1550000010, 0),
EndDate: time.Unix(1550000020, 0),
Reminders: []*TaskReminder{
{
RelativeTo: "due_date",
RelativePeriod: 1,
},
{
RelativeTo: "start_date",
RelativePeriod: -2,
},
{
RelativeTo: "end_date",
RelativePeriod: -1,
},
{
Reminder: time.Unix(1550000020, 0),
},
}}
err := task.Create(s, usr)
assert.NoError(t, err)
assert.Equal(t, task.Reminders[0].Reminder, time.Unix(1550000001, 0))
assert.Equal(t, task.Reminders[1].Reminder, time.Unix(1550000008, 0))
assert.Equal(t, task.Reminders[2].Reminder, time.Unix(1550000019, 0))
assert.Equal(t, task.Reminders[3].Reminder, time.Unix(1550000020, 0))
err = s.Commit()
assert.NoError(t, err)
})
t.Run("empty title", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
@ -369,8 +407,47 @@ func TestTask_Update(t *testing.T) {
assert.Equal(t, int64(3), task.Index)
})
// Deprecated: see next test
t.Run("the same reminder date multiple times should be saved once", func(t *testing.T) {
t.Run("reminders will be updated", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
defer s.Close()
task := &Task{
ID: 1,
ListID: 1,
Title: "test",
DueDate: time.Unix(1550000000, 0),
StartDate: time.Unix(1550000010, 0),
EndDate: time.Unix(1550000020, 0),
Reminders: []*TaskReminder{
{
RelativeTo: "due_date",
RelativePeriod: 1,
},
{
RelativeTo: "start_date",
RelativePeriod: -2,
},
{
RelativeTo: "end_date",
RelativePeriod: -1,
},
{
Reminder: time.Unix(1550000020, 0),
},
},
}
err := task.Update(s, u)
assert.NoError(t, err)
assert.Equal(t, task.Reminders[0].Reminder, time.Unix(1550000001, 0))
assert.Equal(t, task.Reminders[1].Reminder, time.Unix(1550000008, 0))
assert.Equal(t, task.Reminders[2].Reminder, time.Unix(1550000019, 0))
assert.Equal(t, task.Reminders[3].Reminder, time.Unix(1550000020, 0))
err = s.Commit()
assert.NoError(t, err)
db.AssertCount(t, "task_reminders", builder.Eq{"task_id": 1}, 4)
})
t.Run("the same reminder multiple times should be saved once", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
defer s.Close()
@ -378,9 +455,13 @@ func TestTask_Update(t *testing.T) {
task := &Task{
ID: 1,
Title: "test",
ReminderDates: []time.Time{
time.Unix(1674745156, 0),
time.Unix(1674745156, 223),
Reminders: []*TaskReminder{
{
Reminder: time.Unix(1674745156, 0),
},
{
Reminder: time.Unix(1674745156, 223),
},
},
ListID: 1,
}
@ -388,34 +469,6 @@ func TestTask_Update(t *testing.T) {
assert.NoError(t, err)
err = s.Commit()
assert.NoError(t, err)
db.AssertCount(t, "task_reminders", builder.Eq{"task_id": 1}, 1)
})
t.Run("the same reminder multiple times should be saved once", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
defer s.Close()
taskReminders := []*TaskReminder{
{
Reminder: time.Unix(1674745156, 0),
},
{
Reminder: time.Unix(1674745156, 223),
},
}
task := &Task{
ID: 1,
Title: "test",
Reminders: taskReminders,
ListID: 1,
}
err := task.Update(s, u)
assert.NoError(t, err)
err = s.Commit()
assert.NoError(t, err)
db.AssertCount(t, "task_reminders", builder.Eq{"task_id": 1}, 1)
})
}

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

@ -149,7 +149,7 @@ func convertTickTickToVikunja(tasks []*tickTickTask) (result []*models.Namespace
if !t.DueDate.IsZero() && t.Reminder > 0 {
task.Task.Reminders = []*models.TaskReminder{
{
RelativeTo: models.DueDate,
RelativeTo: models.ReminderDueDate,
RelativePeriod: int64((t.Reminder * -1).Seconds()),
},
}

6
pkg/swagger/docs.go
View File

@ -8317,9 +8317,9 @@ const docTemplate = `{
"end_date"
],
"x-enum-varnames": [
"DueDate",
"StartDate",
"EndDate"
"ReminderDueDate",
"ReminderStartDate",
"ReminderEndDate"
]
},
"models.Right": {

6
pkg/swagger/swagger.json
View File

@ -8309,9 +8309,9 @@
"end_date"
],
"x-enum-varnames": [
"DueDate",
"StartDate",
"EndDate"
"ReminderDueDate",
"ReminderStartDate",
"ReminderEndDate"
]
},
"models.Right": {

6
pkg/swagger/swagger.yaml
View File

@ -609,9 +609,9 @@ definitions:
- end_date
type: string
x-enum-varnames:
- DueDate
- StartDate
- EndDate
- ReminderDueDate
- ReminderStartDate
- ReminderEndDate
models.Right:
enum:
- 0