vikunja/vikunja
6
43
Fork 73
Code Issues 151 Pull Requests 16 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 +6
1 changed files with 6 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 7fe3a1ade7 - Show all commits

6
pkg/models/tasks.go
View File

@ -1517,14 +1517,20 @@ func updateRelativeReminderDates(task *Task) (err error) {
case ReminderRelationDueDate:
if !task.DueDate.IsZero() {
reminder.Reminder = task.DueDate.Add(relativeDuration)
} else {
reminder.Reminder = time.Time{}
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.
}
case ReminderRelationStartDate:
if !task.StartDate.IsZero() {
reminder.Reminder = task.StartDate.Add(relativeDuration)
} else {
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
reminder.Reminder = time.Time{}
}
case ReminderRelationEndDate:
if !task.EndDate.IsZero() {
reminder.Reminder = task.EndDate.Add(relativeDuration)
} else {
reminder.Reminder = time.Time{}
}
default:
if reminder.RelativePeriod != 0 {