diff --git a/.golangci.yml b/.golangci.yml
index 19ee2f531..7c75f08de 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -80,6 +80,9 @@ linters:
- linters:
- exhaustive
path: pkg/models/task_collection_filter\.go
+ - linters:
+ - exhaustive
+ path: pkg/models/notifications_refresh\.go
- linters:
- gosec
path: pkg/utils/random_string\.go
diff --git a/pkg/models/notifications_database.go b/pkg/models/notifications_database.go
index aaa71103c..75beef580 100644
--- a/pkg/models/notifications_database.go
+++ b/pkg/models/notifications_database.go
@@ -53,7 +53,13 @@ func (d *DatabaseNotifications) ReadAll(s *xorm.Session, a web.Auth, _ string, p
}
limit, start := getLimitFromPageIndex(page, perPage)
- return notifications.GetNotificationsForUser(s, a.GetID(), limit, start)
+ ns, resultCount, total, err := notifications.GetNotificationsForUser(s, a.GetID(), limit, start)
+ if err != nil {
+ return nil, 0, 0, err
+ }
+
+ refreshNotificationsUsers(s, ns)
+ return ns, resultCount, total, nil
}
// CanUpdate checks if a user can mark a notification as read.
diff --git a/pkg/models/notifications_refresh.go b/pkg/models/notifications_refresh.go
new file mode 100644
index 000000000..81e8a9609
--- /dev/null
+++ b/pkg/models/notifications_refresh.go
@@ -0,0 +1,122 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present 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 License 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 License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see .
+
+package models
+
+import (
+ "encoding/json"
+ "reflect"
+
+ "code.vikunja.io/api/pkg/log"
+ "code.vikunja.io/api/pkg/notifications"
+ "code.vikunja.io/api/pkg/user"
+
+ "xorm.io/xorm"
+)
+
+// maxNotificationUserRefreshDepth bounds the reflection walk so an unexpectedly
+// deep payload cannot recurse without end.
+const maxNotificationUserRefreshDepth = 8
+
+// refreshNotificationsUsers reloads every embedded user of each notification
+// from the database. Notifications serialized before the acting user was
+// resolved with its full profile (#2720) stored only id+username, so without
+// this they keep rendering the auto-generated username instead of the display
+// name. It runs at read time and is not persisted; one cache is shared across
+// the batch so a user recurring across notifications is fetched only once.
+func refreshNotificationsUsers(s *xorm.Session, dbNotifications []*notifications.DatabaseNotification) {
+ cache := make(map[int64]*user.User)
+ for _, dbn := range dbNotifications {
+ refreshNotificationUsers(s, dbn, cache)
+ }
+}
+
+func refreshNotificationUsers(s *xorm.Session, dbn *notifications.DatabaseNotification, cache map[int64]*user.User) {
+ typed, ok := notifications.Lookup(dbn.Name)
+ if !ok {
+ return
+ }
+
+ raw, err := json.Marshal(dbn.Notification)
+ if err != nil {
+ log.Errorf("Could not marshal notification %d to refresh its users: %v", dbn.ID, err)
+ return
+ }
+ if err := json.Unmarshal(raw, typed); err != nil {
+ log.Errorf("Could not unmarshal notification %d to refresh its users: %v", dbn.ID, err)
+ return
+ }
+
+ refreshUsersInValue(s, reflect.ValueOf(typed), cache, 0)
+ dbn.Notification = typed
+}
+
+func refreshUsersInValue(s *xorm.Session, v reflect.Value, cache map[int64]*user.User, depth int) {
+ if depth > maxNotificationUserRefreshDepth || !v.IsValid() {
+ return
+ }
+
+ switch v.Kind() {
+ case reflect.Ptr:
+ if v.IsNil() {
+ return
+ }
+ if u, is := v.Interface().(*user.User); is {
+ refreshUser(s, u, cache)
+ return
+ }
+ refreshUsersInValue(s, v.Elem(), cache, depth+1)
+ case reflect.Struct:
+ for i := 0; i < v.NumField(); i++ {
+ if !v.Type().Field(i).IsExported() {
+ continue
+ }
+ refreshUsersInValue(s, v.Field(i), cache, depth+1)
+ }
+ case reflect.Slice, reflect.Array:
+ for i := 0; i < v.Len(); i++ {
+ refreshUsersInValue(s, v.Index(i), cache, depth+1)
+ }
+ case reflect.Map:
+ for _, key := range v.MapKeys() {
+ refreshUsersInValue(s, v.MapIndex(key), cache, depth+1)
+ }
+ }
+}
+
+// refreshUser overwrites the user in place with its current database row. A
+// disabled or locked account is still returned fully populated, so only a
+// missing user or a real database error leaves the stored value untouched.
+func refreshUser(s *xorm.Session, u *user.User, cache map[int64]*user.User) {
+ if u == nil || u.ID == 0 {
+ return
+ }
+
+ fresh, cached := cache[u.ID]
+ if !cached {
+ loaded, err := user.GetUserByID(s, u.ID)
+ if err != nil && !user.IsErrUserStatusError(err) {
+ cache[u.ID] = nil
+ return
+ }
+ fresh = loaded
+ cache[u.ID] = fresh
+ }
+
+ if fresh != nil {
+ *u = *fresh
+ }
+}
diff --git a/pkg/models/notifications_refresh_test.go b/pkg/models/notifications_refresh_test.go
new file mode 100644
index 000000000..b0cef2a43
--- /dev/null
+++ b/pkg/models/notifications_refresh_test.go
@@ -0,0 +1,109 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present 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 License 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 License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see .
+
+package models
+
+import (
+ "encoding/json"
+ "testing"
+
+ "code.vikunja.io/api/pkg/db"
+ "code.vikunja.io/api/pkg/notifications"
+ "code.vikunja.io/api/pkg/user"
+
+ "github.com/stretchr/testify/require"
+ "xorm.io/xorm"
+)
+
+// TestDatabaseNotifications_ReadAll_RefreshesUsers guards #2720 for notifications
+// already in the database: those were serialized with a partial doer (id +
+// username, no display Name), so reading them must reload the embedded users so
+// the display name is shown. The fix in the dispatch path only helps new
+// notifications; old rows are healed here at read time.
+func TestDatabaseNotifications_ReadAll_RefreshesUsers(t *testing.T) {
+ t.Run("fills in the display name from the database", func(t *testing.T) {
+ db.LoadAndAssertFixtures(t)
+ s := db.NewSession()
+ defer s.Close()
+
+ // user12 has the display name "Name with spaces" in the fixtures.
+ insertStoredNotification(t, s, 1, &TaskAssignedNotification{
+ Doer: &user.User{ID: 12, Username: "user12"},
+ Assignee: &user.User{ID: 12, Username: "user12"},
+ Task: &Task{ID: 1},
+ })
+
+ got := readAssignedNotification(t, s, 1)
+ require.Equal(t, "Name with spaces", got.Doer.GetName())
+ require.Equal(t, "Name with spaces", got.Assignee.GetName())
+ })
+
+ t.Run("keeps the stored value when the user no longer exists", func(t *testing.T) {
+ db.LoadAndAssertFixtures(t)
+ s := db.NewSession()
+ defer s.Close()
+
+ insertStoredNotification(t, s, 1, &TaskAssignedNotification{
+ Doer: &user.User{ID: 999999, Username: "ghost"},
+ Task: &Task{ID: 1},
+ })
+
+ got := readAssignedNotification(t, s, 1)
+ require.Equal(t, "ghost", got.Doer.Username)
+ })
+
+ t.Run("refreshes a disabled user", func(t *testing.T) {
+ db.LoadAndAssertFixtures(t)
+ s := db.NewSession()
+ defer s.Close()
+
+ // user17 is disabled in the fixtures; the reload must still win over the
+ // stale stored value.
+ insertStoredNotification(t, s, 1, &TaskAssignedNotification{
+ Doer: &user.User{ID: 17, Username: "stale"},
+ Task: &Task{ID: 1},
+ })
+
+ got := readAssignedNotification(t, s, 1)
+ require.Equal(t, "user17", got.Doer.Username)
+ })
+}
+
+func insertStoredNotification(t *testing.T, s *xorm.Session, notifiableID int64, n notifications.Notification) {
+ t.Helper()
+ content, err := json.Marshal(n)
+ require.NoError(t, err)
+ _, err = s.Insert(¬ifications.DatabaseNotification{
+ NotifiableID: notifiableID,
+ Notification: json.RawMessage(content),
+ Name: n.Name(),
+ })
+ require.NoError(t, err)
+}
+
+func readAssignedNotification(t *testing.T, s *xorm.Session, notifiableID int64) *TaskAssignedNotification {
+ t.Helper()
+ result, _, _, err := (&DatabaseNotifications{}).ReadAll(s, &user.User{ID: notifiableID}, "", 1, 50)
+ require.NoError(t, err)
+
+ for _, dbn := range result.([]*notifications.DatabaseNotification) {
+ if n, is := dbn.Notification.(*TaskAssignedNotification); is {
+ return n
+ }
+ }
+ t.Fatal("no task.assigned notification was returned")
+ return nil
+}