Fix notification deduplication to prevent pile-up in navbar (#945)
* Fix notification deduplication to prevent pile-up in navbar (#944) When tasks/projects remained pending for multiple days, duplicate notifications accumulated in the navbar instead of showing only the most recent one. Updated notification services to properly handle existing notifications: - Delete unread notifications before creating new ones - Respect dismissed notifications (don't recreate) - Respect read notifications (don't duplicate) Changes: - Updated dueTaskService, deferredTaskService, and dueProjectService - Added comprehensive unit tests with 8 test cases Fixes #944 * Fix lint errors and add GitHub bug template reminder to docs
This commit is contained in:
Chris
GitHub
parent
1d5de49b48
commit
105a913a8d
5 changed files with 355 additions and 11 deletions
|
|
@ -70,8 +70,7 @@ async function checkDueProjects() {
|
|||
continue;
|
||||
}
|
||||
|
||||
// Check for existing notifications (including dismissed ones)
|
||||
// If a notification was dismissed, don't create it again
|
||||
// Check for existing notifications
|
||||
const recentNotifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: project.user_id,
|
||||
|
|
@ -91,11 +90,21 @@ async function checkDueProjects() {
|
|||
);
|
||||
|
||||
if (existingNotification) {
|
||||
// Skip if notification exists, even if it was dismissed
|
||||
// This prevents re-notifying users about tasks they've already dismissed
|
||||
// If notification was dismissed, don't create it again
|
||||
if (existingNotification.dismissed_at) {
|
||||
continue;
|
||||
}
|
||||
|
||||
// If notification is unread, delete it before creating the new one
|
||||
// This prevents duplicate notifications from piling up
|
||||
if (!existingNotification.read_at) {
|
||||
await existingNotification.destroy();
|
||||
} else {
|
||||
// If it was already read, skip creating a new one
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
const { title, message } = generateNotificationContent(
|
||||
project.name,
|
||||
dueDate,
|
||||
|
|
|
|||
|
|
@ -68,9 +68,21 @@ async function checkDeferredTasks() {
|
|||
);
|
||||
|
||||
if (existingNotification) {
|
||||
// If notification was dismissed, don't create it again
|
||||
if (existingNotification.dismissed_at) {
|
||||
continue;
|
||||
}
|
||||
|
||||
// If notification is unread, delete it before creating the new one
|
||||
// This prevents duplicate notifications from piling up
|
||||
if (!existingNotification.read_at) {
|
||||
await existingNotification.destroy();
|
||||
} else {
|
||||
// If it was already read, skip creating a new one
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
const sources = [];
|
||||
if (shouldSendTelegramNotification(task.User, 'deferUntil')) {
|
||||
sources.push('telegram');
|
||||
|
|
|
|||
|
|
@ -68,8 +68,7 @@ async function checkDueTasks() {
|
|||
continue;
|
||||
}
|
||||
|
||||
// Check for existing notifications (including dismissed ones)
|
||||
// If a notification was dismissed, don't create it again
|
||||
// Check for existing notifications
|
||||
const recentNotifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: task.user_id,
|
||||
|
|
@ -89,11 +88,21 @@ async function checkDueTasks() {
|
|||
);
|
||||
|
||||
if (existingNotification) {
|
||||
// Skip if notification exists, even if it was dismissed
|
||||
// This prevents re-notifying users about tasks they've already dismissed
|
||||
// If notification was dismissed, don't create it again
|
||||
if (existingNotification.dismissed_at) {
|
||||
continue;
|
||||
}
|
||||
|
||||
// If notification is unread, delete it before creating the new one
|
||||
// This prevents duplicate notifications from piling up
|
||||
if (!existingNotification.read_at) {
|
||||
await existingNotification.destroy();
|
||||
} else {
|
||||
// If it was already read, skip creating a new one
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
const { title, message } = generateNotificationContent(
|
||||
task.name,
|
||||
dueDate,
|
||||
|
|
|
|||
305
backend/tests/unit/modules/tasks/dueTaskService.test.js
Normal file
305
backend/tests/unit/modules/tasks/dueTaskService.test.js
Normal file
|
|
@ -0,0 +1,305 @@
|
|||
const { Task, Notification, User } = require('../../../../models');
|
||||
const { checkDueTasks } = require('../../../../modules/tasks/dueTaskService');
|
||||
const bcrypt = require('bcrypt');
|
||||
|
||||
describe('dueTaskService', () => {
|
||||
let user;
|
||||
|
||||
beforeEach(async () => {
|
||||
user = await User.create({
|
||||
email: 'test@example.com',
|
||||
password_digest: await bcrypt.hash('password123', 10),
|
||||
notification_preferences: {
|
||||
inApp: {
|
||||
task_due_soon: true,
|
||||
task_overdue: true,
|
||||
},
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkDueTasks', () => {
|
||||
describe('notification deduplication', () => {
|
||||
it('should delete existing unread notification before creating a new one', async () => {
|
||||
const tomorrow = new Date();
|
||||
tomorrow.setDate(tomorrow.getDate() + 1);
|
||||
|
||||
const task = await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: tomorrow,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
// Create initial notification
|
||||
const firstNotification = await Notification.createNotification(
|
||||
{
|
||||
userId: user.id,
|
||||
type: 'task_due_soon',
|
||||
title: 'Task due soon',
|
||||
message: 'Your task "Test Task" is due tomorrow',
|
||||
data: {
|
||||
taskUid: task.uid,
|
||||
taskName: task.name,
|
||||
dueDate: task.due_date,
|
||||
isOverdue: false,
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
expect(firstNotification.read_at).toBeFalsy();
|
||||
|
||||
// Run the check again (simulating the next day's cron job)
|
||||
await checkDueTasks();
|
||||
|
||||
// First notification should be deleted
|
||||
const deletedNotification = await Notification.findByPk(
|
||||
firstNotification.id
|
||||
);
|
||||
expect(deletedNotification).toBeNull();
|
||||
|
||||
// New notification should exist
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_due_soon',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
expect(notifications[0].id).not.toBe(firstNotification.id);
|
||||
expect(notifications[0].data.taskUid).toBe(task.uid);
|
||||
});
|
||||
|
||||
it('should not create duplicate if previous notification was read', async () => {
|
||||
const tomorrow = new Date();
|
||||
tomorrow.setDate(tomorrow.getDate() + 1);
|
||||
|
||||
const task = await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: tomorrow,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
// Create and mark as read
|
||||
const firstNotification = await Notification.createNotification(
|
||||
{
|
||||
userId: user.id,
|
||||
type: 'task_due_soon',
|
||||
title: 'Task due soon',
|
||||
message: 'Your task "Test Task" is due tomorrow',
|
||||
data: {
|
||||
taskUid: task.uid,
|
||||
taskName: task.name,
|
||||
dueDate: task.due_date,
|
||||
isOverdue: false,
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
await firstNotification.markAsRead();
|
||||
|
||||
// Run the check again
|
||||
const result = await checkDueTasks();
|
||||
|
||||
// Should not create a new notification
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_due_soon',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
expect(notifications[0].id).toBe(firstNotification.id);
|
||||
expect(result.notificationsCreated).toBe(0);
|
||||
});
|
||||
|
||||
it('should not create duplicate if previous notification was dismissed', async () => {
|
||||
const tomorrow = new Date();
|
||||
tomorrow.setDate(tomorrow.getDate() + 1);
|
||||
|
||||
const task = await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: tomorrow,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
// Create and dismiss
|
||||
const firstNotification = await Notification.createNotification(
|
||||
{
|
||||
userId: user.id,
|
||||
type: 'task_due_soon',
|
||||
title: 'Task due soon',
|
||||
message: 'Your task "Test Task" is due tomorrow',
|
||||
data: {
|
||||
taskUid: task.uid,
|
||||
taskName: task.name,
|
||||
dueDate: task.due_date,
|
||||
isOverdue: false,
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
await firstNotification.dismiss();
|
||||
|
||||
// Run the check again
|
||||
const result = await checkDueTasks();
|
||||
|
||||
// Should not create a new notification
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_due_soon',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
expect(notifications[0].id).toBe(firstNotification.id);
|
||||
expect(result.notificationsCreated).toBe(0);
|
||||
});
|
||||
|
||||
it('should handle transition from task_due_soon to task_overdue', async () => {
|
||||
const yesterday = new Date();
|
||||
yesterday.setDate(yesterday.getDate() - 1);
|
||||
|
||||
const task = await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: yesterday,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
// Create task_due_soon notification (from when it was due tomorrow)
|
||||
await Notification.createNotification({
|
||||
userId: user.id,
|
||||
type: 'task_due_soon',
|
||||
title: 'Task due soon',
|
||||
message: 'Your task "Test Task" is due tomorrow',
|
||||
data: {
|
||||
taskUid: task.uid,
|
||||
taskName: task.name,
|
||||
dueDate: task.due_date,
|
||||
isOverdue: false,
|
||||
},
|
||||
});
|
||||
|
||||
// Run the check (task is now overdue)
|
||||
await checkDueTasks();
|
||||
|
||||
// Should create task_overdue notification
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
},
|
||||
order: [['created_at', 'DESC']],
|
||||
});
|
||||
|
||||
// Both notifications should exist (different types)
|
||||
expect(notifications.length).toBe(2);
|
||||
expect(notifications[0].type).toBe('task_overdue');
|
||||
expect(notifications[1].type).toBe('task_due_soon');
|
||||
});
|
||||
|
||||
it('should only keep one notification for the same overdue task over multiple days', async () => {
|
||||
const threeDaysAgo = new Date();
|
||||
threeDaysAgo.setDate(threeDaysAgo.getDate() - 3);
|
||||
|
||||
const task = await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: threeDaysAgo,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
// Simulate 3 days of cron jobs
|
||||
for (let i = 0; i < 3; i++) {
|
||||
await checkDueTasks();
|
||||
}
|
||||
|
||||
// Should only have 1 notification
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_overdue',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
expect(notifications[0].data.taskUid).toBe(task.uid);
|
||||
});
|
||||
});
|
||||
|
||||
describe('basic functionality', () => {
|
||||
it('should create notification for task due tomorrow', async () => {
|
||||
const tomorrow = new Date();
|
||||
tomorrow.setDate(tomorrow.getDate() + 1);
|
||||
|
||||
await Task.create({
|
||||
name: 'Test Task',
|
||||
user_id: user.id,
|
||||
due_date: tomorrow,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
const result = await checkDueTasks();
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.notificationsCreated).toBe(1);
|
||||
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_due_soon',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
});
|
||||
|
||||
it('should create notification for overdue task', async () => {
|
||||
const yesterday = new Date();
|
||||
yesterday.setDate(yesterday.getDate() - 1);
|
||||
|
||||
await Task.create({
|
||||
name: 'Overdue Task',
|
||||
user_id: user.id,
|
||||
due_date: yesterday,
|
||||
status: Task.STATUS.NOT_STARTED,
|
||||
});
|
||||
|
||||
const result = await checkDueTasks();
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.notificationsCreated).toBe(1);
|
||||
|
||||
const notifications = await Notification.findAll({
|
||||
where: {
|
||||
user_id: user.id,
|
||||
type: 'task_overdue',
|
||||
},
|
||||
});
|
||||
|
||||
expect(notifications.length).toBe(1);
|
||||
});
|
||||
|
||||
it('should not create notification for completed tasks', async () => {
|
||||
const tomorrow = new Date();
|
||||
tomorrow.setDate(tomorrow.getDate() + 1);
|
||||
|
||||
await Task.create({
|
||||
name: 'Completed Task',
|
||||
user_id: user.id,
|
||||
due_date: tomorrow,
|
||||
status: Task.STATUS.DONE,
|
||||
});
|
||||
|
||||
const result = await checkDueTasks();
|
||||
|
||||
expect(result.notificationsCreated).toBe(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -61,6 +61,15 @@ This document contains preferences, patterns, and memory items specific to worki
|
|||
|
||||
---
|
||||
|
||||
## GitHub Issue Preferences
|
||||
|
||||
### Bug Reports
|
||||
- **Always follow** the GitHub bug template (`.github/ISSUE_TEMPLATE/bug_report.yml`)
|
||||
- Include all required fields: Description, Steps to Reproduce, Expected Behavior, Actual Behavior, OS, Browser
|
||||
- Add technical context in Additional Context section (root cause, solution, files affected)
|
||||
|
||||
---
|
||||
|
||||
## Common Issues & Solutions
|
||||
|
||||
### Date Formatting
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue