From 0cd25f47e53bffaebf08aecb20939ec01b49b244 Mon Sep 17 00:00:00 2001 From: kolaente Date: Sat, 24 Jan 2026 12:50:18 +0100 Subject: [PATCH] fix: populate complete entity data in deletion event webhooks (#2135) Fixes webhook payloads for deletion events that were previously containing incomplete or empty entity data. This occurred because entities were being deleted from the database before the webhook event was dispatched. ## Changes This PR implements four targeted fixes to ensure complete entity data in deletion event webhooks: ### 1. TaskAssignee Deletion (`pkg/models/listeners.go`) - Extended `reloadEventData()` to fetch full assignee user data by ID - Webhook payload now includes complete user object (username, email, timestamps, etc.) ### 2. TaskComment Deletion (`pkg/models/task_comments.go`) - Modified `Delete()` to call `ReadOne()` before deletion - Ensures comment text, author, and timestamps are included in webhook payload - Follows the same pattern used by `Task.Delete()` and `TaskAttachment.Delete()` ### 3. TaskAttachment Deletion (`pkg/models/task_attachment.go`) - Extended `ReadOne()` to fetch the `CreatedBy` user - Webhook payload now includes file creator information ### 4. TaskRelation Deletion (`pkg/models/task_relation.go`) - Modified `Delete()` to fetch complete relation including `CreatedBy` user before deletion - Webhook payload now includes relation timestamps and creator information Fixes #2125 --- pkg/models/listeners.go | 193 ++++++++++++++++++++++++---------- pkg/models/task_attachment.go | 12 ++- pkg/models/task_comments.go | 10 +- pkg/models/task_relation.go | 51 +++++---- 4 files changed, 192 insertions(+), 74 deletions(-) diff --git a/pkg/models/listeners.go b/pkg/models/listeners.go index 6112b8f3a..7317ce949 100644 --- a/pkg/models/listeners.go +++ b/pkg/models/listeners.go @@ -896,65 +896,152 @@ func getProjectIDFromAnyEvent(eventPayload map[string]interface{}) int64 { return 0 } +func reloadDoerInEvent(s *xorm.Session, event map[string]interface{}) (doerID int64, err error) { + doer, has := event["doer"] + if !has || doer == nil { + return 0, nil + } + + // doer can be null in incoming payloads, so guard the type assertion + d, ok := doer.(map[string]interface{}) + if !ok { + return 0, nil + } + + rawDoerID, has := d["id"] + if !has || rawDoerID == nil { + return 0, nil + } + + doerID = getIDAsInt64(rawDoerID) + if doerID <= 0 { + return 0, nil + } + + fullDoer, err := user.GetUserByID(s, doerID) + if err != nil && !user.IsErrUserDoesNotExist(err) { + return 0, err + } + if err == nil { + event["doer"] = fullDoer + } + + return doerID, nil +} + +func reloadTaskInEvent(s *xorm.Session, event map[string]interface{}, doerID int64) error { + task, has := event["task"] + if !has || task == nil || doerID == 0 { + return nil + } + + // guard the type assertion for task as well + t, ok := task.(map[string]interface{}) + if !ok { + return nil + } + + taskID, has := t["id"] + if !has || taskID == nil { + return nil + } + + id := getIDAsInt64(taskID) + if id <= 0 { + return nil + } + + fullTask := Task{ + ID: id, + Expand: []TaskCollectionExpandable{ + TaskCollectionExpandBuckets, + }, + } + err := fullTask.ReadOne(s, &user.User{ID: doerID}) + if err != nil && !IsErrTaskDoesNotExist(err) { + return err + } + if err == nil { + event["task"] = fullTask + } + + return nil +} + +func reloadProjectInEvent(s *xorm.Session, event map[string]interface{}, projectID, doerID int64) error { + _, has := event["project"] + if !has || doerID == 0 { + return nil + } + + project, err := GetProjectSimpleByID(s, projectID) + if err != nil { + if IsErrProjectDoesNotExist(err) { + return nil + } + return err + } + + err = project.ReadOne(s, &user.User{ID: doerID}) + if err != nil && !IsErrProjectDoesNotExist(err) { + return err + } + + if err == nil { + event["project"] = project + } + + return nil +} + +func reloadAssigneeInEvent(s *xorm.Session, event map[string]interface{}) error { + assignee, has := event["assignee"] + if !has || assignee == nil { + return nil + } + + a, ok := assignee.(map[string]interface{}) + if !ok { + return nil + } + + assigneeID := getIDAsInt64(a["id"]) + if assigneeID <= 0 { + return nil + } + + fullAssignee, err := user.GetUserByID(s, assigneeID) + if err != nil && !user.IsErrUserDoesNotExist(err) { + return err + } + if err == nil { + event["assignee"] = fullAssignee + } + + return nil +} + func reloadEventData(s *xorm.Session, event map[string]interface{}, projectID int64) (eventWithData map[string]interface{}, doerID int64, err error) { // Load event data again so that it is always populated in the webhook payload - if doer, has := event["doer"]; has && doer != nil { - // doer can be null in incoming payloads, so guard the type assertion - d, ok := doer.(map[string]interface{}) - if ok { - if rawDoerID, has := d["id"]; has && rawDoerID != nil { - doerID = getIDAsInt64(rawDoerID) - if doerID > 0 { - fullDoer, err := user.GetUserByID(s, doerID) - if err != nil && !user.IsErrUserDoesNotExist(err) { - return nil, 0, err - } - if err == nil { - event["doer"] = fullDoer - } - } - } - } + + doerID, err = reloadDoerInEvent(s, event) + if err != nil { + return nil, 0, err } - if task, has := event["task"]; has && task != nil && doerID != 0 { - // guard the type assertion for task as well - t, ok := task.(map[string]interface{}) - if ok { - if taskID, has := t["id"]; has && taskID != nil { - id := getIDAsInt64(taskID) - if id > 0 { - fullTask := Task{ - ID: id, - Expand: []TaskCollectionExpandable{ - TaskCollectionExpandBuckets, - }, - } - err = fullTask.ReadOne(s, &user.User{ID: doerID}) - if err != nil && !IsErrTaskDoesNotExist(err) { - return - } - if err == nil { - event["task"] = fullTask - } - } - } - } + err = reloadTaskInEvent(s, event, doerID) + if err != nil { + return nil, doerID, err } - if _, has := event["project"]; has && doerID != 0 { - var project *Project - project, err = GetProjectSimpleByID(s, projectID) - if err != nil && !IsErrProjectDoesNotExist(err) { - return - } - err = project.ReadOne(s, &user.User{ID: doerID}) - if err != nil && !IsErrProjectDoesNotExist(err) { - return - } - if err == nil { - event["project"] = project - } + err = reloadProjectInEvent(s, event, projectID, doerID) + if err != nil { + return nil, doerID, err + } + + err = reloadAssigneeInEvent(s, event) + if err != nil { + return nil, doerID, err } return event, doerID, nil diff --git a/pkg/models/task_attachment.go b/pkg/models/task_attachment.go index 9dcb1b806..10d9b1ffb 100644 --- a/pkg/models/task_attachment.go +++ b/pkg/models/task_attachment.go @@ -121,7 +121,17 @@ func (ta *TaskAttachment) ReadOne(s *xorm.Session, _ web.Auth) (err error) { // Get the file ta.File = &files.File{ID: ta.FileID} err = ta.File.LoadFileMetaByID() - return + if err != nil { + return + } + + // Get the creator (non-fatal if user doesn't exist) + ta.CreatedBy, err = user.GetUserByID(s, ta.CreatedByID) + if err != nil && !user.IsErrUserDoesNotExist(err) { + return err + } + + return nil } // ReadAll returns a project with all attachments diff --git a/pkg/models/task_comments.go b/pkg/models/task_comments.go index ae827ef6c..17decfc44 100644 --- a/pkg/models/task_comments.go +++ b/pkg/models/task_comments.go @@ -118,7 +118,12 @@ func (tc *TaskComment) CreateWithTimestamps(s *xorm.Session, a web.Auth) (err er // @Failure 404 {object} web.HTTPError "The task comment was not found." // @Failure 500 {object} models.Message "Internal error" // @Router /tasks/{taskID}/comments/{commentID} [delete] -func (tc *TaskComment) Delete(s *xorm.Session, _ web.Auth) error { +func (tc *TaskComment) Delete(s *xorm.Session, a web.Auth) error { + err := tc.ReadOne(s, a) + if err != nil { + return err + } + deleted, err := s. ID(tc.ID). NoAutoCondition(). @@ -131,6 +136,7 @@ func (tc *TaskComment) Delete(s *xorm.Session, _ web.Auth) error { return err } + doer, _ := user.GetFromAuth(a) task, err := GetTaskByIDSimple(s, tc.TaskID) if err != nil { return err @@ -139,7 +145,7 @@ func (tc *TaskComment) Delete(s *xorm.Session, _ web.Auth) error { return events.Dispatch(&TaskCommentDeletedEvent{ Task: &task, Comment: tc, - Doer: tc.Author, + Doer: doer, }) } diff --git a/pkg/models/task_relation.go b/pkg/models/task_relation.go index bb75f6148..3b55b38f4 100644 --- a/pkg/models/task_relation.go +++ b/pkg/models/task_relation.go @@ -20,12 +20,11 @@ import ( "time" "code.vikunja.io/api/pkg/events" + "code.vikunja.io/api/pkg/user" + "code.vikunja.io/api/pkg/web" "xorm.io/builder" "xorm.io/xorm" - - "code.vikunja.io/api/pkg/user" - "code.vikunja.io/api/pkg/web" ) // RelationKind represents a kind of relation between to tasks @@ -270,6 +269,31 @@ func (rel *TaskRelation) Create(s *xorm.Session, a web.Auth) error { }) } +// ReadOne returns a task relation +func (rel *TaskRelation) ReadOne(s *xorm.Session, _ web.Auth) (err error) { + exists, err := s. + Where("task_id = ? AND other_task_id = ? AND relation_kind = ?", rel.TaskID, rel.OtherTaskID, rel.RelationKind). + Get(rel) + if err != nil { + return err + } + if !exists { + return ErrRelationDoesNotExist{ + TaskID: rel.TaskID, + OtherTaskID: rel.OtherTaskID, + Kind: rel.RelationKind, + } + } + + // Get the creator + rel.CreatedBy, err = user.GetUserByID(s, rel.CreatedByID) + if err != nil && !user.IsErrUserDoesNotExist(err) { + return err + } + + return nil +} + // Delete removes a task relation // @Summary Remove a task relation // @tags task @@ -286,7 +310,13 @@ func (rel *TaskRelation) Create(s *xorm.Session, a web.Auth) error { // @Failure 500 {object} models.Message "Internal error" // @Router /tasks/{taskID}/relations/{relationKind}/{otherTaskID} [delete] func (rel *TaskRelation) Delete(s *xorm.Session, a web.Auth) error { + // Load the full relation first (populates CreatedBy) + err := rel.ReadOne(s, a) + if err != nil { + return err + } + // Build condition for deleting both the forward and inverse relations cond := builder.Or( builder.And( builder.Eq{"task_id": rel.TaskID}, @@ -300,21 +330,6 @@ func (rel *TaskRelation) Delete(s *xorm.Session, a web.Auth) error { ), ) - // Check if the relation exists - exists, err := s. - Where(cond). - Exist(&TaskRelation{}) - if err != nil { - return err - } - if !exists { - return ErrRelationDoesNotExist{ - TaskID: rel.TaskID, - OtherTaskID: rel.OtherTaskID, - Kind: rel.RelationKind, - } - } - _, err = s. Where(cond). Delete(&TaskRelation{})