From 29e63e5ce58b1850b422bba22f69664091104096 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Mon, 1 Jun 2026 19:31:44 +0200 Subject: [PATCH] fix(notifications): restore /clear-all route the frontend already calls (#597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AdminHeader "Clear All" notifications button has been 404'ing for a while: frontend `notifications.service.ts` calls `DELETE /admin/notifications/clear-all`, backend only defined `DELETE /admin/notifications/clear-old`. The /clear-old route was misleadingly named anyway — it tried to delete read OR >30-days-old rows, then had a fallback that nuked EVERY row when nothing matched. Both the frontend and the existing test expect a simple Clear All shape, so just rename to /clear-all, drop the tiered logic, and return the plain `{ message, deletedCount }` payload the test asserts on. The test (adminNotifications.test.js) was hiding the breakage — it was on CI's --testPathIgnorePatterns ignore list and so never ran. Two reasons it failed locally before this fix: 1. Route path mismatch (the actual #597 bug). 2. The mock only stubbed adminAuth — requirePermission lives in its own middleware module and ran for real, 403'ing before the handler. Add a passthrough mock for that too. With both fixed, the test passes. Drop adminNotifications from the CI ignore list so future regressions in this route fail loudly instead of going to ground. --- .github/workflows/tests.yml | 4 +- .../__tests__/adminNotifications.test.js | 7 ++ backend/src/routes/adminNotifications.js | 68 ++++--------------- 3 files changed, 23 insertions(+), 56 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 368c9d91..b4342ae4 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -60,9 +60,9 @@ jobs: # integration/webhookDelivery — supertest fixture # services/backupService.enhanced — knex mock chain # routes/__tests__/adminAuth — supertest fixture - # routes/__tests__/adminNotifications — supertest fixture + # (adminNotifications was excluded; #597 fix re-enables it.) npx jest \ - --testPathIgnorePatterns='/node_modules/|adminSettings\.logo\.test|integration/adminPhotos\.reference|integration/webhookDelivery|backupService\.enhanced|routes/__tests__/adminAuth|routes/__tests__/adminNotifications' \ + --testPathIgnorePatterns='/node_modules/|adminSettings\.logo\.test|integration/adminPhotos\.reference|integration/webhookDelivery|backupService\.enhanced|routes/__tests__/adminAuth' \ --ci frontend: diff --git a/backend/src/routes/__tests__/adminNotifications.test.js b/backend/src/routes/__tests__/adminNotifications.test.js index d1416545..0cbe62f3 100644 --- a/backend/src/routes/__tests__/adminNotifications.test.js +++ b/backend/src/routes/__tests__/adminNotifications.test.js @@ -28,6 +28,13 @@ jest.mock('../../middleware/auth', () => ({ adminAuth: (_req, _res, next) => next(), })); +// requirePermission is its own module — without this mock the real +// implementation runs, queries role_permissions on the mocked db, and +// 403s before we ever reach the handler. +jest.mock('../../middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), +})); + const { db } = require('../../database/db'); const notificationsRouter = require('../adminNotifications'); diff --git a/backend/src/routes/adminNotifications.js b/backend/src/routes/adminNotifications.js index bd87a780..b7d97f97 100644 --- a/backend/src/routes/adminNotifications.js +++ b/backend/src/routes/adminNotifications.js @@ -98,62 +98,22 @@ router.put('/read-all', adminAuth, requirePermission('settings.edit'), async (re } }); -// Delete old notifications (older than 30 days and read) -router.delete('/clear-old', adminAuth, requirePermission('settings.edit'), async (req, res) => { +// Clear all notifications (#597). +// +// The frontend AdminHeader "Clear All" button hits this — its service +// at `notifications.service.ts` does DELETE /admin/notifications/clear-all. +// The previous /clear-old route was named for an "older than 30 days +// and read" semantic but had a fallback that deleted EVERYTHING when +// nothing matched the date filter, so it was effectively a confusingly +// named Clear All anyway. Drop the rename and the branching, return +// the simple deletedCount the existing test (and frontend toast) expect. +router.delete('/clear-all', adminAuth, requirePermission('settings.edit'), async (req, res) => { try { - // Use database-agnostic date calculation - const thirtyDaysAgo = new Date(); - thirtyDaysAgo.setDate(thirtyDaysAgo.getDate() - 30); - - let deletedCount = 0; - const client = db?.client?.config?.client; - - if (client === 'pg') { - const primaryResult = await db.raw( - ` - WITH deleted AS ( - DELETE FROM activity_logs - WHERE read_at IS NOT NULL OR created_at < ? - RETURNING id - ) - SELECT COUNT(*)::int AS count FROM deleted - `, - [thirtyDaysAgo.toISOString()] - ); - deletedCount = primaryResult.rows?.[0]?.count || 0; - - if (deletedCount === 0) { - const fallbackResult = await db.raw( - ` - WITH deleted AS ( - DELETE FROM activity_logs - RETURNING id - ) - SELECT COUNT(*)::int AS count FROM deleted - ` - ); - deletedCount = fallbackResult.rows?.[0]?.count || 0; - } - } else { - deletedCount = await db('activity_logs') - .where(function () { - this.whereNotNull('read_at') - .orWhere('created_at', '<', thirtyDaysAgo); - }) - .delete(); - - if (deletedCount === 0) { - deletedCount = await db('activity_logs').delete(); - } - } - - res.json({ - message: deletedCount > 0 ? 'Old notifications cleared' : 'No notifications to clear', - deletedCount - }); + const deletedCount = await db('activity_logs').delete(); + res.json({ message: 'All notifications cleared', deletedCount }); } catch (error) { - console.error('Clear old notifications error:', error); - res.status(500).json({ error: 'Failed to clear old notifications' }); + console.error('Clear notifications error:', error); + res.status(500).json({ error: 'Failed to clear notifications' }); } });