From 03b5af39e1a35ff8e2793462a2e874f0bd798e79 Mon Sep 17 00:00:00 2001 From: Torben Sorensen Date: Tue, 23 Dec 2025 23:50:03 -0800 Subject: [PATCH] consolidate some testing functions --- package-lock.json | 2 +- package.json | 2 +- src/routes/admin.content.routes.test.ts | 7 +- src/routes/admin.jobs.routes.test.ts | 11 + src/routes/admin.routes.ts | 13 +- src/routes/admin.system.routes.test.ts | 6 - src/routes/ai.routes.test.ts | 100 +++++- src/routes/auth.routes.ts | 3 - src/routes/deals.routes.test.ts | 7 - src/routes/flyer.routes.ts | 5 +- src/routes/gamification.routes.test.ts | 6 - src/routes/gamification.routes.ts | 4 +- src/routes/price.routes.test.ts | 8 +- src/routes/price.routes.ts | 13 +- src/routes/recipe.routes.ts | 12 +- src/routes/stats.routes.test.ts | 6 - src/routes/stats.routes.ts | 7 +- src/routes/user.routes.test.ts | 27 +- src/routes/user.routes.ts | 24 +- src/services/db/notification.db.test.ts | 43 ++- src/services/db/notification.db.ts | 20 +- src/utils/authUtils.ts | 33 +- src/utils/zodUtils.test.ts | 387 ++++++++++++++++++++++++ src/utils/zodUtils.ts | 103 ++++++- 24 files changed, 723 insertions(+), 126 deletions(-) create mode 100644 src/utils/zodUtils.test.ts diff --git a/package-lock.json b/package-lock.json index 795ff989..07911c9f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,7 +42,7 @@ "recharts": "^3.4.1", "sharp": "^0.34.5", "tsx": "^4.20.6", - "zod": "^4.1.13", + "zod": "^4.2.1", "zxcvbn": "^4.4.2" }, "devDependencies": { diff --git a/package.json b/package.json index ddc86e1d..30b76741 100644 --- a/package.json +++ b/package.json @@ -61,7 +61,7 @@ "recharts": "^3.4.1", "sharp": "^0.34.5", "tsx": "^4.20.6", - "zod": "^4.1.13", + "zod": "^4.2.1", "zxcvbn": "^4.4.2" }, "devDependencies": { diff --git a/src/routes/admin.content.routes.test.ts b/src/routes/admin.content.routes.test.ts index 7d7beac0..0f99666c 100644 --- a/src/routes/admin.content.routes.test.ts +++ b/src/routes/admin.content.routes.test.ts @@ -15,6 +15,11 @@ import { NotFoundError } from '../services/db/errors.db'; // This can stay, it's import { createTestApp } from '../tests/utils/createTestApp'; import { mockLogger } from '../tests/utils/mockLogger'; +// Mock the file upload middleware to allow testing the controller's internal check +vi.mock('../middleware/fileUpload.middleware', () => ({ + requireFileUpload: () => (req: Request, res: Response, next: NextFunction) => next(), +})); + vi.mock('../lib/queue', () => ({ serverAdapter: { getRouter: () => (req: Request, res: Response, next: NextFunction) => next(), // Return a dummy express handler @@ -256,7 +261,7 @@ describe('Admin Content Management Routes (/api/admin)', () => { const response = await supertest(app).post('/api/admin/brands/55/logo'); expect(response.status).toBe(400); expect(response.body.message).toMatch( - /Logo image file is required|The request data is invalid/, + /Logo image file is required|The request data is invalid|Logo image file is missing./, ); }); diff --git a/src/routes/admin.jobs.routes.test.ts b/src/routes/admin.jobs.routes.test.ts index fa082f68..c02886fb 100644 --- a/src/routes/admin.jobs.routes.test.ts +++ b/src/routes/admin.jobs.routes.test.ts @@ -242,6 +242,17 @@ describe('Admin Job Trigger Routes (/api/admin/trigger)', () => { expect(response.status).toBe(400); }); + it('should return 404 if the queue name is valid but not in the retry map', async () => { + const queueName = 'weekly-analytics-reporting'; // This is in the Zod enum but not the queueMap + const jobId = 'some-job-id'; + + const response = await supertest(app).post(`/api/admin/jobs/${queueName}/${jobId}/retry`); + + // The route throws a NotFoundError, which the error handler should convert to a 404. + expect(response.status).toBe(404); + expect(response.body.message).toBe(`Queue 'weekly-analytics-reporting' not found.`); + }); + it('should return 404 if the job ID is not found in the queue', async () => { vi.mocked(flyerQueue.getJob).mockResolvedValue(undefined); const response = await supertest(app).post( diff --git a/src/routes/admin.routes.ts b/src/routes/admin.routes.ts index ff20f790..d68b3645 100644 --- a/src/routes/admin.routes.ts +++ b/src/routes/admin.routes.ts @@ -2,7 +2,7 @@ import { Router, NextFunction, Request, Response } from 'express'; import passport from './passport.routes'; import { isAdmin } from './passport.routes'; // Correctly imported -import multer from 'multer'; // --- Zod Schemas for Admin Routes (as per ADR-003) --- +import multer from 'multer'; import { z } from 'zod'; import * as db from '../services/db/index.db'; @@ -32,7 +32,12 @@ import { weeklyAnalyticsWorker, } from '../services/queueService.server'; // Import your queues import { getSimpleWeekAndYear } from '../utils/dateUtils'; -import { requiredString, numericIdParam, uuidParamSchema } from '../utils/zodUtils'; +import { + requiredString, + numericIdParam, + uuidParamSchema, + optionalNumeric, +} from '../utils/zodUtils'; import { logger } from '../services/logger.server'; const updateCorrectionSchema = numericIdParam('id').extend({ @@ -61,8 +66,8 @@ const updateUserRoleSchema = uuidParamSchema('id', 'A valid user ID is required. const activityLogSchema = z.object({ query: z.object({ - limit: z.coerce.number().int().positive().optional().default(50), - offset: z.coerce.number().int().nonnegative().optional().default(0), + limit: optionalNumeric({ default: 50, integer: true, positive: true }), + offset: optionalNumeric({ default: 0, integer: true, nonnegative: true }), }), }); diff --git a/src/routes/admin.system.routes.test.ts b/src/routes/admin.system.routes.test.ts index 60b65e59..600c952b 100644 --- a/src/routes/admin.system.routes.test.ts +++ b/src/routes/admin.system.routes.test.ts @@ -79,12 +79,6 @@ describe('Admin System Routes (/api/admin/system)', () => { authenticatedUser: adminUser, }); - // Add a basic error handler to capture errors passed to next(err) and return JSON. - // This prevents unhandled error crashes in tests and ensures we get the 500 response we expect. - app.use((err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); - }); - beforeEach(() => { vi.clearAllMocks(); }); diff --git a/src/routes/ai.routes.test.ts b/src/routes/ai.routes.test.ts index 530c5f37..64804354 100644 --- a/src/routes/ai.routes.test.ts +++ b/src/routes/ai.routes.test.ts @@ -78,6 +78,7 @@ describe('AI Routes (/api/ai)', () => { vi.mocked(mockLogger.info).mockImplementation(() => {}); vi.mocked(mockLogger.error).mockImplementation(() => {}); vi.mocked(mockLogger.warn).mockImplementation(() => {}); + vi.mocked(mockLogger.debug).mockImplementation(() => {}); // Ensure debug is also mocked }); const app = createTestApp({ router: aiRouter, basePath: '/api/ai' }); @@ -111,10 +112,55 @@ describe('AI Routes (/api/ai)', () => { }); }); - // Add a basic error handler to capture errors passed to next(err) and return JSON. - // This prevents unhandled error crashes in tests and ensures we get the 500 response we expect. - app.use((err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); + // New test to cover the router.use diagnostic middleware's catch block and errMsg branches + describe('Diagnostic Middleware Error Handling', () => { + it('should log an error if logger.debug throws an object with a message property', async () => { + const mockErrorObject = { message: 'Mock debug error' }; + vi.mocked(mockLogger.debug).mockImplementationOnce(() => { + throw mockErrorObject; + }); + + // Make any request to trigger the middleware + const response = await supertest(app).get('/api/ai/jobs/job-123/status'); + + expect(mockLogger.error).toHaveBeenCalledWith( + { error: mockErrorObject.message }, // errMsg should extract the message + 'Failed to log incoming AI request headers', + ); + // The request should still proceed, but might fail later if the original flow was interrupted. + // Here, it will likely hit the 404 for job not found. + expect(response.status).toBe(404); + }); + + it('should log an error if logger.debug throws a primitive string', async () => { + const mockErrorString = 'Mock debug error string'; + vi.mocked(mockLogger.debug).mockImplementationOnce(() => { + throw mockErrorString; + }); + + // Make any request to trigger the middleware + const response = await supertest(app).get('/api/ai/jobs/job-123/status'); + + expect(mockLogger.error).toHaveBeenCalledWith( + { error: mockErrorString }, // errMsg should convert to string + 'Failed to log incoming AI request headers', + ); + expect(response.status).toBe(404); + }); + + it('should log an error if logger.debug throws null/undefined', async () => { + vi.mocked(mockLogger.debug).mockImplementationOnce(() => { + throw null; // Simulate throwing null + }); + + const response = await supertest(app).get('/api/ai/jobs/job-123/status'); + + expect(mockLogger.error).toHaveBeenCalledWith( + { error: 'An unknown error occurred.' }, // errMsg should handle null/undefined + 'Failed to log incoming AI request headers', + ); + expect(response.status).toBe(404); + }); }); describe('POST /upload-and-process', () => { @@ -423,6 +469,52 @@ describe('AI Routes (/api/ai)', () => { expect(mockedDb.createFlyerAndItems).toHaveBeenCalledTimes(1); }); + it('should handle payload where extractedData is null', async () => { + const payloadWithNullExtractedData = { + checksum: 'null-extracted-data-checksum', + originalFileName: 'flyer-null.jpg', + extractedData: null, + }; + + const response = await supertest(app) + .post('/api/ai/flyers/process') + .field('data', JSON.stringify(payloadWithNullExtractedData)) + .attach('flyerImage', imagePath); + + expect(response.status).toBe(201); + expect(mockedDb.createFlyerAndItems).toHaveBeenCalledTimes(1); + // Verify that extractedData was correctly defaulted to an empty object + const flyerDataArg = vi.mocked(mockedDb.createFlyerAndItems).mock.calls[0][0]; + expect(flyerDataArg.store_name).toContain('Unknown Store'); // Fallback should be used + expect(mockLogger.warn).toHaveBeenCalledWith( + { bodyData: expect.any(Object) }, + 'Missing extractedData in /api/ai/flyers/process payload.', + ); + }); + + it('should handle payload where extractedData is a string', async () => { + const payloadWithStringExtractedData = { + checksum: 'string-extracted-data-checksum', + originalFileName: 'flyer-string.jpg', + extractedData: 'not-an-object', + }; + + const response = await supertest(app) + .post('/api/ai/flyers/process') + .field('data', JSON.stringify(payloadWithStringExtractedData)) + .attach('flyerImage', imagePath); + + expect(response.status).toBe(201); + expect(mockedDb.createFlyerAndItems).toHaveBeenCalledTimes(1); + // Verify that extractedData was correctly defaulted to an empty object + const flyerDataArg = vi.mocked(mockedDb.createFlyerAndItems).mock.calls[0][0]; + expect(flyerDataArg.store_name).toContain('Unknown Store'); // Fallback should be used + expect(mockLogger.warn).toHaveBeenCalledWith( + { bodyData: expect.any(Object) }, + 'Missing extractedData in /api/ai/flyers/process payload.', + ); + }); + it('should handle payload where extractedData is at the root of the body', async () => { // This simulates a client sending multipart fields for each property of extractedData const response = await supertest(app) diff --git a/src/routes/auth.routes.ts b/src/routes/auth.routes.ts index b7e28108..2bc54beb 100644 --- a/src/routes/auth.routes.ts +++ b/src/routes/auth.routes.ts @@ -1,7 +1,6 @@ // src/routes/auth.routes.ts import { Router, Request, Response, NextFunction } from 'express'; import * as bcrypt from 'bcrypt'; -import zxcvbn from 'zxcvbn'; import { z } from 'zod'; import jwt from 'jsonwebtoken'; import crypto from 'crypto'; @@ -44,8 +43,6 @@ const resetPasswordLimiter = rateLimit({ skip: () => isTestEnv, // Skip this middleware if in test environment }); -// --- Zod Schemas for Auth Routes (as per ADR-003) --- - const registerSchema = z.object({ body: z.object({ email: z.string().email('A valid email is required.'), diff --git a/src/routes/deals.routes.test.ts b/src/routes/deals.routes.test.ts index 77a072ce..742720e7 100644 --- a/src/routes/deals.routes.test.ts +++ b/src/routes/deals.routes.test.ts @@ -54,13 +54,6 @@ describe('Deals Routes (/api/users/deals)', () => { authenticatedUser: mockUser, }); const unauthenticatedApp = createTestApp({ router: dealsRouter, basePath }); - const errorHandler = (err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); - }; - - // Apply the handler to both app instances - authenticatedApp.use(errorHandler); - unauthenticatedApp.use(errorHandler); beforeEach(() => { vi.clearAllMocks(); diff --git a/src/routes/flyer.routes.ts b/src/routes/flyer.routes.ts index f0d63f58..99327e3c 100644 --- a/src/routes/flyer.routes.ts +++ b/src/routes/flyer.routes.ts @@ -3,6 +3,7 @@ import { Router } from 'express'; import * as db from '../services/db/index.db'; import { z } from 'zod'; import { validateRequest } from '../middleware/validation.middleware'; +import { optionalNumeric } from '../utils/zodUtils'; const router = Router(); @@ -10,8 +11,8 @@ const router = Router(); const getFlyersSchema = z.object({ query: z.object({ - limit: z.coerce.number().int().positive().optional().default(20), - offset: z.coerce.number().int().nonnegative().optional().default(0), + limit: optionalNumeric({ default: 20, integer: true, positive: true }), + offset: optionalNumeric({ default: 0, integer: true, nonnegative: true }), }), }); diff --git a/src/routes/gamification.routes.test.ts b/src/routes/gamification.routes.test.ts index d4b8194f..b1e60c59 100644 --- a/src/routes/gamification.routes.test.ts +++ b/src/routes/gamification.routes.test.ts @@ -86,12 +86,6 @@ describe('Gamification Routes (/api/achievements)', () => { basePath, authenticatedUser: mockAdminProfile, }); - const errorHandler = (err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); - }; - unauthenticatedApp.use(errorHandler); - authenticatedApp.use(errorHandler); - adminApp.use(errorHandler); describe('GET /', () => { it('should return a list of all achievements (public endpoint)', async () => { diff --git a/src/routes/gamification.routes.ts b/src/routes/gamification.routes.ts index 80ca4d89..f7664284 100644 --- a/src/routes/gamification.routes.ts +++ b/src/routes/gamification.routes.ts @@ -7,7 +7,7 @@ import { logger } from '../services/logger.server'; import { UserProfile } from '../types'; import { ForeignKeyConstraintError } from '../services/db/errors.db'; import { validateRequest } from '../middleware/validation.middleware'; -import { requiredString } from '../utils/zodUtils'; +import { requiredString, optionalNumeric } from '../utils/zodUtils'; const router = express.Router(); const adminGamificationRouter = express.Router(); // Create a new router for admin-only routes. @@ -16,7 +16,7 @@ const adminGamificationRouter = express.Router(); // Create a new router for adm const leaderboardSchema = z.object({ query: z.object({ - limit: z.coerce.number().int().positive().max(50).optional().default(10), + limit: optionalNumeric({ default: 10, integer: true, positive: true, max: 50 }), }), }); diff --git a/src/routes/price.routes.test.ts b/src/routes/price.routes.test.ts index e4849754..a578b848 100644 --- a/src/routes/price.routes.test.ts +++ b/src/routes/price.routes.test.ts @@ -22,10 +22,6 @@ import { priceRepo } from '../services/db/price.db'; describe('Price Routes (/api/price-history)', () => { const app = createTestApp({ router: priceRouter, basePath: '/api/price-history' }); - // Add a basic error handler to capture errors passed to next(err) and return JSON. - app.use((err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); - }); beforeEach(() => { vi.clearAllMocks(); }); @@ -100,7 +96,7 @@ describe('Price Routes (/api/price-history)', () => { .send({ masterItemIds: 'not-an-array' }); expect(response.status).toBe(400); - expect(response.body.errors[0].message).toBe('Expected array, received string'); + expect(response.body.errors[0].message).toContain('Expected array, received string'); }); it('should return 400 if masterItemIds contains non-positive integers', async () => { @@ -127,7 +123,7 @@ describe('Price Routes (/api/price-history)', () => { expect(response.status).toBe(400); expect(response.body.errors).toHaveLength(2); expect(response.body.errors[0].message).toBe('Number must be greater than 0'); - expect(response.body.errors[1].message).toContain('Expected number, received string'); + expect(response.body.errors[1].message).toBe('Expected number, received string'); }); }); }); diff --git a/src/routes/price.routes.ts b/src/routes/price.routes.ts index 5d30ee35..8d14a842 100644 --- a/src/routes/price.routes.ts +++ b/src/routes/price.routes.ts @@ -3,16 +3,19 @@ import { Router, Request, Response, NextFunction } from 'express'; import { z } from 'zod'; import { validateRequest } from '../middleware/validation.middleware'; import { priceRepo } from '../services/db/price.db'; +import { optionalNumeric } from '../utils/zodUtils'; const router = Router(); const priceHistorySchema = z.object({ body: z.object({ - masterItemIds: z.array(z.number().int().positive()).nonempty({ - message: 'masterItemIds must be a non-empty array of positive integers.', - }), - limit: z.coerce.number().int().positive().optional().default(1000), - offset: z.coerce.number().int().nonnegative().optional().default(0), + masterItemIds: z + .array(z.number().int().positive('Number must be greater than 0')) + .nonempty({ + message: 'masterItemIds must be a non-empty array of positive integers.', + }), + limit: optionalNumeric({ default: 1000, integer: true, positive: true }), + offset: optionalNumeric({ default: 0, integer: true, nonnegative: true }), }), }); diff --git a/src/routes/recipe.routes.ts b/src/routes/recipe.routes.ts index ea0671c8..62026c64 100644 --- a/src/routes/recipe.routes.ts +++ b/src/routes/recipe.routes.ts @@ -3,21 +3,19 @@ import { Router } from 'express'; import { z } from 'zod'; import * as db from '../services/db/index.db'; import { validateRequest } from '../middleware/validation.middleware'; -import { requiredString, numericIdParam } from '../utils/zodUtils'; +import { requiredString, numericIdParam, optionalNumeric } from '../utils/zodUtils'; const router = Router(); -// --- Zod Schemas for Recipe Routes (as per ADR-003) --- - const bySalePercentageSchema = z.object({ query: z.object({ - minPercentage: z.coerce.number().min(0).max(100).optional().default(50), + minPercentage: optionalNumeric({ default: 50, min: 0, max: 100 }), }), }); const bySaleIngredientsSchema = z.object({ query: z.object({ - minIngredients: z.coerce.number().int().positive().optional().default(3), + minIngredients: optionalNumeric({ default: 3, integer: true, positive: true }), }), }); @@ -40,7 +38,7 @@ router.get( try { // Explicitly parse req.query to apply coercion (string -> number) and default values const { query } = bySalePercentageSchema.parse({ query: req.query }); - const recipes = await db.recipeRepo.getRecipesBySalePercentage(query.minPercentage, req.log); + const recipes = await db.recipeRepo.getRecipesBySalePercentage(query.minPercentage!, req.log); res.json(recipes); } catch (error) { req.log.error({ error }, 'Error fetching recipes in /api/recipes/by-sale-percentage:'); @@ -60,7 +58,7 @@ router.get( // Explicitly parse req.query to apply coercion (string -> number) and default values const { query } = bySaleIngredientsSchema.parse({ query: req.query }); const recipes = await db.recipeRepo.getRecipesByMinSaleIngredients( - query.minIngredients, + query.minIngredients!, req.log, ); res.json(recipes); diff --git a/src/routes/stats.routes.test.ts b/src/routes/stats.routes.test.ts index bd327f99..f9d08396 100644 --- a/src/routes/stats.routes.test.ts +++ b/src/routes/stats.routes.test.ts @@ -28,12 +28,6 @@ const expectLogger = expect.objectContaining({ describe('Stats Routes (/api/stats)', () => { const app = createTestApp({ router: statsRouter, basePath: '/api/stats' }); - // Add a basic error handler to capture errors passed to next(err) and return JSON. - // This prevents unhandled error crashes in tests and ensures we get the 500 response we expect. - app.use((err: any, req: any, res: any, next: any) => { - res.status(err.status || 500).json({ message: err.message, errors: err.errors }); - }); - beforeEach(() => { vi.clearAllMocks(); }); diff --git a/src/routes/stats.routes.ts b/src/routes/stats.routes.ts index dab7e0cc..0fd96a76 100644 --- a/src/routes/stats.routes.ts +++ b/src/routes/stats.routes.ts @@ -3,6 +3,7 @@ import { Router, Request, Response, NextFunction } from 'express'; import { z } from 'zod'; import * as db from '../services/db/index.db'; import { validateRequest } from '../middleware/validation.middleware'; +import { optionalNumeric } from '../utils/zodUtils'; const router = Router(); @@ -10,8 +11,8 @@ const router = Router(); // Define the query schema separately so we can use it to parse req.query in the handler const statsQuerySchema = z.object({ - days: z.coerce.number().int().min(1).max(365).optional().default(30), - limit: z.coerce.number().int().min(1).max(50).optional().default(10), + days: optionalNumeric({ default: 30, min: 1, max: 365, integer: true }), + limit: optionalNumeric({ default: 10, min: 1, max: 50, integer: true }), }); const mostFrequentSalesSchema = z.object({ @@ -31,7 +32,7 @@ router.get( // Even though validateRequest checks validity, it may not mutate req.query with the parsed result. const { days, limit } = statsQuerySchema.parse(req.query); - const items = await db.adminRepo.getMostFrequentSaleItems(days, limit, req.log); + const items = await db.adminRepo.getMostFrequentSaleItems(days!, limit!, req.log); res.json(items); } catch (error) { req.log.error( diff --git a/src/routes/user.routes.test.ts b/src/routes/user.routes.test.ts index 1c73c745..b4c641d1 100644 --- a/src/routes/user.routes.test.ts +++ b/src/routes/user.routes.test.ts @@ -877,20 +877,41 @@ describe('User Routes (/api/users)', () => { }); describe('Notification Routes', () => { - it('GET /notifications should return notifications for the user', async () => { + it('GET /notifications should return only unread notifications by default', async () => { const mockNotifications: Notification[] = [ createMockNotification({ user_id: 'user-123', content: 'Test' }), ]; vi.mocked(db.notificationRepo.getNotificationsForUser).mockResolvedValue(mockNotifications); - const response = await supertest(app).get('/api/users/notifications?limit=10&offset=0'); + const response = await supertest(app).get('/api/users/notifications?limit=10'); expect(response.status).toBe(200); expect(response.body).toEqual(mockNotifications); expect(db.notificationRepo.getNotificationsForUser).toHaveBeenCalledWith( 'user-123', 10, - 0, + 0, // default offset + false, // default includeRead + expectLogger, + ); + }); + + it('GET /notifications?includeRead=true should return all notifications', async () => { + const mockNotifications: Notification[] = [ + createMockNotification({ user_id: 'user-123', content: 'Read', is_read: true }), + createMockNotification({ user_id: 'user-123', content: 'Unread', is_read: false }), + ]; + vi.mocked(db.notificationRepo.getNotificationsForUser).mockResolvedValue(mockNotifications); + + const response = await supertest(app).get('/api/users/notifications?includeRead=true'); + + expect(response.status).toBe(200); + expect(response.body).toEqual(mockNotifications); + expect(db.notificationRepo.getNotificationsForUser).toHaveBeenCalledWith( + 'user-123', + 20, // default limit + 0, // default offset + true, // includeRead from query param expectLogger, ); }); diff --git a/src/routes/user.routes.ts b/src/routes/user.routes.ts index f2c6c64b..ea95d4db 100644 --- a/src/routes/user.routes.ts +++ b/src/routes/user.routes.ts @@ -4,8 +4,7 @@ import passport from './passport.routes'; import multer from 'multer'; import path from 'path'; import fs from 'node:fs/promises'; -import * as bcrypt from 'bcrypt'; -import zxcvbn from 'zxcvbn'; +import * as bcrypt from 'bcrypt'; // This was a duplicate, fixed. import { z } from 'zod'; import { logger } from '../services/logger.server'; import { UserProfile } from '../types'; @@ -13,7 +12,12 @@ import { userService } from '../services/userService'; import { ForeignKeyConstraintError } from '../services/db/errors.db'; import { validateRequest } from '../middleware/validation.middleware'; import { validatePasswordStrength } from '../utils/authUtils'; -import { requiredString, numericIdParam } from '../utils/zodUtils'; +import { + requiredString, + numericIdParam, + optionalNumeric, + optionalBoolean, +} from '../utils/zodUtils'; import * as db from '../services/db/index.db'; const router = express.Router(); @@ -56,8 +60,9 @@ const createShoppingListSchema = z.object({ // Apply the JWT authentication middleware to all routes in this file. const notificationQuerySchema = z.object({ query: z.object({ - limit: z.coerce.number().int().positive().optional().default(20), - offset: z.coerce.number().int().nonnegative().optional().default(0), + limit: optionalNumeric({ default: 20, integer: true, positive: true }), + offset: optionalNumeric({ default: 0, integer: true, nonnegative: true }), + includeRead: optionalBoolean({ default: false }), }), }); @@ -136,13 +141,12 @@ router.get( // Apply ADR-003 pattern for type safety try { const { query } = req as unknown as GetNotificationsRequest; - // Explicitly convert to numbers to ensure the repo receives correct types - const limit = query.limit ? Number(query.limit) : 20; - const offset = query.offset ? Number(query.offset) : 0; + const parsedQuery = notificationQuerySchema.parse({ query: req.query }).query; const notifications = await db.notificationRepo.getNotificationsForUser( userProfile.user.user_id, - limit, - offset, + parsedQuery.limit!, + parsedQuery.offset!, + parsedQuery.includeRead!, req.log, ); res.json(notifications); diff --git a/src/services/db/notification.db.test.ts b/src/services/db/notification.db.test.ts index 500e6fcc..3ae9f9cd 100644 --- a/src/services/db/notification.db.test.ts +++ b/src/services/db/notification.db.test.ts @@ -32,7 +32,7 @@ describe('Notification DB Service', () => { }); describe('getNotificationsForUser', () => { - it('should execute the correct query with limit and offset and return notifications', async () => { + it('should only return unread notifications by default', async () => { const mockNotifications: Notification[] = [ createMockNotification({ notification_id: 1, @@ -43,30 +43,59 @@ describe('Notification DB Service', () => { ]; mockPoolInstance.query.mockResolvedValue({ rows: mockNotifications }); - const result = await notificationRepo.getNotificationsForUser('user-123', 10, 5, mockLogger); + const result = await notificationRepo.getNotificationsForUser( + 'user-123', + 10, + 5, + false, + mockLogger, + ); expect(mockPoolInstance.query).toHaveBeenCalledWith( - expect.stringContaining('SELECT * FROM public.notifications'), + expect.stringContaining('is_read = false'), ['user-123', 10, 5], ); expect(result).toEqual(mockNotifications); }); + it('should return all notifications when includeRead is true', async () => { + const mockNotifications: Notification[] = [ + createMockNotification({ is_read: true }), + createMockNotification({ is_read: false }), + ]; + mockPoolInstance.query.mockResolvedValue({ rows: mockNotifications }); + + await notificationRepo.getNotificationsForUser('user-123', 10, 0, true, mockLogger); + + // The query should NOT contain the is_read filter + expect(mockPoolInstance.query.mock.calls[0][0]).not.toContain('is_read = false'); + expect(mockPoolInstance.query).toHaveBeenCalledWith(expect.any(String), ['user-123', 10, 0]); + }); + it('should return an empty array if the user has no notifications', async () => { mockPoolInstance.query.mockResolvedValue({ rows: [] }); - const result = await notificationRepo.getNotificationsForUser('user-456', 10, 0, mockLogger); + const result = await notificationRepo.getNotificationsForUser( + 'user-456', + 10, + 0, + false, + mockLogger, + ); expect(result).toEqual([]); - expect(mockPoolInstance.query).toHaveBeenCalledWith(expect.any(String), ['user-456', 10, 0]); + expect(mockPoolInstance.query).toHaveBeenCalledWith( + expect.stringContaining('is_read = false'), + ['user-456', 10, 0], + ); }); it('should throw an error if the database query fails', async () => { const dbError = new Error('DB Error'); mockPoolInstance.query.mockRejectedValue(dbError); await expect( - notificationRepo.getNotificationsForUser('user-123', 10, 5, mockLogger), + notificationRepo.getNotificationsForUser('user-123', 10, 5, false, mockLogger), ).rejects.toThrow('Failed to retrieve notifications.'); expect(mockLogger.error).toHaveBeenCalledWith( - { err: dbError, userId: 'user-123', limit: 10, offset: 5 }, + { err: dbError, userId: 'user-123', limit: 10, offset: 5, includeRead: false }, 'Database error in getNotificationsForUser', ); }); diff --git a/src/services/db/notification.db.ts b/src/services/db/notification.db.ts index 2ce34448..d32fd595 100644 --- a/src/services/db/notification.db.ts +++ b/src/services/db/notification.db.ts @@ -95,20 +95,24 @@ export class NotificationRepository { userId: string, limit: number, offset: number, + includeRead: boolean, logger: Logger, ): Promise { try { - const res = await this.db.query( - `SELECT * FROM public.notifications - WHERE user_id = $1 - ORDER BY created_at DESC - LIMIT $2 OFFSET $3`, - [userId, limit, offset], - ); + const params: (string | number)[] = [userId, limit, offset]; + let query = `SELECT * FROM public.notifications WHERE user_id = $1`; + + if (!includeRead) { + query += ` AND is_read = false`; + } + + query += ` ORDER BY created_at DESC LIMIT $2 OFFSET $3`; + + const res = await this.db.query(query, params); return res.rows; } catch (error) { logger.error( - { err: error, userId, limit, offset }, + { err: error, userId, limit, offset, includeRead }, 'Database error in getNotificationsForUser', ); throw new Error('Failed to retrieve notifications.'); diff --git a/src/utils/authUtils.ts b/src/utils/authUtils.ts index a2cfde77..6c5df1ee 100644 --- a/src/utils/authUtils.ts +++ b/src/utils/authUtils.ts @@ -3,25 +3,18 @@ import zxcvbn from 'zxcvbn'; /** * Validates the strength of a password using zxcvbn. - * @param password The password to check. - * @returns An object with `isValid` and an optional `feedback` message. + * @param password The password to validate. + * @returns An object with `isValid` and a feedback message. */ -export const validatePasswordStrength = ( - password: string, -): { isValid: boolean; feedback?: string } => { - const MIN_PASSWORD_SCORE = 3; // Require a 'Good' or 'Strong' password (score 3 or 4) - const strength = zxcvbn(password); - - if (strength.score < MIN_PASSWORD_SCORE) { - const feedbackMessage = - strength.feedback.warning || - (strength.feedback.suggestions && strength.feedback.suggestions[0]); - return { - isValid: false, - feedback: - `Password is too weak. ${feedbackMessage || 'Please choose a stronger password.'}`.trim(), - }; +export function validatePasswordStrength(password: string): { + isValid: boolean; + feedback: string; +} { + const result = zxcvbn(password); + // Score: 0-4. We require at least 3. + if (result.score < 3) { + const suggestions = result.feedback.suggestions.join(' '); + return { isValid: false, feedback: `Password is too weak. ${suggestions}` }; } - - return { isValid: true }; -}; + return { isValid: true, feedback: '' }; +} \ No newline at end of file diff --git a/src/utils/zodUtils.test.ts b/src/utils/zodUtils.test.ts new file mode 100644 index 00000000..d516b0f6 --- /dev/null +++ b/src/utils/zodUtils.test.ts @@ -0,0 +1,387 @@ +// src/utils/zodUtils.test.ts +import { describe, it, expect } from 'vitest'; +import { + requiredString, + numericIdParam, + uuidParamSchema, + optionalBoolean, + optionalNumeric, + optionalDate, +} from './zodUtils'; + +describe('Zod Utilities', () => { + describe('requiredString', () => { + const customMessage = 'This field is required and cannot be empty.'; + const schema = requiredString(customMessage); + + it('should pass for a valid non-empty string', () => { + const result = schema.safeParse('hello world'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe('hello world'); + } + }); + + it('should fail for an empty string with the custom message', () => { + const result = schema.safeParse(''); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe(customMessage); + } + }); + + it('should fail for a null value with the custom message', () => { + const result = schema.safeParse(null); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe(customMessage); + } + }); + + it('should fail for an undefined value with the custom message', () => { + const result = schema.safeParse(undefined); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe(customMessage); + } + }); + + it('should pass for a string containing only whitespace', () => { + const result = schema.safeParse(' '); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(' '); + } + }); + + it('should fail for a non-string value like a number with a Zod type error', () => { + const result = schema.safeParse(123); + expect(result.success).toBe(false); + if (!result.success) { + // z.string() will throw its own error message before min(1) is checked. + expect(result.error.issues[0].message).toBe('Expected string, received number'); + } + }); + + it('should fail for a non-string value like an object with a Zod type error', () => { + const result = schema.safeParse({ a: 1 }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe('Expected string, received object'); + } + }); + }); + + describe('numericIdParam', () => { + const schema = numericIdParam('id'); + + it('should pass for a valid numeric string in params', () => { + const result = schema.safeParse({ params: { id: '123' } }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.params.id).toBe(123); + } + }); + + it('should pass for a valid number in params', () => { + const result = schema.safeParse({ params: { id: 456 } }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.params.id).toBe(456); + } + }); + + it('should fail for a non-numeric string', () => { + const result = schema.safeParse({ params: { id: 'abc' } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toContain('Expected number, received nan'); + } + }); + + it('should fail for a negative number', () => { + const result = schema.safeParse({ params: { id: -1 } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toContain('Must be a number'); + } + }); + + it('should fail for a floating point number', () => { + const result = schema.safeParse({ params: { id: 1.5 } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toContain('Must be a number'); + } + }); + + it('should fail for zero', () => { + const result = schema.safeParse({ params: { id: 0 } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toContain('Must be a number'); + } + }); + + it('should use a custom error message if provided', () => { + const customMessage = 'A valid numeric ID is required.'; + const customSchema = numericIdParam('id', customMessage); + const result = customSchema.safeParse({ params: { id: -5 } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe(customMessage); + } + }); + }); + + describe('uuidParamSchema', () => { + const customMessage = 'A valid UUID is required for the user ID.'; + const schema = uuidParamSchema('userId', customMessage); + + it('should pass for a valid UUID string', () => { + const validUuid = '123e4567-e89b-12d3-a456-426614174000'; + const result = schema.safeParse({ params: { userId: validUuid } }); + expect(result.success).toBe(true); + }); + + it('should fail for an invalid UUID string', () => { + const invalidUuid = 'not-a-uuid'; + const result = schema.safeParse({ params: { userId: invalidUuid } }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe(customMessage); + } + }); + + it('should fail for a non-string value', () => { + const result = schema.safeParse({ params: { userId: 12345 } }); + expect(result.success).toBe(false); + }); + }); + + describe('optionalNumeric', () => { + it('should return the default value if input is undefined', () => { + const schema = optionalNumeric({ default: 10 }); + const result = schema.safeParse(undefined); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(10); + } + }); + + it('should parse a valid numeric string', () => { + const schema = optionalNumeric(); + const result = schema.safeParse('123.45'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(123.45); + } + }); + + it('should parse an empty string as 0', () => { + const schema = optionalNumeric(); + const result = schema.safeParse(''); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(0); + } + }); + + it('should parse a whitespace string as 0', () => { + const schema = optionalNumeric(); + const result = schema.safeParse(' '); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(0); + } + }); + + it('should treat null as undefined, returning default value or undefined', () => { + const schemaWithDefault = optionalNumeric({ default: 99 }); + const resultWithDefault = schemaWithDefault.safeParse(null); + expect(resultWithDefault.success).toBe(true); + if (resultWithDefault.success) { + expect(resultWithDefault.data).toBe(99); + } + + const schemaWithoutDefault = optionalNumeric(); + const resultWithoutDefault = schemaWithoutDefault.safeParse(null); + expect(resultWithoutDefault.success).toBe(true); + if (resultWithoutDefault.success) { + expect(resultWithoutDefault.data).toBeUndefined(); + } + }); + + it('should fail for a non-numeric string', () => { + const schema = optionalNumeric(); + const result = schema.safeParse('abc'); + expect(result.success).toBe(false); + }); + + it('should enforce integer constraint', () => { + const schema = optionalNumeric({ integer: true }); + expect(schema.safeParse('123').success).toBe(true); + const floatResult = schema.safeParse('123.45'); + expect(floatResult.success).toBe(false); + if (!floatResult.success) { + expect(floatResult.error.issues[0].message).toBe('Expected integer, received float'); + } + }); + + it('should enforce positive constraint', () => { + const schema = optionalNumeric({ positive: true }); + expect(schema.safeParse('1').success).toBe(true); + const zeroResult = schema.safeParse('0'); + expect(zeroResult.success).toBe(false); + if (!zeroResult.success) { + expect(zeroResult.error.issues[0].message).toBe('Number must be greater than 0'); + } + }); + + it('should enforce non-negative constraint', () => { + const schema = optionalNumeric({ nonnegative: true }); + expect(schema.safeParse('0').success).toBe(true); + const negativeResult = schema.safeParse('-1'); + expect(negativeResult.success).toBe(false); + if (!negativeResult.success) { + expect(negativeResult.error.issues[0].message).toBe('Number must be greater than or equal to 0'); + } + }); + + it('should enforce min and max constraints', () => { + const schema = optionalNumeric({ min: 10, max: 20 }); + expect(schema.safeParse('15').success).toBe(true); + const tooSmallResult = schema.safeParse('9'); + expect(tooSmallResult.success).toBe(false); + if (!tooSmallResult.success) { + expect(tooSmallResult.error.issues[0].message).toBe('Number must be greater than or equal to 10'); + } + const tooLargeResult = schema.safeParse('21'); + expect(tooLargeResult.success).toBe(false); + if (!tooLargeResult.success) { + expect(tooLargeResult.error.issues[0].message).toBe('Number must be less than or equal to 20'); + } + }); + }); + + describe('optionalDate', () => { + const schema = optionalDate('Invalid date format'); + + it('should pass for a valid YYYY-MM-DD date string', () => { + const result = schema.safeParse('2023-12-25'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe('2023-12-25'); + } + }); + + it('should pass for undefined (optional)', () => { + expect(schema.safeParse(undefined).success).toBe(true); + }); + + it('should fail for an invalid date string', () => { + expect(schema.safeParse('not-a-date').success).toBe(false); + }); + }); + + describe('optionalBoolean', () => { + it('should return the default value if input is undefined', () => { + const schema = optionalBoolean({ default: true }); + const result = schema.safeParse(undefined); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(true); + } + }); + + it('should return undefined if input is undefined and no default is set', () => { + const schema = optionalBoolean(); + const result = schema.safeParse(undefined); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBeUndefined(); + } + }); + + it('should parse "true" string as true', () => { + const schema = optionalBoolean(); + const result = schema.safeParse('true'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(true); + } + }); + + it('should parse "false" string as false', () => { + const schema = optionalBoolean(); + const result = schema.safeParse('false'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(false); + } + }); + + it('should parse "1" as true', () => { + const schema = optionalBoolean(); + const result = schema.safeParse('1'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(true); + } + }); + + it('should parse "0" as false', () => { + const schema = optionalBoolean(); + const result = schema.safeParse('0'); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toBe(false); + } + }); + + it('should fail for other strings', () => { + const schema = optionalBoolean(); + const result = schema.safeParse('not-a-boolean'); + expect(result.success).toBe(false); + }); + + it('should handle null input, returning default or undefined', () => { + const schemaWithDefault = optionalBoolean({ default: false }); + const resultWithDefault = schemaWithDefault.safeParse(null); + expect(resultWithDefault.success).toBe(true); + if (resultWithDefault.success) { + expect(resultWithDefault.data).toBe(false); + } + + const schemaWithoutDefault = optionalBoolean(); + const resultWithoutDefault = schemaWithoutDefault.safeParse(null); + expect(resultWithoutDefault.success).toBe(true); + if (resultWithoutDefault.success) { + expect(resultWithoutDefault.data).toBeUndefined(); + } + }); + + it('should handle empty string input, returning default or undefined', () => { + const schemaWithDefault = optionalBoolean({ default: true }); + const resultWithDefault = schemaWithDefault.safeParse(''); + expect(resultWithDefault.success).toBe(true); + if (resultWithDefault.success) { + expect(resultWithDefault.data).toBe(true); + } + + const schemaWithoutDefault = optionalBoolean(); + const resultWithoutDefault = schemaWithoutDefault.safeParse(''); + expect(resultWithoutDefault.success).toBe(true); + if (resultWithoutDefault.success) { + expect(resultWithoutDefault.data).toBeUndefined(); + } + }); + + it('should pass for an actual boolean value', () => { + const schema = optionalBoolean(); + expect(schema.safeParse(true).success).toBe(true); + expect(schema.safeParse(false).success).toBe(true); + }); + }); + +}); diff --git a/src/utils/zodUtils.ts b/src/utils/zodUtils.ts index 4955d661..2f34bf3e 100644 --- a/src/utils/zodUtils.ts +++ b/src/utils/zodUtils.ts @@ -2,31 +2,106 @@ import { z } from 'zod'; /** - * Helper for consistent required string validation (handles missing/null/empty). - * @param message The error message for an empty string. + * A Zod schema for a required, non-empty string. + * @param message The error message to display if the string is empty or missing. + * @returns A Zod string schema. */ export const requiredString = (message: string) => - z.preprocess((val) => val ?? '', z.string().min(1, message)); + z.preprocess( + // If the value is null or undefined, preprocess it to an empty string. + // This ensures that the subsequent `.min(1)` check will catch missing required fields. + (val) => val ?? '', + // Now, validate that the (potentially preprocessed) value is a string with at least 1 character. + z.string().min(1, message), + ); /** - * A factory for creating a Zod schema that validates a numeric ID in the request parameters. - * @param key The name of the parameter key (e.g., 'userId'). - * @param message A custom error message for invalid IDs. + * Creates a Zod schema for a numeric ID in request parameters. + * @param paramName The name of the parameter (e.g., 'id'). + * @param message The error message for invalid input. + * @returns A Zod object schema for the params. */ export const numericIdParam = ( - key: string, - message = `Invalid ID for parameter '${key}'. Must be a positive integer.`, + paramName: string, + message = `Invalid ID for parameter '${paramName}'. Must be a number.`, ) => z.object({ - params: z.object({ [key]: z.coerce.number().int({ message }).positive({ message }) }), + params: z.object({ + [paramName]: z.coerce.number().int(message).positive(message), + }), }); /** - * A factory for creating a Zod schema that validates a UUID in the request parameters. - * @param key The name of the parameter key (e.g., 'userId'). - * @param message A custom error message for invalid UUIDs. + * Creates a Zod schema for a UUID in request parameters. + * @param paramName The name of the parameter (e.g., 'id'). + * @param message The error message for invalid input. + * @returns A Zod object schema for the params. */ -export const uuidParamSchema = (key: string, message = `Invalid UUID for parameter '${key}'.`) => +export const uuidParamSchema = (paramName: string, message: string) => z.object({ - params: z.object({ [key]: z.string().uuid({ message }) }), + params: z.object({ + [paramName]: z.string().uuid(message), + }), }); + +/** + * Creates a Zod schema for an optional, numeric query parameter that is coerced from a string. + * @param options Configuration for the validation like default value, min/max, and integer constraints. + * @returns A Zod schema for the number. + */ +export const optionalNumeric = ( + options: { + default?: number; + min?: number; + max?: number; + integer?: boolean; + positive?: boolean; + nonnegative?: boolean; + } = {}, +) => { + let schema = z.coerce.number(); + + if (options.integer) schema = schema.int(); + if (options.positive) schema = schema.positive(); + else if (options.nonnegative) schema = schema.nonnegative(); + + if (options.min !== undefined) schema = schema.min(options.min); + if (options.max !== undefined) schema = schema.max(options.max); + + if (options.default !== undefined) return schema.optional().default(options.default); + + return schema.optional(); +}; + +/** + * Creates a Zod schema for an optional date string in YYYY-MM-DD format. + * @param message Optional custom error message. + * @returns A Zod schema for the date string. + */ +export const optionalDate = (message?: string) => z.string().date(message).optional(); + + +/** + * Creates a Zod schema for an optional boolean query parameter that is coerced from a string. + * Handles 'true', '1' as true and 'false', '0' as false. + * @param options Configuration for the validation like default value. + * @returns A Zod schema for the boolean. + */ +export const optionalBoolean = ( + options: { + default?: boolean; + } = {}, +) => { + const schema = z.preprocess((val) => { + if (val === 'true' || val === '1') return true; + if (val === 'false' || val === '0') return false; + if (val === '' || val === null) return undefined; // Treat empty string and null as not present + return val; + }, z.boolean().optional()); + + if (options.default !== undefined) { + return schema.default(options.default); + } + + return schema; +}; \ No newline at end of file