From 54ed002d1bfb1215596c5ba485833efe76407fcb Mon Sep 17 00:00:00 2001 From: ce72 Date: Wed, 8 Mar 2023 12:44:05 +0100 Subject: [PATCH] fix: add tests and cleanup --- pkg/models/task_reminder.go | 14 ++-- pkg/models/tasks.go | 81 +++++++++++----------- pkg/models/tasks_test.go | 42 ++++++++++- pkg/modules/migration/ticktick/ticktick.go | 2 +- pkg/swagger/docs.go | 12 ++-- pkg/swagger/swagger.json | 12 ++-- pkg/swagger/swagger.yaml | 15 ++-- 7 files changed, 109 insertions(+), 69 deletions(-) diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 0f0e70c28..917e37c6e 100644 --- a/pkg/models/task_reminder.go +++ b/pkg/models/task_reminder.go @@ -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"` diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 8e429c50c..9762fb2a8 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -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 +} + +// 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 { + 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)) @@ -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 { 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 diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go index 84b7a79cb..4a1e819b0 100644 --- a/pkg/models/tasks_test.go +++ b/pkg/models/tasks_test.go @@ -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) { diff --git a/pkg/modules/migration/ticktick/ticktick.go b/pkg/modules/migration/ticktick/ticktick.go index 8036ffa88..fd3822a3b 100644 --- a/pkg/modules/migration/ticktick/ticktick.go +++ b/pkg/modules/migration/ticktick/ticktick.go @@ -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()), }, } diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index 728221ea7..54833a08a 100644 --- a/pkg/swagger/docs.go +++ b/pkg/swagger/docs.go @@ -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": { diff --git a/pkg/swagger/swagger.json b/pkg/swagger/swagger.json index 773189696..31b64b40e 100644 --- a/pkg/swagger/swagger.json +++ b/pkg/swagger/swagger.json @@ -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": { diff --git a/pkg/swagger/swagger.yaml b/pkg/swagger/swagger.yaml index a7b1a80e5..8ba6ea3a8 100644 --- a/pkg/swagger/swagger.yaml +++ b/pkg/swagger/swagger.yaml @@ -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: