feat(notifications): notify users of their own actions by default#909
feat(notifications): notify users of their own actions by default#909timothyfroehlich merged 2 commits intomainfrom
Conversation
Users requested notifications for their own actions as a personal record/reminder in their inbox. This changes the default behavior of createNotification to include the actor in the recipient list. Changes: - notifications.ts: Set includeActor default to true - issues.ts: Remove redundant includeActor: true flags (now default) - issues.test.ts: Remove includeActor from test expectations - Integration tests: Add explicit includeActor: false to tests that specifically test actor exclusion or recipient preference behavior - database-queries.test.ts: Fix invalid UUID that was using "some-other-user-id" instead of a valid UUID format Fixes #849 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR changes the default notification behavior to include actors (users who perform actions) in the recipient list, addressing user feedback that they want a personal record of their actions in their inbox. The change is implemented by modifying the includeActor parameter default from undefined to true in the createNotification function.
Changes:
- Changed
includeActordefault parameter fromundefinedtotrueincreateNotification - Removed redundant
includeActor: trueflags from service call sites (3 locations in issues service) - Updated integration tests to explicitly set
includeActor: falsewhen testing actor exclusion scenarios - Fixed pre-existing invalid UUID bug in database-queries.test.ts
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib/notifications.ts | Changed default value of includeActor parameter from undefined to true |
| src/services/issues.ts | Removed redundant includeActor: true flags from createIssue, updateIssueStatus, and assignIssue functions |
| src/services/issues.test.ts | Updated test expectations to remove redundant includeActor: true assertions |
| src/test/integration/notifications.test.ts | Added explicit includeActor: false to tests that verify actor exclusion behavior |
| src/test/integration/database-queries.test.ts | Fixed invalid UUID and added explicit includeActor: false with clarifying comment |
| resourceId: issue.id, | ||
| resourceType: "issue", | ||
| actorId: actor.id, | ||
| includeActor: false, // Explicitly test actor exclusion | ||
| commentContent: "Test comment", | ||
| }, | ||
| db |
There was a problem hiding this comment.
Critical test coverage gap: There is no test that validates the new default behavior where actors ARE notified by default. The existing tests either explicitly exclude the actor with includeActor false, or they don't verify that the actor receives a notification. Consider adding a test like "should notify the actor by default" that creates a notification with an actor, doesn't specify includeActor, and verifies that the actor receives the notification. This would ensure the new default behavior is properly tested and prevent regressions.
|
Closing - implementation needs rework |
Addresses Copilot review feedback: adds a test that verifies actors ARE notified by default when includeActor is not specified. This ensures the new default behavior is properly tested. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Thanks for the review feedback! I've added a test |
1 participant
Summary
Users requested notifications for their own actions as a personal record/reminder in their inbox. This changes the default behavior of
createNotificationto include the actor in the recipient list.includeActordefault fromundefinedtotrueincludeActor: trueflags from call sitesFixes #849
Test plan
🤖 Generated with Claude Code