diff --git a/pkg/models/task_overdue_reminder.go b/pkg/models/task_overdue_reminder.go index 2b91f1741..e10ef126e 100644 --- a/pkg/models/task_overdue_reminder.go +++ b/pkg/models/task_overdue_reminder.go @@ -32,7 +32,7 @@ import ( "xorm.io/xorm" ) -func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (usersWithTasks map[int64]*userWithTasks, err error) { +func getUndoneOverdueTasks(s *xorm.Session, now time.Time, cond builder.Cond) (usersWithTasks map[int64]*userWithTasks, err error) { now = utils.GetTimeWithoutSeconds(now) nextMinute := now.Add(1 * time.Minute) @@ -55,7 +55,7 @@ func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (usersWithTasks map[i taskIDs = append(taskIDs, task.ID) } - users, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) + users, err := getTaskUsersForTasks(s, taskIDs, cond) if err != nil { return } @@ -111,17 +111,38 @@ type userWithTasks struct { // RegisterOverdueReminderCron registers a function which checks once a day for tasks that are overdue and not done. func RegisterOverdueReminderCron() { + webhookEnabled := config.WebhooksEnabled.GetBool() + emailEnabled := config.ServiceEnableEmailReminders.GetBool() && config.MailerEnabled.GetBool() + + if !emailEnabled && !webhookEnabled { + return + } + + if !emailEnabled { + log.Info("Mailer is disabled, not sending overdue reminders per mail") + } + err := cron.Schedule("* * * * *", func() { s := db.NewSession() defer s.Close() now := time.Now() - uts, err := getUndoneOverdueTasks(s, now) + + var cond builder.Cond + if emailEnabled && !webhookEnabled { + cond = builder.Eq{"users.overdue_tasks_reminders_enabled": true} + } + + uts, err := getUndoneOverdueTasks(s, now, cond) if err != nil { log.Errorf("[Undone Overdue Tasks Reminder] Could not get undone overdue tasks in the next minute: %s", err) return } + if len(uts) == 0 { + return + } + log.Debugf("[Undone Overdue Tasks Reminder] Sending reminders to %d users", len(uts)) taskIDs := []int64{} @@ -137,54 +158,57 @@ func RegisterOverdueReminderCron() { return } - // Dispatch webhook events, deduplicated by task ID across all users - dispatchedTasks := make(map[int64]bool) for _, ut := range uts { - for _, t := range ut.tasks { - if dispatchedTasks[t.ID] { - continue + if emailEnabled && ut.user.OverdueTasksRemindersEnabled { + var n notifications.Notification = &UndoneTasksOverdueNotification{ + User: ut.user, + Tasks: ut.tasks, + Projects: projects, } - dispatchedTasks[t.ID] = true - err = events.Dispatch(&TaskOverdueEvent{ - Task: t, - Project: projects[t.ProjectID], - }) - if err != nil { - log.Errorf("[Undone Overdue Tasks Reminder] Could not dispatch overdue event for task %d: %s", t.ID, err) - } - } - } - if !config.ServiceEnableEmailReminders.GetBool() || !config.MailerEnabled.GetBool() { - return - } - - for _, ut := range uts { - var n notifications.Notification = &UndoneTasksOverdueNotification{ - User: ut.user, - Tasks: ut.tasks, - Projects: projects, - } - - if len(ut.tasks) == 1 { - // We know there's only one entry in the map so this is actually O(1) and we can use it to get the - // first entry without knowing the key of it. - for _, t := range ut.tasks { - n = &UndoneTaskOverdueNotification{ - User: ut.user, - Task: t, - Project: projects[t.ProjectID], + if len(ut.tasks) == 1 { + for _, t := range ut.tasks { + n = &UndoneTaskOverdueNotification{ + User: ut.user, + Task: t, + Project: projects[t.ProjectID], + } } } + + err = notifications.Notify(ut.user, n, s) + if err != nil { + log.Errorf("[Undone Overdue Tasks Reminder] Could not notify user %d: %s", ut.user.ID, err) + return + } } - err = notifications.Notify(ut.user, n, s) - if err != nil { - log.Errorf("[Undone Overdue Tasks Reminder] Could not notify user %d: %s", ut.user.ID, err) - return + // Dispatch webhook events + if webhookEnabled { + // Per-task events + for _, t := range ut.tasks { + err = events.Dispatch(&TaskOverdueEvent{ + Task: t, + User: ut.user, + Project: projects[t.ProjectID], + }) + if err != nil { + log.Errorf("[Undone Overdue Tasks Reminder] Could not dispatch overdue event for task %d: %s", t.ID, err) + } + } + + // Batch event + err = events.Dispatch(&TasksOverdueEvent{ + Tasks: mapToSlice(ut.tasks), + User: ut.user, + Projects: projects, + }) + if err != nil { + log.Errorf("[Undone Overdue Tasks Reminder] Could not dispatch batch overdue event for user %d: %s", ut.user.ID, err) + } } - log.Debugf("[Undone Overdue Tasks Reminder] Sent reminder email for %d tasks to user %d", len(ut.tasks), ut.user.ID) + log.Debugf("[Undone Overdue Tasks Reminder] Sent reminder for %d tasks to user %d", len(ut.tasks), ut.user.ID) } if err := s.Commit(); err != nil { @@ -195,3 +219,11 @@ func RegisterOverdueReminderCron() { log.Fatalf("Could not register undone overdue tasks reminder cron: %s", err) } } + +func mapToSlice(m map[int64]*Task) []*Task { + tasks := make([]*Task, 0, len(m)) + for _, t := range m { + tasks = append(tasks, t) + } + return tasks +} diff --git a/pkg/models/task_overdue_reminder_test.go b/pkg/models/task_overdue_reminder_test.go index 4a465b03e..e02cd2619 100644 --- a/pkg/models/task_overdue_reminder_test.go +++ b/pkg/models/task_overdue_reminder_test.go @@ -35,7 +35,7 @@ func TestGetUndoneOverDueTasks(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-01-01T01:13:00Z") require.NoError(t, err) - tasks, err := getUndoneOverdueTasks(s, now) + tasks, err := getUndoneOverdueTasks(s, now, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) require.NoError(t, err) assert.Empty(t, tasks) }) @@ -46,7 +46,7 @@ func TestGetUndoneOverDueTasks(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-12-01T09:00:00Z") require.NoError(t, err) - uts, err := getUndoneOverdueTasks(s, now) + uts, err := getUndoneOverdueTasks(s, now, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) require.NoError(t, err) require.Len(t, uts, 1) assert.Len(t, uts[1].tasks, 2) @@ -71,7 +71,7 @@ func TestGetUndoneOverDueTasks(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-11-01T01:13:00Z") require.NoError(t, err) - tasks, err := getUndoneOverdueTasks(s, now) + tasks, err := getUndoneOverdueTasks(s, now, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) require.NoError(t, err) assert.Empty(t, tasks) }) diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 2257600ea..3fddbc49d 100644 --- a/pkg/models/task_reminder.go +++ b/pkg/models/task_reminder.go @@ -241,7 +241,7 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) ( return } -func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time) (reminderNotifications []*ReminderDueNotification, err error) { +func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time, cond builder.Cond) (reminderNotifications []*ReminderDueNotification, err error) { now = utils.GetTimeWithoutNanoSeconds(now) reminderNotifications = []*ReminderDueNotification{} @@ -275,7 +275,7 @@ func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time) (remi return } - usersWithReminders, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.email_reminders_enabled": true}) + usersWithReminders, err := getTaskUsersForTasks(s, taskIDs, cond) if err != nil { return } @@ -327,9 +327,10 @@ func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time) (remi seen[r.TaskID][u.User.ID] = true reminderNotifications = append(reminderNotifications, &ReminderDueNotification{ - User: u.User, - Task: u.Task, - Project: projects[u.Task.ProjectID], + User: u.User, + Task: u.Task, + Project: projects[u.Task.ProjectID], + TaskReminder: r, }) } } @@ -341,8 +342,18 @@ func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time) (remi // RegisterReminderCron registers a cron function which runs every minute to check if any reminders are due the // next minute to send emails. func RegisterReminderCron() { - tz := config.GetTimeZone() + webhookEnabled := config.WebhooksEnabled.GetBool() + emailEnabled := config.ServiceEnableEmailReminders.GetBool() && config.MailerEnabled.GetBool() + if !emailEnabled && !webhookEnabled { + return + } + + if !emailEnabled { + log.Info("Mailer is disabled, not sending reminders per mail") + } + + tz := config.GetTimeZone() log.Debugf("[Task Reminder Cron] Timezone is %s", tz) err := cron.Schedule("* * * * *", func() { @@ -350,7 +361,16 @@ func RegisterReminderCron() { defer s.Close() now := time.Now() - reminders, err := getTasksWithRemindersDueAndTheirUsers(s, now) + + // When only email is enabled, filter to email-enabled users for efficiency. + // When webhooks are enabled, we need all users so the event system can + // look up matching webhooks. + var cond builder.Cond + if emailEnabled && !webhookEnabled { + cond = builder.Eq{"users.email_reminders_enabled": true} + } + + reminders, err := getTasksWithRemindersDueAndTheirUsers(s, now, cond) if err != nil { log.Errorf("[Task Reminder Cron] Could not get tasks with reminders in the next minute: %s", err) return @@ -362,34 +382,28 @@ func RegisterReminderCron() { log.Debugf("[Task Reminder Cron] Sending %d reminders", len(reminders)) - // Dispatch webhook events, deduplicated by task ID - dispatchedTasks := make(map[int64]bool) for _, n := range reminders { - if dispatchedTasks[n.Task.ID] { - continue - } - dispatchedTasks[n.Task.ID] = true - err = events.Dispatch(&TaskReminderFiredEvent{ - Task: n.Task, - Project: n.Project, - }) - if err != nil { - log.Errorf("[Task Reminder Cron] Could not dispatch reminder event for task %d: %s", n.Task.ID, err) - } - } - - if !config.ServiceEnableEmailReminders.GetBool() || !config.MailerEnabled.GetBool() { - return - } - - for _, n := range reminders { - err = notifications.Notify(n.User, n, s) - if err != nil { - log.Errorf("[Task Reminder Cron] Could not notify user %d: %s", n.User.ID, err) - return + if emailEnabled && n.User.EmailRemindersEnabled { + err = notifications.Notify(n.User, n, s) + if err != nil { + log.Errorf("[Task Reminder Cron] Could not notify user %d: %s", n.User.ID, err) + return + } } - log.Debugf("[Task Reminder Cron] Sent reminder email for task %d to user %d", n.Task.ID, n.User.ID) + if webhookEnabled { + err = events.Dispatch(&TaskReminderFiredEvent{ + Task: n.Task, + User: n.User, + Project: n.Project, + Reminder: n.TaskReminder, + }) + if err != nil { + log.Errorf("[Task Reminder Cron] Could not dispatch reminder event for task %d: %s", n.Task.ID, err) + } + } + + log.Debugf("[Task Reminder Cron] Sent reminder for task %d to user %d", n.Task.ID, n.User.ID) } if err := s.Commit(); err != nil { diff --git a/pkg/models/task_reminder_test.go b/pkg/models/task_reminder_test.go index 46cf15543..62216c1b1 100644 --- a/pkg/models/task_reminder_test.go +++ b/pkg/models/task_reminder_test.go @@ -22,6 +22,8 @@ import ( "code.vikunja.io/api/pkg/db" + "xorm.io/builder" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -34,7 +36,7 @@ func TestReminderGetTasksInTheNextMinute(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-12-01T01:12:00Z") require.NoError(t, err) - notifications, err := getTasksWithRemindersDueAndTheirUsers(s, now) + notifications, err := getTasksWithRemindersDueAndTheirUsers(s, now, builder.Eq{"users.email_reminders_enabled": true}) require.NoError(t, err) assert.Len(t, notifications, 1) assert.Equal(t, int64(27), notifications[0].Task.ID) @@ -46,7 +48,7 @@ func TestReminderGetTasksInTheNextMinute(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-12-02T01:13:00Z") require.NoError(t, err) - taskIDs, err := getTasksWithRemindersDueAndTheirUsers(s, now) + taskIDs, err := getTasksWithRemindersDueAndTheirUsers(s, now, builder.Eq{"users.email_reminders_enabled": true}) require.NoError(t, err) assert.Empty(t, taskIDs) })