From 3af01834449f43d72491fe33c81fecbfd45057dc Mon Sep 17 00:00:00 2001 From: Torben Sorensen Date: Tue, 25 Nov 2025 16:01:06 -0800 Subject: [PATCH] ts fixes from reorg + unit test work --- src/features/flyer/AnalysisPanel.test.tsx | 3 +- src/features/flyer/BulkImporter.tsx | 6 +- src/features/flyer/ProcessingStatus.test.tsx | 4 +- src/pages/admin/WatchedItemsList.test.tsx | 149 -------------- src/pages/admin/WatchedItemsList.tsx | 186 ------------------ .../components/AdminBrandManager.test.tsx | 9 +- .../admin/components/AdminBrandManager.tsx | 2 + .../admin/components/ProfileManager.test.tsx | 2 +- .../admin/components/SystemCheck.test.tsx | 49 +++-- 9 files changed, 46 insertions(+), 364 deletions(-) delete mode 100644 src/pages/admin/WatchedItemsList.test.tsx delete mode 100644 src/pages/admin/WatchedItemsList.tsx diff --git a/src/features/flyer/AnalysisPanel.test.tsx b/src/features/flyer/AnalysisPanel.test.tsx index 724794d4..74e10f5b 100644 --- a/src/features/flyer/AnalysisPanel.test.tsx +++ b/src/features/flyer/AnalysisPanel.test.tsx @@ -69,7 +69,8 @@ describe('AnalysisPanel', () => { it('should switch tabs and update the generate button text', () => { render(); - fireEvent.click(screen.getByRole('button', { name: /deep dive/i })); + // The tab elements now correctly have the role 'tab', not 'button'. + fireEvent.click(screen.getByRole('tab', { name: /deep dive/i })); expect(screen.getByRole('button', { name: /generate deep dive/i })).toBeInTheDocument(); }); diff --git a/src/features/flyer/BulkImporter.tsx b/src/features/flyer/BulkImporter.tsx index fe7dde7e..88d9fa80 100644 --- a/src/features/flyer/BulkImporter.tsx +++ b/src/features/flyer/BulkImporter.tsx @@ -71,7 +71,11 @@ export const BulkImporter: React.FC = ({ onProcess, isProcess )} - + {/* The `hidden` class was making the label non-interactive in tests. + It's better to make it visually hidden but still accessible. + We use `sr-only` (screen-reader only) classes for this. + */} + ); diff --git a/src/features/flyer/ProcessingStatus.test.tsx b/src/features/flyer/ProcessingStatus.test.tsx index cc800da3..708fa10b 100644 --- a/src/features/flyer/ProcessingStatus.test.tsx +++ b/src/features/flyer/ProcessingStatus.test.tsx @@ -46,7 +46,7 @@ describe('ProcessingStatus', () => { // Completed stage const completedStageText = screen.getByTestId('stage-text-0'); expect(completedStageText).toHaveClass('text-gray-700'); - expect(within(screen.getByTestId('stage-icon-0')).getByTestId('check-circle-icon')).toBeInTheDocument(); + expect(screen.getByTestId('stage-icon-0').querySelector('svg')).toHaveClass('text-green-500'); // In-progress stage` const inProgressStageText = screen.getByTestId('stage-text-1'); @@ -61,7 +61,7 @@ describe('ProcessingStatus', () => { // Non-critical error stage const nonCriticalErrorStageText = screen.getByTestId('stage-text-3'); expect(nonCriticalErrorStageText).toHaveClass('text-yellow-600'); - expect(within(screen.getByTestId('stage-icon-3')).getByTestId('exclamation-triangle-icon')).toBeInTheDocument(); + expect(screen.getByTestId('stage-icon-3').querySelector('svg')).toHaveClass('text-yellow-500'); expect(screen.getByText(/optional/i)).toBeInTheDocument(); // Critical error stage diff --git a/src/pages/admin/WatchedItemsList.test.tsx b/src/pages/admin/WatchedItemsList.test.tsx deleted file mode 100644 index 028bd894..00000000 --- a/src/pages/admin/WatchedItemsList.test.tsx +++ /dev/null @@ -1,149 +0,0 @@ -// src/pages/admin/WatchedItemsList.test.tsx -import React from 'react'; -import { render, screen, fireEvent, waitFor } from '@testing-library/react'; -import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; -import { WatchedItemsList } from './WatchedItemsList'; // This path is now relative to the new folder -import type { MasterGroceryItem, User } from '../../types'; - -// Mock the logger -vi.mock('../../services/logger', () => ({ - logger: { - info: vi.fn(), - warn: vi.fn(), - error: vi.fn(), - debug: vi.fn(), - }, -})); - -const mockOnAddItem = vi.fn(); -const mockOnRemoveItem = vi.fn(); -const mockOnAddItemToList = vi.fn(); - -const mockUser: User = { user_id: 'user-123', email: 'test@example.com' }; - -const mockItems: MasterGroceryItem[] = [ - { master_grocery_item_id: 1, name: 'Apples', category_id: 1, category_name: 'Produce', created_at: '' }, - { master_grocery_item_id: 2, name: 'Milk', category_id: 2, category_name: 'Dairy', created_at: '' }, - { master_grocery_item_id: 3, name: 'Bread', category_id: 3, category_name: 'Bakery', created_at: '' }, -]; - -const defaultProps = { - items: mockItems, - onAddItem: mockOnAddItem, - onRemoveItem: mockOnRemoveItem, - user: mockUser, - activeListId: 1, - onAddItemToList: mockOnAddItemToList, -}; - -describe('WatchedItemsList', () => { - beforeEach(() => { - vi.clearAllMocks(); - mockOnAddItem.mockResolvedValue(undefined); - mockOnRemoveItem.mockResolvedValue(undefined); - }); - - it('should render a login message when user is not authenticated', () => { - render(); - expect(screen.getByText(/please log in to create and manage your personal watchlist/i)).toBeInTheDocument(); - expect(screen.queryByRole('form')).not.toBeInTheDocument(); - }); - - it('should render the form and item list when user is authenticated', () => { - render(); - expect(screen.getByPlaceholderText(/add item/i)).toBeInTheDocument(); - expect(screen.getByRole('combobox', { name: /filter by category/i })).toBeInTheDocument(); - expect(screen.getByText('Apples')).toBeInTheDocument(); - expect(screen.getByText('Milk')).toBeInTheDocument(); - expect(screen.getByText('Bread')).toBeInTheDocument(); - }); - - it('should allow adding a new item', async () => { - render(); - - fireEvent.change(screen.getByPlaceholderText(/add item/i), { target: { value: 'Cheese' } }); - fireEvent.change(screen.getByRole('combobox', { name: '' }), { target: { value: 'Dairy' } }); // Category select - - fireEvent.submit(screen.getByRole('button', { name: 'Add' })); - - await waitFor(() => { - expect(mockOnAddItem).toHaveBeenCalledWith('Cheese', 'Dairy'); - }); - - // Check if form resets - expect(screen.getByPlaceholderText(/add item/i)).toHaveValue(''); - }); - - it('should show a loading spinner while adding an item', async () => { - (mockOnAddItem as Mock).mockImplementation(() => new Promise(() => {})); // Never resolves - render(); - - fireEvent.change(screen.getByPlaceholderText(/add item/i), { target: { value: 'Cheese' } }); - fireEvent.change(screen.getByRole('combobox', { name: '' }), { target: { value: 'Dairy' } }); - - const addButton = screen.getByRole('button', { name: 'Add' }); - fireEvent.click(addButton); - - await waitFor(() => { - expect(addButton.querySelector('svg.animate-spin')).toBeInTheDocument(); - expect(addButton).toBeDisabled(); - }); - }); - - it('should allow removing an item', async () => { - render(); - const removeButton = screen.getByRole('button', { name: /remove apples/i }); - fireEvent.click(removeButton); - - await waitFor(() => { - expect(mockOnRemoveItem).toHaveBeenCalledWith(1); // ID for Apples is 1 - }); - }); - - it('should filter items by category', () => { - render(); - const categoryFilter = screen.getByRole('combobox', { name: /filter by category/i }); - - fireEvent.change(categoryFilter, { target: { value: 'Dairy' } }); - - expect(screen.getByText('Milk')).toBeInTheDocument(); - expect(screen.queryByText('Apples')).not.toBeInTheDocument(); - expect(screen.queryByText('Bread')).not.toBeInTheDocument(); - }); - - it('should sort items ascending and descending', () => { - render(); - const sortButton = screen.getByRole('button', { name: /sort items descending/i }); - - const itemsAsc = screen.getAllByRole('listitem'); - expect(itemsAsc[0]).toHaveTextContent('Apples'); - expect(itemsAsc[1]).toHaveTextContent('Bread'); - expect(itemsAsc[2]).toHaveTextContent('Milk'); - - // Click to sort descending - fireEvent.click(sortButton); - - const itemsDesc = screen.getAllByRole('listitem'); - expect(itemsDesc[0]).toHaveTextContent('Milk'); - expect(itemsDesc[1]).toHaveTextContent('Bread'); - expect(itemsDesc[2]).toHaveTextContent('Apples'); - }); - - it('should call onAddItemToList when plus icon is clicked', () => { - render(); - const addToListButton = screen.getByTitle('Add Apples to list'); - fireEvent.click(addToListButton); - expect(mockOnAddItemToList).toHaveBeenCalledWith(1); // ID for Apples - }); - - it('should disable the add to list button if activeListId is null', () => { - render(); - const addToListButton = screen.getByTitle('Select a shopping list first'); - expect(addToListButton).toBeDisabled(); - }); - - it('should display a message when the list is empty', () => { - render(); - expect(screen.getByText(/your watchlist is empty/i)).toBeInTheDocument(); - }); -}); \ No newline at end of file diff --git a/src/pages/admin/WatchedItemsList.tsx b/src/pages/admin/WatchedItemsList.tsx deleted file mode 100644 index bb30629b..00000000 --- a/src/pages/admin/WatchedItemsList.tsx +++ /dev/null @@ -1,186 +0,0 @@ -// src/pages/admin/WatchedItemsList.tsx -import React, { useState, useMemo } from 'react'; -import type { MasterGroceryItem, User } from '../../types'; -import { EyeIcon } from '../../components/icons/EyeIcon'; -import { LoadingSpinner } from '../../components/LoadingSpinner'; -import { SortAscIcon } from '../../components/icons/SortAscIcon'; -import { SortDescIcon } from '../../components/icons/SortDescIcon'; -import { CATEGORIES } from '../../types'; -import { TrashIcon } from '../../components/icons/TrashIcon'; -import { UserIcon } from '../../components/icons/UserIcon'; -import { PlusCircleIcon } from '../../components/icons/PlusCircleIcon'; -import { logger } from '../../services/logger'; -interface WatchedItemsListProps { - items: MasterGroceryItem[]; - onAddItem: (itemName: string, category: string) => Promise; - onRemoveItem: (masterItemId: number) => Promise; - user: User | null; - activeListId: number | null; - onAddItemToList: (masterItemId: number) => void; -} - -export const WatchedItemsList: React.FC = ({ items, onAddItem, onRemoveItem, user, activeListId, onAddItemToList }) => { - const [newItemName, setNewItemName] = useState(''); - const [newCategory, setNewCategory] = useState(''); - const [isAdding, setIsAdding] = useState(false); - const [sortOrder, setSortOrder] = useState<'asc' | 'desc'>('asc'); - const [categoryFilter, setCategoryFilter] = useState('all'); - - const handleSubmit = async (e: React.FormEvent) => { - e.preventDefault(); - if (!newItemName.trim() || !newCategory) return; - - setIsAdding(true); - try { - await onAddItem(newItemName, newCategory); - setNewItemName(''); - setNewCategory(''); - } catch (error) { - // Error is handled in the parent component - logger.error('Failed to add watched item from WatchedItemsList', { error }); - } finally { - setIsAdding(false); - } - }; - - const handleSortToggle = () => { - setSortOrder(prev => (prev === 'asc' ? 'desc' : 'asc')); - }; - - const availableCategories = useMemo(() => { - const cats = new Set(items.map(i => i.category_name).filter((c): c is string => !!c)); - return Array.from(cats).sort(); - }, [items]); - - const sortedAndFilteredItems = useMemo(() => { - const filteredItems = categoryFilter === 'all' - ? items - : items.filter(item => item.category_name === categoryFilter); - - return [...filteredItems].sort((a, b) => { - if (sortOrder === 'asc') { - return a.name.localeCompare(b.name); - } else { - return b.name.localeCompare(a.name); - } - }); - }, [items, sortOrder, categoryFilter]); - - if (!user) { - return ( -
-
- -

Personalize Your Deals

-

- Please log in to create and manage your personal watchlist. -

-
-
- ); - } - - return ( -
-
-

- - Your Watched Items -

-
- {items.length > 0 && ( - - )} - {items.length > 1 && ( - - )} -
-
- -
- setNewItemName(e.target.value)} - placeholder="Add item (e.g., Avocados)" - className="grow block w-full px-3 py-2 bg-white dark:bg-gray-800 border border-gray-300 dark:border-gray-600 rounded-md shadow-sm placeholder-gray-400 focus:outline-none focus:ring-brand-primary focus:border-brand-primary sm:text-sm" - disabled={isAdding} - /> -
- - -
-
- - {sortedAndFilteredItems.length > 0 ? ( -
    - {sortedAndFilteredItems.map(item => ( -
  • -
    - {item.name} - {item.category_name} -
    -
    - - -
    -
    • - ))} -
- ) : ( -

- {categoryFilter === 'all' - ? 'Your watchlist is empty. Add items above to start tracking prices.' - : `No watched items in the "${categoryFilter}" category.`} -

- )} -
- ); -}; \ No newline at end of file diff --git a/src/pages/admin/components/AdminBrandManager.test.tsx b/src/pages/admin/components/AdminBrandManager.test.tsx index 1855caca..ddc6352d 100644 --- a/src/pages/admin/components/AdminBrandManager.test.tsx +++ b/src/pages/admin/components/AdminBrandManager.test.tsx @@ -67,7 +67,8 @@ describe('AdminBrandManager', () => { await waitFor(() => expect(screen.getByText('No Frills')).toBeInTheDocument()); const file = new File(['logo'], 'logo.png', { type: 'image/png' }); - const input = screen.getAllByRole('textbox', { hidden: true })[0]; // Find the first file input + // Use the new accessible label to find the correct input. + const input = screen.getByLabelText('Upload logo for No Frills'); fireEvent.change(input, { target: { files: [file] } }); @@ -89,7 +90,7 @@ describe('AdminBrandManager', () => { await waitFor(() => expect(screen.getByText('No Frills')).toBeInTheDocument()); const file = new File(['logo'], 'logo.png', { type: 'image/png' }); - const input = screen.getAllByRole('textbox', { hidden: true })[0]; + const input = screen.getByLabelText('Upload logo for No Frills'); fireEvent.change(input, { target: { files: [file] } }); @@ -104,7 +105,7 @@ describe('AdminBrandManager', () => { await waitFor(() => expect(screen.getByText('No Frills')).toBeInTheDocument()); const file = new File(['text'], 'document.txt', { type: 'text/plain' }); - const input = screen.getAllByRole('textbox', { hidden: true })[0]; + const input = screen.getByLabelText('Upload logo for No Frills'); fireEvent.change(input, { target: { files: [file] } }); @@ -120,7 +121,7 @@ describe('AdminBrandManager', () => { await waitFor(() => expect(screen.getByText('No Frills')).toBeInTheDocument()); const file = new File(['a'.repeat(3 * 1024 * 1024)], 'large.png', { type: 'image/png' }); - const input = screen.getAllByRole('textbox', { hidden: true })[0]; + const input = screen.getByLabelText('Upload logo for No Frills'); fireEvent.change(input, { target: { files: [file] } }); diff --git a/src/pages/admin/components/AdminBrandManager.tsx b/src/pages/admin/components/AdminBrandManager.tsx index 7221b29e..0f75159f 100644 --- a/src/pages/admin/components/AdminBrandManager.tsx +++ b/src/pages/admin/components/AdminBrandManager.tsx @@ -96,6 +96,8 @@ export const AdminBrandManager: React.FC = () => { e.target.files && handleLogoUpload(brand.brand_id, e.target.files[0])} diff --git a/src/pages/admin/components/ProfileManager.test.tsx b/src/pages/admin/components/ProfileManager.test.tsx index f65bd4b5..f8c667c8 100644 --- a/src/pages/admin/components/ProfileManager.test.tsx +++ b/src/pages/admin/components/ProfileManager.test.tsx @@ -29,7 +29,7 @@ vi.mock('../../../services/logger', () => ({ })); // Mock the notificationService to prevent actual toasts and spy on calls -vi.mock('../services/notificationService', () => ({ +vi.mock('../../../services/notificationService', () => ({ notifySuccess: vi.fn(), notifyError: vi.fn(), })); diff --git a/src/pages/admin/components/SystemCheck.test.tsx b/src/pages/admin/components/SystemCheck.test.tsx index 52e70cfb..b91bc103 100644 --- a/src/pages/admin/components/SystemCheck.test.tsx +++ b/src/pages/admin/components/SystemCheck.test.tsx @@ -54,8 +54,10 @@ describe('SystemCheck', () => { render(); // Initially, all checks should be in 'running' state due to auto-run + // However, the API key check is synchronous and resolves immediately. + // So we only expect the other 6 checks to display "Checking...". expect(screen.getByText('Gemini API Key')).toBeInTheDocument(); - expect(screen.getAllByText('Checking...')).toHaveLength(7); // All 7 checks + expect(screen.getAllByText('Checking...')).toHaveLength(6); // Wait for all checks to complete await waitFor(() => { @@ -77,9 +79,8 @@ describe('SystemCheck', () => { setViteApiKey(undefined); render(); - await waitFor(() => { - expect(screen.getByText('VITE_API_KEY is missing. Please add it to your .env file.')).toBeInTheDocument(); - }); + // This check is synchronous, so no need to wait. + expect(screen.getByText('VITE_API_KEY is missing. Please add it to your .env file.')).toBeInTheDocument(); // Other checks should not run if API key check fails early expect(apiClient.pingBackend).not.toHaveBeenCalled(); }); @@ -91,7 +92,9 @@ describe('SystemCheck', () => { await waitFor(() => { expect(screen.getByText('Network error')).toBeInTheDocument(); - expect(screen.getByText('Skipped: Backend server is not reachable.')).toBeInTheDocument(); + // Multiple checks will be skipped, so we use getAllByText. + const skippedMessages = screen.getAllByText('Skipped: Backend server is not reachable.'); + expect(skippedMessages.length).toBe(5); // PM2, DB Pool, Schema, Seed, Storage }); // Dependent checks should be skipped/failed @@ -163,7 +166,9 @@ describe('SystemCheck', () => { expect(rerunButton.querySelector('svg')).toBeInTheDocument(); // Check for spinner inside button // All checks should show 'Checking...' - expect(screen.getAllByText('Checking...')).toHaveLength(7); + // The API key check is synchronous, so it resolves instantly. + // We only expect the 6 async checks to be in the "Checking..." state. + expect(screen.getAllByText('Checking...')).toHaveLength(6); }); it('should re-run checks when the "Re-run Checks" button is clicked', async () => { @@ -171,9 +176,8 @@ describe('SystemCheck', () => { render(); // Wait for initial auto-run to complete - await waitFor(() => { - expect(screen.getByText('VITE_API_KEY is set.')).toBeInTheDocument(); - }); + // This is more reliable than waiting for a specific check. + await screen.findByText(/finished in/i); // Reset mocks for the re-run (apiClient.pingBackend as Mock).mockResolvedValueOnce(true); @@ -183,11 +187,13 @@ describe('SystemCheck', () => { (apiClient.checkPm2Status as Mock).mockResolvedValueOnce({ success: true, message: 'PM2 OK (re-run)' }); (apiClient.loginUser as Mock).mockResolvedValueOnce({}); - fireEvent.click(screen.getByRole('button', { name: /re-run checks/i })); + const rerunButton = screen.getByRole('button', { name: /re-run checks/i }); + fireEvent.click(rerunButton); // Expect checks to go back to 'Checking...' state await waitFor(() => { - expect(screen.getAllByText('Checking...')).toHaveLength(7); + // Again, only 6 checks will show "Checking..." because the API key check is instant. + expect(screen.getAllByText('Checking...')).toHaveLength(6); }); // Wait for re-run to complete @@ -207,13 +213,15 @@ describe('SystemCheck', () => { render(); await waitFor(() => { - // Check for pass icons (green checkmark) - const passIcons = screen.getAllByTestId('check-circle-icon'); + // Instead of test-ids, we check for the result: the icon's color class. + // This is more robust as it doesn't depend on the icon component's internal props. + const passIcons = container.querySelectorAll('svg.text-green-500'); // 6 checks should pass (API key, backend, PM2, DB Pool, Seed, Storage) - expect(passIcons.length).toBeGreaterThanOrEqual(5); // At least 5, as PM2, DB Pool, Seed, Storage are dependent on backend + expect(passIcons.length).toBe(6); - // Check for fail icon (red X) - expect(screen.getByTestId('x-circle-icon')).toBeInTheDocument(); + // Check for the fail icon's color class + const failIcon = container.querySelector('svg.text-red-500'); + expect(failIcon).toBeInTheDocument(); }); }); @@ -225,10 +233,11 @@ describe('SystemCheck', () => { await waitFor(() => { expect(screen.getByText('PM2 not running (optional)')).toBeInTheDocument(); - // Ensure the '(optional)' text is present - expect(screen.getByText('(optional)')).toBeInTheDocument(); - // The error icon for an optional check should be ExclamationTriangleIcon - expect(screen.getByTestId('exclamation-triangle-icon')).toBeInTheDocument(); + // The component logic does not render the "(optional)" text when the check fails, + // so we should not test for it in this state. + // Instead, we verify the correct warning icon is shown by its color class. + const warningIcon = container.querySelector('svg.text-yellow-500'); + expect(warningIcon).toBeInTheDocument(); }); });