ami/tl-vikunja
1
0
Fork 0
Code Activity

feat: add time zone setting for reminders (#1092)

Browse Source 
Instead of naeveily checking for all reminders due in the next minute, we now check all reminders in all time zones in the next minutes. This essentially means checking for reminders due in the next 14 or past 12 hours. We then check for each user who would receive a reminder from that result if it is actually due in their time zone.

This should prevent issues where users would get the reminder in the time zone of their server, not in their own.

Co-authored-by: kolaente <k@knt.li>
Reviewed-on: https://kolaente.dev/vikunja/api/pulls/1092
Co-authored-by: konrad <k@knt.li>
Co-committed-by: konrad <k@knt.li>
This commit is contained in:
konrad
2022-01-16 11:05:56 +00:00
parent e116fbad79
commit 61d49c3a56
12 changed files with 279 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files

50
pkg/migration/20220112211537.go Normal file
View File

@ -0,0 +1,50 @@
// Vikunja is a to-do list application to facilitate your life.
// Copyright 2018-2021 Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public Licensee as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public Licensee for more details.
//
// You should have received a copy of the GNU Affero General Public Licensee
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package migration
import (
"code.vikunja.io/api/pkg/config"
"src.techknowlogick.com/xormigrate"
"xorm.io/xorm"
)
type users20220112211537 struct {
Timezone string `xorm:"varchar(255) null" json:"-"`
}
func (users20220112211537) TableName() string {
return "users"
}
func init() {
migrations = append(migrations, &xormigrate.Migration{
ID: "20220112211537",
Description: "Add time zone setting for users",
Migrate: func(tx *xorm.Engine) error {
err := tx.Sync2(users20220112211537{})
if err != nil {
return err
}
_, err = tx.Update(&users20220112211537{Timezone: config.GetTimeZone().String()})
return err
},
Rollback: func(tx *xorm.Engine) error {
return nil
},
})
}

88
pkg/models/task_reminder.go
View File

@ -61,11 +61,11 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (
// Get all creators of tasks
creators := make(map[int64]*user.User, len(taskIDs))
err = s.
Select("users.id, users.username, users.email, users.name").
Select("users.id, users.username, users.email, users.name, users.timezone").
Join("LEFT", "tasks", "tasks.created_by_id = users.id").
In("tasks.id", taskIDs).
Where(cond).
GroupBy("tasks.id, users.id, users.username, users.email, users.name").
GroupBy("tasks.id, users.id, users.username, users.email, users.name, users.timezone").
Find(&creators)
if err != nil {
return
@ -77,14 +77,14 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (
return
}
for _, taskID := range taskIDs {
u, exists := creators[taskMap[taskID].CreatedByID]
for _, task := range taskMap {
u, exists := creators[task.CreatedByID]
if !exists {
continue
}
taskUsers = append(taskUsers, &taskUser{
Task: taskMap[taskID],
Task: taskMap[task.ID],
User: u,
})
}
@ -110,8 +110,9 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (
return
}
func getTasksWithRemindersInTheNextMinute(s *xorm.Session, now time.Time) (taskIDs []int64, err error) {
func getTasksWithRemindersDueAndTheirUsers(s *xorm.Session, now time.Time) (reminderNotifications []*ReminderDueNotification, err error) {
now = utils.GetTimeWithoutNanoSeconds(now)
reminderNotifications = []*ReminderDueNotification{}
nextMinute := now.Add(1 * time.Minute)
@ -120,7 +121,8 @@ func getTasksWithRemindersInTheNextMinute(s *xorm.Session, now time.Time) (taskI
reminders := []*TaskReminder{}
err = s.
Join("INNER", "tasks", "tasks.id = task_reminders.task_id").
Where("reminder >= ? and reminder < ?", now.Format(dbTimeFormat), nextMinute.Format(dbTimeFormat)).
// All reminders from -12h to +14h to include all time zones
Where("reminder >= ? and reminder < ?", now.Add(time.Hour*-12).Format(dbTimeFormat), nextMinute.Add(time.Hour*14).Format(dbTimeFormat)).
And("tasks.done = false").
Find(&reminders)
if err != nil {
@ -133,11 +135,56 @@ func getTasksWithRemindersInTheNextMinute(s *xorm.Session, now time.Time) (taskI
return
}
// We're sending a reminder to everyone who is assigned to the task or has created it.
var taskIDs []int64
for _, r := range reminders {
taskIDs = append(taskIDs, r.TaskID)
}
if len(taskIDs) == 0 {
return
}
usersWithReminders, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.email_reminders_enabled": true})
if err != nil {
return
}
usersPerTask := make(map[int64][]*taskUser, len(usersWithReminders))
for _, ur := range usersWithReminders {
usersPerTask[ur.Task.ID] = append(usersPerTask[ur.Task.ID], ur)
}
// Time zone cache per time zone string to avoid parsing the same time zone over and over again
tzs := make(map[string]*time.Location)
// Figure out which reminders are actually due in the time zone of the users
for _, r := range reminders {
for _, u := range usersPerTask[r.TaskID] {
if u.User.Timezone == "" {
u.User.Timezone = config.GetTimeZone().String()
}
// I think this will break once there's more reminders than what we can handle in one minute
tz, exists := tzs[u.User.Timezone]
if !exists {
tz, err = time.LoadLocation(u.User.Timezone)
if err != nil {
return
}
tzs[u.User.Timezone] = tz
}
actualReminder := r.Reminder.In(tz)
if (actualReminder.After(now) && actualReminder.Before(now.Add(time.Minute))) || actualReminder.Equal(now) {
reminderNotifications = append(reminderNotifications, &ReminderDueNotification{
User: u.User,
Task: u.Task,
})
}
}
}
return
}
@ -162,37 +209,26 @@ func RegisterReminderCron() {
defer s.Close()
now := time.Now()
taskIDs, err := getTasksWithRemindersInTheNextMinute(s, now)
reminders, err := getTasksWithRemindersDueAndTheirUsers(s, now)
if err != nil {
log.Errorf("[Task Reminder Cron] Could not get tasks with reminders in the next minute: %s", err)
return
}
if len(taskIDs) == 0 {
if len(reminders) == 0 {
return
}
users, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.email_reminders_enabled": true})
if err != nil {
log.Errorf("[Task Reminder Cron] Could not get task users to send them reminders: %s", err)
return
}
log.Debugf("[Task Reminder Cron] Sending %d reminders", len(reminders))
log.Debugf("[Task Reminder Cron] Sending reminders to %d users", len(users))
for _, u := range users {
n := &ReminderDueNotification{
User: u.User,
Task: u.Task,
}
err = notifications.Notify(u.User, n)
for _, n := range reminders {
err = notifications.Notify(n.User, n)
if err != nil {
log.Errorf("[Task Reminder Cron] Could not notify user %d: %s", u.User.ID, err)
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", u.Task.ID, u.User.ID)
log.Debugf("[Task Reminder Cron] Sent reminder email for task %d to user %d", n.Task.ID, n.User.ID)
}
})
if err != nil {

8
pkg/models/task_reminder_test.go
View File

@ -32,10 +32,10 @@ func TestReminderGetTasksInTheNextMinute(t *testing.T) {
now, err := time.Parse(time.RFC3339Nano, "2018-12-01T01:13:00Z")
assert.NoError(t, err)
taskIDs, err := getTasksWithRemindersInTheNextMinute(s, now)
notifications, err := getTasksWithRemindersDueAndTheirUsers(s, now)
assert.NoError(t, err)
assert.Len(t, taskIDs, 1)
assert.Equal(t, int64(27), taskIDs[0])
assert.Len(t, notifications, 1)
assert.Equal(t, int64(27), notifications[0].Task.ID)
})
t.Run("Found No Tasks", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
@ -44,7 +44,7 @@ func TestReminderGetTasksInTheNextMinute(t *testing.T) {
now, err := time.Parse(time.RFC3339Nano, "2018-12-02T01:13:00Z")
assert.NoError(t, err)
taskIDs, err := getTasksWithRemindersInTheNextMinute(s, now)
taskIDs, err := getTasksWithRemindersDueAndTheirUsers(s, now)
assert.NoError(t, err)
assert.Len(t, taskIDs, 0)
})

36
pkg/routes/api/v1/user_settings.go
View File

@ -19,12 +19,13 @@ package v1
import (
"net/http"
"code.vikunja.io/api/pkg/db"
"github.com/labstack/echo/v4"
"github.com/tkuchiki/go-timezone"
"code.vikunja.io/api/pkg/db"
"code.vikunja.io/api/pkg/models"
user2 "code.vikunja.io/api/pkg/user"
"code.vikunja.io/web/handler"
"github.com/labstack/echo/v4"
)
// UserAvatarProvider holds the user avatar provider type
@ -52,6 +53,8 @@ type UserSettings struct {
WeekStart int `json:"week_start"`
// The user's language
Language string `json:"language"`
// The user's time zone. Used to send task reminders in the time zone of the user.
Timezone string `json:"timezone"`
}
// GetUserAvatarProvider returns the currently set user avatar
@ -180,6 +183,7 @@ func UpdateGeneralUserSettings(c echo.Context) error {
user.DefaultListID = us.DefaultListID
user.WeekStart = us.WeekStart
user.Language = us.Language
user.Timezone = us.Timezone
_, err = user2.UpdateUser(s, user)
if err != nil {
@ -194,3 +198,31 @@ func UpdateGeneralUserSettings(c echo.Context) error {
return c.JSON(http.StatusOK, &models.Message{Message: "The settings were updated successfully."})
}
// GetAvailableTimezones
// @Summary Get all available time zones on this vikunja instance
// @Description Because available time zones depend on the system Vikunja is running on, this endpoint returns a list of all valid time zones this particular Vikunja instance can handle. The list of time zones is not sorted, you should sort it on the client.
// @tags user
// @Accept json
// @Produce json
// @Security JWTKeyAuth
// @Success 200 {array} string "All available time zones."
// @Failure 500 {object} models.Message "Internal server error."
// @Router /user/timezones [get]
func GetAvailableTimezones(c echo.Context) error {
allTimezones := timezone.New().Timezones()
timezoneMap := make(map[string]bool) // to filter all duplicates
for _, s := range allTimezones {
for _, t := range s {
timezoneMap[t] = true
}
}
ts := []string{}
for s := range timezoneMap {
ts = append(ts, s)
}
return c.JSON(http.StatusOK, ts)
}

1
pkg/routes/api/v1/user_show.go
View File

@ -74,6 +74,7 @@ func UserShow(c echo.Context) error {
DefaultListID: u.DefaultListID,
WeekStart: u.WeekStart,
Language: u.Language,
Timezone: u.Timezone,
},
DeletionScheduledAt: u.DeletionScheduledAt,
IsLocalUser: u.Issuer == user.IssuerLocal,

1
pkg/routes/routes.go
View File

@ -321,6 +321,7 @@ func registerAPIRoutes(a *echo.Group) {
u.POST("/settings/general", apiv1.UpdateGeneralUserSettings)
u.POST("/export/request", apiv1.RequestUserDataExport)
u.POST("/export/download", apiv1.DownloadUserDataExport)
u.GET("/timezones", apiv1.GetAvailableTimezones)
if config.ServiceEnableTotp.GetBool() {
u.GET("/settings/totp", apiv1.UserTOTP)

41
pkg/swagger/docs.go
View File

@ -7260,6 +7260,43 @@ var doc = `{
}
}
},
"/user/timezones": {
"get": {
"security": [
{
"JWTKeyAuth": []
}
],
"description": "Because available time zones depend on the system Vikunja is running on, this endpoint returns a list of all valid time zones this particular Vikunja instance can handle. The list of time zones is not sorted, you should sort it on the client.",
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"tags": [
"user"
],
"summary": "Get all available time zones on this vikunja instance",
"responses": {
"200": {
"description": "All available time zones.",
"schema": {
"type": "array",
"items": {
"type": "string"
}
}
},
"500": {
"description": "Internal server error.",
"schema": {
"$ref": "#/definitions/models.Message"
}
}
}
}
},
"/user/token": {
"post": {
"description": "Returns a new valid jwt user token with an extended length.",
@ -8978,6 +9015,10 @@ var doc = `{
"description": "If enabled, the user will get an email for their overdue tasks each morning.",
"type": "boolean"
},
"timezone": {
"description": "The user's time zone. Used to send task reminders in the time zone of the user.",
"type": "string"
},
"week_start": {
"description": "The day when the week starts for this user. 0 = sunday, 1 = monday, etc.",
"type": "integer"

41
pkg/swagger/swagger.json
View File

@ -7244,6 +7244,43 @@
}
}
},
"/user/timezones": {
"get": {
"security": [
{
"JWTKeyAuth": []
}
],
"description": "Because available time zones depend on the system Vikunja is running on, this endpoint returns a list of all valid time zones this particular Vikunja instance can handle. The list of time zones is not sorted, you should sort it on the client.",
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"tags": [
"user"
],
"summary": "Get all available time zones on this vikunja instance",
"responses": {
"200": {
"description": "All available time zones.",
"schema": {
"type": "array",
"items": {
"type": "string"
}
}
},
"500": {
"description": "Internal server error.",
"schema": {
"$ref": "#/definitions/models.Message"
}
}
}
}
},
"/user/token": {
"post": {
"description": "Returns a new valid jwt user token with an extended length.",
@ -8962,6 +8999,10 @@
"description": "If enabled, the user will get an email for their overdue tasks each morning.",
"type": "boolean"
},
"timezone": {
"description": "The user's time zone. Used to send task reminders in the time zone of the user.",
"type": "string"
},
"week_start": {
"description": "The day when the week starts for this user. 0 = sunday, 1 = monday, etc.",
"type": "integer"

30
pkg/swagger/swagger.yaml
View File

@ -1284,6 +1284,10 @@ definitions:
description: If enabled, the user will get an email for their overdue tasks
each morning.
type: boolean
timezone:
description: The user's time zone. Used to send task reminders in the time
zone of the user.
type: string
week_start:
description: The day when the week starts for this user. 0 = sunday, 1 = monday,
etc.
@ -6212,6 +6216,32 @@ paths:
summary: Totp QR Code
tags:
- user
/user/timezones:
get:
consumes:
- application/json
description: Because available time zones depend on the system Vikunja is running
on, this endpoint returns a list of all valid time zones this particular Vikunja
instance can handle. The list of time zones is not sorted, you should sort
it on the client.
produces:
- application/json
responses:
"200":
description: All available time zones.
schema:
items:
type: string
type: array
"500":
description: Internal server error.
schema:
$ref: '#/definitions/models.Message'
security:
- JWTKeyAuth: []
summary: Get all available time zones on this vikunja instance
tags:
- user
/user/token:
post:
consumes:

12
pkg/user/user.go
View File

@ -95,6 +95,7 @@ type User struct {
DefaultListID int64 `xorm:"bigint null index" json:"-"`
WeekStart int `xorm:"null" json:"-"`
Language string `xorm:"varchar(50) null" json:"-"`
Timezone string `xorm:"varchar(255) null" json:"-"`
DeletionScheduledAt time.Time `xorm:"datetime null" json:"-"`
DeletionLastReminderSent time.Time `xorm:"datetime null" json:"-"`
@ -462,6 +463,16 @@ func UpdateUser(s *xorm.Session, user *User) (updatedUser *User, err error) {
}
}
// Check if we have a valid time zone
if user.Timezone == "" {
user.Timezone = config.GetTimeZone().String()
}
_, err = time.LoadLocation(user.Timezone)
if err != nil {
return
}
// Update it
_, err = s.
ID(user.ID).
@ -479,6 +490,7 @@ func UpdateUser(s *xorm.Session, user *User) (updatedUser *User, err error) {
"default_list_id",
"week_start",
"language",
"timezone",
).
Update(user)
if err != nil {