From 057c4c9174bd2cae651b80ff0ecb6db773c321f2 Mon Sep 17 00:00:00 2001 From: Torben Sorensen Date: Sun, 4 Jan 2026 11:28:47 -0800 Subject: [PATCH] more and more test fixes --- src/App.test.tsx | 17 ++- src/App.tsx | 12 +- src/components/AchievementsList.test.tsx | 11 +- src/components/FlyerCorrectionTool.test.tsx | 50 +++++++++ src/components/RecipeSuggester.test.tsx | 46 ++++++++ src/features/charts/PriceChart.test.tsx | 12 ++ src/features/charts/TopDeals.tsx | 8 +- src/pages/admin/FlyerReviewPage.test.tsx | 4 +- src/pages/admin/FlyerReviewPage.tsx | 2 +- src/services/aiService.server.test.ts | 20 +++- src/services/authService.test.ts | 62 ++++++----- src/services/authService.ts | 37 +++++-- src/services/db/errors.db.ts | 45 ++++++-- src/services/db/flyer.db.test.ts | 4 +- src/services/db/user.db.test.ts | 104 +++++------------- src/services/db/user.db.ts | 11 +- src/services/flyerAiProcessor.server.test.ts | 6 +- src/services/flyerAiProcessor.server.ts | 2 +- .../flyerProcessingService.server.test.ts | 36 +++++- src/services/gamificationService.ts | 41 ++++++- src/services/userService.test.ts | 7 +- src/services/userService.ts | 43 +++++--- src/utils/pdfConverter.test.ts | 63 +++++++++-- src/utils/pdfConverter.ts | 21 ++-- src/utils/priceParser.test.ts | 5 + src/utils/serverUtils.test.ts | 85 ++++++++++++++ 26 files changed, 544 insertions(+), 210 deletions(-) create mode 100644 src/utils/serverUtils.test.ts diff --git a/src/App.test.tsx b/src/App.test.tsx index b31fc90d..a10d94c2 100644 --- a/src/App.test.tsx +++ b/src/App.test.tsx @@ -71,10 +71,13 @@ vi.mock('./components/Header', async () => { return { Header: MockHeader }; }); -vi.mock('./pages/HomePage', async () => { - const { MockHomePage } = await import('./tests/utils/componentMocks'); - return { HomePage: MockHomePage }; -}); +vi.mock('./pages/HomePage', () => ({ + HomePage: (props: any) => ( +
+ Mock Home Page +
+ ), +})); vi.mock('./pages/admin/AdminPage', async () => { const { MockAdminPage } = await import('./tests/utils/componentMocks'); @@ -361,12 +364,8 @@ describe('App Component', () => { it('should select a flyer when flyerId is present in the URL', async () => { renderApp(['/flyers/2']); - // The HomePage mock will be rendered. The important part is that the selection logic - // in App.tsx runs and passes the correct `selectedFlyer` prop down. - // Since HomePage is mocked, we can't see the direct result, but we can - // infer that the logic ran without crashing and the correct route was matched. await waitFor(() => { - expect(screen.getByTestId('home-page-mock')).toBeInTheDocument(); + expect(screen.getByTestId('home-page-mock')).toHaveAttribute('data-selected-flyer-id', '2'); }); }); diff --git a/src/App.tsx b/src/App.tsx index 8141cdd3..2fdcd0ea 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1,6 +1,6 @@ // src/App.tsx import React, { useState, useCallback, useEffect } from 'react'; -import { Routes, Route, useParams } from 'react-router-dom'; +import { Routes, Route, useLocation, matchPath } from 'react-router-dom'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import * as pdfjsLib from 'pdfjs-dist'; import { Footer } from './components/Footer'; @@ -45,7 +45,9 @@ function App() { const { flyers } = useFlyers(); const [selectedFlyer, setSelectedFlyer] = useState(null); const { openModal, closeModal, isModalOpen } = useModal(); - const params = useParams<{ flyerId?: string }>(); + const location = useLocation(); + const match = matchPath('/flyers/:flyerId', location.pathname); + const flyerIdFromUrl = match?.params.flyerId; // This hook now handles initialization effects (OAuth, version check, theme) // and returns the theme/unit state needed by other components. @@ -57,7 +59,7 @@ function App() { console.log('[App] Render:', { flyersCount: flyers.length, selectedFlyerId: selectedFlyer?.flyer_id, - paramsFlyerId: params?.flyerId, // This was a duplicate, fixed. + flyerIdFromUrl, authStatus, profileId: userProfile?.user.user_id, }); @@ -139,8 +141,6 @@ function App() { // New effect to handle routing to a specific flyer ID from the URL useEffect(() => { - const flyerIdFromUrl = params.flyerId; - if (flyerIdFromUrl && flyers.length > 0) { const flyerId = parseInt(flyerIdFromUrl, 10); const flyerToSelect = flyers.find((f) => f.flyer_id === flyerId); @@ -148,7 +148,7 @@ function App() { handleFlyerSelect(flyerToSelect); } } - }, [flyers, handleFlyerSelect, selectedFlyer, params.flyerId]); + }, [flyers, handleFlyerSelect, selectedFlyer, flyerIdFromUrl]); // Read the application version injected at build time. // This will only be available in the production build, not during local development. diff --git a/src/components/AchievementsList.test.tsx b/src/components/AchievementsList.test.tsx index cf933454..e9f4956d 100644 --- a/src/components/AchievementsList.test.tsx +++ b/src/components/AchievementsList.test.tsx @@ -23,6 +23,7 @@ describe('AchievementsList', () => { points_value: 15, }), createMockUserAchievement({ achievement_id: 3, name: 'Unknown Achievement', icon: 'star' }), // This icon is not in the component's map + createMockUserAchievement({ achievement_id: 4, name: 'No Icon Achievement', icon: '' }), // Triggers the fallback for missing name ]; renderWithProviders(); @@ -41,7 +42,15 @@ describe('AchievementsList', () => { // Check achievement with default icon expect(screen.getByText('Unknown Achievement')).toBeInTheDocument(); - expect(screen.getByText('๐Ÿ†')).toBeInTheDocument(); // Default icon + // We expect at least one trophy (for unknown achievement). + // Since we added another one that produces a trophy (No Icon), we use getAllByText. + expect(screen.getAllByText('๐Ÿ†').length).toBeGreaterThan(0); + + // Check achievement with missing icon (empty string) + expect(screen.getByText('No Icon Achievement')).toBeInTheDocument(); + // Verify the specific placeholder class is rendered, ensuring the early return in Icon component is hit + const noIconCard = screen.getByText('No Icon Achievement').closest('.bg-white'); + expect(noIconCard?.querySelector('.icon-placeholder')).toBeInTheDocument(); }); it('should render a message when there are no achievements', () => { diff --git a/src/components/FlyerCorrectionTool.test.tsx b/src/components/FlyerCorrectionTool.test.tsx index 5ee38ec1..f9a87eda 100644 --- a/src/components/FlyerCorrectionTool.test.tsx +++ b/src/components/FlyerCorrectionTool.test.tsx @@ -252,4 +252,54 @@ describe('FlyerCorrectionTool', () => { expect(mockedNotifyError).toHaveBeenCalledWith('An unknown error occurred.'); }); }); + + it('should handle API failure response (ok: false) correctly', async () => { + console.log('TEST: Starting "should handle API failure response (ok: false) correctly"'); + mockedAiApiClient.rescanImageArea.mockResolvedValue({ + ok: false, + json: async () => ({ message: 'Custom API Error' }), + } as Response); + + renderWithProviders(); + + // Wait for image fetch + await waitFor(() => expect(global.fetch).toHaveBeenCalled()); + + // Draw selection + const canvas = screen.getByRole('dialog').querySelector('canvas')!; + fireEvent.mouseDown(canvas, { clientX: 10, clientY: 10 }); + fireEvent.mouseMove(canvas, { clientX: 50, clientY: 50 }); + fireEvent.mouseUp(canvas); + + // Click extract + fireEvent.click(screen.getByRole('button', { name: /extract store name/i })); + + await waitFor(() => { + expect(mockedNotifyError).toHaveBeenCalledWith('Custom API Error'); + }); + }); + + it('should redraw the canvas when the image loads', () => { + console.log('TEST: Starting "should redraw the canvas when the image loads"'); + const clearRectSpy = vi.fn(); + // Override the getContext mock for this test to capture the spy + window.HTMLCanvasElement.prototype.getContext = vi.fn(() => ({ + clearRect: clearRectSpy, + strokeRect: vi.fn(), + setLineDash: vi.fn(), + strokeStyle: '', + lineWidth: 0, + })) as any; + + renderWithProviders(); + const image = screen.getByAltText('Flyer for correction'); + + // The draw function is called on mount via useEffect, so we clear that call. + clearRectSpy.mockClear(); + + // Simulate image load event which triggers onLoad={draw} + fireEvent.load(image); + + expect(clearRectSpy).toHaveBeenCalled(); + }); }); diff --git a/src/components/RecipeSuggester.test.tsx b/src/components/RecipeSuggester.test.tsx index f44cf91a..6c2e6fdc 100644 --- a/src/components/RecipeSuggester.test.tsx +++ b/src/components/RecipeSuggester.test.tsx @@ -153,4 +153,50 @@ describe('RecipeSuggester Component', () => { }); console.log('TEST: Previous error cleared successfully'); }); + + it('uses default error message when API error response has no message', async () => { + console.log('TEST: Verifying default error message for API failure'); + const user = userEvent.setup(); + renderWithProviders(); + + const input = screen.getByLabelText(/Ingredients:/i); + await user.type(input, 'mystery'); + + // Mock API failure response without a message property + mockedApiClient.suggestRecipe.mockResolvedValue({ + ok: false, + json: async () => ({}), // Empty object + } as Response); + + const button = screen.getByRole('button', { name: /Suggest a Recipe/i }); + await user.click(button); + + await waitFor(() => { + expect(screen.getByText('Failed to get suggestion.')).toBeInTheDocument(); + }); + }); + + it('handles non-Error objects thrown during fetch', async () => { + console.log('TEST: Verifying handling of non-Error exceptions'); + const user = userEvent.setup(); + renderWithProviders(); + + const input = screen.getByLabelText(/Ingredients:/i); + await user.type(input, 'chaos'); + + // Mock a rejection that is NOT an Error object + mockedApiClient.suggestRecipe.mockRejectedValue('Something weird happened'); + + const button = screen.getByRole('button', { name: /Suggest a Recipe/i }); + await user.click(button); + + await waitFor(() => { + expect(screen.getByText('An unknown error occurred.')).toBeInTheDocument(); + }); + + expect(logger.error).toHaveBeenCalledWith( + { error: 'Something weird happened' }, + 'Failed to fetch recipe suggestion.' + ); + }); }); \ No newline at end of file diff --git a/src/features/charts/PriceChart.test.tsx b/src/features/charts/PriceChart.test.tsx index 235cc88f..29fbe6d5 100644 --- a/src/features/charts/PriceChart.test.tsx +++ b/src/features/charts/PriceChart.test.tsx @@ -77,6 +77,18 @@ describe('PriceChart', () => { expect(screen.getByText(/no deals for your watched items/i)).toBeInTheDocument(); }); + it('should render an error message when an error occurs', () => { + mockedUseActiveDeals.mockReturnValue({ + ...mockedUseActiveDeals(), + activeDeals: [], + isLoading: false, + error: 'Failed to fetch deals.', + }); + + render(); + expect(screen.getByText('Failed to fetch deals.')).toBeInTheDocument(); + }); + it('should render the table with deal items when data is provided', () => { render(); diff --git a/src/features/charts/TopDeals.tsx b/src/features/charts/TopDeals.tsx index d3685e5f..0919afbf 100644 --- a/src/features/charts/TopDeals.tsx +++ b/src/features/charts/TopDeals.tsx @@ -8,9 +8,13 @@ interface TopDealsProps { export const TopDeals: React.FC = ({ items }) => { const topDeals = useMemo(() => { + // Use a type guard in the filter to inform TypeScript that price_in_cents is non-null + // in subsequent operations. This allows removing the redundant nullish coalescing in sort. return [...items] - .filter((item) => item.price_in_cents !== null) // Only include items with a parseable price - .sort((a, b) => (a.price_in_cents ?? Infinity) - (b.price_in_cents ?? Infinity)) + .filter( + (item): item is FlyerItem & { price_in_cents: number } => item.price_in_cents !== null, + ) + .sort((a, b) => a.price_in_cents - b.price_in_cents) .slice(0, 10); }, [items]); diff --git a/src/pages/admin/FlyerReviewPage.test.tsx b/src/pages/admin/FlyerReviewPage.test.tsx index fd5ccc10..292a6e1b 100644 --- a/src/pages/admin/FlyerReviewPage.test.tsx +++ b/src/pages/admin/FlyerReviewPage.test.tsx @@ -73,7 +73,7 @@ describe('FlyerReviewPage', () => { file_name: 'flyer3.jpg', created_at: '2023-01-03T00:00:00Z', store: null, - icon_url: 'http://example.com/icon2.jpg', + icon_url: null, }, ]; @@ -103,7 +103,7 @@ describe('FlyerReviewPage', () => { const unknownStoreItem = screen.getByText('Unknown Store').closest('li'); const unknownStoreImage = within(unknownStoreItem!).getByRole('img'); expect(unknownStoreImage).not.toHaveAttribute('src'); - expect(unknownStoreImage).not.toHaveAttribute('alt'); + expect(unknownStoreImage).toHaveAttribute('alt', 'Unknown Store'); }); it('renders error message when API response is not ok', async () => { diff --git a/src/pages/admin/FlyerReviewPage.tsx b/src/pages/admin/FlyerReviewPage.tsx index 1c706a55..af3c7055 100644 --- a/src/pages/admin/FlyerReviewPage.tsx +++ b/src/pages/admin/FlyerReviewPage.tsx @@ -73,7 +73,7 @@ export const FlyerReviewPage: React.FC = () => { flyers.map((flyer) => (
  • - {flyer.store?.name} + {flyer.store?.name

    {flyer.store?.name || 'Unknown Store'}

    {flyer.file_name}

    diff --git a/src/services/aiService.server.test.ts b/src/services/aiService.server.test.ts index 5f40c869..96406e30 100644 --- a/src/services/aiService.server.test.ts +++ b/src/services/aiService.server.test.ts @@ -30,12 +30,13 @@ import { logger as mockLoggerInstance } from './logger.server'; // Explicitly unmock the service under test to ensure we import the real implementation. vi.unmock('./aiService.server'); -const { mockGenerateContent, mockToBuffer, mockExtract, mockSharp } = vi.hoisted(() => { +const { mockGenerateContent, mockToBuffer, mockExtract, mockSharp, mockAdminLogActivity } = vi.hoisted(() => { const mockGenerateContent = vi.fn(); const mockToBuffer = vi.fn(); const mockExtract = vi.fn(() => ({ toBuffer: mockToBuffer })); const mockSharp = vi.fn(() => ({ extract: mockExtract })); - return { mockGenerateContent, mockToBuffer, mockExtract, mockSharp }; + const mockAdminLogActivity = vi.fn(); + return { mockGenerateContent, mockToBuffer, mockExtract, mockSharp, mockAdminLogActivity }; }); // Mock sharp, as it's a direct dependency of the service. @@ -82,6 +83,12 @@ vi.mock('../utils/imageProcessor', () => ({ generateFlyerIcon: vi.fn(), })); +vi.mock('./db/admin.db', () => ({ + AdminRepository: vi.fn().mockImplementation(() => ({ + logActivity: mockAdminLogActivity, + })), +})); + // Import mocked modules to assert on them import * as dbModule from './db/index.db'; import { flyerQueue } from './queueService.server'; @@ -123,6 +130,7 @@ describe('AI Service (Server)', () => { vi.restoreAllMocks(); vi.clearAllMocks(); mockGenerateContent.mockReset(); + mockAdminLogActivity.mockClear(); // Reset modules to ensure the service re-initializes with the mocks mockAiClient.generateContent.mockResolvedValue({ @@ -341,8 +349,6 @@ describe('AI Service (Server)', () => { expect(logger.error).toHaveBeenCalledWith( { error: nonRetriableError }, // The first model in the list is now 'gemini-2.5-flash' `[AIService Adapter] Model 'gemini-2.5-flash' failed with a non-retriable error.`, - { error: nonRetriableError }, // The first model in the list - `[AIService Adapter] Model '${models[0]}' failed with a non-retriable error.`, ); }); @@ -1119,6 +1125,7 @@ describe('AI Service (Server)', () => { }), expect.arrayContaining([expect.objectContaining({ item: 'Milk' })]), mockLoggerInstance, + expect.anything(), ); }); @@ -1145,6 +1152,7 @@ describe('AI Service (Server)', () => { }), [], // No items mockLoggerInstance, + expect.anything(), ); }); @@ -1176,6 +1184,7 @@ describe('AI Service (Server)', () => { }), ]), mockLoggerInstance, + expect.anything(), ); expect(mockLoggerInstance.warn).toHaveBeenCalledWith( expect.stringContaining('extractedData.store_name missing'), @@ -1192,7 +1201,7 @@ describe('AI Service (Server)', () => { ); expect(result).toHaveProperty('flyer_id', 100); - expect(dbModule.adminRepo.logActivity).toHaveBeenCalledWith( + expect(mockAdminLogActivity).toHaveBeenCalledWith( expect.objectContaining({ action: 'flyer_processed', userId: mockProfile.user.user_id, @@ -1260,6 +1269,7 @@ describe('AI Service (Server)', () => { expect.objectContaining({ checksum: 'str-body' }), expect.anything(), mockLoggerInstance, + expect.anything(), ); }); }); diff --git a/src/services/authService.test.ts b/src/services/authService.test.ts index d0fe43f5..c66d2785 100644 --- a/src/services/authService.test.ts +++ b/src/services/authService.test.ts @@ -4,6 +4,27 @@ import type { UserProfile } from '../types'; import type * as jsonwebtoken from 'jsonwebtoken'; import { DatabaseError } from './processingErrors'; +const { transactionalUserRepoMocks, transactionalAdminRepoMocks } = vi.hoisted(() => { + return { + transactionalUserRepoMocks: { + updateUserPassword: vi.fn(), + deleteResetToken: vi.fn(), + createPasswordResetToken: vi.fn(), + createUser: vi.fn(), + }, + transactionalAdminRepoMocks: { + logActivity: vi.fn(), + }, + }; +}); + +vi.mock('./db/user.db', () => ({ + UserRepository: vi.fn().mockImplementation(() => transactionalUserRepoMocks), +})); +vi.mock('./db/admin.db', () => ({ + AdminRepository: vi.fn().mockImplementation(() => transactionalAdminRepoMocks), +})); + describe('AuthService', () => { let authService: typeof import('./authService').authService; let bcrypt: typeof import('bcrypt'); @@ -15,8 +36,6 @@ describe('AuthService', () => { let UniqueConstraintError: typeof import('./db/errors.db').UniqueConstraintError; let RepositoryError: typeof import('./db/errors.db').RepositoryError; let withTransaction: typeof import('./db/index.db').withTransaction; - let transactionalUserRepoMocks: any; - let transactionalAdminRepoMocks: any; const reqLog = {}; // Mock request logger object const mockUser = { @@ -42,19 +61,6 @@ describe('AuthService', () => { vi.stubEnv('JWT_SECRET', 'test-secret'); vi.stubEnv('FRONTEND_URL', 'http://localhost:3000'); - transactionalUserRepoMocks = { - updateUserPassword: vi.fn(), - deleteResetToken: vi.fn(), - createPasswordResetToken: vi.fn(), - createUser: vi.fn(), - }; - transactionalAdminRepoMocks = { - logActivity: vi.fn(), - }; - - const MockTransactionalUserRepository = vi.fn(() => transactionalUserRepoMocks); - const MockTransactionalAdminRepository = vi.fn(() => transactionalAdminRepoMocks); - // Mock all dependencies before dynamically importing the service // Core modules like bcrypt, jsonwebtoken, and crypto are now mocked globally in tests-setup-unit.ts vi.mock('bcrypt'); @@ -79,12 +85,6 @@ describe('AuthService', () => { vi.mock('./logger.server', () => ({ logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }, })); - vi.mock('./db/user.db', () => ({ - UserRepository: MockTransactionalUserRepository, - })); - vi.mock('./db/admin.db', () => ({ - AdminRepository: MockTransactionalAdminRepository, - })); vi.mock('./emailService.server', () => ({ sendPasswordResetEmail: vi.fn(), })); @@ -103,6 +103,8 @@ describe('AuthService', () => { vi.mocked(withTransaction).mockImplementation(async (callback: any) => { return callback({}); // Mock client }); + const { validatePasswordStrength } = await import('../utils/authUtils'); + vi.mocked(validatePasswordStrength).mockReturnValue({ isValid: true, feedback: '' }); sendPasswordResetEmail = (await import('./emailService.server')).sendPasswordResetEmail; UniqueConstraintError = (await import('./db/errors.db')).UniqueConstraintError; RepositoryError = (await import('./db/errors.db')).RepositoryError; @@ -356,11 +358,19 @@ describe('AuthService', () => { const dbError = new Error('DB connection failed'); vi.mocked(userRepo.findUserByRefreshToken).mockRejectedValue(dbError); - await expect(authService.getUserByRefreshToken('any-token', reqLog)).rejects.toThrow(DatabaseError); - expect(logger.error).toHaveBeenCalledWith( - { error: dbError, refreshToken: 'any-token' }, - 'An unexpected error occurred while fetching user by refresh token.', - ); + // Use a try-catch to assert on the error instance properties, which is more robust + // than `toBeInstanceOf` in some complex module mocking scenarios in Vitest. + try { + await authService.getUserByRefreshToken('any-token', reqLog); + expect.fail('Expected an error to be thrown'); + } catch (error: any) { + expect(error.name).toBe('DatabaseError'); + expect(error.message).toBe('DB connection failed'); + expect(logger.error).toHaveBeenCalledWith( + { error: dbError, refreshToken: 'any-token' }, + 'An unexpected error occurred while fetching user by refresh token.', + ); + } }); it('should re-throw a RepositoryError if finding the user fails with a known error', async () => { diff --git a/src/services/authService.ts b/src/services/authService.ts index bf5f25d0..2736f431 100644 --- a/src/services/authService.ts +++ b/src/services/authService.ts @@ -52,12 +52,15 @@ class AuthService { return newUser; }).catch((error: unknown) => { - // The repository layer already logs and throws specific, typed errors. - // We only need to catch, log the high-level operation failure, and re-throw. - logger.error({ error, email }, `User registration failed.`); - // Re-throw the original, specific error (e.g., UniqueConstraintError) - // so the route handler can generate a precise HTTP response (e.g., 409 Conflict). - throw error; + // Re-throw known repository errors (like UniqueConstraintError) to allow for specific handling upstream. + if (error instanceof RepositoryError) { + throw error; + } + // For unknown errors, log them and wrap them in a generic DatabaseError + // to standardize the error contract of the service layer. + const message = error instanceof Error ? error.message : 'An unknown error occurred during registration.'; + logger.error({ error, email }, `User registration failed with an unexpected error.`); + throw new DatabaseError(message); }); } @@ -138,10 +141,14 @@ class AuthService { return token; } catch (error) { - logger.error({ error, email }, `An error occurred during /forgot-password for email: ${email}`); - // Re-throw the original error, which might be a specific RepositoryError - // or a generic DatabaseError from the underlying layers. - throw error; + // Re-throw known repository errors to allow for specific handling upstream. + if (error instanceof RepositoryError) { + throw error; + } + // For unknown errors, log them and wrap them in a generic DatabaseError. + const message = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ error, email }, `An unexpected error occurred during password reset for email: ${email}`); + throw new DatabaseError(message); } } @@ -184,8 +191,14 @@ class AuthService { return true; }).catch((error) => { - logger.error({ error }, `An error occurred during password update.`); - throw error; + // Re-throw known repository errors to allow for specific handling upstream. + if (error instanceof RepositoryError) { + throw error; + } + // For unknown errors, log them and wrap them in a generic DatabaseError. + const message = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ error }, `An unexpected error occurred during password update.`); + throw new DatabaseError(message); }); } diff --git a/src/services/db/errors.db.ts b/src/services/db/errors.db.ts index 7d6e1924..2f0da20f 100644 --- a/src/services/db/errors.db.ts +++ b/src/services/db/errors.db.ts @@ -127,6 +127,15 @@ export interface HandleDbErrorOptions { defaultMessage?: string; } +/** + * A type guard to check if an error object is a PostgreSQL error with a code. + */ +function isPostgresError( + error: unknown, +): error is { code: string; constraint?: string; detail?: string } { + return typeof error === 'object' && error !== null && 'code' in error; +} + /** * Centralized error handler for database repositories. * Logs the error and throws appropriate custom errors based on PostgreSQL error codes. @@ -143,18 +152,34 @@ export function handleDbError( throw error; } - // Log the raw error - logger.error({ err: error, ...logContext }, logMessage); + if (isPostgresError(error)) { + const { code, constraint, detail } = error; + const enhancedLogContext = { err: error, code, constraint, detail, ...logContext }; - if (error instanceof Error && 'code' in error) { - const code = (error as any).code; + // Log the detailed error first + logger.error(enhancedLogContext, logMessage); - if (code === '23505') throw new UniqueConstraintError(options.uniqueMessage); - if (code === '23503') throw new ForeignKeyConstraintError(options.fkMessage); - if (code === '23502') throw new NotNullConstraintError(options.notNullMessage); - if (code === '23514') throw new CheckConstraintError(options.checkMessage); - if (code === '22P02') throw new InvalidTextRepresentationError(options.invalidTextMessage); - if (code === '22003') throw new NumericValueOutOfRangeError(options.numericOutOfRangeMessage); + // Now, throw the appropriate custom error + switch (code) { + case '23505': // unique_violation + throw new UniqueConstraintError(options.uniqueMessage); + case '23503': // foreign_key_violation + throw new ForeignKeyConstraintError(options.fkMessage); + case '23502': // not_null_violation + throw new NotNullConstraintError(options.notNullMessage); + case '23514': // check_violation + throw new CheckConstraintError(options.checkMessage); + case '22P02': // invalid_text_representation + throw new InvalidTextRepresentationError(options.invalidTextMessage); + case '22003': // numeric_value_out_of_range + throw new NumericValueOutOfRangeError(options.numericOutOfRangeMessage); + default: + // If it's a PG error but not one we handle specifically, fall through to the generic error. + break; + } + } else { + // Log the error if it wasn't a recognized Postgres error + logger.error({ err: error, ...logContext }, logMessage); } // Fallback generic error diff --git a/src/services/db/flyer.db.test.ts b/src/services/db/flyer.db.test.ts index 3b9da460..e1962268 100644 --- a/src/services/db/flyer.db.test.ts +++ b/src/services/db/flyer.db.test.ts @@ -18,6 +18,7 @@ import { NotFoundError, CheckConstraintError, } from './errors.db'; +import { DatabaseError } from '../processingErrors'; import type { FlyerInsert, FlyerItemInsert, @@ -439,7 +440,8 @@ describe('Flyer DB Service', () => { // Here, we just expect it to be thrown. await expect( createFlyerAndItems(flyerData, itemsData, mockLogger, mockClient as unknown as PoolClient), - ).rejects.toThrow(dbError); + // The error is wrapped by handleDbError, so we check for the wrapped error. + ).rejects.toThrow(new DatabaseError('Failed to insert flyer into database.')); }); }); diff --git a/src/services/db/user.db.test.ts b/src/services/db/user.db.test.ts index e44670ef..bf4303d2 100644 --- a/src/services/db/user.db.test.ts +++ b/src/services/db/user.db.test.ts @@ -28,6 +28,8 @@ import { mockPoolInstance } from '../../tests/setup/tests-setup-unit'; import { createMockUserProfile, createMockUser } from '../../tests/utils/mockFactories'; import { UniqueConstraintError, ForeignKeyConstraintError, NotFoundError } from './errors.db'; import type { Profile, ActivityLogItem, SearchQuery, UserProfile, User } from '../../types'; +import { ShoppingRepository } from './shopping.db'; +import { PersonalizationRepository } from './personalization.db'; // Mock other db services that are used by functions in user.db.ts // Update mocks to put methods on prototype so spyOn works in exportUserData tests @@ -115,7 +117,7 @@ describe('User DB Service', () => { }); describe('createUser', () => { - it('should create a user and profile using the provided client', async () => { + it('should create a user and profile successfully', async () => { const mockUser = { user_id: 'new-user-id', email: 'new@example.com', @@ -153,14 +155,11 @@ describe('User DB Service', () => { updated_at: mockDbProfile.updated_at, }; - vi.mocked(withTransaction).mockImplementation(async (callback: any) => { - const mockClient = { query: vi.fn(), release: vi.fn() }; - (mockClient.query as Mock) - .mockResolvedValueOnce({ rows: [] }) // set_config - .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user - .mockResolvedValueOnce({ rows: [mockDbProfile] }); // SELECT profile - return callback(mockClient as unknown as PoolClient); - }); + // Mock the sequence of queries on the main pool instance + (mockPoolInstance.query as Mock) + .mockResolvedValueOnce({ rows: [] }) // set_config + .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user + .mockResolvedValueOnce({ rows: [mockDbProfile] }); // SELECT profile const result = await userRepo.createUser( 'new@example.com', @@ -169,52 +168,38 @@ describe('User DB Service', () => { mockLogger, ); - // Use objectContaining because the real implementation might have other DB-generated fields. - // We can't do a deep equality check on the user object because the mock factory will generate different timestamps. expect(result.user.user_id).toEqual(expectedProfile.user.user_id); expect(result.full_name).toEqual(expectedProfile.full_name); - // eslint-disable-next-line @typescript-eslint/no-unused-vars expect(result).toEqual(expect.objectContaining(expectedProfile)); - expect(withTransaction).toHaveBeenCalledTimes(1); }); - it('should rollback the transaction if creating the user fails', async () => { + it('should throw an error if creating the user fails', async () => { const dbError = new Error('User insert failed'); - vi.mocked(withTransaction).mockImplementation(async (callback) => { - const mockClient = { query: vi.fn() }; - mockClient.query.mockRejectedValueOnce(dbError); // set_config or INSERT fails - await expect(callback(mockClient as unknown as PoolClient)).rejects.toThrow(dbError); - throw dbError; - }); + mockPoolInstance.query.mockRejectedValue(dbError); await expect( userRepo.createUser('fail@example.com', 'badpass', {}, mockLogger), ).rejects.toThrow('Failed to create user in database.'); expect(mockLogger.error).toHaveBeenCalledWith( { err: dbError, email: 'fail@example.com' }, - 'Error during createUser transaction', + 'Error during createUser', ); }); - it('should rollback the transaction if fetching the final profile fails', async () => { + it('should throw an error if fetching the final profile fails', async () => { const mockUser = { user_id: 'new-user-id', email: 'new@example.com' }; const dbError = new Error('Profile fetch failed'); - vi.mocked(withTransaction).mockImplementation(async (callback) => { - const mockClient = { query: vi.fn() }; - mockClient.query - .mockResolvedValueOnce({ rows: [] }) // set_config - .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user - .mockRejectedValueOnce(dbError); // SELECT profile fails - await expect(callback(mockClient as unknown as PoolClient)).rejects.toThrow(dbError); - throw dbError; - }); + (mockPoolInstance.query as Mock) + .mockResolvedValueOnce({ rows: [] }) // set_config + .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user + .mockRejectedValueOnce(dbError); // SELECT profile fails await expect(userRepo.createUser('fail@example.com', 'pass', {}, mockLogger)).rejects.toThrow( 'Failed to create user in database.', ); expect(mockLogger.error).toHaveBeenCalledWith( { err: dbError, email: 'fail@example.com' }, - 'Error during createUser transaction', + 'Error during createUser', ); }); @@ -222,7 +207,7 @@ describe('User DB Service', () => { const dbError = new Error('duplicate key value violates unique constraint'); (dbError as Error & { code: string }).code = '23505'; - vi.mocked(withTransaction).mockRejectedValue(dbError); + mockPoolInstance.query.mockRejectedValue(dbError); try { await userRepo.createUser('exists@example.com', 'pass', {}, mockLogger); @@ -232,36 +217,26 @@ describe('User DB Service', () => { // After confirming the error type, we can safely access its properties. // This satisfies TypeScript's type checker for the 'unknown' type. if (error instanceof Error) { - expect(error.message).toBe('A user with this email address already exists.'); + expect(error.message).toBe('A user with this email address already exists.'); // This message comes from the options in handleDbError } } - - expect(withTransaction).toHaveBeenCalledTimes(1); expect(mockLogger.warn).toHaveBeenCalledWith(`Attempted to create a user with an existing email: exists@example.com`); }); it('should throw an error if profile is not found after user creation', async () => { const mockUser = { user_id: 'new-user-id', email: 'no-profile@example.com' }; - vi.mocked(withTransaction).mockImplementation(async (callback) => { - const mockClient = { query: vi.fn() }; - mockClient.query - .mockResolvedValueOnce({ rows: [] }) // set_config - .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user succeeds - .mockResolvedValueOnce({ rows: [] }); // SELECT profile returns nothing - // The callback will throw, which is caught and re-thrown by withTransaction - await expect(callback(mockClient as unknown as PoolClient)).rejects.toThrow( - 'Failed to create or retrieve user profile after registration.', - ); - throw new Error('Internal failure'); // Simulate re-throw from withTransaction - }); + (mockPoolInstance.query as Mock) + .mockResolvedValueOnce({ rows: [] }) // set_config + .mockResolvedValueOnce({ rows: [mockUser] }) // INSERT user succeeds + .mockResolvedValueOnce({ rows: [] }); // SELECT profile returns nothing await expect( userRepo.createUser('no-profile@example.com', 'pass', {}, mockLogger), ).rejects.toThrow('Failed to create user in database.'); expect(mockLogger.error).toHaveBeenCalledWith( { err: expect.any(Error), email: 'no-profile@example.com' }, - 'Error during createUser transaction', + 'Error during createUser', ); }); }); @@ -669,23 +644,12 @@ describe('User DB Service', () => { }); describe('deleteRefreshToken', () => { - it('should execute an UPDATE query to set the refresh token to NULL', async () => { - mockPoolInstance.query.mockResolvedValue({ rows: [] }); - await userRepo.deleteRefreshToken('a-token', mockLogger); - expect(mockPoolInstance.query).toHaveBeenCalledWith( - 'UPDATE public.users SET refresh_token = NULL WHERE refresh_token = $1', - ['a-token'], - ); - }); - it('should log an error but not throw if the database query fails', async () => { const dbError = new Error('DB Error'); mockPoolInstance.query.mockRejectedValue(dbError); - // The function is designed to swallow errors, so we expect it to resolve. await expect(userRepo.deleteRefreshToken('a-token', mockLogger)).resolves.toBeUndefined(); - // We can still check that the query was attempted. expect(mockPoolInstance.query).toHaveBeenCalled(); expect(mockLogger.error).toHaveBeenCalledWith( { err: dbError }, @@ -764,10 +728,13 @@ describe('User DB Service', () => { }); it('should log an error if the database query fails', async () => { - mockPoolInstance.query.mockRejectedValue(new Error('DB Error')); - await userRepo.deleteResetToken('token-hash', mockLogger); + const dbError = new Error('DB Error'); + mockPoolInstance.query.mockRejectedValue(dbError); + await expect(userRepo.deleteResetToken('token-hash', mockLogger)).rejects.toThrow( + 'Failed to delete password reset token.', + ); expect(mockLogger.error).toHaveBeenCalledWith( - { err: expect.any(Error), tokenHash: 'token-hash' }, + { err: dbError, tokenHash: 'token-hash' }, 'Database error in deleteResetToken', ); }); @@ -800,18 +767,7 @@ describe('User DB Service', () => { }); describe('exportUserData', () => { - // Import the mocked withTransaction helper - let withTransaction: Mock; - beforeEach(async () => { - const connDb = await import('./connection.db'); - // Cast to Mock for type-safe access to mock properties - withTransaction = connDb.withTransaction as Mock; - }); - it('should call profile, watched items, and shopping list functions', async () => { - const { ShoppingRepository } = await import('./shopping.db'); - const { PersonalizationRepository } = await import('./personalization.db'); - const findProfileSpy = vi.spyOn(UserRepository.prototype, 'findUserProfileById'); findProfileSpy.mockResolvedValue( createMockUserProfile({ user: createMockUser({ user_id: '123', email: '123@example.com' }) }), diff --git a/src/services/db/user.db.ts b/src/services/db/user.db.ts index c85833db..dbfa59e1 100644 --- a/src/services/db/user.db.ts +++ b/src/services/db/user.db.ts @@ -129,12 +129,6 @@ export class UserRepository { logger.debug({ user: fullUserProfile }, `[DB createUser] Fetched full profile for new user:`); return fullUserProfile; } catch (error) { - // Specific handling for unique constraint violation on user creation - if (error instanceof Error && 'code' in error && (error as any).code === '23505') { - logger.warn(`Attempted to create a user with an existing email: ${email}`); - throw new UniqueConstraintError('A user with this email address already exists.'); - } - // Fallback to generic handler for all other errors handleDbError(error, logger, 'Error during createUser', { email }, { uniqueMessage: 'A user with this email address already exists.', defaultMessage: 'Failed to create user in database.', @@ -466,9 +460,8 @@ export class UserRepository { refreshToken, ]); } catch (error) { - handleDbError(error, logger, 'Database error in deleteRefreshToken', {}, { - defaultMessage: 'Failed to delete refresh token.', - }); + // This is a non-critical operation, so we just log the error and continue. + logger.error({ err: error }, 'Database error in deleteRefreshToken'); } } diff --git a/src/services/flyerAiProcessor.server.test.ts b/src/services/flyerAiProcessor.server.test.ts index e526dbfc..2de4965a 100644 --- a/src/services/flyerAiProcessor.server.test.ts +++ b/src/services/flyerAiProcessor.server.test.ts @@ -277,10 +277,8 @@ describe('FlyerAiProcessor', () => { expect(result.needsReview).toBe(true); expect(logger.warn).toHaveBeenCalledWith( - expect.objectContaining({ - qualityIssues: ['Missing store name', 'No items were extracted', 'Missing both valid_from and valid_to dates'], - }), - 'AI response has quality issues. Issues: Missing store name, No items were extracted, Missing both valid_from and valid_to dates', + { rawData: mockAiResponse, qualityIssues: ['Missing store name', 'No items were extracted', 'Missing both valid_from and valid_to dates'] }, + 'AI response has quality issues. Flagging for review. Issues: Missing store name, No items were extracted, Missing both valid_from and valid_to dates', ); }); }); diff --git a/src/services/flyerAiProcessor.server.ts b/src/services/flyerAiProcessor.server.ts index 178c2793..f1d71e2b 100644 --- a/src/services/flyerAiProcessor.server.ts +++ b/src/services/flyerAiProcessor.server.ts @@ -155,7 +155,7 @@ export class FlyerAiProcessor { } // 2. Items: Append all found items to the master list. - mergedData.items.push(...batchResult.items); + mergedData.items.push(...(batchResult.items || [])); } logger.info(`Batch processing complete. Total items extracted: ${mergedData.items.length}`); diff --git a/src/services/flyerProcessingService.server.test.ts b/src/services/flyerProcessingService.server.test.ts index 264d9dbb..f87beeee 100644 --- a/src/services/flyerProcessingService.server.test.ts +++ b/src/services/flyerProcessingService.server.test.ts @@ -36,6 +36,7 @@ import { UnsupportedFileTypeError, TransformationError, } from './processingErrors'; +import { NotFoundError } from './db/errors.db'; import { FlyerFileHandler } from './flyerFileHandler.server'; import { FlyerAiProcessor } from './flyerAiProcessor.server'; import type { IFileSystem, ICommandExecutor } from './flyerFileHandler.server'; @@ -53,6 +54,8 @@ vi.mock('./db/flyer.db', () => ({ vi.mock('./db/index.db', () => ({ personalizationRepo: { getAllMasterItems: vi.fn() }, adminRepo: { logActivity: vi.fn() }, + flyerRepo: { getFlyerById: vi.fn() }, + withTransaction: vi.fn(), })); vi.mock('./logger.server', () => ({ logger: { @@ -79,6 +82,10 @@ describe('FlyerProcessingService', () => { beforeEach(() => { vi.clearAllMocks(); + // Provide a default mock implementation for withTransaction that just executes the callback. + // This is needed for the happy path tests. Tests for transaction failures will override this. + vi.mocked(mockedDb.withTransaction).mockImplementation(async (callback: any) => callback({})); + // Spy on the real transformer's method and provide a mock implementation. // This is more robust than mocking the entire class constructor. vi.spyOn(FlyerDataTransformer.prototype, 'transform').mockResolvedValue({ @@ -194,6 +201,9 @@ describe('FlyerProcessingService', () => { expect(result).toEqual({ flyerId: 1 }); expect(mockFileHandler.prepareImageInputs).toHaveBeenCalledWith(job.data.filePath, job, expect.any(Object)); expect(mockAiProcessor.extractAndValidateData).toHaveBeenCalledTimes(1); + // Verify that the transaction function was called. + expect(mockedDb.withTransaction).toHaveBeenCalledTimes(1); + // Verify that the functions inside the transaction were called. expect(createFlyerAndItems).toHaveBeenCalledTimes(1); expect(mockedDb.adminRepo.logActivity).toHaveBeenCalledTimes(1); expect(mockCleanupQueue.add).toHaveBeenCalledWith( @@ -215,6 +225,8 @@ describe('FlyerProcessingService', () => { await service.processJob(job); + // Verify transaction and inner calls + expect(mockedDb.withTransaction).toHaveBeenCalledTimes(1); expect(mockFileHandler.prepareImageInputs).toHaveBeenCalledWith('/tmp/flyer.pdf', job, expect.any(Object)); expect(mockAiProcessor.extractAndValidateData).toHaveBeenCalledTimes(1); expect(createFlyerAndItems).toHaveBeenCalledTimes(1); @@ -362,6 +374,8 @@ describe('FlyerProcessingService', () => { await service.processJob(job); + // Verify transaction and inner calls + expect(mockedDb.withTransaction).toHaveBeenCalledTimes(1); expect(mockFileHandler.prepareImageInputs).toHaveBeenCalledWith('/tmp/flyer.gif', job, expect.any(Object)); expect(mockAiProcessor.extractAndValidateData).toHaveBeenCalledTimes(1); expect(mockCleanupQueue.add).toHaveBeenCalledWith( @@ -375,8 +389,11 @@ describe('FlyerProcessingService', () => { const job = createMockJob({}); const { logger } = await import('./logger.server'); const dbError = new Error('Database transaction failed'); - vi.mocked(createFlyerAndItems).mockRejectedValue(dbError); - + + // To test the DB failure, we make the transaction itself fail when called. + // This is more realistic than mocking the inner function `createFlyerAndItems`. + vi.mocked(mockedDb.withTransaction).mockRejectedValue(dbError); + // The service wraps the generic DB error in a DatabaseError, but _reportErrorAndThrow re-throws the original. await expect(service.processJob(job)).rejects.toThrow(dbError); @@ -591,14 +608,23 @@ describe('FlyerProcessingService', () => { ); }); - it('should skip processing and return "skipped" if paths array is empty', async () => { + it('should skip processing and return "skipped" if paths array is empty and paths cannot be derived', async () => { const job = createMockCleanupJob({ flyerId: 1, paths: [] }); + // Mock that the flyer cannot be found in the DB, so paths cannot be derived. + vi.mocked(mockedDb.flyerRepo.getFlyerById).mockRejectedValue(new NotFoundError('Not found')); + const result = await service.processCleanupJob(job); expect(mocks.unlink).not.toHaveBeenCalled(); - expect(result).toEqual({ status: 'skipped', reason: 'no paths' }); + expect(result).toEqual({ status: 'skipped', reason: 'no paths derived' }); const { logger } = await import('./logger.server'); - expect(logger.warn).toHaveBeenCalledWith('Job received no paths to clean. Skipping.'); + // Check for both warnings: the attempt to derive, and the final skip message. + expect(logger.warn).toHaveBeenCalledWith( + 'Cleanup job for flyer 1 received no paths. Attempting to derive paths from DB.', + ); + expect(logger.warn).toHaveBeenCalledWith( + 'Job received no paths and could not derive any from the database. Skipping.', + ); }); }); }); diff --git a/src/services/gamificationService.ts b/src/services/gamificationService.ts index 76474eb0..9a047fad 100644 --- a/src/services/gamificationService.ts +++ b/src/services/gamificationService.ts @@ -2,6 +2,7 @@ import { gamificationRepo } from './db/index.db'; import type { Logger } from 'pino'; +import { ForeignKeyConstraintError } from './db/errors.db'; class GamificationService { /** @@ -11,9 +12,22 @@ class GamificationService { * @param log The logger instance. */ async awardAchievement(userId: string, achievementName: string, log: Logger): Promise { - // The repository layer handles database errors, including logging and throwing specific error types. - // This service method simply orchestrates the call. - return gamificationRepo.awardAchievement(userId, achievementName, log); + try { + await gamificationRepo.awardAchievement(userId, achievementName, log); + } catch (error) { + if (error instanceof ForeignKeyConstraintError) { + // This is an expected error (e.g., achievement name doesn't exist), + // which the repository layer should have already logged with appropriate context. + // We re-throw it so the calling layer (e.g., an admin route) can handle it. + throw error; + } + // For unexpected, generic errors, we log them at the service level before re-throwing. + log.error( + { error, userId, achievementName }, + 'Error awarding achievement via admin endpoint:', + ); + throw error; + } } /** @@ -21,7 +35,12 @@ class GamificationService { * @param log The logger instance. */ async getAllAchievements(log: Logger) { - return gamificationRepo.getAllAchievements(log); + try { + return await gamificationRepo.getAllAchievements(log); + } catch (error) { + log.error({ error }, 'Error in getAllAchievements service method'); + throw error; + } } /** @@ -30,7 +49,12 @@ class GamificationService { * @param log The logger instance. */ async getLeaderboard(limit: number, log: Logger) { - return gamificationRepo.getLeaderboard(limit, log); + try { + return await gamificationRepo.getLeaderboard(limit, log); + } catch (error) { + log.error({ error, limit }, 'Error fetching leaderboard in service method.'); + throw error; + } } /** @@ -39,7 +63,12 @@ class GamificationService { * @param log The logger instance. */ async getUserAchievements(userId: string, log: Logger) { - return gamificationRepo.getUserAchievements(userId, log); + try { + return await gamificationRepo.getUserAchievements(userId, log); + } catch (error) { + log.error({ error, userId }, 'Error fetching user achievements in service method.'); + throw error; + } } } diff --git a/src/services/userService.test.ts b/src/services/userService.test.ts index 6c989616..1c7d878a 100644 --- a/src/services/userService.test.ts +++ b/src/services/userService.test.ts @@ -191,13 +191,12 @@ describe('UserService', () => { mocks.mockUpsertAddress.mockRejectedValue(dbError); // Act & Assert - await expect(userService.upsertUserAddress(user, addressData, logger)).rejects.toThrow( - DatabaseError, - ); + // The service should wrap the generic error in a `DatabaseError`. + await expect(userService.upsertUserAddress(user, addressData, logger)).rejects.toBeInstanceOf(DatabaseError); // Assert that the error was logged correctly expect(logger.error).toHaveBeenCalledWith( - { err: dbError }, + { err: dbError, userId: user.user.user_id }, `Transaction to upsert user address failed: ${dbError.message}`, ); }); diff --git a/src/services/userService.ts b/src/services/userService.ts index 798ada0c..91fdf2be 100644 --- a/src/services/userService.ts +++ b/src/services/userService.ts @@ -43,8 +43,11 @@ class UserService { return addressId; }) .catch((error) => { - logger.error({ err: error, userId: userprofile.user.user_id }, `Transaction to upsert user address failed.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, userId: userprofile.user.user_id }, `Transaction to upsert user address failed: ${errorMessage}`); + // Wrap the original error in a service-level DatabaseError to standardize the error contract, + // as this is an unexpected failure within the transaction boundary. + throw new DatabaseError(errorMessage); }); } @@ -64,8 +67,10 @@ class UserService { logger.info(`Successfully deleted ${deletedCount} expired tokens.`); return { deletedCount }; } catch (error) { - logger.error({ err: error, attemptsMade: job.attemptsMade }, `Expired token cleanup job failed.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, attemptsMade: job.attemptsMade }, `Expired token cleanup job failed: ${errorMessage}`); + // This is a background job, but wrapping in a standard error type is good practice. + throw new DatabaseError(errorMessage); } } @@ -86,8 +91,10 @@ class UserService { if (error instanceof NotFoundError) { throw error; } - logger.error({ err: error, userId }, `Failed to update user avatar.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, userId }, `Failed to update user avatar: ${errorMessage}`); + // Wrap unexpected errors. + throw new DatabaseError(errorMessage); } } /** @@ -102,8 +109,10 @@ class UserService { const hashedPassword = await bcrypt.hash(newPassword, saltRounds); await db.userRepo.updateUserPassword(userId, hashedPassword, logger); } catch (error) { - logger.error({ err: error, userId }, `Failed to update user password.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, userId }, `Failed to update user password: ${errorMessage}`); + // Wrap unexpected errors. + throw new DatabaseError(errorMessage); } } @@ -128,8 +137,10 @@ class UserService { if (error instanceof NotFoundError || error instanceof ValidationError) { throw error; } - logger.error({ err: error, userId }, `Failed to delete user account.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, userId }, `Failed to delete user account: ${errorMessage}`); + // Wrap unexpected errors. + throw new DatabaseError(errorMessage); } } @@ -150,8 +161,10 @@ class UserService { if (error instanceof NotFoundError) { throw error; } - logger.error({ err: error, userId: userProfile.user.user_id, addressId }, `Failed to get user address.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + logger.error({ err: error, userId: userProfile.user.user_id, addressId }, `Failed to get user address: ${errorMessage}`); + // Wrap unexpected errors. + throw new DatabaseError(errorMessage); } } @@ -172,8 +185,10 @@ class UserService { if (error instanceof ValidationError) { throw error; } - log.error({ err: error, deleterId, userToDeleteId }, `Admin failed to delete user account.`); - throw error; + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.'; + log.error({ err: error, deleterId, userToDeleteId }, `Admin failed to delete user account: ${errorMessage}`); + // Wrap unexpected errors. + throw new DatabaseError(errorMessage); } } } diff --git a/src/utils/pdfConverter.test.ts b/src/utils/pdfConverter.test.ts index 40af09f6..ad529028 100644 --- a/src/utils/pdfConverter.test.ts +++ b/src/utils/pdfConverter.test.ts @@ -3,8 +3,16 @@ * @vitest-environment jsdom */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, Mocked } from 'vitest'; import { convertPdfToImageFiles } from './pdfConverter'; +import { logger } from '../services/logger.client'; + +// Mock the logger before other imports to spy on its methods +vi.mock('../services/logger.client', () => ({ + logger: { + warn: vi.fn(), + }, +})); // Mock the entire pdfjs-dist library const mockPdfPage = { @@ -14,7 +22,9 @@ const mockPdfPage = { const mockPdfDocument = { numPages: 3, - getPage: vi.fn(() => Promise.resolve(mockPdfPage)), + // Explicitly type the mock function to accept a number and return the correct promise type. + // This resolves the TypeScript error when using mockImplementation with arguments later. + getPage: vi.fn<(pageNumber: number) => Promise>(() => Promise.resolve(mockPdfPage)), }; vi.mock('pdfjs-dist', () => ({ @@ -205,19 +215,56 @@ describe('pdfConverter', () => { expect(getDocument).toHaveBeenCalled(); }); - it('should throw an error if conversion results in zero images for a non-empty PDF', async () => { + it('should throw a specific error if all pages of a non-empty PDF fail to convert', async () => { // Arrange: Ensure the document appears to have pages mockPdfDocument.numPages = 1; const pdfFile = new File(['pdf-content'], 'flyer.pdf', { type: 'application/pdf' }); - // Mock getPage to fail for the first page. This simulates a corrupted page - // within an otherwise valid PDF document, which is what the function's - // Promise.allSettled logic is designed to handle. + // Mock getPage to fail for the only page. This simulates a scenario where + // the PDF has pages, but none can be rendered, causing the `imageFiles` array + // to be empty. vi.mocked(mockPdfDocument.getPage).mockRejectedValueOnce(new Error('Corrupted page')); - // Act & Assert: The function should catch the settled promise and re-throw the reason. + // Act & Assert: The function should now catch the settled promise, find that no + // images were generated, and throw the specific "zero images" error, covering line 133. + await expect(convertPdfToImageFiles(pdfFile)).rejects.toThrow( + 'PDF conversion resulted in zero images, though the PDF has pages. It might be corrupted or contain non-standard content.', + ); + }); - await expect(convertPdfToImageFiles(pdfFile)).rejects.toThrow('Corrupted page'); + it('should successfully process a PDF even if some pages fail to convert', async () => { + // Arrange: 3-page PDF where the 2nd page will fail + mockPdfDocument.numPages = 3; + const pdfFile = new File(['pdf-content'], 'partial-success.pdf', { type: 'application/pdf' }); + const onProgress = vi.fn(); + const mockedLogger = logger as Mocked; + + // Mock getPage to fail only for the second page + vi.mocked(mockPdfDocument.getPage).mockImplementation(async (pageNumber: number) => { + if (pageNumber === 2) { + throw new Error('Simulated page 2 corruption'); + } + // Return the standard mock page for other pages + return mockPdfPage; + }); + + // Act + const { imageFiles, pageCount } = await convertPdfToImageFiles(pdfFile, onProgress); + + // Assert + // Total page count should still be 3 + expect(pageCount).toBe(3); + // Only 2 pages should have converted successfully + expect(imageFiles).toHaveLength(2); + // The progress callback should have been called for the 2 successful pages + expect(onProgress).toHaveBeenCalledTimes(2); + expect(onProgress).toHaveBeenCalledWith(1, 3); + expect(onProgress).toHaveBeenCalledWith(3, 3); + // The failure of page 2 should be logged as a warning + expect(mockedLogger.warn).toHaveBeenCalledWith( + { error: new Error('Simulated page 2 corruption') }, + 'A page failed to convert during PDF processing.', + ); }); it('should throw an error if FileReader fails', async () => { diff --git a/src/utils/pdfConverter.ts b/src/utils/pdfConverter.ts index 7b257fa2..ded2efd2 100644 --- a/src/utils/pdfConverter.ts +++ b/src/utils/pdfConverter.ts @@ -116,17 +116,18 @@ export const convertPdfToImageFiles = async ( // Process all pages in parallel and collect the results. const settledResults = await Promise.allSettled(pagePromises); - // Check for any hard failures and re-throw the first one encountered. - const firstRejected = settledResults.find((r) => r.status === 'rejected') as - | PromiseRejectedResult - | undefined; - if (firstRejected) { - throw firstRejected.reason; - } + // Filter for fulfilled promises and extract their values. This allows for partial + // success if some pages convert and others fail. + const imageFiles = settledResults + .filter((result): result is PromiseFulfilledResult => result.status === 'fulfilled') + .map((result) => result.value); - // Collect all successfully rendered image files. Since we've already checked for rejections, - // we know all results are fulfilled and can safely extract their values. - const imageFiles = settledResults.map((result) => (result as PromiseFulfilledResult).value); + // Log any pages that failed to convert, without stopping the entire process. + settledResults.forEach((result) => { + if (result.status === 'rejected') { + logger.warn({ error: result.reason }, 'A page failed to convert during PDF processing.'); + } + }); if (imageFiles.length === 0 && pageCount > 0) { throw new Error( diff --git a/src/utils/priceParser.test.ts b/src/utils/priceParser.test.ts index d606c097..265b4ccb 100644 --- a/src/utils/priceParser.test.ts +++ b/src/utils/priceParser.test.ts @@ -69,4 +69,9 @@ describe('parsePriceToCents', () => { expect(parsePriceToCents(' $10.99 ')).toBe(1099); expect(parsePriceToCents(' 99ยข ')).toBe(99); }); + + it('should return null for a price string that matches the pattern but results in NaN (e.g., "$." or ".")', () => { + expect(parsePriceToCents('$.')).toBeNull(); + expect(parsePriceToCents('.')).toBeNull(); + }); }); diff --git a/src/utils/serverUtils.test.ts b/src/utils/serverUtils.test.ts new file mode 100644 index 00000000..5acc2649 --- /dev/null +++ b/src/utils/serverUtils.test.ts @@ -0,0 +1,85 @@ +// src/utils/serverUtils.test.ts +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Logger } from 'pino'; +import { getBaseUrl } from './serverUtils'; + +// Create a mock logger to spy on its methods +const createMockLogger = (): Logger => + ({ + warn: vi.fn(), + // Add other logger methods if they were used, but only `warn` is relevant here. + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + fatal: vi.fn(), + trace: vi.fn(), + silent: vi.fn(), + child: vi.fn(() => createMockLogger()), + level: 'info', + }) as unknown as Logger; + +describe('serverUtils', () => { + describe('getBaseUrl', () => { + const originalEnv = process.env; + let mockLogger: Logger; + + beforeEach(() => { + // Reset mocks and environment variables before each test for isolation + vi.resetModules(); + process.env = { ...originalEnv }; + mockLogger = createMockLogger(); + }); + + afterEach(() => { + // Restore original environment variables after each test + process.env = originalEnv; + }); + + it('should use FRONTEND_URL if it is a valid URL', () => { + process.env.FRONTEND_URL = 'https://valid.example.com'; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('https://valid.example.com'); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should trim a trailing slash from FRONTEND_URL', () => { + process.env.FRONTEND_URL = 'https://valid.example.com/'; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('https://valid.example.com'); + }); + + it('should use BASE_URL if FRONTEND_URL is not set', () => { + delete process.env.FRONTEND_URL; + process.env.BASE_URL = 'https://base.example.com'; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('https://base.example.com'); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should fall back to localhost with default port 3000 if no URL is provided', () => { + delete process.env.FRONTEND_URL; + delete process.env.BASE_URL; + delete process.env.PORT; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('http://localhost:3000'); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should fall back to localhost with the specified PORT if no URL is provided', () => { + delete process.env.FRONTEND_URL; + delete process.env.BASE_URL; + process.env.PORT = '8888'; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('http://localhost:8888'); + }); + + it('should log a warning and fall back if FRONTEND_URL is invalid (does not start with http)', () => { + process.env.FRONTEND_URL = 'invalid.url.com'; + const baseUrl = getBaseUrl(mockLogger); + expect(baseUrl).toBe('http://localhost:3000'); + expect(mockLogger.warn).toHaveBeenCalledWith( + "[getBaseUrl] FRONTEND_URL/BASE_URL is invalid or incomplete ('invalid.url.com'). Falling back to default local URL: http://localhost:3000", + ); + }); + }); +}); \ No newline at end of file