diff --git a/services/api/internal/bootstrap/app.go b/services/api/internal/bootstrap/app.go index a686f4644..544b5a712 100644 --- a/services/api/internal/bootstrap/app.go +++ b/services/api/internal/bootstrap/app.go @@ -172,7 +172,8 @@ func New(cfg *config.Config) (*App, error) { sprintService := sprintsvc.NewCachedSprintService(sprintsvc.New(sprintRepo, taskRepo, publisher), cacheStore, cfg.Cache.SprintTTL, log) viewService := sprintsvc.NewCachedViewService(sprintsvc.NewViewService(viewRepo, sprintRepo, taskRepo, publisher), cacheStore, cfg.Cache.SprintTTL, log) notificationService := notificationsvc.New(notificationRepo, projectRepo, publisher). - WithEventPublishing(userRepo, cfg.Server.PublicURL) + WithEventPublishing(userRepo, cfg.Server.PublicURL). + WithTitleLookup(taskRepo, docRepo) agentService := agentsvc.New(agentRepo, projectService, publisher, pluginRepo) // environmentService calls agent-runner via the same AI_AGENT_URL/ // AI_AGENT_INTERNAL_KEY pair AgentHandler already uses for its fast diff --git a/services/api/internal/platform/plugin/email_test.go b/services/api/internal/platform/plugin/email_test.go index 801f66bc1..822c3b2f8 100644 --- a/services/api/internal/platform/plugin/email_test.go +++ b/services/api/internal/platform/plugin/email_test.go @@ -9,7 +9,8 @@ import ( func TestDialAndSendSMTP_BlocksPrivateTargets(t *testing.T) { // A live loopback listener proves the block happens before any connect. - ln, err := net.Listen("tcp", "127.0.0.1:0") + var lc net.ListenConfig + ln, err := lc.Listen(context.Background(), "tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) } diff --git a/services/api/internal/repository/postgres/document_repository.go b/services/api/internal/repository/postgres/document_repository.go index 869636dd6..427660b8c 100644 --- a/services/api/internal/repository/postgres/document_repository.go +++ b/services/api/internal/repository/postgres/document_repository.go @@ -388,6 +388,22 @@ func (r *DocumentRepository) FindDocumentByID(_ context.Context, id uuid.UUID) ( return documentFromRecord(rec), nil } +// FindDocumentTitleByID returns only the title of a non-deleted document — a +// cheaper read than FindDocumentByID (which pulls the full row, including the +// content) for callers that need just the title, e.g. building a notification +// event payload. Returns ErrDocNotFound when the document doesn't exist or is +// deleted. +func (r *DocumentRepository) FindDocumentTitleByID(_ context.Context, id uuid.UUID) (string, error) { + var title string + if err := r.db.Get(&title, `SELECT title FROM documents WHERE id = $1 AND deleted_at IS NULL`, id.String()); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return "", docdom.ErrDocNotFound + } + return "", err + } + return title, nil +} + // CreateDocument persists a new document. func (r *DocumentRepository) CreateDocument(ctx context.Context, d *docdom.Document) error { rec := documentToRecord(d) diff --git a/services/api/internal/repository/postgres/task_repository.go b/services/api/internal/repository/postgres/task_repository.go index 71a5feb6b..6c1ceda1d 100644 --- a/services/api/internal/repository/postgres/task_repository.go +++ b/services/api/internal/repository/postgres/task_repository.go @@ -1272,6 +1272,23 @@ func (r *TaskRepository) FindTaskByID(ctx context.Context, id uuid.UUID) (*taskd return t, nil } +// FindTaskTitleByID returns only the title of a non-deleted task — a cheaper +// read than FindTaskByID (which pulls the full row, including the description +// JSONB, plus a second assignee query) for callers that need just the title, +// e.g. building a notification event payload. Returns ErrTaskNotFound when the +// task doesn't exist or is deleted. +func (r *TaskRepository) FindTaskTitleByID(ctx context.Context, id uuid.UUID) (string, error) { + var title string + err := r.db.GetContext(ctx, &title, `SELECT title FROM tasks WHERE id = $1 AND deleted_at IS NULL`, id.String()) + if errors.Is(err, sql.ErrNoRows) { + return "", taskdom.ErrTaskNotFound + } + if err != nil { + return "", fmt.Errorf("task repo: find title by id: %w", err) + } + return title, nil +} + // FindTaskByNumber returns the task with the given project-scoped task number (non-deleted). func (r *TaskRepository) FindTaskByNumber(ctx context.Context, projectID uuid.UUID, taskNumber int64) (*taskdom.Task, error) { var rec taskRecord diff --git a/services/api/internal/service/notification/service.go b/services/api/internal/service/notification/service.go index c4e365d96..5b8ad8ed4 100644 --- a/services/api/internal/service/notification/service.go +++ b/services/api/internal/service/notification/service.go @@ -14,7 +14,6 @@ import ( projectdom "github.com/Paca-AI/api/internal/domain/project" userdom "github.com/Paca-AI/api/internal/domain/user" "github.com/Paca-AI/api/internal/events" - "github.com/Paca-AI/api/internal/platform/messaging" ) // userLookup resolves a recipient's name/email for the notification.assigned/ @@ -41,18 +40,47 @@ type memberLookup interface { ListMembers(ctx context.Context, projectID uuid.UUID) ([]*projectdom.ProjectMember, error) } +// taskLookup resolves a task's title for the entity_title field of the +// notification.{assigned,mentioned,task_description_mentioned} plugin-event +// payloads, so a subscribing plugin (e.g. an email sender) can name the task +// a notification is about instead of only linking to it. Deliberately a +// title-only read, not FindTaskByID, so this doesn't pull the full task row +// (description JSONB + assignees) on the synchronous comment/update path. +type taskLookup interface { + FindTaskTitleByID(ctx context.Context, id uuid.UUID) (string, error) +} + +// docLookup resolves a document's title for the entity_title field of the +// notification.doc_mentioned plugin-event payload. Title-only, for the same +// reason as taskLookup (avoids loading the full document content). +type docLookup interface { + FindDocumentTitleByID(ctx context.Context, id uuid.UUID) (string, error) +} + +// eventPublisher is the subset of messaging.Publisher this service uses: +// Append for the durable plugin-event stream and Publish for the real-time +// channel. Declared as an interface (rather than the concrete +// *messaging.Publisher) so a test can capture what's published; +// *messaging.Publisher satisfies it. +type eventPublisher interface { + Append(ctx context.Context, stream, eventType string, payload any) error + Publish(ctx context.Context, channel string, payload any) error +} + // Svc implements notificationdom.Service. type Svc struct { repo notificationdom.Repository memberRepo memberLookup - publisher *messaging.Publisher + publisher eventPublisher userRepo userLookup + taskRepo taskLookup + docRepo docLookup publicURL string } // New returns a configured Svc. // publisher may be nil; real-time events are then skipped silently. -func New(repo notificationdom.Repository, memberRepo memberLookup, publisher *messaging.Publisher) *Svc { +func New(repo notificationdom.Repository, memberRepo memberLookup, publisher eventPublisher) *Svc { return &Svc{repo: repo, memberRepo: memberRepo, publisher: publisher} } @@ -70,6 +98,17 @@ func (s *Svc) WithEventPublishing(userRepo userLookup, publicURL string) *Svc { return s } +// WithTitleLookup configures resolving the task/document title carried as +// entity_title in the notification plugin-event payloads. Both lookups are +// optional and best-effort: when a repo is nil (or the lookup fails) the +// title is simply empty in the payload — exactly the behaviour before this was +// added — so a subscribing plugin must tolerate an empty entity_title. +func (s *Svc) WithTitleLookup(taskRepo taskLookup, docRepo docLookup) *Svc { + s.taskRepo = taskRepo + s.docRepo = docRepo + return s +} + // publishNotificationEvent resolves recipientUserID's name/email and // publishes topic to the plugin event stream with recipient/actor names and // a direct link to whatever was mentioned/assigned — the same event fires @@ -80,7 +119,10 @@ func (s *Svc) WithEventPublishing(userRepo userLookup, publicURL string) *Svc { // errors are swallowed, matching publishCreated's existing convention, // since the in-app notification (already created by the caller, where one // exists) is this service's primary responsibility. -func (s *Svc) publishNotificationEvent(ctx context.Context, topic string, recipientUserID uuid.UUID, actorName, linkURL string) { +// entityTitle is the title of the task or document the notification is about +// (resolved via WithTitleLookup); it may be empty when no lookup is configured +// or the entity can't be loaded, and a subscribing plugin must tolerate that. +func (s *Svc) publishNotificationEvent(ctx context.Context, topic string, recipientUserID uuid.UUID, actorName, linkURL, entityTitle string) { if s.publisher == nil || s.userRepo == nil { return } @@ -99,10 +141,38 @@ func (s *Svc) publishNotificationEvent(ctx context.Context, topic string, recipi "recipient_name": recipient.FullName, "actor_name": actorName, "link_url": linkURL, + "entity_title": entityTitle, } _ = s.publisher.Append(ctx, events.StreamPluginEvents, topic, payload) } +// taskTitle resolves taskID's title for an event payload, returning "" when no +// task lookup is configured or the task can't be loaded — best-effort, like +// the rest of publishNotificationEvent. +func (s *Svc) taskTitle(ctx context.Context, taskID uuid.UUID) string { + if s.taskRepo == nil { + return "" + } + title, err := s.taskRepo.FindTaskTitleByID(ctx, taskID) + if err != nil { + return "" + } + return title +} + +// docTitle resolves docID's title for an event payload, returning "" when no +// document lookup is configured or the document can't be loaded. +func (s *Svc) docTitle(ctx context.Context, docID uuid.UUID) string { + if s.docRepo == nil { + return "" + } + title, err := s.docRepo.FindDocumentTitleByID(ctx, docID) + if err != nil { + return "" + } + return title +} + // resolveUserName returns userID's full name, or "" if it can't be resolved // (no userRepo configured, or the lookup fails). func (s *Svc) resolveUserName(ctx context.Context, userID uuid.UUID) string { @@ -176,7 +246,7 @@ func (s *Svc) NotifyAssigned(ctx context.Context, in notificationdom.NotifyAssig if actorMember != nil { actorName = actorMember.FullName } - s.publishNotificationEvent(ctx, events.TopicNotificationAssigned, assigneeMember.UserID, actorName, s.taskURL(in.ProjectID, taskID)) + s.publishNotificationEvent(ctx, events.TopicNotificationAssigned, assigneeMember.UserID, actorName, s.taskURL(in.ProjectID, taskID), s.taskTitle(ctx, taskID)) return nil } @@ -223,6 +293,9 @@ func (s *Svc) NotifyMentioned(ctx context.Context, in notificationdom.NotifyMent if actorMember, err := s.memberRepo.FindMemberByID(ctx, actorID); err == nil { actorName = actorMember.FullName } + // Resolved once (not per-mentioned-user) — the task is the same for every + // notification created below. + taskTitle := s.taskTitle(ctx, in.TaskID) seen := make(map[uuid.UUID]bool) // avoid duplicate notifications for _, mentionedUserID := range mentionedUserIDs { // Skip self-mention. @@ -255,7 +328,7 @@ func (s *Svc) NotifyMentioned(ctx context.Context, in notificationdom.NotifyMent continue // best-effort; don't abort for one failure } s.publishCreated(ctx, n, mentionedUserID) - s.publishNotificationEvent(ctx, events.TopicNotificationMentioned, mentionedUserID, actorName, s.taskURL(in.ProjectID, taskID)) + s.publishNotificationEvent(ctx, events.TopicNotificationMentioned, mentionedUserID, actorName, s.taskURL(in.ProjectID, taskID), taskTitle) } return nil } @@ -286,7 +359,7 @@ func (s *Svc) NotifyDocMentioned(ctx context.Context, mentionedUserID, actorUser return // mentioned user isn't a member of this project; nothing to notify } actorName := s.resolveUserName(ctx, actorUserID) - s.publishNotificationEvent(ctx, events.TopicNotificationDocMentioned, mentionedUserID, actorName, s.docURL(projectID, docID)) + s.publishNotificationEvent(ctx, events.TopicNotificationDocMentioned, mentionedUserID, actorName, s.docURL(projectID, docID), s.docTitle(ctx, docID)) } // NotifyTaskDescriptionMentioned publishes a @@ -303,7 +376,7 @@ func (s *Svc) NotifyTaskDescriptionMentioned(ctx context.Context, mentionedUserI return // mentioned user isn't a member of this project; nothing to notify } actorName := s.resolveUserName(ctx, actorUserID) - s.publishNotificationEvent(ctx, events.TopicNotificationTaskDescriptionMentioned, mentionedUserID, actorName, s.taskURL(projectID, taskID)) + s.publishNotificationEvent(ctx, events.TopicNotificationTaskDescriptionMentioned, mentionedUserID, actorName, s.taskURL(projectID, taskID), s.taskTitle(ctx, taskID)) } // ListNotifications returns up to limit notifications for the user, newest diff --git a/services/api/internal/service/notification/service_test.go b/services/api/internal/service/notification/service_test.go index 5eca89032..32f1ff248 100644 --- a/services/api/internal/service/notification/service_test.go +++ b/services/api/internal/service/notification/service_test.go @@ -223,6 +223,81 @@ func (r *fakeUserRepo) callCount() int { return len(r.calls) } +// fakeTaskRepo / fakeDocRepo back the optional WithTitleLookup dependency, +// tracking lookups so a test can prove the entity_title resolution runs (and, +// for the client-supplied-mention cases, that it runs only once the recipient +// has passed the project-membership check). +type fakeTaskRepo struct { + mu sync.Mutex + title string + calls int +} + +func (r *fakeTaskRepo) FindTaskTitleByID(_ context.Context, _ uuid.UUID) (string, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls++ + return r.title, nil +} + +func (r *fakeTaskRepo) callCount() int { + r.mu.Lock() + defer r.mu.Unlock() + return r.calls +} + +type fakeDocRepo struct { + mu sync.Mutex + title string + calls int +} + +func (r *fakeDocRepo) FindDocumentTitleByID(_ context.Context, _ uuid.UUID) (string, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls++ + return r.title, nil +} + +func (r *fakeDocRepo) callCount() int { + r.mu.Lock() + defer r.mu.Unlock() + return r.calls +} + +// fakePublisher satisfies eventPublisher and captures every Append so a test +// can assert the exact payload published to the plugin event stream (the real +// *messaging.Publisher is a concrete Redis-backed struct that can't be +// observed in a unit test). +type capturedEvent struct { + stream string + eventType string + payload any +} + +type fakePublisher struct { + mu sync.Mutex + appends []capturedEvent +} + +func (p *fakePublisher) Append(_ context.Context, stream, eventType string, payload any) error { + p.mu.Lock() + defer p.mu.Unlock() + p.appends = append(p.appends, capturedEvent{stream: stream, eventType: eventType, payload: payload}) + return nil +} + +func (p *fakePublisher) Publish(_ context.Context, _ string, _ any) error { return nil } + +func (p *fakePublisher) lastAppend() (capturedEvent, bool) { + p.mu.Lock() + defer p.mu.Unlock() + if len(p.appends) == 0 { + return capturedEvent{}, false + } + return p.appends[len(p.appends)-1], true +} + type errCreateRepo struct { *fakeNotificationRepo } @@ -1112,3 +1187,169 @@ func TestNotifyTaskDescriptionMentioned_MemberNotified(t *testing.T) { t.Errorf("expected the actor-name lookup to proceed for a mentioned project member, got %d calls", got) } } + +// --- entity_title resolution (WithTitleLookup) ------------------------------ +// +// These tests run with a nil publisher as the rest of the file does: the title +// helpers are evaluated as arguments to publishNotificationEvent, so they run +// (and the lookup is observable) even though the publish itself is a no-op. + +func TestNotifyAssigned_ResolvesTaskTitle(t *testing.T) { + ctx := context.Background() + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + tasks := &fakeTaskRepo{title: "Ship the landing page"} + svc := New(repo, members, nil).WithTitleLookup(tasks, nil) + + projectID := uuid.New() + actorUserID := uuid.New() + assigneeUserID := uuid.New() + actorMemberID := uuid.New() + assigneeMemberID := uuid.New() + members.add(&projectdom.ProjectMember{ID: actorMemberID, ProjectID: projectID, UserID: actorUserID, Username: "actor"}) + members.add(&projectdom.ProjectMember{ID: assigneeMemberID, ProjectID: projectID, UserID: assigneeUserID, Username: "assignee"}) + + if err := svc.NotifyAssigned(ctx, notificationdom.NotifyAssignedInput{ + TaskID: uuid.New(), + ProjectID: projectID, + NewAssigneeMemberID: assigneeMemberID, + ActorUserID: actorUserID, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := tasks.callCount(); got != 1 { + t.Errorf("expected the task title to be resolved once for the assignment event, got %d lookups", got) + } +} + +func TestNotifyMentioned_ResolvesTaskTitleOnce(t *testing.T) { + ctx := context.Background() + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + tasks := &fakeTaskRepo{title: "Ship the landing page"} + svc := New(repo, members, nil).WithTitleLookup(tasks, nil) + + projectID := uuid.New() + actorUserID := uuid.New() + actorMemberID := uuid.New() + alice := uuid.New() + bob := uuid.New() + members.add(&projectdom.ProjectMember{ID: actorMemberID, ProjectID: projectID, UserID: actorUserID, Username: "actor"}) + members.add(&projectdom.ProjectMember{ID: uuid.New(), ProjectID: projectID, UserID: alice, Username: "alice"}) + members.add(&projectdom.ProjectMember{ID: uuid.New(), ProjectID: projectID, UserID: bob, Username: "bob"}) + + if err := svc.NotifyMentioned(ctx, notificationdom.NotifyMentionedInput{ + TaskID: uuid.New(), + ProjectID: projectID, + CommentText: "hey @alice and @bob", + ActorMemberID: actorMemberID, + ActorUserID: actorUserID, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Two members mentioned, but the task title is the same for both — it must + // be resolved once, not once per recipient. + if got := tasks.callCount(); got != 1 { + t.Errorf("expected the task title to be resolved once for the whole comment, got %d lookups", got) + } +} + +func TestNotifyDocMentioned_ResolvesTitleOnlyForMember(t *testing.T) { + ctx := context.Background() + projectID := uuid.New() + actorUserID := uuid.New() + + // Member path: title resolved exactly once. + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + users := newFakeUserRepo() + docs := &fakeDocRepo{title: "Q4 roadmap"} + svc := New(repo, members, nil).WithEventPublishing(users, "https://paca.example").WithTitleLookup(nil, docs) + mentioned := uuid.New() + members.add(&projectdom.ProjectMember{ID: uuid.New(), ProjectID: projectID, UserID: mentioned, Username: "alice"}) + svc.NotifyDocMentioned(ctx, mentioned, actorUserID, projectID, uuid.New()) + if got := docs.callCount(); got != 1 { + t.Errorf("expected the document title to be resolved once for a mentioned member, got %d lookups", got) + } + + // Non-member path: membership check short-circuits before any title lookup. + docs2 := &fakeDocRepo{title: "Q4 roadmap"} + svc2 := New(newFakeNotificationRepo(), newFakeMemberRepo(), nil).WithEventPublishing(newFakeUserRepo(), "https://paca.example").WithTitleLookup(nil, docs2) + svc2.NotifyDocMentioned(ctx, uuid.New(), actorUserID, projectID, uuid.New()) + if got := docs2.callCount(); got != 0 { + t.Errorf("expected no document title lookup for a mention naming a non-member, got %d lookups", got) + } +} + +func TestNotifyTaskDescriptionMentioned_ResolvesTitleOnlyForMember(t *testing.T) { + ctx := context.Background() + projectID := uuid.New() + actorUserID := uuid.New() + + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + users := newFakeUserRepo() + tasks := &fakeTaskRepo{title: "Ship the landing page"} + svc := New(repo, members, nil).WithEventPublishing(users, "https://paca.example").WithTitleLookup(tasks, nil) + mentioned := uuid.New() + members.add(&projectdom.ProjectMember{ID: uuid.New(), ProjectID: projectID, UserID: mentioned, Username: "alice"}) + svc.NotifyTaskDescriptionMentioned(ctx, mentioned, actorUserID, projectID, uuid.New()) + if got := tasks.callCount(); got != 1 { + t.Errorf("expected the task title to be resolved once for a mentioned member, got %d lookups", got) + } + + tasks2 := &fakeTaskRepo{title: "Ship the landing page"} + svc2 := New(newFakeNotificationRepo(), newFakeMemberRepo(), nil).WithEventPublishing(newFakeUserRepo(), "https://paca.example").WithTitleLookup(tasks2, nil) + svc2.NotifyTaskDescriptionMentioned(ctx, uuid.New(), actorUserID, projectID, uuid.New()) + if got := tasks2.callCount(); got != 0 { + t.Errorf("expected no task title lookup for a mention naming a non-member, got %d lookups", got) + } +} + +// TestNotifyAssigned_PublishesEntityTitle is the direct output test the rest +// of this group's count-based assertions can't give: it captures the payload +// handed to the publisher and proves entity_title carries the resolved title. +func TestNotifyAssigned_PublishesEntityTitle(t *testing.T) { + ctx := context.Background() + repo := newFakeNotificationRepo() + members := newFakeMemberRepo() + users := newFakeUserRepo() + pub := &fakePublisher{} + tasks := &fakeTaskRepo{title: "Ship the landing page"} + svc := New(repo, members, pub). + WithEventPublishing(users, "https://paca.example"). + WithTitleLookup(tasks, nil) + + projectID := uuid.New() + actorUserID := uuid.New() + assigneeUserID := uuid.New() + actorMemberID := uuid.New() + assigneeMemberID := uuid.New() + members.add(&projectdom.ProjectMember{ID: actorMemberID, ProjectID: projectID, UserID: actorUserID, Username: "actor"}) + members.add(&projectdom.ProjectMember{ID: assigneeMemberID, ProjectID: projectID, UserID: assigneeUserID, Username: "assignee"}) + email := "assignee@example.com" + users.byID[assigneeUserID] = &userdom.User{ID: assigneeUserID, FullName: "Assignee", Email: &email} + + if err := svc.NotifyAssigned(ctx, notificationdom.NotifyAssignedInput{ + TaskID: uuid.New(), + ProjectID: projectID, + NewAssigneeMemberID: assigneeMemberID, + ActorUserID: actorUserID, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + evt, ok := pub.lastAppend() + if !ok { + t.Fatal("expected a plugin event to be published") + } + payload, ok := evt.payload.(map[string]any) + if !ok { + t.Fatalf("expected a map payload, got %T", evt.payload) + } + if got := payload["entity_title"]; got != "Ship the landing page" { + t.Errorf("expected entity_title=%q in the published payload, got %v", "Ship the landing page", got) + } +}