#1416: Add relative Reminders #1427
|
@ -33,24 +33,26 @@ import (
|
|||
"code.vikunja.io/api/pkg/user"
|
||||
)
|
||||
|
||||
// ReminderRelation represents the task attribute which the period based reminder relates to
|
||||
// ReminderRelation represents the date attribute of the task which a period based reminder relates to
|
||||
type ReminderRelation string
|
||||
|
||||
// All valid ReminderRelations
|
||||
const (
|
||||
ReminderDueDate ReminderRelation = `due_date`
|
||||
ReminderStartDate ReminderRelation = `start_date`
|
||||
ReminderEndDate ReminderRelation = `end_date`
|
||||
ReminderRelationDueDate ReminderRelation = `due_date`
|
||||
ReminderRelationStartDate ReminderRelation = `start_date`
|
||||
ReminderRelationEndDate ReminderRelation = `end_date`
|
||||
)
|
||||
|
||||
// TaskReminder holds a reminder on a task
|
||||
// TaskReminder holds a reminder on a task.
|
||||
// If RelativeTo and the assciated date field are defined, then the attribute Reminder will be computed.
|
||||
// If RelativeTo is missing, than Reminder must be given.
|
||||
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'" 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.
|
||||
// A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date. Default: 0, tiggers when RelativeTo is due.
|
||||
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"`
|
||||
|
|
|
@ -63,7 +63,7 @@ type Task struct {
|
|||
DueDate time.Time `xorm:"DATETIME INDEX null 'due_date'" json:"due_date"`
|
||||
// An array of datetimes when the user wants to be reminded of the task.
|
||||
//
|
||||
// Deprecated: Use Reminders[]
|
||||
// Deprecated: Use Reminders
|
||||
ReminderDates []time.Time `xorm:"-" json:"reminder_dates"`
|
||||
// An array of reminders that are associated with this task.
|
||||
Reminders []*TaskReminder `xorm:"-" json:"reminders"`
|
||||
|
@ -975,7 +975,7 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
|
|||
|
||||
// Deprecated: the if clause can be removed when ReminderDates will be removed
|
||||
if t.ReminderDates != nil {
|
||||
t.updateRemindersFromReminderDates(t.ReminderDates)
|
||||
t.overwriteRemindersWithReminderDates(t.ReminderDates)
|
||||
}
|
||||
|
||||
// Update the reminders
|
||||
|
@ -1068,7 +1068,7 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
|
|||
|
||||
// Deprecated: This statement can be removed when ReminderDates will be removed
|
||||
if t.ReminderDates != nil {
|
||||
t.updateRemindersFromReminderDates(t.ReminderDates)
|
||||
t.overwriteRemindersWithReminderDates(t.ReminderDates)
|
||||
}
|
||||
|
||||
// Update the reminders
|
||||
|
@ -1502,24 +1502,42 @@ 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(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 now the method just creates TaskReminder objects from the ReminderDates and overwrites Reminder.
|
||||
func (t *Task) overwriteRemindersWithReminderDates(reminderDates []time.Time) {
|
||||
// If the client still sends old reminder_dates, then these will overwrite
|
||||
// the Reminders, if the were sent by the client, too.
|
||||
// We assume that clients still using the old API with reminder_dates do not understand the new reminders.
|
||||
// Clients who want to use the new Reminder structure must explicitey unset reminder_dates.
|
||||
|
||||
// start with empty Reminders
|
||||
reminders := make([]*TaskReminder, 0)
|
||||
|
||||
// remove absolute triggers in Reminders
|
||||
updatedReminders := make([]*TaskReminder, 0)
|
||||
for _, reminder := range t.Reminders {
|
||||
if reminder.RelativeTo != "" {
|
||||
updatedReminders = append(updatedReminders, reminder)
|
||||
}
|
||||
}
|
||||
// append absolute triggers from ReminderDates
|
||||
for _, reminderDate := range reminderDates {
|
||||
updatedReminders = append(updatedReminders, &TaskReminder{TaskID: t.ID, Reminder: reminderDate})
|
||||
reminders = append(reminders, &TaskReminder{TaskID: t.ID, Reminder: reminderDate})
|
||||
}
|
||||
t.Reminders = reminders
|
||||
}
|
||||
|
||||
konrad marked this conversation as resolved
Outdated
|
||||
// Set the absolute trigger dates for Reminders with relative period
|
||||
func updateRelativeReminderDates(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 {
|
||||
ce72 marked this conversation as resolved
Outdated
konrad
commented
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
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
Since there's no dedicated endpoint just for reminders, I think right here is a good place for a validation. I think 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
ok ok
|
||||
case ReminderRelationDueDate:
|
||||
if !dueDate.IsZero() {
|
||||
reminder.Reminder = dueDate.Add(relativeDuration)
|
||||
}
|
||||
case ReminderRelationStartDate:
|
||||
if !startDate.IsZero() {
|
||||
reminder.Reminder = startDate.Add(relativeDuration)
|
||||
}
|
||||
case ReminderRelationEndDate:
|
||||
if !endDate.IsZero() {
|
||||
reminder.Reminder = endDate.Add(relativeDuration)
|
||||
}
|
||||
}
|
||||
}
|
||||
t.Reminders = updatedReminders
|
||||
}
|
||||
|
||||
// Removes all old reminders and adds the new ones. This is a lot easier and less buggy than
|
||||
|
@ -1535,7 +1553,7 @@ func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder, dueDa
|
|||
return
|
||||
}
|
||||
|
||||
setRelativeReminderDates(reminders, dueDate, startDate, endDate)
|
||||
updateRelativeReminderDates(reminders, dueDate, startDate, endDate)
|
||||
|
||||
// Resolve duplicates and sort them
|
||||
reminderMap := make(map[int64]TaskReminder, len(reminders))
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
Please use a pointer to Please use a pointer to `TaskReminder`.
ce72
commented
ok ok
|
||||
|
@ -1554,18 +1572,19 @@ func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder, dueDa
|
|||
RelativePeriod: r.RelativePeriod,
|
||||
RelativeTo: r.RelativeTo}
|
||||
_, err = s.Insert(taskReminder)
|
||||
t.Reminders = append(t.Reminders, taskReminder)
|
||||
t.ReminderDates = append(t.ReminderDates, taskReminder.Reminder)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
t.Reminders = append(t.Reminders, taskReminder)
|
||||
t.ReminderDates = append(t.ReminderDates, taskReminder.Reminder)
|
||||
}
|
||||
|
||||
// sort reminders
|
||||
sort.Slice(t.Reminders, func(i, j int) bool {
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
Why sort them here? Just for the updated task response? Why sort them here? Just for the updated task response?
ce72
commented
Yes. It simplifies the tests too. Yes. It simplifies the tests too.
|
||||
return t.Reminders[i].Reminder.Before(t.Reminders[j].Reminder)
|
||||
})
|
||||
|
||||
if len(reminders) == 0 {
|
||||
if len(t.Reminders) == 0 {
|
||||
t.Reminders = nil
|
||||
t.ReminderDates = nil
|
||||
}
|
||||
|
@ -1574,26 +1593,6 @@ func (t *Task) updateReminders(s *xorm.Session, reminders []*TaskReminder, dueDa
|
|||
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)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func updateTaskLastUpdated(s *xorm.Session, task *Task) error {
|
||||
_, err := s.ID(task.ID).Cols("updated").Update(task)
|
||||
return err
|
||||
|
|
|
@ -103,9 +103,11 @@ func TestTask_Create(t *testing.T) {
|
|||
assert.NoError(t, err)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 1, 0, time.Local), task.Reminders[0].Reminder)
|
||||
assert.Equal(t, int64(1), task.Reminders[0].RelativePeriod)
|
||||
assert.Equal(t, ReminderDueDate, task.Reminders[0].RelativeTo)
|
||||
assert.Equal(t, ReminderRelationDueDate, task.Reminders[0].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 8, 0, time.Local), task.Reminders[1].Reminder)
|
||||
assert.Equal(t, ReminderRelationStartDate, task.Reminders[1].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 19, 0, time.Local), task.Reminders[2].Reminder)
|
||||
assert.Equal(t, ReminderRelationEndDate, task.Reminders[2].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 23, 0, 0, 0, time.Local), task.Reminders[3].Reminder)
|
||||
err = s.Commit()
|
||||
assert.NoError(t, err)
|
||||
|
@ -442,9 +444,11 @@ func TestTask_Update(t *testing.T) {
|
|||
assert.NoError(t, err)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 1, 0, time.Local), task.Reminders[0].Reminder)
|
||||
assert.Equal(t, int64(1), task.Reminders[0].RelativePeriod)
|
||||
assert.Equal(t, ReminderDueDate, task.Reminders[0].RelativeTo)
|
||||
assert.Equal(t, ReminderRelationDueDate, task.Reminders[0].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 8, 0, time.Local), task.Reminders[1].Reminder)
|
||||
assert.Equal(t, ReminderRelationStartDate, task.Reminders[1].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 22, 5, 19, 0, time.Local), task.Reminders[2].Reminder)
|
||||
assert.Equal(t, ReminderRelationEndDate, task.Reminders[2].RelativeTo)
|
||||
assert.Equal(t, time.Date(2023, time.March, 7, 23, 0, 0, 0, time.Local), task.Reminders[3].Reminder)
|
||||
err = s.Commit()
|
||||
assert.NoError(t, err)
|
||||
|
@ -474,6 +478,40 @@ func TestTask_Update(t *testing.T) {
|
|||
assert.NoError(t, err)
|
||||
db.AssertCount(t, "task_reminders", builder.Eq{"task_id": 1}, 1)
|
||||
})
|
||||
t.Run("update relative reminder when start_date changes", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// given task with start_date and relative reminder for start_date
|
||||
taskBefore := &Task{
|
||||
Title: "test",
|
||||
ListID: 1,
|
||||
StartDate: time.Date(2022, time.March, 8, 8, 5, 20, 0, time.Local),
|
||||
Reminders: []*TaskReminder{
|
||||
{
|
||||
RelativeTo: "start_date",
|
||||
RelativePeriod: -60,
|
||||
},
|
||||
}}
|
||||
err := taskBefore.Create(s, u)
|
||||
assert.NoError(t, err)
|
||||
err = s.Commit()
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, time.Date(2022, time.March, 8, 8, 4, 20, 0, time.Local), taskBefore.Reminders[0].Reminder)
|
||||
|
||||
// when start_date is modified
|
||||
task := taskBefore
|
||||
task.StartDate = time.Date(2023, time.March, 8, 8, 5, 0, 0, time.Local)
|
||||
task.ReminderDates = nil
|
||||
err = task.Update(s, u)
|
||||
assert.NoError(t, err)
|
||||
|
||||
// then reminder time is updated
|
||||
assert.Equal(t, time.Date(2023, time.March, 8, 8, 4, 0, 0, time.Local), task.Reminders[0].Reminder)
|
||||
err = s.Commit()
|
||||
assert.NoError(t, err)
|
||||
})
|
||||
}
|
||||
|
||||
func TestTask_Delete(t *testing.T) {
|
||||
|
|
|
@ -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.ReminderDueDate,
|
||||
RelativeTo: models.ReminderRelationDueDate,
|
||||
RelativePeriod: int64((t.Reminder * -1).Seconds()),
|
||||
},
|
||||
}
|
||||
|
|
|
@ -7790,7 +7790,7 @@ const docTemplate = `{
|
|||
]
|
||||
},
|
||||
"reminder_dates": {
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders[]",
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
|
@ -8317,9 +8317,9 @@ const docTemplate = `{
|
|||
"end_date"
|
||||
],
|
||||
"x-enum-varnames": [
|
||||
"ReminderDueDate",
|
||||
"ReminderStartDate",
|
||||
"ReminderEndDate"
|
||||
"ReminderRelationDueDate",
|
||||
"ReminderRelationStartDate",
|
||||
"ReminderRelationEndDate"
|
||||
]
|
||||
},
|
||||
"models.Right": {
|
||||
|
@ -8537,7 +8537,7 @@ const docTemplate = `{
|
|||
]
|
||||
},
|
||||
"reminder_dates": {
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders[]",
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
|
@ -8721,7 +8721,7 @@ const docTemplate = `{
|
|||
"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.",
|
||||
"description": "A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date. Default: 0, tiggers when RelativeTo is due.",
|
||||
"type": "integer"
|
||||
},
|
||||
"relative_to": {
|
||||
|
|
|
@ -7782,7 +7782,7 @@
|
|||
]
|
||||
},
|
||||
"reminder_dates": {
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders[]",
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
|
@ -8309,9 +8309,9 @@
|
|||
"end_date"
|
||||
],
|
||||
"x-enum-varnames": [
|
||||
"ReminderDueDate",
|
||||
"ReminderStartDate",
|
||||
"ReminderEndDate"
|
||||
"ReminderRelationDueDate",
|
||||
"ReminderRelationStartDate",
|
||||
"ReminderRelationEndDate"
|
||||
]
|
||||
},
|
||||
"models.Right": {
|
||||
|
@ -8529,7 +8529,7 @@
|
|||
]
|
||||
},
|
||||
"reminder_dates": {
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders[]",
|
||||
"description": "An array of datetimes when the user wants to be reminded of the task.\n\nDeprecated: Use Reminders",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
|
@ -8713,7 +8713,7 @@
|
|||
"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.",
|
||||
"description": "A period in seconds relative to another date argument. Negative values mean the reminder triggers before the date. Default: 0, tiggers when RelativeTo is due.",
|
||||
"type": "integer"
|
||||
},
|
||||
"relative_to": {
|
||||
|
|
|
@ -199,7 +199,7 @@ definitions:
|
|||
description: |-
|
||||
An array of datetimes when the user wants to be reminded of the task.
|
||||
|
||||
Deprecated: Use Reminders[]
|
||||
Deprecated: Use Reminders
|
||||
items:
|
||||
type: string
|
||||
type: array
|
||||
|
@ -609,9 +609,9 @@ definitions:
|
|||
- end_date
|
||||
type: string
|
||||
x-enum-varnames:
|
||||
- ReminderDueDate
|
||||
- ReminderStartDate
|
||||
- ReminderEndDate
|
||||
- ReminderRelationDueDate
|
||||
- ReminderRelationStartDate
|
||||
- ReminderRelationEndDate
|
||||
models.Right:
|
||||
enum:
|
||||
- 0
|
||||
|
@ -782,7 +782,7 @@ definitions:
|
|||
description: |-
|
||||
An array of datetimes when the user wants to be reminded of the task.
|
||||
|
||||
Deprecated: Use Reminders[]
|
||||
Deprecated: Use Reminders
|
||||
items:
|
||||
type: string
|
||||
type: array
|
||||
|
@ -915,8 +915,9 @@ definitions:
|
|||
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.
|
||||
description: 'A period in seconds relative to another date argument. Negative
|
||||
values mean the reminder triggers before the date. Default: 0, tiggers when
|
||||
RelativeTo is due.'
|
||||
type: integer
|
||||
relative_to:
|
||||
allOf:
|
||||
|
|
Loading…
Reference in New Issue
Block a user
I think we can get rid of all the
else
clauses and move them before theswitch
statements so that the logic inside the switch will override the empty reminder time.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?Ok. Not sure if it's better, but ok.
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.
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.