From 8c31fe4352a6427a12a0eb4eb2d8c15f7e79a047 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:22:51 +0530 Subject: [PATCH 01/10] feat: convert a memory into an album Adds a three-dot menu to the memory grid tiles, matching the album cards, with a Convert to Album action. POST /albums/from-memory copies the memory's photos into a new album in one transaction; the memory is left untouched and clips are skipped, since albums hold images only. --- backend/app/database/albums.py | 28 ++++ backend/app/routes/albums.py | 90 ++++++++++++ backend/app/schemas/album.py | 18 +++ backend/tests/test_albums.py | 107 ++++++++++++++ backend/tests/test_albums_db.py | 39 ++++++ frontend/src/api/api-functions/albums.ts | 17 +++ frontend/src/api/apiEndpoints.ts | 1 + .../Memories/ConvertMemoryToAlbumDialog.tsx | 132 ++++++++++++++++++ .../src/components/Memories/MemoryCard.tsx | 129 +++++++++++------ .../Memories/__tests__/MemoryCard.test.tsx | 72 ++++++++++ frontend/src/pages/Memories/Memories.tsx | 15 +- frontend/src/types/Album.ts | 10 ++ 12 files changed, 612 insertions(+), 46 deletions(-) create mode 100644 frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx create mode 100644 frontend/src/components/Memories/__tests__/MemoryCard.test.tsx diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index d39cd1ceb..6f546d092 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -132,6 +132,34 @@ def db_insert_album( conn.close() +def db_create_album_with_images( + album_id: str, album_name: str, description: str, image_ids: list[str] +) -> int: + """ + Create an album and link its images in a single transaction. + + Both halves commit together, so a failed link never strands an empty album. + Takes image ids rather than the id of whatever they came from, so the + caller owns that choice. Returns the number of images actually linked. + """ + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute( + """ + INSERT INTO albums (album_id, album_name, description, is_locked, password_hash) + VALUES (?, ?, ?, 0, NULL) + """, + (album_id, album_name, description), + ) + # Foreign keys are on for this connection, so an image id that no + # longer exists rolls the album back with it rather than half-writing. + cursor.executemany( + "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", + [(album_id, image_id) for image_id in image_ids], + ) + return cursor.rowcount + + def db_update_album( album_id: str, album_name: str, diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index adecc6afc..bafc8cf81 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -1,9 +1,13 @@ from fastapi import APIRouter, HTTPException, status, Body, Path +import sqlite3 import uuid from app.schemas.album import ( GetAlbumsResponse, CreateAlbumRequest, CreateAlbumResponse, + CreateAlbumFromMemoryData, + CreateAlbumFromMemoryRequest, + CreateAlbumFromMemoryResponse, GetAlbumResponse, GetAlbumImagesRequest, GetAlbumImagesResponse, @@ -18,6 +22,7 @@ db_get_album_by_name, db_get_album, db_insert_album, + db_create_album_with_images, db_update_album, db_delete_album, db_get_album_images, @@ -27,6 +32,7 @@ db_get_album_cover_path, verify_album_password, ) +from app.database.memories import db_get_memory, db_get_memory_images router = APIRouter() @@ -91,6 +97,90 @@ def create_album(body: CreateAlbumRequest): ) +# POST /albums/from-memory - Create an album from a curated memory +@router.post("/from-memory", response_model=CreateAlbumFromMemoryResponse) +def create_album_from_memory(body: CreateAlbumFromMemoryRequest = Body(...)): + """ + Copy a memory's photos into a new album. + + The memory itself is left untouched, so the same one can be converted + again under a different name. Any clips are left behind: album_images + references images, and albums have no video support. + """ + memory = db_get_memory(body.memory_id) + if not memory: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Memory Not Found", + message="No memory exists with the provided ID.", + ).model_dump(), + ) + + image_ids = [image["id"] for image in db_get_memory_images(body.memory_id)] + if not image_ids: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Empty Memory", + message="This memory has no photos to convert.", + ).model_dump(), + ) + + if db_get_album_by_name(body.name): + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=ErrorResponse( + success=False, + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", + ).model_dump(), + ) + + album_id = str(uuid.uuid4()) + try: + image_count = db_create_album_with_images( + album_id, body.name, memory.get("subtitle") or "", image_ids + ) + except sqlite3.IntegrityError as e: + # The name check above is not atomic. Re-check rather than assume a + # conflict: the same error covers an image that vanished mid-request. + if db_get_album_by_name(body.name): + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=ErrorResponse( + success=False, + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", + ).model_dump(), + ) + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, + error="Internal Server Error", + message=f"Failed to create album from memory: {str(e)}", + ).model_dump(), + ) + except Exception as e: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, + error="Internal Server Error", + message=f"Failed to create album from memory: {str(e)}", + ).model_dump(), + ) + + return CreateAlbumFromMemoryResponse( + success=True, + message=f"Created album '{body.name}' with {image_count} photos", + data=CreateAlbumFromMemoryData(album_id=album_id, image_count=image_count), + ) + + # GET /albums/{album_id} - Get specific album details @router.get("/{album_id}", response_model=GetAlbumResponse) def get_album(album_id: str = Path(...)): diff --git a/backend/app/schemas/album.py b/backend/app/schemas/album.py index cced4adbe..f520ff1af 100644 --- a/backend/app/schemas/album.py +++ b/backend/app/schemas/album.py @@ -30,6 +30,11 @@ def check_password(cls, value, info: ValidationInfo): return value +class CreateAlbumFromMemoryRequest(BaseModel): + memory_id: str = Field(..., min_length=1) + name: str = Field(..., min_length=1) + + class UpdateAlbumRequest(BaseModel): name: str description: Optional[str] = "" @@ -85,6 +90,19 @@ class CreateAlbumResponse(BaseModel): album_id: str +class CreateAlbumFromMemoryData(BaseModel): + album_id: str + image_count: int + + +class CreateAlbumFromMemoryResponse(BaseModel): + # The {success, message, data} envelope the project standardised on, unlike + # the flat responses above that predate it. + success: bool + message: str + data: CreateAlbumFromMemoryData + + class GetAlbumResponse(BaseModel): success: bool data: Album diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index b24cb7376..aa5cf94e5 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -31,6 +31,24 @@ def mock_db_album(): } +@pytest.fixture +def mock_memory(): + """A memories row as db_get_memory returns it, trimmed to what the route reads.""" + return { + "memory_id": str(uuid.uuid4()), + "title": "3 years ago in Paris", + "subtitle": "July 2022 · Paris", + } + + +@pytest.fixture +def mock_memory_images(): + return [ + {"id": str(uuid.uuid4()), "sort_order": 0}, + {"id": str(uuid.uuid4()), "sort_order": 1}, + ] + + @pytest.fixture def mock_db_locked_album(): return { @@ -543,3 +561,92 @@ def test_remove_multiple_images_from_album(self, mock_db_album): mock_remove_bulk.assert_called_once_with( album_id, image_ids_to_remove["image_ids"] ) + + +class TestCreateAlbumFromMemory: + """Test suite for converting a curated memory into an album.""" + + def test_create_album_from_memory_success(self, mock_memory, mock_memory_images): + with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( + "app.routes.albums.db_get_memory_images" + ) as mock_get_images, patch( + "app.routes.albums.db_get_album_by_name" + ) as mock_get_by_name, patch( + "app.routes.albums.db_create_album_with_images" + ) as mock_create: + mock_get_memory.return_value = mock_memory + mock_get_images.return_value = mock_memory_images + mock_get_by_name.return_value = None + mock_create.return_value = len(mock_memory_images) + + response = client.post( + "/albums/from-memory", + json={"memory_id": mock_memory["memory_id"], "name": "Paris 2022"}, + ) + assert response.status_code == 200 + + json_response = response.json() + assert json_response["success"] is True + assert json_response["data"]["image_count"] == len(mock_memory_images) + uuid.UUID(json_response["data"]["album_id"]) + + # The memory's subtitle becomes the album description, and only the + # image ids are handed over - clips are left behind. + album_id, name, description, image_ids = mock_create.call_args.args + assert name == "Paris 2022" + assert description == mock_memory["subtitle"] + assert image_ids == [image["id"] for image in mock_memory_images] + + def test_create_album_from_memory_not_found(self): + with patch("app.routes.albums.db_get_memory") as mock_get_memory: + mock_get_memory.return_value = None + + response = client.post( + "/albums/from-memory", + json={"memory_id": str(uuid.uuid4()), "name": "Paris 2022"}, + ) + assert response.status_code == 404 + assert response.json()["detail"]["error"] == "Memory Not Found" + + def test_create_album_from_empty_memory(self, mock_memory): + """A memory with no photos cannot become an album.""" + with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( + "app.routes.albums.db_get_memory_images" + ) as mock_get_images, patch( + "app.routes.albums.db_create_album_with_images" + ) as mock_create: + mock_get_memory.return_value = mock_memory + mock_get_images.return_value = [] + + response = client.post( + "/albums/from-memory", + json={"memory_id": mock_memory["memory_id"], "name": "Paris 2022"}, + ) + assert response.status_code == 400 + assert response.json()["detail"]["error"] == "Empty Memory" + mock_create.assert_not_called() + + def test_create_album_from_memory_duplicate_name( + self, mock_memory, mock_memory_images, mock_db_album + ): + with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( + "app.routes.albums.db_get_memory_images" + ) as mock_get_images, patch( + "app.routes.albums.db_get_album_by_name" + ) as mock_get_by_name, patch( + "app.routes.albums.db_create_album_with_images" + ) as mock_create: + mock_get_memory.return_value = mock_memory + mock_get_images.return_value = mock_memory_images + mock_get_by_name.return_value = tuple(mock_db_album.values()) + + response = client.post( + "/albums/from-memory", + json={ + "memory_id": mock_memory["memory_id"], + "name": mock_db_album["album_name"], + }, + ) + assert response.status_code == 409 + assert response.json()["detail"]["error"] == "Album Already Exists" + mock_create.assert_not_called() diff --git a/backend/tests/test_albums_db.py b/backend/tests/test_albums_db.py index b494a6329..80a952c53 100644 --- a/backend/tests/test_albums_db.py +++ b/backend/tests/test_albums_db.py @@ -10,6 +10,7 @@ from app.database.albums import ( db_create_albums_table, db_create_album_images_table, + db_create_album_with_images, db_get_all_albums, db_get_album_by_name, db_get_album, @@ -244,6 +245,44 @@ def test_remove_images_drops_only_the_named_ones(self, test_db): assert db_get_album_images("album-1") == ["img-2"] +class TestCreateAlbumWithImages: + def test_creates_the_album_and_links_every_image(self, test_db): + make_images(test_db, ["img-1", "img-2"]) + + linked = db_create_album_with_images( + "album-1", "Paris 2022", "July 2022", ["img-1", "img-2"] + ) + + assert linked == 2 + assert db_get_album_images("album-1") == ["img-1", "img-2"] + album = db_get_album("album-1") + assert album[1] == "Paris 2022" + assert album[2] == "July 2022" + # Always an open album: locking is done afterwards, from Edit Album. + assert bool(album[3]) is False + assert album[4] is None + + def test_an_unknown_image_leaves_no_album_behind(self, test_db): + """The album and its links commit together, or not at all.""" + make_images(test_db, ["img-1"]) + + with pytest.raises(sqlite3.IntegrityError): + db_create_album_with_images( + "album-1", "Paris 2022", "", ["img-1", "img-missing"] + ) + + assert db_get_album("album-1") is None + + def test_a_duplicate_name_is_rejected(self, test_db): + make_album("album-1", "Paris 2022") + make_images(test_db, ["img-1"]) + + with pytest.raises(sqlite3.IntegrityError): + db_create_album_with_images("album-2", "Paris 2022", "", ["img-1"]) + + assert db_get_album("album-2") is None + + # ############################## # Password handling # ############################## diff --git a/frontend/src/api/api-functions/albums.ts b/frontend/src/api/api-functions/albums.ts index 98a16597a..6f755f8f7 100644 --- a/frontend/src/api/api-functions/albums.ts +++ b/frontend/src/api/api-functions/albums.ts @@ -1,8 +1,11 @@ import { albumsEndpoints } from '../apiEndpoints'; import { apiClient } from '../axiosConfig'; import { APIResponse } from '@/types/API'; +import type { BackendRes } from '@/hooks/useQueryExtension'; import { CreateAlbumRequest, + CreateAlbumFromMemoryData, + CreateAlbumFromMemoryRequest, UpdateAlbumRequest, AddImagesToAlbumRequest, GetAlbumImagesRequest, @@ -44,6 +47,20 @@ export const createAlbum = async ( return response.data; }; +/** + * Create an album from a curated memory's photos + * @param data - Source memory and the new album's name + */ +export const createAlbumFromMemory = async ( + data: CreateAlbumFromMemoryRequest, +): Promise> => { + const response = await apiClient.post( + albumsEndpoints.createAlbumFromMemory, + data, + ); + return response.data; +}; + /** * Update an existing album * @param albumId - Album UUID diff --git a/frontend/src/api/apiEndpoints.ts b/frontend/src/api/apiEndpoints.ts index 0ac942d95..384f56975 100644 --- a/frontend/src/api/apiEndpoints.ts +++ b/frontend/src/api/apiEndpoints.ts @@ -48,6 +48,7 @@ export const albumsEndpoints = { getAllAlbums: '/albums/', getAlbumById: (albumId: string) => `/albums/${albumId}`, createAlbum: '/albums/', + createAlbumFromMemory: '/albums/from-memory', updateAlbum: (albumId: string) => `/albums/${albumId}`, deleteAlbum: (albumId: string) => `/albums/${albumId}`, addImagesToAlbum: (albumId: string) => `/albums/${albumId}/images`, diff --git a/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx b/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx new file mode 100644 index 000000000..5aa06a295 --- /dev/null +++ b/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx @@ -0,0 +1,132 @@ +import React, { useEffect, useState } from 'react'; +import { useNavigate } from 'react-router'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog'; +import { Button } from '@/components/ui/button'; +import { Input } from '@/components/ui/input'; +import { Label } from '@/components/ui/label'; +import { ROUTES } from '@/constants/routes'; +import { usePictoMutation } from '@/hooks/useQueryExtension'; +import { createAlbumFromMemory } from '@/api/api-functions'; +import { useMutationFeedback } from '@/hooks/useMutationFeedback'; +import type { MemoryCard } from '@/api/api-functions/memories'; +import { formatPhotoCount } from '@/utils/memories'; + +interface ConvertMemoryToAlbumDialogProps { + memory: MemoryCard | null; + isOpen: boolean; + onClose: () => void; +} + +/** + * Copies a memory's photos into a new album. The name is pre-filled from the + * memory's title; everything else about the album is edited afterwards. + */ +export const ConvertMemoryToAlbumDialog: React.FC< + ConvertMemoryToAlbumDialogProps +> = ({ memory, isOpen, onClose }) => { + const navigate = useNavigate(); + const [name, setName] = useState(''); + const [error, setError] = useState(''); + + // One mounted dialog serves every tile, so the name follows the selected + // memory rather than the first render. + useEffect(() => { + if (memory) { + setName(memory.title); + setError(''); + } + }, [memory]); + + const convertMutation = usePictoMutation({ + mutationFn: createAlbumFromMemory, + autoInvalidateTags: ['albums'], + }); + + useMutationFeedback(convertMutation, { + loadingMessage: 'Creating album...', + successTitle: 'Success', + successMessage: 'Album created from memory!', + errorTitle: 'Error', + // A duplicate name comes back as a 409 and getErrorMessage surfaces the + // backend's own wording, so the dialog stays open to be renamed. + errorMessage: 'Failed to create the album. Please try again.', + onSuccess: () => { + const albumId = convertMutation.successData?.album_id; + onClose(); + if (albumId) { + navigate(`/${ROUTES.ALBUMS}/${albumId}`); + } + }, + }); + + const handleSubmit = (e: React.FormEvent) => { + e.preventDefault(); + + if (!memory) return; + + if (!name.trim()) { + setError('Album name is required'); + return; + } + + setError(''); + convertMutation.mutate({ memory_id: memory.memory_id, name: name.trim() }); + }; + + return ( + + +
+ + Convert to Album + + Creates an album from this memory's{' '} + {formatPhotoCount(memory?.image_count ?? 0)}. The memory itself is + kept. + {memory !== null && + memory.video_count > 0 && + ' Videos are left out — albums hold photos only.'} + + + +
+
+ + setName(e.target.value)} + className={error ? 'border-destructive' : ''} + /> + {error &&

{error}

} +
+
+ + + + + +
+ + + ); +}; diff --git a/frontend/src/components/Memories/MemoryCard.tsx b/frontend/src/components/Memories/MemoryCard.tsx index 12a0a98bd..dd16b80bb 100644 --- a/frontend/src/components/Memories/MemoryCard.tsx +++ b/frontend/src/components/Memories/MemoryCard.tsx @@ -1,7 +1,14 @@ import React from 'react'; -import { Images } from 'lucide-react'; +import { FolderPlus, Images, MoreVertical } from 'lucide-react'; import { Badge } from '@/components/ui/badge'; +import { Button } from '@/components/ui/button'; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from '@/components/ui/dropdown-menu'; import { createImageErrorHandler } from '@/utils/imageFallback'; import type { MemoryCard as MemoryCardType } from '@/api/api-functions/memories'; import { @@ -17,64 +24,96 @@ const handleCoverError = createImageErrorHandler(MEMORY_PLACEHOLDER_IMAGE); interface MemoryCardProps { memory: MemoryCardType; onOpen: (memoryId: string) => void; + onConvertToAlbum: (memoryId: string) => void; } /** * Grid tile for one memory. The cover scales and the caption lifts on hover; - * the whole tile opens the story viewer. + * the tile opens the story viewer, and the actions menu sits beside it rather + * than inside it - a button cannot contain another button. */ -export const MemoryCard: React.FC = ({ memory, onOpen }) => { +export const MemoryCard: React.FC = ({ + memory, + onOpen, + onConvertToAlbum, +}) => { const subtitle = formatMemorySubtitle(memory); const isUnviewed = memory.viewed_at === null; return ( - + + {/* Actions menu, matching the album grid's affordance. */} +
+ + + + + + onConvertToAlbum(memory.memory_id)} + > + + Convert to Album + + +
- + ); }; diff --git a/frontend/src/components/Memories/__tests__/MemoryCard.test.tsx b/frontend/src/components/Memories/__tests__/MemoryCard.test.tsx new file mode 100644 index 000000000..f00ece19d --- /dev/null +++ b/frontend/src/components/Memories/__tests__/MemoryCard.test.tsx @@ -0,0 +1,72 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen } from '@/test-utils'; +import type { MemoryCard as MemoryCardType } from '@/api/api-functions/memories'; +import { MemoryCard } from '../MemoryCard'; + +jest.mock('@tauri-apps/api/core', () => ({ + invoke: jest.fn().mockResolvedValue(null), + convertFileSrc: jest.fn((path: string) => `asset://localhost/${path}`), +})); + +const memory: MemoryCardType = { + memory_id: 'mem-1', + dedupe_key: 'import:2024-07-26..2024-07-26', + event_type: 'import_event', + status: 'complete', + title: 'Beach', + subtitle: '26 July 2024', + place_label: null, + center_lat: null, + center_lon: null, + surface_date: '2024-07-26', + period_start: '2024-07-26T10:00:00', + period_end: '2024-07-26T10:05:00', + image_count: 4, + video_count: 0, + cover_image_id: null, + cover_thumbnail_path: null, + score: 1, + notified_at: null, + viewed_at: null, + dismissed: false, + created_at: '2024-07-26T12:00:00', +}; + +const renderCard = () => { + const onOpen = jest.fn(); + const onConvertToAlbum = jest.fn(); + render( + , + ); + return { onOpen, onConvertToAlbum }; +}; + +describe('MemoryCard', () => { + it('opens the memory when the tile is clicked', async () => { + const user = userEvent.setup(); + const { onOpen } = renderCard(); + + await user.click( + screen.getByRole('button', { name: 'Open memory: Beach' }), + ); + + expect(onOpen).toHaveBeenCalledWith('mem-1'); + }); + + it('converts the memory from the actions menu without opening it', async () => { + const user = userEvent.setup(); + const { onOpen, onConvertToAlbum } = renderCard(); + + await user.click(screen.getByRole('button', { name: 'Options for Beach' })); + await user.click(await screen.findByText('Convert to Album')); + + expect(onConvertToAlbum).toHaveBeenCalledWith('mem-1'); + // The menu is a sibling of the tile, not a child, so acting on it must + // never open the story viewer. + expect(onOpen).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/pages/Memories/Memories.tsx b/frontend/src/pages/Memories/Memories.tsx index 3d70802d0..c2bf07ca7 100644 --- a/frontend/src/pages/Memories/Memories.tsx +++ b/frontend/src/pages/Memories/Memories.tsx @@ -1,13 +1,15 @@ -import React, { useEffect } from 'react'; +import React, { useEffect, useState } from 'react'; import { RefreshCw } from 'lucide-react'; import { Button } from '@/components/ui/button'; import { MemoryCard } from '@/components/Memories/MemoryCard'; +import { ConvertMemoryToAlbumDialog } from '@/components/Memories/ConvertMemoryToAlbumDialog'; import { MemoryStoryViewer } from '@/components/Memories/MemoryStoryViewer'; import { showInfoDialog } from '@/features/infoDialogSlice'; import { useAppDispatch, useAppSelector } from '@/store/hooks'; import { useUserPreferences } from '@/hooks/useUserPreferences'; import { useMemories, useRefreshMemories } from '@/hooks/useMemories'; +import type { MemoryCard as MemoryCardType } from '@/api/api-functions/memories'; import { openMemory, selectActiveMemoryId, @@ -33,6 +35,10 @@ export const Memories: React.FC = () => { const dispatch = useAppDispatch(); const activeMemoryId = useAppSelector(selectActiveMemoryId); + const [memoryToConvert, setMemoryToConvert] = useState( + null, + ); + const memoriesQuery = useMemories({ limit: 60 }); const { refresh, isRefreshing, status: statusQuery } = useRefreshMemories(); const { memoriesPreferences } = useUserPreferences(); @@ -120,12 +126,19 @@ export const Memories: React.FC = () => { key={memory.memory_id} memory={memory} onOpen={(id) => dispatch(openMemory(id))} + onConvertToAlbum={() => setMemoryToConvert(memory)} /> ))} )} + setMemoryToConvert(null)} + /> + {activeMemoryId && ( Date: Tue, 4 Aug 2026 10:57:37 +0530 Subject: [PATCH 02/10] docs: regenerate the openapi spec for /albums/from-memory --- docs/backend/backend_python/openapi.json | 102 +++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/docs/backend/backend_python/openapi.json b/docs/backend/backend_python/openapi.json index d6f10904a..3dce9f3e4 100644 --- a/docs/backend/backend_python/openapi.json +++ b/docs/backend/backend_python/openapi.json @@ -467,6 +467,48 @@ } } }, + "/albums/from-memory": { + "post": { + "tags": [ + "Albums" + ], + "summary": "Create Album From Memory", + "description": "Copy a memory's photos into a new album.\n\nThe memory itself is left untouched, so the same one can be converted\nagain under a different name. Any clips are left behind: album_images\nreferences images, and albums have no video support.", + "operationId": "create_album_from_memory_albums_from_memory_post", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/CreateAlbumFromMemoryRequest" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "Successful Response", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/CreateAlbumFromMemoryResponse" + } + } + } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } + } + } + } + }, "/albums/{album_id}": { "get": { "tags": [ @@ -2633,6 +2675,66 @@ ], "title": "ClusterMetadata" }, + "CreateAlbumFromMemoryData": { + "properties": { + "album_id": { + "type": "string", + "title": "Album Id" + }, + "image_count": { + "type": "integer", + "title": "Image Count" + } + }, + "type": "object", + "required": [ + "album_id", + "image_count" + ], + "title": "CreateAlbumFromMemoryData" + }, + "CreateAlbumFromMemoryRequest": { + "properties": { + "memory_id": { + "type": "string", + "minLength": 1, + "title": "Memory Id" + }, + "name": { + "type": "string", + "minLength": 1, + "title": "Name" + } + }, + "type": "object", + "required": [ + "memory_id", + "name" + ], + "title": "CreateAlbumFromMemoryRequest" + }, + "CreateAlbumFromMemoryResponse": { + "properties": { + "success": { + "type": "boolean", + "title": "Success" + }, + "message": { + "type": "string", + "title": "Message" + }, + "data": { + "$ref": "#/components/schemas/CreateAlbumFromMemoryData" + } + }, + "type": "object", + "required": [ + "success", + "message", + "data" + ], + "title": "CreateAlbumFromMemoryResponse" + }, "CreateAlbumRequest": { "properties": { "name": { From c995372c00187e15ae5549ae7f74ca5226109152 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:57:46 +0530 Subject: [PATCH 03/10] style: scale the card controls with the grid breakpoints The album and memory tiles already grow as the window widens, but the actions menu and the lock badge were a fixed size, so they looked heavy on a small window. They now step at lg and xl alongside the grid, and keep their current size at xl. Icons switch to size-* because the button variant forces size-4 onto any svg without a size- class, which silently overrode the old h-5 w-5. --- frontend/src/components/Albums/AlbumCard.tsx | 15 +++++++++------ frontend/src/components/Memories/MemoryCard.tsx | 11 +++++++---- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/frontend/src/components/Albums/AlbumCard.tsx b/frontend/src/components/Albums/AlbumCard.tsx index af9be9fec..d78020f4e 100644 --- a/frontend/src/components/Albums/AlbumCard.tsx +++ b/frontend/src/components/Albums/AlbumCard.tsx @@ -55,21 +55,24 @@ export const AlbumCard: React.FC = ({ /> {/* Lock Icon for Locked Albums */} {album.is_locked && ( -
- +
+
)} - {/* Actions Menu */} -
+ {/* Actions Menu. Sized off the same breakpoints as the grid, so the + control keeps its proportions as the cards grow. */} +
diff --git a/frontend/src/components/Memories/MemoryCard.tsx b/frontend/src/components/Memories/MemoryCard.tsx index dd16b80bb..e87ba7e3b 100644 --- a/frontend/src/components/Memories/MemoryCard.tsx +++ b/frontend/src/components/Memories/MemoryCard.tsx @@ -90,17 +90,20 @@ export const MemoryCard: React.FC = ({
- {/* Actions menu, matching the album grid's affordance. */} -
+ {/* Actions menu, matching the album grid's affordance - including how it + scales with the breakpoints that resize the cards. */} +
From 19ad2ef60853cd45b6d403579233a41a8b299515 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:07:27 +0530 Subject: [PATCH 04/10] Revert "style: scale the card controls with the grid breakpoints" This reverts commit c995372c00187e15ae5549ae7f74ca5226109152. --- frontend/src/components/Albums/AlbumCard.tsx | 15 ++++++--------- frontend/src/components/Memories/MemoryCard.tsx | 11 ++++------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/frontend/src/components/Albums/AlbumCard.tsx b/frontend/src/components/Albums/AlbumCard.tsx index d78020f4e..af9be9fec 100644 --- a/frontend/src/components/Albums/AlbumCard.tsx +++ b/frontend/src/components/Albums/AlbumCard.tsx @@ -55,24 +55,21 @@ export const AlbumCard: React.FC = ({ /> {/* Lock Icon for Locked Albums */} {album.is_locked && ( -
- +
+
)} - {/* Actions Menu. Sized off the same breakpoints as the grid, so the - control keeps its proportions as the cards grow. */} -
+ {/* Actions Menu */} +
diff --git a/frontend/src/components/Memories/MemoryCard.tsx b/frontend/src/components/Memories/MemoryCard.tsx index e87ba7e3b..dd16b80bb 100644 --- a/frontend/src/components/Memories/MemoryCard.tsx +++ b/frontend/src/components/Memories/MemoryCard.tsx @@ -90,20 +90,17 @@ export const MemoryCard: React.FC = ({
- {/* Actions menu, matching the album grid's affordance - including how it - scales with the breakpoints that resize the cards. */} -
+ {/* Actions menu, matching the album grid's affordance. */} +
From b652f98cbba91973bcd85ad0c3092e0dae0e093c Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:12:03 +0530 Subject: [PATCH 05/10] style: give the album and memory grids the gallery's intrinsic layout Both grids stepped their column count at breakpoints, so every card resized as the window changed. They now use the same auto-fill grid the chronological and ranked galleries use: cards hold a near-constant width and the column count follows the window instead. The class string was already repeated across the three galleries, so it moves to a constant they all share. --- frontend/src/components/Media/ChronologicalGallery.tsx | 4 +++- .../src/components/Media/ChronologicalVideoGallery.tsx | 4 +++- frontend/src/components/Media/RankedGallery.tsx | 4 +++- frontend/src/constants/layout.ts | 9 +++++++++ frontend/src/pages/Album/Album.tsx | 6 ++++-- frontend/src/pages/Memories/Memories.tsx | 5 +++-- 6 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 frontend/src/constants/layout.ts diff --git a/frontend/src/components/Media/ChronologicalGallery.tsx b/frontend/src/components/Media/ChronologicalGallery.tsx index f228f8f14..80b7de4b7 100644 --- a/frontend/src/components/Media/ChronologicalGallery.tsx +++ b/frontend/src/components/Media/ChronologicalGallery.tsx @@ -1,6 +1,8 @@ import { useMemo, useRef, useEffect, useCallback } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { ImageCard } from '@/components/Media/ImageCard'; +import { MEDIA_GRID_CLASS } from '@/constants/layout'; +import { cn } from '@/lib/utils'; import { Image } from '@/types/Media'; import { groupImagesByYearMonthFromMetadata } from '@/utils/dateUtils'; import { setCurrentViewIndex } from '@/features/imageSlice'; @@ -160,7 +162,7 @@ export const ChronologicalGallery = ({
{/* Images Grid */} -
+
{imgs.map((img) => { const chronologicalIndex = imageIndexMap.get(img.id) ?? -1; diff --git a/frontend/src/components/Media/ChronologicalVideoGallery.tsx b/frontend/src/components/Media/ChronologicalVideoGallery.tsx index a5824b1dc..a84c829d7 100644 --- a/frontend/src/components/Media/ChronologicalVideoGallery.tsx +++ b/frontend/src/components/Media/ChronologicalVideoGallery.tsx @@ -1,6 +1,8 @@ import { useMemo, useRef, useEffect, useCallback } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { VideoCard } from '@/components/Media/VideoCard'; +import { MEDIA_GRID_CLASS } from '@/constants/layout'; +import { cn } from '@/lib/utils'; import { Video } from '@/types/Media'; import { groupImagesByYearMonthFromMetadata } from '@/utils/dateUtils'; import { MonthMarker } from './ChronologicalGallery'; @@ -155,7 +157,7 @@ export const ChronologicalVideoGallery = ({
{/* Videos Grid */} -
+
{vids.map((video) => { const chronologicalIndex = videoIndexMap.get(video.id) ?? -1; diff --git a/frontend/src/components/Media/RankedGallery.tsx b/frontend/src/components/Media/RankedGallery.tsx index 3209c65a4..ff23c0535 100644 --- a/frontend/src/components/Media/RankedGallery.tsx +++ b/frontend/src/components/Media/RankedGallery.tsx @@ -1,6 +1,8 @@ import { useMemo } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { ImageCard } from '@/components/Media/ImageCard'; +import { MEDIA_GRID_CLASS } from '@/constants/layout'; +import { cn } from '@/lib/utils'; import { Image } from '@/types/Media'; import { setCurrentViewIndex } from '@/features/imageSlice'; import { MediaView } from './MediaView'; @@ -36,7 +38,7 @@ export function RankedGallery({ {titleRight &&
{titleRight}
}
)} -
+
{images.map((img) => (
{isLoading ? ( -
+
{Array.from({ length: 10 }).map((_, index) => ( ))} @@ -198,7 +200,7 @@ function Albums() { ) : albums.length === 0 ? ( ) : ( -
+
{sortedAlbums.map((album) => ( {
{memoriesQuery.isLoading ? ( -
+
{Array.from({ length: 10 }).map((_, index) => ( ))} @@ -120,7 +121,7 @@ export const Memories: React.FC = () => { ) : memories.length === 0 ? ( ) : ( -
+
{memories.map((memory) => ( Date: Tue, 4 Aug 2026 12:00:16 +0530 Subject: [PATCH 06/10] feat: sort albums by creation and update time, and remember the choice Adds created_at and updated_at to albums, both migrated onto existing databases. created_at is written once and never touched again; updated_at moves on a rename, a description or lock change, and on photos being added or removed, since to a user that is the album changing. Albums predating the columns have no timestamps rather than fabricated ones. They read as oldest, and the listing now orders by rowid so their fallback order is the order they were made in. The sort choice also survives a reload now, stored per surface in localStorage like the theme. AI Tagging gets it from the same hook: it uses the same dropdown and reset the same way. --- backend/app/database/albums.py | 55 +++++-- backend/app/routes/albums.py | 4 + backend/app/schemas/album.py | 4 + backend/tests/test_albums.py | 14 ++ backend/tests/test_albums_db.py | 140 ++++++++++++++++++ docs/backend/backend_python/openapi.json | 22 +++ .../src/components/GallerySortDropdown.tsx | 2 +- .../hooks/__tests__/usePersistedSort.test.ts | 46 ++++++ frontend/src/hooks/usePersistedSort.ts | 31 ++++ frontend/src/pages/AITagging/AITagging.tsx | 18 ++- frontend/src/pages/Album/Album.tsx | 39 ++++- frontend/src/pages/Album/AlbumDetail.tsx | 2 + frontend/src/pages/__tests__/Album.test.tsx | 62 ++++++++ frontend/src/types/Album.ts | 4 + 14 files changed, 426 insertions(+), 17 deletions(-) create mode 100644 frontend/src/hooks/__tests__/usePersistedSort.test.ts create mode 100644 frontend/src/hooks/usePersistedSort.ts diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 6f546d092..79f0868a4 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -17,18 +17,29 @@ def db_create_albums_table() -> None: description TEXT, is_locked BOOLEAN DEFAULT 0, password_hash TEXT, - cover_image_path TEXT + cover_image_path TEXT, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME DEFAULT CURRENT_TIMESTAMP ) """ ) # Shipped databases predate the is_hidden -> is_locked rename and the - # cover_image_path column, and CREATE IF NOT EXISTS won't add either. + # cover_image_path and created_at columns, and CREATE IF NOT EXISTS + # won't add any of them. cursor.execute("PRAGMA table_info(albums)") columns = {row[1] for row in cursor.fetchall()} if "is_locked" not in columns and "is_hidden" in columns: cursor.execute("ALTER TABLE albums RENAME COLUMN is_hidden TO is_locked") if "cover_image_path" not in columns: cursor.execute("ALTER TABLE albums ADD COLUMN cover_image_path TEXT") + if "created_at" not in columns: + # No default: SQLite rejects a non-constant one on ALTER TABLE, and + # stamping every existing album with the upgrade time would be a + # date that never happened. They stay NULL and read as oldest, + # which their insertion order already reflects. + cursor.execute("ALTER TABLE albums ADD COLUMN created_at DATETIME") + if "updated_at" not in columns: + cursor.execute("ALTER TABLE albums ADD COLUMN updated_at DATETIME") conn.commit() finally: if conn is not None: @@ -63,13 +74,28 @@ def db_create_album_images_table() -> None: conn.close() +def _touch_album(cursor: sqlite3.Cursor, album_id: str) -> None: + """ + Mark an album as changed just now. + + Adding or removing photos counts: to a user, that is the album changing, + not just its name or its lock. + """ + cursor.execute( + "UPDATE albums SET updated_at = CURRENT_TIMESTAMP WHERE album_id = ?", + (album_id,), + ) + + def db_get_all_albums(): """Get all albums (both locked and unlocked).""" conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: + # Insertion order, so albums predating created_at keep the order they + # were made in rather than an arbitrary one. cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path FROM albums" + "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums ORDER BY rowid" ) albums = cursor.fetchall() return albums @@ -82,7 +108,7 @@ def db_get_album_by_name(name: str): cursor = conn.cursor() try: cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path FROM albums WHERE album_name = ?", + "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums WHERE album_name = ?", (name,), ) album = cursor.fetchone() @@ -96,7 +122,7 @@ def db_get_album(album_id: str): cursor = conn.cursor() try: cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path FROM albums WHERE album_id = ?", + "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums WHERE album_id = ?", (album_id,), ) album = cursor.fetchone() @@ -120,10 +146,12 @@ def db_insert_album( password_hash = bcrypt.hashpw( password.encode("utf-8"), bcrypt.gensalt() ).decode("utf-8") + # created_at is set here rather than left to the column default: a + # database migrated with ALTER TABLE has no default to fall back on. cursor.execute( """ - INSERT INTO albums (album_id, album_name, description, is_locked, password_hash) - VALUES (?, ?, ?, ?, ?) + INSERT INTO albums (album_id, album_name, description, is_locked, password_hash, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, (album_id, album_name, description, int(is_locked), password_hash), ) @@ -146,8 +174,8 @@ def db_create_album_with_images( cursor = conn.cursor() cursor.execute( """ - INSERT INTO albums (album_id, album_name, description, is_locked, password_hash) - VALUES (?, ?, ?, 0, NULL) + INSERT INTO albums (album_id, album_name, description, is_locked, password_hash, created_at, updated_at) + VALUES (?, ?, ?, 0, NULL, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, (album_id, album_name, description), ) @@ -178,7 +206,8 @@ def db_update_album( cursor.execute( """ UPDATE albums - SET album_name = ?, description = ?, is_locked = ?, password_hash = ? + SET album_name = ?, description = ?, is_locked = ?, password_hash = ?, + updated_at = CURRENT_TIMESTAMP WHERE album_id = ? """, (album_name, description, int(is_locked), password_hash, album_id), @@ -188,7 +217,8 @@ def db_update_album( cursor.execute( """ UPDATE albums - SET album_name = ?, description = ?, is_locked = ? + SET album_name = ?, description = ?, is_locked = ?, + updated_at = CURRENT_TIMESTAMP WHERE album_id = ? """, (album_name, description, int(is_locked), album_id), @@ -275,6 +305,7 @@ def db_add_images_to_album(album_id: str, image_ids: list[str]): "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", [(album_id, img_id) for img_id in valid_images], ) + _touch_album(cursor, album_id) conn.commit() @@ -293,6 +324,7 @@ def db_remove_image_from_album(album_id: str, image_id: str): "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", (album_id, image_id), ) + _touch_album(cursor, album_id) else: raise ValueError("Image not found in the specified album") @@ -305,6 +337,7 @@ def db_remove_images_from_album(album_id: str, image_ids: list[str]): "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", [(album_id, img_id) for img_id in image_ids], ) + _touch_album(cursor, album_id) conn.commit() finally: conn.close() diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index bafc8cf81..2491e97ba 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -61,6 +61,8 @@ def get_albums(): None if is_locked else db_get_album_cover_path(album[0]) ), image_count=image_count, + created_at=album[6], + updated_at=album[7], ) ) return GetAlbumsResponse(success=True, albums=album_list) @@ -207,6 +209,8 @@ def get_album(album_id: str = Path(...)): # Same reasoning as the listing: the cover gives away the contents. cover_image_path=(None if is_locked else db_get_album_cover_path(album_id)), image_count=image_count, + created_at=album[6], + updated_at=album[7], ) return GetAlbumResponse(success=True, data=album_obj) except Exception as e: diff --git a/backend/app/schemas/album.py b/backend/app/schemas/album.py index f520ff1af..f9f0cb37f 100644 --- a/backend/app/schemas/album.py +++ b/backend/app/schemas/album.py @@ -10,6 +10,10 @@ class Album(BaseModel): is_locked: bool cover_image_path: Optional[str] = None image_count: int = 0 + # Null for albums that predate these columns; they read as oldest. + created_at: Optional[str] = None + # Touched by metadata edits and by adding or removing photos. + updated_at: Optional[str] = None # ############################## diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index aa5cf94e5..5f5dc59b3 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -142,6 +142,8 @@ def test_get_all_albums_public_only(self, mock_db_album): mock_db_album["is_locked"], mock_db_album["password_hash"], None, # cover_image_path + "2026-08-01 09:00:00", # created_at + "2026-08-05 09:00:00", # updated_at ) ] @@ -177,6 +179,8 @@ def test_get_all_albums_include_hidden(self, mock_db_album, mock_db_locked_album mock_db_album["is_locked"], mock_db_album["password_hash"], None, # cover_image_path + "2026-08-01 09:00:00", # created_at + "2026-08-05 09:00:00", # updated_at ), ( mock_db_locked_album["album_id"], @@ -185,6 +189,8 @@ def test_get_all_albums_include_hidden(self, mock_db_album, mock_db_locked_album mock_db_locked_album["is_locked"], mock_db_locked_album["password_hash"], None, # cover_image_path + "2026-08-02 09:00:00", # created_at + "2026-08-06 09:00:00", # updated_at ), ] @@ -215,6 +221,8 @@ def test_locked_album_cover_is_withheld(self, mock_db_album, mock_db_locked_albu mock_db_album["is_locked"], mock_db_album["password_hash"], None, + "2026-08-01 09:00:00", + "2026-08-05 09:00:00", ), ( mock_db_locked_album["album_id"], @@ -223,6 +231,8 @@ def test_locked_album_cover_is_withheld(self, mock_db_album, mock_db_locked_albu mock_db_locked_album["is_locked"], mock_db_locked_album["password_hash"], None, + "2026-08-02 09:00:00", + "2026-08-06 09:00:00", ), ] mock_cover.return_value = "/photos/secret.jpg" @@ -251,6 +261,8 @@ def test_get_album_by_id_withholds_a_locked_cover(self, mock_db_locked_album): mock_db_locked_album["is_locked"], mock_db_locked_album["password_hash"], None, + "2026-08-02 09:00:00", + "2026-08-06 09:00:00", ) mock_cover.return_value = "/photos/secret.jpg" @@ -287,6 +299,8 @@ def test_get_album_by_id_success(self, mock_db_album): mock_db_album["is_locked"], mock_db_album["password_hash"], None, # cover_image_path + "2026-08-01 09:00:00", # created_at + "2026-08-05 09:00:00", # updated_at ) response = client.get(f"/albums/{mock_db_album['album_id']}") diff --git a/backend/tests/test_albums_db.py b/backend/tests/test_albums_db.py index 80a952c53..ff4ded48a 100644 --- a/backend/tests/test_albums_db.py +++ b/backend/tests/test_albums_db.py @@ -18,6 +18,8 @@ db_update_album, db_delete_album, db_get_album_images, + db_add_images_to_album, + db_remove_image_from_album, db_remove_images_from_album, db_get_album_cover_path, verify_album_password, @@ -245,6 +247,144 @@ def test_remove_images_drops_only_the_named_ones(self, test_db): assert db_get_album_images("album-1") == ["img-2"] +class TestAlbumCreatedAt: + def test_insert_stamps_a_creation_time(self, test_db): + make_album("album-1", "Trip") + + assert db_get_album("album-1")[6] is not None + + def test_create_with_images_stamps_a_creation_time(self, test_db): + make_images(test_db, ["img-1"]) + + db_create_album_with_images("album-1", "Paris", "", ["img-1"]) + + assert db_get_album("album-1")[6] is not None + + def test_edits_never_change_it(self, test_db): + """Renaming, re-describing or adding photos is not a new creation.""" + make_album("album-1", "Trip") + make_images(test_db, ["img-1"]) + created = db_get_album("album-1")[6] + + db_update_album("album-1", "Trip Renamed", "A new description", False) + db_add_images_to_album("album-1", ["img-1"]) + + album = db_get_album("album-1") + assert album[6] == created + # ...but all of that does count as an update. + assert album[7] >= created + + def test_listing_keeps_insertion_order(self, test_db): + """Albums predating created_at sort by when they were made.""" + make_album("album-1", "First") + make_album("album-2", "Second") + + assert [album[0] for album in db_get_all_albums()] == ["album-1", "album-2"] + + def test_migration_adds_the_column_to_a_legacy_table(self, test_db): + """A database shipped before created_at existed still upgrades.""" + conn = sqlite3.connect(test_db) + conn.execute("DROP TABLE albums") + conn.execute( + """ + CREATE TABLE albums ( + album_id TEXT PRIMARY KEY, + album_name TEXT UNIQUE, + description TEXT, + is_hidden BOOLEAN DEFAULT 0, + password_hash TEXT + ) + """ + ) + conn.execute( + "INSERT INTO albums (album_id, album_name) VALUES ('old-1', 'Old')" + ) + conn.commit() + conn.close() + + db_create_albums_table() + + old = db_get_album("old-1") + assert old[1] == "Old" + # Nothing to backfill it with, so it reads as oldest. + assert old[6] is None + # And the table still takes new albums. + make_album("album-1", "New") + assert db_get_album("album-1")[6] is not None + + +class TestAlbumUpdatedAt: + """ + updated_at is compared against a pinned earlier value rather than against + a timestamp taken during the test: CURRENT_TIMESTAMP has one-second + resolution, so two writes in the same second read as equal. + """ + + EARLIER = "2020-01-01 00:00:00" + + def set_updated_at(self, db_path: str, album_id: str) -> None: + conn = sqlite3.connect(db_path) + conn.execute( + "UPDATE albums SET updated_at = ? WHERE album_id = ?", + (self.EARLIER, album_id), + ) + conn.commit() + conn.close() + + def test_insert_stamps_an_update_time(self, test_db): + make_album("album-1", "Trip") + + assert db_get_album("album-1")[7] is not None + + def test_editing_the_album_touches_it(self, test_db): + make_album("album-1", "Trip") + self.set_updated_at(test_db, "album-1") + + db_update_album("album-1", "Trip Renamed", "", False) + + assert db_get_album("album-1")[7] > self.EARLIER + + def test_adding_images_touches_it(self, test_db): + """Adding photos is the commonest way an album changes.""" + make_album("album-1", "Trip") + make_images(test_db, ["img-1"]) + self.set_updated_at(test_db, "album-1") + + db_add_images_to_album("album-1", ["img-1"]) + + assert db_get_album("album-1")[7] > self.EARLIER + + def test_removing_an_image_touches_it(self, test_db): + make_album("album-1", "Trip") + make_images(test_db, ["img-1"]) + db_add_images_to_album("album-1", ["img-1"]) + self.set_updated_at(test_db, "album-1") + + db_remove_image_from_album("album-1", "img-1") + + assert db_get_album("album-1")[7] > self.EARLIER + + def test_removing_images_in_bulk_touches_it(self, test_db): + make_album("album-1", "Trip") + make_images(test_db, ["img-1", "img-2"]) + db_add_images_to_album("album-1", ["img-1", "img-2"]) + self.set_updated_at(test_db, "album-1") + + db_remove_images_from_album("album-1", ["img-1"]) + + assert db_get_album("album-1")[7] > self.EARLIER + + def test_a_failed_removal_leaves_it_alone(self, test_db): + """The image was never in the album, so nothing about it changed.""" + make_album("album-1", "Trip") + self.set_updated_at(test_db, "album-1") + + with pytest.raises(ValueError): + db_remove_image_from_album("album-1", "img-missing") + + assert db_get_album("album-1")[7] == self.EARLIER + + class TestCreateAlbumWithImages: def test_creates_the_album_and_links_every_image(self, test_db): make_images(test_db, ["img-1", "img-2"]) diff --git a/docs/backend/backend_python/openapi.json b/docs/backend/backend_python/openapi.json index 3dce9f3e4..8bece365d 100644 --- a/docs/backend/backend_python/openapi.json +++ b/docs/backend/backend_python/openapi.json @@ -2622,6 +2622,28 @@ "type": "integer", "title": "Image Count", "default": 0 + }, + "created_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Created At" + }, + "updated_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Updated At" } }, "type": "object", diff --git a/frontend/src/components/GallerySortDropdown.tsx b/frontend/src/components/GallerySortDropdown.tsx index 3ae391395..79b1d7ebc 100644 --- a/frontend/src/components/GallerySortDropdown.tsx +++ b/frontend/src/components/GallerySortDropdown.tsx @@ -22,7 +22,7 @@ export interface SortOption { export type GallerySortValue = 'best_match' | 'date'; -const GALLERY_SORT_OPTIONS: SortOption[] = [ +export const GALLERY_SORT_OPTIONS: SortOption[] = [ { value: 'best_match', label: 'Best match', icon: Star }, { value: 'date', label: 'Date', icon: Calendar }, ]; diff --git a/frontend/src/hooks/__tests__/usePersistedSort.test.ts b/frontend/src/hooks/__tests__/usePersistedSort.test.ts new file mode 100644 index 000000000..9a83e2607 --- /dev/null +++ b/frontend/src/hooks/__tests__/usePersistedSort.test.ts @@ -0,0 +1,46 @@ +import { act, renderHook } from '@testing-library/react'; +import { usePersistedSort } from '../usePersistedSort'; + +const KEY = 'test-sort'; +const VALUES = ['name', 'date'] as const; +type TestSort = (typeof VALUES)[number]; + +const renderSort = () => + renderHook(() => usePersistedSort(KEY, 'name', VALUES)); + +describe('usePersistedSort', () => { + beforeEach(() => { + localStorage.clear(); + }); + + it('starts on the default when nothing is stored', () => { + const { result } = renderSort(); + + expect(result.current[0]).toBe('name'); + }); + + it('restores the stored selection', () => { + localStorage.setItem(KEY, 'date'); + + const { result } = renderSort(); + + expect(result.current[0]).toBe('date'); + }); + + it('keeps the selection across a remount', () => { + const first = renderSort(); + act(() => first.result.current[1]('date')); + first.unmount(); + + // A remount stands in for the reload that used to reset the sort. + expect(renderSort().result.current[0]).toBe('date'); + }); + + it('falls back to the default for a sort that no longer exists', () => { + localStorage.setItem(KEY, 'photoCount'); + + const { result } = renderSort(); + + expect(result.current[0]).toBe('name'); + }); +}); diff --git a/frontend/src/hooks/usePersistedSort.ts b/frontend/src/hooks/usePersistedSort.ts new file mode 100644 index 000000000..4ea13eae3 --- /dev/null +++ b/frontend/src/hooks/usePersistedSort.ts @@ -0,0 +1,31 @@ +import { useCallback, useState } from 'react'; + +/** + * A sort selection that survives a reload, stored per surface like the theme. + * + * The stored value is checked against the options currently on offer: one left + * behind by an older build would otherwise sort the grid by nothing, with no + * option showing as selected and no way to tell why. + */ +export function usePersistedSort( + storageKey: string, + defaultValue: T, + allowedValues: readonly T[], +): [T, (value: T) => void] { + const [sort, setSort] = useState(() => { + const stored = localStorage.getItem(storageKey) as T | null; + return stored !== null && allowedValues.includes(stored) + ? stored + : defaultValue; + }); + + const selectSort = useCallback( + (value: T) => { + setSort(value); + localStorage.setItem(storageKey, value); + }, + [storageKey], + ); + + return [sort, selectSort]; +} diff --git a/frontend/src/pages/AITagging/AITagging.tsx b/frontend/src/pages/AITagging/AITagging.tsx index ac4851039..43583a261 100644 --- a/frontend/src/pages/AITagging/AITagging.tsx +++ b/frontend/src/pages/AITagging/AITagging.tsx @@ -18,13 +18,27 @@ import { Badge } from '@/components/ui/badge'; import { Button } from '@/components/ui/button'; import { formatPeopleTitle } from '@/utils/personUtils'; import { RankedGallery } from '@/components/Media/RankedGallery'; -import { GallerySortDropdown } from '@/components/GallerySortDropdown'; +import { + GALLERY_SORT_OPTIONS, + GallerySortDropdown, + type GallerySortValue, +} from '@/components/GallerySortDropdown'; +import { usePersistedSort } from '@/hooks/usePersistedSort'; + +const AI_TAGGING_SORT_STORAGE_KEY = 'pictopy-ai-tagging-sort'; + +// Derived from the options above so a removed sort stops being restorable. +const GALLERY_SORT_VALUES = GALLERY_SORT_OPTIONS.map((option) => option.value); export const AITagging = () => { const dispatch = useDispatch(); const scrollableRef = useRef(null); const [monthMarkers, setMonthMarkers] = useState([]); - const [sortMode, setSortMode] = useState<'best_match' | 'date'>('best_match'); + const [sortMode, setSortMode] = usePersistedSort( + AI_TAGGING_SORT_STORAGE_KEY, + 'best_match', + GALLERY_SORT_VALUES, + ); const [searchState, setSearchState] = useState<{ active: boolean; peopleNames: string[]; diff --git a/frontend/src/pages/Album/Album.tsx b/frontend/src/pages/Album/Album.tsx index 7c7fd9047..57497e36b 100644 --- a/frontend/src/pages/Album/Album.tsx +++ b/frontend/src/pages/Album/Album.tsx @@ -2,7 +2,14 @@ import { useEffect, useState } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { useNavigate } from 'react-router'; import { Button } from '@/components/ui/button'; -import { Plus, RefreshCw, ArrowDownAZ, Images } from 'lucide-react'; +import { + Plus, + RefreshCw, + ArrowDownAZ, + CalendarClock, + History, + Images, +} from 'lucide-react'; import { AlbumCard } from '@/components/Albums/AlbumCard'; import { CreateAlbumDialog } from '@/components/Albums/CreateAlbumDialog'; import { EditAlbumDialog } from '@/components/Albums/EditAlbumDialog'; @@ -15,6 +22,7 @@ import { setAlbums } from '@/features/albumsSlice'; import { selectAlbums } from '@/features/albumSelectors'; import { showInfoDialog } from '@/features/infoDialogSlice'; import { useMutationFeedback } from '@/hooks/useMutationFeedback'; +import { usePersistedSort } from '@/hooks/usePersistedSort'; import { MEDIA_GRID_CLASS } from '@/constants/layout'; import { cn } from '@/lib/utils'; import { Album } from '@/types/Album'; @@ -23,13 +31,28 @@ import { type SortOption, } from '@/components/GallerySortDropdown'; -type AlbumSortValue = 'name' | 'photoCount'; +type AlbumSortValue = 'name' | 'photoCount' | 'dateCreated' | 'recentlyUpdated'; const ALBUM_SORT_OPTIONS: SortOption[] = [ { value: 'name', label: 'Name (A-Z)', icon: ArrowDownAZ }, { value: 'photoCount', label: 'Photo Count', icon: Images }, + { value: 'dateCreated', label: 'Date Created', icon: CalendarClock }, + { value: 'recentlyUpdated', label: 'Recently Updated', icon: History }, ]; +const ALBUM_SORT_STORAGE_KEY = 'pictopy-albums-sort'; + +// Derived from the options above so a removed sort stops being restorable. +const ALBUM_SORT_VALUES = ALBUM_SORT_OPTIONS.map((option) => option.value); + +/** + * Newest first. SQLite timestamps are zero-padded, so they compare correctly + * as strings. Albums predating these columns have no timestamp: they read as + * oldest and keep the insertion order the backend lists them in. + */ +const newestFirst = (a: string | null, b: string | null): number => + (b ?? '').localeCompare(a ?? ''); + // Mirrors an AlbumCard: the same 4/5 cover as a memory tile, plus the name and // count bars, so the grid does not jump when the real cards arrive. const AlbumCardSkeleton: React.FC = () => ( @@ -53,7 +76,11 @@ function Albums() { const [isPasswordDialogOpen, setIsPasswordDialogOpen] = useState(false); const [albumToDelete, setAlbumToDelete] = useState(null); const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false); - const [sortBy, setSortBy] = useState('name'); + const [sortBy, setSortBy] = usePersistedSort( + ALBUM_SORT_STORAGE_KEY, + 'name', + ALBUM_SORT_VALUES, + ); const { data: albumsData, @@ -100,6 +127,8 @@ function Albums() { is_locked: Boolean(album.is_locked), cover_image_path: album.cover_image_path, image_count: album.image_count || 0, + created_at: album.created_at ?? null, + updated_at: album.updated_at ?? null, })) as Album[]; dispatch(setAlbums(albumsList)); } @@ -159,6 +188,10 @@ function Albums() { return a.name.localeCompare(b.name); } else if (sortBy === 'photoCount') { return b.image_count - a.image_count; + } else if (sortBy === 'dateCreated') { + return newestFirst(a.created_at, b.created_at); + } else if (sortBy === 'recentlyUpdated') { + return newestFirst(a.updated_at, b.updated_at); } return 0; }); diff --git a/frontend/src/pages/Album/AlbumDetail.tsx b/frontend/src/pages/Album/AlbumDetail.tsx index b8ba7fae4..c65e1e856 100644 --- a/frontend/src/pages/Album/AlbumDetail.tsx +++ b/frontend/src/pages/Album/AlbumDetail.tsx @@ -128,6 +128,8 @@ export const AlbumDetail = () => { is_locked: backendAlbum.is_locked || false, cover_image_path: backendAlbum.cover_image_path, image_count: backendAlbum.image_count || 0, + created_at: backendAlbum.created_at ?? null, + updated_at: backendAlbum.updated_at ?? null, }; dispatch(setSelectedAlbum(albumInfo)); } diff --git a/frontend/src/pages/__tests__/Album.test.tsx b/frontend/src/pages/__tests__/Album.test.tsx index ac2ea1d8e..5bb5d222d 100644 --- a/frontend/src/pages/__tests__/Album.test.tsx +++ b/frontend/src/pages/__tests__/Album.test.tsx @@ -129,6 +129,68 @@ describe('Albums page', () => { expect(cover.getAttribute('src')).toMatch(/placeholder-album/); }, 30000); + const chooseSort = async ( + user: ReturnType, + label: RegExp, + ) => { + await user.click(screen.getByRole('button', { name: /sort by/i })); + const menuItems = await screen.findAllByRole('menuitem'); + const option = menuItems.find((item) => label.test(item.textContent || '')); + await user.click(option as HTMLElement); + }; + + const albumOrder = () => + screen.getAllByRole('heading', { level: 3 }).map((h) => h.textContent); + + // 'Legacy' predates both timestamp columns, so it carries neither. + const timestampedAlbums = [ + { album_id: 'a1', album_name: 'Legacy', image_count: 1 }, + { + album_id: 'a2', + album_name: 'MadeFirst', + image_count: 1, + created_at: '2026-07-01 10:00:00', + updated_at: '2026-08-09 10:00:00', + }, + { + album_id: 'a3', + album_name: 'MadeLast', + image_count: 1, + created_at: '2026-08-01 10:00:00', + updated_at: '2026-08-02 10:00:00', + }, + ]; + + test('sorts by creation date, newest first', async () => { + const user = userEvent.setup(); + serverAlbums = timestampedAlbums; + + render(); + await screen.findByText('Legacy'); + + await chooseSort(user, /date created/i); + + await waitFor(() => + expect(albumOrder()).toEqual(['MadeLast', 'MadeFirst', 'Legacy']), + ); + }, 30000); + + // The album made first was edited most recently, so the two date sorts must + // not agree - otherwise this would pass against either field. + test('sorts by last update, most recent first', async () => { + const user = userEvent.setup(); + serverAlbums = timestampedAlbums; + + render(); + await screen.findByText('Legacy'); + + await chooseSort(user, /recently updated/i); + + await waitFor(() => + expect(albumOrder()).toEqual(['MadeFirst', 'MadeLast', 'Legacy']), + ); + }, 30000); + test('shows skeletons while loading instead of a blocking loader', async () => { let releaseAlbums: (value: unknown) => void = () => {}; mockGetAllAlbums.mockImplementation( diff --git a/frontend/src/types/Album.ts b/frontend/src/types/Album.ts index c83175646..0115d947f 100644 --- a/frontend/src/types/Album.ts +++ b/frontend/src/types/Album.ts @@ -5,6 +5,10 @@ export interface Album { is_locked: boolean; cover_image_path?: string; image_count: number; + /** Null for albums that predate the backend recording these times. */ + created_at: string | null; + /** Touched by metadata edits and by adding or removing photos. */ + updated_at: string | null; } export interface AlbumFormData { From 4a3371ceb4b74e98a62f4ce27fdda131f05f9ab1 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:19:38 +0530 Subject: [PATCH 07/10] refactor: address review feedback on the album changes Album reads now return an AlbumRow TypedDict instead of positional tuples, so the routes look columns up by name rather than by index. The from-memory route declares its error responses and return type, and its request rejects names and ids that are blank once trimmed. Adding images that are already in an album, or removing ones that are not, no longer marks the album as updated, matching what the single image removal already did. The convert dialog resets on opening rather than only when the memory changes, so a name typed and abandoned does not come back. --- backend/app/database/albums.py | 67 ++++- backend/app/routes/albums.py | 58 ++-- backend/app/schemas/album.py | 9 + backend/tests/test_albums.py | 248 +++++++++--------- backend/tests/test_albums_db.py | 82 ++++-- docs/backend/backend_python/openapi.json | 63 +++++ .../Memories/ConvertMemoryToAlbumDialog.tsx | 7 +- .../ConvertMemoryToAlbumDialog.test.tsx | 63 +++++ ...Sort.test.ts => usePersistedSort.test.tsx} | 0 9 files changed, 401 insertions(+), 196 deletions(-) create mode 100644 frontend/src/components/Memories/__tests__/ConvertMemoryToAlbumDialog.test.tsx rename frontend/src/hooks/__tests__/{usePersistedSort.test.ts => usePersistedSort.test.tsx} (100%) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 79f0868a4..878f77cb3 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -1,9 +1,45 @@ import sqlite3 +from typing import Any, List, Optional, Tuple, TypedDict + import bcrypt from app.config.settings import DATABASE_PATH from app.database.connection import get_db_connection +class AlbumRow(TypedDict): + """A row of the albums table, as the read helpers below return it.""" + + album_id: str + album_name: str + description: Optional[str] + is_locked: bool + password_hash: Optional[str] + cover_image_path: Optional[str] + created_at: Optional[str] + updated_at: Optional[str] + + +# Named once so the SELECTs and the mapper below cannot drift apart. +_ALBUM_COLUMNS = ( + "album_id, album_name, description, is_locked, " + "password_hash, cover_image_path, created_at, updated_at" +) + + +def _to_album_row(row: Tuple[Any, ...]) -> AlbumRow: + """Map a SELECT of _ALBUM_COLUMNS onto a named record.""" + return AlbumRow( + album_id=row[0], + album_name=row[1], + description=row[2], + is_locked=bool(row[3]), + password_hash=row[4], + cover_image_path=row[5], + created_at=row[6], + updated_at=row[7], + ) + + def db_create_albums_table() -> None: conn = None try: @@ -87,46 +123,43 @@ def _touch_album(cursor: sqlite3.Cursor, album_id: str) -> None: ) -def db_get_all_albums(): +def db_get_all_albums() -> List[AlbumRow]: """Get all albums (both locked and unlocked).""" conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: # Insertion order, so albums predating created_at keep the order they # were made in rather than an arbitrary one. - cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums ORDER BY rowid" - ) - albums = cursor.fetchall() - return albums + cursor.execute(f"SELECT {_ALBUM_COLUMNS} FROM albums ORDER BY rowid") + return [_to_album_row(row) for row in cursor.fetchall()] finally: conn.close() -def db_get_album_by_name(name: str): +def db_get_album_by_name(name: str) -> Optional[AlbumRow]: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums WHERE album_name = ?", + f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_name = ?", (name,), ) album = cursor.fetchone() - return album if album else None + return _to_album_row(album) if album else None finally: conn.close() -def db_get_album(album_id: str): +def db_get_album(album_id: str) -> Optional[AlbumRow]: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: cursor.execute( - "SELECT album_id, album_name, description, is_locked, password_hash, cover_image_path, created_at, updated_at FROM albums WHERE album_id = ?", + f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_id = ?", (album_id,), ) album = cursor.fetchone() - return album if album else None + return _to_album_row(album) if album else None finally: conn.close() @@ -305,7 +338,11 @@ def db_add_images_to_album(album_id: str, image_ids: list[str]): "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", [(album_id, img_id) for img_id in valid_images], ) - _touch_album(cursor, album_id) + # Every id may already be in the album, in which case OR IGNORE writes + # nothing and the album has not actually changed. Read before touching: + # the touch overwrites rowcount. + if cursor.rowcount: + _touch_album(cursor, album_id) conn.commit() @@ -337,7 +374,9 @@ def db_remove_images_from_album(album_id: str, image_ids: list[str]): "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", [(album_id, img_id) for img_id in image_ids], ) - _touch_album(cursor, album_id) + # Same as the insert: ids that were not in the album delete nothing. + if cursor.rowcount: + _touch_album(cursor, album_id) conn.commit() finally: conn.close() diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 2491e97ba..36226336d 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -45,24 +45,24 @@ def get_albums(): album_list = [] for album in albums: # Get image count for each album - image_ids = db_get_album_images(album[0]) + image_ids = db_get_album_images(album["album_id"]) image_count = len(image_ids) - is_locked = bool(album[3]) + is_locked = album["is_locked"] album_list.append( Album( - album_id=album[0], - album_name=album[1], - description=album[2] or "", + album_id=album["album_id"], + album_name=album["album_name"], + description=album["description"] or "", is_locked=is_locked, # A locked album's cover would show the very content the # password is protecting, so never send it. cover_image_path=( - None if is_locked else db_get_album_cover_path(album[0]) + None if is_locked else db_get_album_cover_path(album["album_id"]) ), image_count=image_count, - created_at=album[6], - updated_at=album[7], + created_at=album["created_at"], + updated_at=album["updated_at"], ) ) return GetAlbumsResponse(success=True, albums=album_list) @@ -100,8 +100,14 @@ def create_album(body: CreateAlbumRequest): # POST /albums/from-memory - Create an album from a curated memory -@router.post("/from-memory", response_model=CreateAlbumFromMemoryResponse) -def create_album_from_memory(body: CreateAlbumFromMemoryRequest = Body(...)): +@router.post( + "/from-memory", + response_model=CreateAlbumFromMemoryResponse, + responses={code: {"model": ErrorResponse} for code in [400, 404, 409, 500]}, +) +def create_album_from_memory( + body: CreateAlbumFromMemoryRequest, +) -> CreateAlbumFromMemoryResponse: """ Copy a memory's photos into a new album. @@ -200,17 +206,17 @@ def get_album(album_id: str = Path(...)): image_ids = db_get_album_images(album_id) image_count = len(image_ids) - is_locked = bool(album[3]) + is_locked = album["is_locked"] album_obj = Album( - album_id=album[0], - album_name=album[1], - description=album[2] or "", + album_id=album["album_id"], + album_name=album["album_name"], + description=album["description"] or "", is_locked=is_locked, # Same reasoning as the listing: the cover gives away the contents. cover_image_path=(None if is_locked else db_get_album_cover_path(album_id)), image_count=image_count, - created_at=album[6], - updated_at=album[7], + created_at=album["created_at"], + updated_at=album["updated_at"], ) return GetAlbumResponse(success=True, data=album_obj) except Exception as e: @@ -238,15 +244,7 @@ def update_album(album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) ).model_dump(), ) - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_locked": bool(album[3]), - "password_hash": album[4], - } - - if album_dict["is_locked"]: + if album["is_locked"]: if not body.current_password: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, @@ -326,15 +324,7 @@ def get_album_images( ).model_dump(), ) - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_locked": bool(album[3]), - "password_hash": album[4], - } - - if album_dict["is_locked"]: + if album["is_locked"]: if not body.password: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, diff --git a/backend/app/schemas/album.py b/backend/app/schemas/album.py index f9f0cb37f..c5cf7f558 100644 --- a/backend/app/schemas/album.py +++ b/backend/app/schemas/album.py @@ -38,6 +38,15 @@ class CreateAlbumFromMemoryRequest(BaseModel): memory_id: str = Field(..., min_length=1) name: str = Field(..., min_length=1) + @field_validator("memory_id", "name") + def check_not_blank(cls, value: str) -> str: + # min_length counts the spaces, so " " would otherwise get through + # and create an album with a blank name. + cleaned = value.strip() + if not cleaned: + raise ValueError("must not be blank") + return cleaned + class UpdateAlbumRequest(BaseModel): name: str diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index 5f5dc59b3..2b20da30c 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -20,6 +20,20 @@ # ############################## +def album_row(album: dict, cover_image_path=None, created_at=None, updated_at=None): + """An albums row as the database helpers return it.""" + return { + "album_id": album["album_id"], + "album_name": album["album_name"], + "description": album["description"], + "is_locked": album["is_locked"], + "password_hash": album["password_hash"], + "cover_image_path": cover_image_path, + "created_at": created_at, + "updated_at": updated_at, + } + + @pytest.fixture def mock_db_album(): return { @@ -114,12 +128,14 @@ def test_create_album_duplicate_name(self): } with patch("app.routes.albums.db_get_album_by_name") as mock_get_by_name: - mock_get_by_name.return_value = ( - "existing-id", - "Existing Album", - "desc", - 0, - None, + mock_get_by_name.return_value = album_row( + { + "album_id": "existing-id", + "album_name": "Existing Album", + "description": "desc", + "is_locked": False, + "password_hash": None, + } ) response = client.post("/albums/", json=album_data) @@ -135,15 +151,10 @@ def test_get_all_albums_public_only(self, mock_db_album): """ with patch("app.routes.albums.db_get_all_albums") as mock_get_all: mock_get_all.return_value = [ - ( - mock_db_album["album_id"], - mock_db_album["album_name"], - mock_db_album["description"], - mock_db_album["is_locked"], - mock_db_album["password_hash"], - None, # cover_image_path - "2026-08-01 09:00:00", # created_at - "2026-08-05 09:00:00", # updated_at + album_row( + mock_db_album, + created_at="2026-08-01 09:00:00", + updated_at="2026-08-05 09:00:00", ) ] @@ -172,25 +183,15 @@ def test_get_all_albums_include_hidden(self, mock_db_album, mock_db_locked_album """ with patch("app.routes.albums.db_get_all_albums") as mock_get_all: mock_get_all.return_value = [ - ( - mock_db_album["album_id"], - mock_db_album["album_name"], - mock_db_album["description"], - mock_db_album["is_locked"], - mock_db_album["password_hash"], - None, # cover_image_path - "2026-08-01 09:00:00", # created_at - "2026-08-05 09:00:00", # updated_at + album_row( + mock_db_album, + created_at="2026-08-01 09:00:00", + updated_at="2026-08-05 09:00:00", ), - ( - mock_db_locked_album["album_id"], - mock_db_locked_album["album_name"], - mock_db_locked_album["description"], - mock_db_locked_album["is_locked"], - mock_db_locked_album["password_hash"], - None, # cover_image_path - "2026-08-02 09:00:00", # created_at - "2026-08-06 09:00:00", # updated_at + album_row( + mock_db_locked_album, + created_at="2026-08-02 09:00:00", + updated_at="2026-08-06 09:00:00", ), ] @@ -214,25 +215,15 @@ def test_locked_album_cover_is_withheld(self, mock_db_album, mock_db_locked_albu "app.routes.albums.db_get_album_cover_path" ) as mock_cover: mock_get_all.return_value = [ - ( - mock_db_album["album_id"], - mock_db_album["album_name"], - mock_db_album["description"], - mock_db_album["is_locked"], - mock_db_album["password_hash"], - None, - "2026-08-01 09:00:00", - "2026-08-05 09:00:00", + album_row( + mock_db_album, + created_at="2026-08-01 09:00:00", + updated_at="2026-08-05 09:00:00", ), - ( - mock_db_locked_album["album_id"], - mock_db_locked_album["album_name"], - mock_db_locked_album["description"], - mock_db_locked_album["is_locked"], - mock_db_locked_album["password_hash"], - None, - "2026-08-02 09:00:00", - "2026-08-06 09:00:00", + album_row( + mock_db_locked_album, + created_at="2026-08-02 09:00:00", + updated_at="2026-08-06 09:00:00", ), ] mock_cover.return_value = "/photos/secret.jpg" @@ -254,15 +245,10 @@ def test_get_album_by_id_withholds_a_locked_cover(self, mock_db_locked_album): with patch("app.routes.albums.db_get_album") as mock_get_album, patch( "app.routes.albums.db_get_album_cover_path" ) as mock_cover: - mock_get_album.return_value = ( - mock_db_locked_album["album_id"], - mock_db_locked_album["album_name"], - mock_db_locked_album["description"], - mock_db_locked_album["is_locked"], - mock_db_locked_album["password_hash"], - None, - "2026-08-02 09:00:00", - "2026-08-06 09:00:00", + mock_get_album.return_value = album_row( + mock_db_locked_album, + created_at="2026-08-02 09:00:00", + updated_at="2026-08-06 09:00:00", ) mock_cover.return_value = "/photos/secret.jpg" @@ -292,15 +278,10 @@ def test_get_album_by_id_success(self, mock_db_album): Test fetching a single album by its ID successfully. """ with patch("app.routes.albums.db_get_album") as mock_get_album: - mock_get_album.return_value = ( - mock_db_album["album_id"], - mock_db_album["album_name"], - mock_db_album["description"], - mock_db_album["is_locked"], - mock_db_album["password_hash"], - None, # cover_image_path - "2026-08-01 09:00:00", # created_at - "2026-08-05 09:00:00", # updated_at + mock_get_album.return_value = album_row( + mock_db_album, + created_at="2026-08-01 09:00:00", + updated_at="2026-08-05 09:00:00", ) response = client.get(f"/albums/{mock_db_album['album_id']}") @@ -337,7 +318,15 @@ def test_get_album_by_id_not_found(self): [ # Case 1: Public album (no password protection) ( - ("abc-123", "Old Name", "Old Desc", 0, None, 0), + album_row( + { + "album_id": "abc-123", + "album_name": "Old Name", + "description": "Old Desc", + "is_locked": False, + "password_hash": None, + } + ), { "name": "Updated Public Album", "description": "Updated description", @@ -350,13 +339,16 @@ def test_get_album_by_id_not_found(self): ), # Case 2: Locked album with correct current password ( - ( - "abc-456", - "Locked Album", - "Secret", - 1, - bcrypt.hashpw("oldpass".encode(), bcrypt.gensalt()).decode(), - 0, + album_row( + { + "album_id": "abc-456", + "album_name": "Locked Album", + "description": "Secret", + "is_locked": True, + "password_hash": bcrypt.hashpw( + "oldpass".encode(), bcrypt.gensalt() + ).decode(), + } ), { "name": "Updated Locked Album", @@ -370,13 +362,16 @@ def test_get_album_by_id_not_found(self): ), # Case 3: Locked album with incorrect current password ( - ( - "abc-789", - "Locked Album", - "Secret", - 1, - bcrypt.hashpw("correctpass".encode(), bcrypt.gensalt()).decode(), - 0, + album_row( + { + "album_id": "abc-789", + "album_name": "Locked Album", + "description": "Secret", + "is_locked": True, + "password_hash": bcrypt.hashpw( + "correctpass".encode(), bcrypt.gensalt() + ).decode(), + } ), { "name": "Invalid Attempt", @@ -401,7 +396,9 @@ def test_update_album( mock_get_album.return_value = album_data mock_verify.return_value = verify_password_return - response = client.put(f"/albums/{album_data[0]}", json=request_data) + response = client.put( + f"/albums/{album_data['album_id']}", json=request_data + ) assert response.status_code == expected_status if expected_status == 200: @@ -416,14 +413,7 @@ def test_delete_album_success(self, mock_db_album): Test successfully deleting an existing album. """ album_id = mock_db_album["album_id"] - album_tuple = ( - album_id, - mock_db_album["album_name"], - mock_db_album["description"], - int(mock_db_album["is_locked"]), - mock_db_album["password_hash"], - 0, # image_count - ) + album_tuple = album_row(mock_db_album) with patch("app.routes.albums.db_get_album") as mock_get_album, patch( "app.routes.albums.db_delete_album" @@ -458,14 +448,7 @@ def test_add_images_to_album_success(self, mock_db_album): ] } - album_tuple = ( - album_id, - mock_db_album["album_name"], - mock_db_album["description"], - int(mock_db_album["is_locked"]), - mock_db_album["password_hash"], - 0, # image_count - ) + album_tuple = album_row(mock_db_album) with patch("app.routes.albums.db_get_album") as mock_get_album, patch( "app.routes.albums.db_add_images_to_album" @@ -494,14 +477,7 @@ def test_get_album_images_success(self, mock_db_album): "2d4bff29-1111-43a4-9e76-b78504bea999", ] - album_tuple = ( - album_id, - mock_db_album["album_name"], - mock_db_album["description"], - int(mock_db_album["is_locked"]), - mock_db_album["password_hash"], - 0, # image_count - ) + album_tuple = album_row(mock_db_album) with patch("app.routes.albums.db_get_album") as mock_get_album, patch( "app.routes.albums.db_get_album_images" @@ -527,14 +503,7 @@ def test_remove_image_from_album_success(self, mock_db_album): album_id = mock_db_album["album_id"] image_id = "71abff29-27b4-43a4-9e76-b78504bea325" - album_tuple = ( - album_id, - mock_db_album["album_name"], - mock_db_album["description"], - int(mock_db_album["is_locked"]), - mock_db_album["password_hash"], - 0, # image_count - ) + album_tuple = album_row(mock_db_album) with patch("app.routes.albums.db_get_album") as mock_get_album, patch( "app.routes.albums.db_remove_image_from_album" @@ -563,7 +532,7 @@ def test_remove_multiple_images_from_album(self, mock_db_album): with patch("app.routes.albums.db_get_album") as mock_get, patch( "app.routes.albums.db_remove_images_from_album" ) as mock_remove_bulk: - mock_get.return_value = tuple(mock_db_album.values()) + mock_get.return_value = album_row(mock_db_album) response = client.request( "DELETE", f"/albums/{album_id}/images", json=image_ids_to_remove ) @@ -607,10 +576,51 @@ def test_create_album_from_memory_success(self, mock_memory, mock_memory_images) # The memory's subtitle becomes the album description, and only the # image ids are handed over - clips are left behind. album_id, name, description, image_ids = mock_create.call_args.args + # The id that was persisted has to be the one handed back, or the + # client navigates to an album that does not exist. + assert album_id == json_response["data"]["album_id"] assert name == "Paris 2022" assert description == mock_memory["subtitle"] assert image_ids == [image["id"] for image in mock_memory_images] + @pytest.mark.parametrize( + "payload", + [ + {"memory_id": "mem-1", "name": " "}, + {"memory_id": " ", "name": "Paris 2022"}, + ], + ) + def test_create_album_from_memory_rejects_blank_fields(self, payload): + """A name of spaces satisfies min_length but is not a name.""" + with patch("app.routes.albums.db_create_album_with_images") as mock_create: + response = client.post("/albums/from-memory", json=payload) + + assert response.status_code == 422 + mock_create.assert_not_called() + + def test_create_album_from_memory_trims_the_name( + self, mock_memory, mock_memory_images + ): + with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( + "app.routes.albums.db_get_memory_images" + ) as mock_get_images, patch( + "app.routes.albums.db_get_album_by_name" + ) as mock_get_by_name, patch( + "app.routes.albums.db_create_album_with_images" + ) as mock_create: + mock_get_memory.return_value = mock_memory + mock_get_images.return_value = mock_memory_images + mock_get_by_name.return_value = None + mock_create.return_value = len(mock_memory_images) + + response = client.post( + "/albums/from-memory", + json={"memory_id": mock_memory["memory_id"], "name": " Paris "}, + ) + + assert response.status_code == 200 + assert mock_create.call_args.args[1] == "Paris" + def test_create_album_from_memory_not_found(self): with patch("app.routes.albums.db_get_memory") as mock_get_memory: mock_get_memory.return_value = None @@ -652,7 +662,7 @@ def test_create_album_from_memory_duplicate_name( ) as mock_create: mock_get_memory.return_value = mock_memory mock_get_images.return_value = mock_memory_images - mock_get_by_name.return_value = tuple(mock_db_album.values()) + mock_get_by_name.return_value = album_row(mock_db_album) response = client.post( "/albums/from-memory", diff --git a/backend/tests/test_albums_db.py b/backend/tests/test_albums_db.py index ff4ded48a..f3db472e0 100644 --- a/backend/tests/test_albums_db.py +++ b/backend/tests/test_albums_db.py @@ -152,7 +152,10 @@ def test_migrates_a_legacy_is_hidden_schema(self, test_db): assert "is_hidden" not in columns assert {"is_locked", "cover_image_path"} <= set(columns) # the legacy row survives, its flag carried over under the new name - assert [row[1:4] for row in db_get_all_albums()] == [("Old", None, 1)] + legacy = db_get_all_albums() + assert [ + (row["album_name"], row["description"], row["is_locked"]) for row in legacy + ] == [("Old", None, True)] @pytest.mark.parametrize( "create_table", [db_create_albums_table, db_create_album_images_table] @@ -184,7 +187,9 @@ def test_insert_then_fetch_by_id_and_name(self, test_db): make_album("album-1", "Summer Trip", "Fun times") by_id = db_get_album("album-1") - assert by_id[:3] == ("album-1", "Summer Trip", "Fun times") + assert by_id["album_id"] == "album-1" + assert by_id["album_name"] == "Summer Trip" + assert by_id["description"] == "Fun times" assert db_get_album_by_name("Summer Trip") == by_id def test_missing_album_returns_none(self, test_db): @@ -203,8 +208,11 @@ def test_get_all_albums_returns_locked_albums_too(self, test_db): make_album("album-2", "Secret", locked=True, password="pw") rows = db_get_all_albums() - assert sorted(row[1] for row in rows) == ["Public", "Secret"] - assert {row[1]: row[3] for row in rows} == {"Public": 0, "Secret": 1} + assert sorted(row["album_name"] for row in rows) == ["Public", "Secret"] + assert {row["album_name"]: row["is_locked"] for row in rows} == { + "Public": False, + "Secret": True, + } def test_update_changes_fields(self, test_db): make_album("album-1", "Old", "Old desc") @@ -212,9 +220,9 @@ def test_update_changes_fields(self, test_db): db_update_album("album-1", "New", "New desc", True, None) album = db_get_album("album-1") - assert album[1] == "New" - assert album[2] == "New desc" - assert album[3] == 1 + assert album["album_name"] == "New" + assert album["description"] == "New desc" + assert album["is_locked"] is True def test_delete_removes_the_row(self, test_db): make_album("album-1", "Trip") @@ -251,35 +259,38 @@ class TestAlbumCreatedAt: def test_insert_stamps_a_creation_time(self, test_db): make_album("album-1", "Trip") - assert db_get_album("album-1")[6] is not None + assert db_get_album("album-1")["created_at"] is not None def test_create_with_images_stamps_a_creation_time(self, test_db): make_images(test_db, ["img-1"]) db_create_album_with_images("album-1", "Paris", "", ["img-1"]) - assert db_get_album("album-1")[6] is not None + assert db_get_album("album-1")["created_at"] is not None def test_edits_never_change_it(self, test_db): """Renaming, re-describing or adding photos is not a new creation.""" make_album("album-1", "Trip") make_images(test_db, ["img-1"]) - created = db_get_album("album-1")[6] + created = db_get_album("album-1")["created_at"] db_update_album("album-1", "Trip Renamed", "A new description", False) db_add_images_to_album("album-1", ["img-1"]) album = db_get_album("album-1") - assert album[6] == created + assert album["created_at"] == created # ...but all of that does count as an update. - assert album[7] >= created + assert album["updated_at"] >= created def test_listing_keeps_insertion_order(self, test_db): """Albums predating created_at sort by when they were made.""" make_album("album-1", "First") make_album("album-2", "Second") - assert [album[0] for album in db_get_all_albums()] == ["album-1", "album-2"] + assert [album["album_id"] for album in db_get_all_albums()] == [ + "album-1", + "album-2", + ] def test_migration_adds_the_column_to_a_legacy_table(self, test_db): """A database shipped before created_at existed still upgrades.""" @@ -305,12 +316,12 @@ def test_migration_adds_the_column_to_a_legacy_table(self, test_db): db_create_albums_table() old = db_get_album("old-1") - assert old[1] == "Old" + assert old["album_name"] == "Old" # Nothing to backfill it with, so it reads as oldest. - assert old[6] is None + assert old["created_at"] is None # And the table still takes new albums. make_album("album-1", "New") - assert db_get_album("album-1")[6] is not None + assert db_get_album("album-1")["created_at"] is not None class TestAlbumUpdatedAt: @@ -334,7 +345,7 @@ def set_updated_at(self, db_path: str, album_id: str) -> None: def test_insert_stamps_an_update_time(self, test_db): make_album("album-1", "Trip") - assert db_get_album("album-1")[7] is not None + assert db_get_album("album-1")["updated_at"] is not None def test_editing_the_album_touches_it(self, test_db): make_album("album-1", "Trip") @@ -342,7 +353,7 @@ def test_editing_the_album_touches_it(self, test_db): db_update_album("album-1", "Trip Renamed", "", False) - assert db_get_album("album-1")[7] > self.EARLIER + assert db_get_album("album-1")["updated_at"] > self.EARLIER def test_adding_images_touches_it(self, test_db): """Adding photos is the commonest way an album changes.""" @@ -352,7 +363,7 @@ def test_adding_images_touches_it(self, test_db): db_add_images_to_album("album-1", ["img-1"]) - assert db_get_album("album-1")[7] > self.EARLIER + assert db_get_album("album-1")["updated_at"] > self.EARLIER def test_removing_an_image_touches_it(self, test_db): make_album("album-1", "Trip") @@ -362,7 +373,7 @@ def test_removing_an_image_touches_it(self, test_db): db_remove_image_from_album("album-1", "img-1") - assert db_get_album("album-1")[7] > self.EARLIER + assert db_get_album("album-1")["updated_at"] > self.EARLIER def test_removing_images_in_bulk_touches_it(self, test_db): make_album("album-1", "Trip") @@ -372,7 +383,26 @@ def test_removing_images_in_bulk_touches_it(self, test_db): db_remove_images_from_album("album-1", ["img-1"]) - assert db_get_album("album-1")[7] > self.EARLIER + assert db_get_album("album-1")["updated_at"] > self.EARLIER + + def test_re_adding_the_same_images_leaves_it_alone(self, test_db): + """Nothing was inserted, so nothing about the album changed.""" + make_album("album-1", "Trip") + make_images(test_db, ["img-1"]) + db_add_images_to_album("album-1", ["img-1"]) + self.set_updated_at(test_db, "album-1") + + db_add_images_to_album("album-1", ["img-1"]) + + assert db_get_album("album-1")["updated_at"] == self.EARLIER + + def test_bulk_removing_absent_images_leaves_it_alone(self, test_db): + make_album("album-1", "Trip") + self.set_updated_at(test_db, "album-1") + + db_remove_images_from_album("album-1", ["img-missing"]) + + assert db_get_album("album-1")["updated_at"] == self.EARLIER def test_a_failed_removal_leaves_it_alone(self, test_db): """The image was never in the album, so nothing about it changed.""" @@ -382,7 +412,7 @@ def test_a_failed_removal_leaves_it_alone(self, test_db): with pytest.raises(ValueError): db_remove_image_from_album("album-1", "img-missing") - assert db_get_album("album-1")[7] == self.EARLIER + assert db_get_album("album-1")["updated_at"] == self.EARLIER class TestCreateAlbumWithImages: @@ -396,11 +426,11 @@ def test_creates_the_album_and_links_every_image(self, test_db): assert linked == 2 assert db_get_album_images("album-1") == ["img-1", "img-2"] album = db_get_album("album-1") - assert album[1] == "Paris 2022" - assert album[2] == "July 2022" + assert album["album_name"] == "Paris 2022" + assert album["description"] == "July 2022" # Always an open album: locking is done afterwards, from Edit Album. - assert bool(album[3]) is False - assert album[4] is None + assert album["is_locked"] is False + assert album["password_hash"] is None def test_an_unknown_image_leaves_no_album_behind(self, test_db): """The album and its links commit together, or not at all.""" diff --git a/docs/backend/backend_python/openapi.json b/docs/backend/backend_python/openapi.json index 8bece365d..2a7756c38 100644 --- a/docs/backend/backend_python/openapi.json +++ b/docs/backend/backend_python/openapi.json @@ -496,6 +496,46 @@ } } }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + } + } + } + }, + "404": { + "description": "Not Found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + } + } + } + }, "422": { "description": "Validation Error", "content": { @@ -6010,6 +6050,29 @@ ], "title": "ToggleFavouriteRequest" }, + "app__schemas__album__ErrorResponse": { + "properties": { + "success": { + "type": "boolean", + "title": "Success", + "default": false + }, + "message": { + "type": "string", + "title": "Message" + }, + "error": { + "type": "string", + "title": "Error" + } + }, + "type": "object", + "required": [ + "message", + "error" + ], + "title": "ErrorResponse" + }, "app__schemas__face_clusters__ErrorResponse": { "properties": { "success": { diff --git a/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx b/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx index 5aa06a295..2d091406e 100644 --- a/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx +++ b/frontend/src/components/Memories/ConvertMemoryToAlbumDialog.tsx @@ -36,13 +36,14 @@ export const ConvertMemoryToAlbumDialog: React.FC< const [error, setError] = useState(''); // One mounted dialog serves every tile, so the name follows the selected - // memory rather than the first render. + // memory rather than the first render. Keyed on opening as well, so a name + // typed and then abandoned does not come back the next time it opens. useEffect(() => { - if (memory) { + if (isOpen && memory) { setName(memory.title); setError(''); } - }, [memory]); + }, [isOpen, memory]); const convertMutation = usePictoMutation({ mutationFn: createAlbumFromMemory, diff --git a/frontend/src/components/Memories/__tests__/ConvertMemoryToAlbumDialog.test.tsx b/frontend/src/components/Memories/__tests__/ConvertMemoryToAlbumDialog.test.tsx new file mode 100644 index 000000000..261ba4d18 --- /dev/null +++ b/frontend/src/components/Memories/__tests__/ConvertMemoryToAlbumDialog.test.tsx @@ -0,0 +1,63 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen } from '@/test-utils'; +import type { MemoryCard } from '@/api/api-functions/memories'; +import { ConvertMemoryToAlbumDialog } from '../ConvertMemoryToAlbumDialog'; + +jest.mock('@/api/api-functions', () => ({ + createAlbumFromMemory: jest.fn(), +})); + +const memory = { + memory_id: 'mem-1', + title: 'Beach', + subtitle: '26 July 2024', + image_count: 4, + video_count: 0, +} as MemoryCard; + +const renderDialog = (isOpen: boolean) => + render( + , + ); + +const nameInput = () => screen.getByLabelText(/album name/i); + +describe('ConvertMemoryToAlbumDialog', () => { + it('prefills the name from the memory title', () => { + renderDialog(true); + + expect(nameInput()).toHaveValue('Beach'); + }); + + // The parent happens to null the memory between opens, but the dialog must + // not depend on that: reopening for the same memory has to start clean. + it('clears a typed name when reopened for the same memory', async () => { + const user = userEvent.setup(); + const { rerender } = renderDialog(true); + + await user.clear(nameInput()); + await user.type(nameInput(), 'Something else'); + expect(nameInput()).toHaveValue('Something else'); + + rerender( + , + ); + rerender( + , + ); + + expect(nameInput()).toHaveValue('Beach'); + }); +}); diff --git a/frontend/src/hooks/__tests__/usePersistedSort.test.ts b/frontend/src/hooks/__tests__/usePersistedSort.test.tsx similarity index 100% rename from frontend/src/hooks/__tests__/usePersistedSort.test.ts rename to frontend/src/hooks/__tests__/usePersistedSort.test.tsx From e37efa78b4f700e5b208434d8516e33af0ce5de5 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:53:23 +0530 Subject: [PATCH 08/10] refactor: tighten the album database and route layers All album reads now go through a module-private _connect(), matching the videos module, so the foreign-key pragma is never skipped. The three select statements are built once from the shared column list rather than interpolated at each call site. The from-memory route documents its errors with an envelope model, since HTTPException nests the payload under detail and the generated spec was describing a shape no client receives. Its duplicated conflict and internal-error blocks collapse into two helpers, and the raises inside except blocks now chain the original error. --- backend/app/database/albums.py | 60 ++++++++++-------- backend/app/routes/albums.py | 77 +++++++++--------------- backend/app/schemas/album.py | 11 ++++ backend/tests/test_albums.py | 4 +- docs/backend/backend_python/openapi.json | 33 ++++++---- frontend/src/hooks/usePersistedSort.ts | 8 +-- 6 files changed, 103 insertions(+), 90 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 878f77cb3..16e2e0951 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -19,12 +19,24 @@ class AlbumRow(TypedDict): updated_at: Optional[str] +def _connect() -> sqlite3.Connection: + conn = sqlite3.connect(DATABASE_PATH) + # Ensure ON DELETE CASCADE and other FKs are enforced + conn.execute("PRAGMA foreign_keys = ON") + return conn + + # Named once so the SELECTs and the mapper below cannot drift apart. _ALBUM_COLUMNS = ( "album_id, album_name, description, is_locked, " "password_hash, cover_image_path, created_at, updated_at" ) +# Built once from the column list rather than interpolated at each call site. +_SELECT_ALL_ALBUMS = f"SELECT {_ALBUM_COLUMNS} FROM albums ORDER BY rowid" +_SELECT_ALBUM_BY_NAME = f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_name = ?" +_SELECT_ALBUM_BY_ID = f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_id = ?" + def _to_album_row(row: Tuple[Any, ...]) -> AlbumRow: """Map a SELECT of _ALBUM_COLUMNS onto a named record.""" @@ -43,7 +55,7 @@ def _to_album_row(row: Tuple[Any, ...]) -> AlbumRow: def db_create_albums_table() -> None: conn = None try: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() cursor.execute( """ @@ -85,7 +97,7 @@ def db_create_albums_table() -> None: def db_create_album_images_table() -> None: conn = None try: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() cursor.execute( """ @@ -125,25 +137,22 @@ def _touch_album(cursor: sqlite3.Cursor, album_id: str) -> None: def db_get_all_albums() -> List[AlbumRow]: """Get all albums (both locked and unlocked).""" - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: # Insertion order, so albums predating created_at keep the order they # were made in rather than an arbitrary one. - cursor.execute(f"SELECT {_ALBUM_COLUMNS} FROM albums ORDER BY rowid") + cursor.execute(_SELECT_ALL_ALBUMS) return [_to_album_row(row) for row in cursor.fetchall()] finally: conn.close() def db_get_album_by_name(name: str) -> Optional[AlbumRow]: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: - cursor.execute( - f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_name = ?", - (name,), - ) + cursor.execute(_SELECT_ALBUM_BY_NAME, (name,)) album = cursor.fetchone() return _to_album_row(album) if album else None finally: @@ -151,13 +160,10 @@ def db_get_album_by_name(name: str) -> Optional[AlbumRow]: def db_get_album(album_id: str) -> Optional[AlbumRow]: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: - cursor.execute( - f"SELECT {_ALBUM_COLUMNS} FROM albums WHERE album_id = ?", - (album_id,), - ) + cursor.execute(_SELECT_ALBUM_BY_ID, (album_id,)) album = cursor.fetchone() return _to_album_row(album) if album else None finally: @@ -169,9 +175,9 @@ def db_insert_album( album_name: str, description: str = "", is_locked: bool = False, - password: str = None, + password: Optional[str] = None, ): - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: password_hash = None @@ -183,7 +189,10 @@ def db_insert_album( # database migrated with ALTER TABLE has no default to fall back on. cursor.execute( """ - INSERT INTO albums (album_id, album_name, description, is_locked, password_hash, created_at, updated_at) + INSERT INTO albums ( + album_id, album_name, description, is_locked, + password_hash, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, (album_id, album_name, description, int(is_locked), password_hash), @@ -207,7 +216,10 @@ def db_create_album_with_images( cursor = conn.cursor() cursor.execute( """ - INSERT INTO albums (album_id, album_name, description, is_locked, password_hash, created_at, updated_at) + INSERT INTO albums ( + album_id, album_name, description, is_locked, + password_hash, created_at, updated_at + ) VALUES (?, ?, ?, 0, NULL, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, (album_id, album_name, description), @@ -226,9 +238,9 @@ def db_update_album( album_name: str, description: str, is_locked: bool, - password: str = None, + password: Optional[str] = None, ): - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: if password is not None: @@ -269,7 +281,7 @@ def db_delete_album(album_id: str): def db_get_album_cover_path(album_id: str) -> str | None: """Path of the album's cover: its first image, by insertion order.""" - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: cursor.execute( @@ -290,7 +302,7 @@ def db_get_album_cover_path(album_id: str) -> str | None: def db_get_album_images(album_id: str): - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: cursor.execute( @@ -367,7 +379,7 @@ def db_remove_image_from_album(album_id: str, image_id: str): def db_remove_images_from_album(album_id: str, image_ids: list[str]): - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: cursor.executemany( @@ -383,7 +395,7 @@ def db_remove_images_from_album(album_id: str, image_ids: list[str]): def verify_album_password(album_id: str, password: str) -> bool: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: cursor.execute( diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 36226336d..d6e9f471c 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -8,6 +8,7 @@ CreateAlbumFromMemoryData, CreateAlbumFromMemoryRequest, CreateAlbumFromMemoryResponse, + ErrorResponseEnvelope, GetAlbumResponse, GetAlbumImagesRequest, GetAlbumImagesResponse, @@ -37,6 +38,26 @@ router = APIRouter() +def _album_exists(name: str) -> HTTPException: + return HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=ErrorResponse( + success=False, + error="Album Already Exists", + message=f"Album '{name}' is already in the database.", + ).model_dump(), + ) + + +def _internal_error(message: str) -> HTTPException: + return HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, error="Internal Server Error", message=message + ).model_dump(), + ) + + # GET /albums/ - Get all albums (including locked ones) @router.get("/", response_model=GetAlbumsResponse) def get_albums(): @@ -73,14 +94,7 @@ def get_albums(): def create_album(body: CreateAlbumRequest): existing_album = db_get_album_by_name(body.name) if existing_album: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=ErrorResponse( - success=False, - error="Album Already Exists", - message=f"Album '{body.name}' is already in the database.", - ).model_dump(), - ) + raise _album_exists(body.name) album_id = str(uuid.uuid4()) try: @@ -89,21 +103,14 @@ def create_album(body: CreateAlbumRequest): ) return CreateAlbumResponse(success=True, album_id=album_id) except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Internal Server Error", - message=f"Failed to create album: {str(e)}", - ).model_dump(), - ) + raise _internal_error(f"Failed to create album: {e}") from e # POST /albums/from-memory - Create an album from a curated memory @router.post( "/from-memory", response_model=CreateAlbumFromMemoryResponse, - responses={code: {"model": ErrorResponse} for code in [400, 404, 409, 500]}, + responses={code: {"model": ErrorResponseEnvelope} for code in [400, 404, 409, 500]}, ) def create_album_from_memory( body: CreateAlbumFromMemoryRequest, @@ -138,14 +145,7 @@ def create_album_from_memory( ) if db_get_album_by_name(body.name): - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=ErrorResponse( - success=False, - error="Album Already Exists", - message=f"Album '{body.name}' is already in the database.", - ).model_dump(), - ) + raise _album_exists(body.name) album_id = str(uuid.uuid4()) try: @@ -156,31 +156,10 @@ def create_album_from_memory( # The name check above is not atomic. Re-check rather than assume a # conflict: the same error covers an image that vanished mid-request. if db_get_album_by_name(body.name): - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=ErrorResponse( - success=False, - error="Album Already Exists", - message=f"Album '{body.name}' is already in the database.", - ).model_dump(), - ) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Internal Server Error", - message=f"Failed to create album from memory: {str(e)}", - ).model_dump(), - ) + raise _album_exists(body.name) from e + raise _internal_error(f"Failed to create album from memory: {e}") from e except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Internal Server Error", - message=f"Failed to create album from memory: {str(e)}", - ).model_dump(), - ) + raise _internal_error(f"Failed to create album from memory: {e}") from e return CreateAlbumFromMemoryResponse( success=True, diff --git a/backend/app/schemas/album.py b/backend/app/schemas/album.py index c5cf7f558..9a6c7b471 100644 --- a/backend/app/schemas/album.py +++ b/backend/app/schemas/album.py @@ -135,3 +135,14 @@ class ErrorResponse(BaseModel): success: bool = False message: str error: str + + +class ErrorResponseEnvelope(BaseModel): + """ + How an ErrorResponse actually reaches the client. + + HTTPException nests whatever it is given under `detail`, so documenting + ErrorResponse alone would describe a shape no client ever receives. + """ + + detail: ErrorResponse diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index 2b20da30c..116566279 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -346,7 +346,7 @@ def test_get_album_by_id_not_found(self): "description": "Secret", "is_locked": True, "password_hash": bcrypt.hashpw( - "oldpass".encode(), bcrypt.gensalt() + b"oldpass", bcrypt.gensalt() ).decode(), } ), @@ -369,7 +369,7 @@ def test_get_album_by_id_not_found(self): "description": "Secret", "is_locked": True, "password_hash": bcrypt.hashpw( - "correctpass".encode(), bcrypt.gensalt() + b"correctpass", bcrypt.gensalt() ).decode(), } ), diff --git a/docs/backend/backend_python/openapi.json b/docs/backend/backend_python/openapi.json index 2a7756c38..3cf429f57 100644 --- a/docs/backend/backend_python/openapi.json +++ b/docs/backend/backend_python/openapi.json @@ -501,7 +501,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + "$ref": "#/components/schemas/ErrorResponseEnvelope" } } } @@ -511,7 +511,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + "$ref": "#/components/schemas/ErrorResponseEnvelope" } } } @@ -521,7 +521,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + "$ref": "#/components/schemas/ErrorResponseEnvelope" } } } @@ -531,7 +531,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + "$ref": "#/components/schemas/ErrorResponseEnvelope" } } } @@ -1597,14 +1597,9 @@ "in": "query", "required": false, "schema": { - "allOf": [ - { - "$ref": "#/components/schemas/InputType" - } - ], + "$ref": "#/components/schemas/InputType", "description": "Choose input type: 'path' or 'base64'", - "default": "path", - "title": "Input Type" + "default": "path" }, "description": "Choose input type: 'path' or 'base64'" } @@ -2975,6 +2970,19 @@ ], "title": "DeleteMemoryResponse" }, + "ErrorResponseEnvelope": { + "properties": { + "detail": { + "$ref": "#/components/schemas/app__schemas__album__ErrorResponse" + } + }, + "type": "object", + "required": [ + "detail" + ], + "title": "ErrorResponseEnvelope", + "description": "How an ErrorResponse actually reaches the client.\n\nHTTPException nests whatever it is given under `detail`, so documenting\nErrorResponse alone would describe a shape no client ever receives." + }, "FaceSearchRequest": { "properties": { "path": { @@ -3808,6 +3816,7 @@ "metadata": { "anyOf": [ { + "additionalProperties": true, "type": "object" }, { @@ -4739,6 +4748,7 @@ "signals": { "anyOf": [ { + "additionalProperties": true, "type": "object" }, { @@ -4966,6 +4976,7 @@ "metadata": { "anyOf": [ { + "additionalProperties": true, "type": "object" }, { diff --git a/frontend/src/hooks/usePersistedSort.ts b/frontend/src/hooks/usePersistedSort.ts index 4ea13eae3..768035470 100644 --- a/frontend/src/hooks/usePersistedSort.ts +++ b/frontend/src/hooks/usePersistedSort.ts @@ -13,10 +13,10 @@ export function usePersistedSort( allowedValues: readonly T[], ): [T, (value: T) => void] { const [sort, setSort] = useState(() => { - const stored = localStorage.getItem(storageKey) as T | null; - return stored !== null && allowedValues.includes(stored) - ? stored - : defaultValue; + // Matching against the allowed values narrows the stored string to T, so + // no cast is needed to trust what came out of storage. + const stored = localStorage.getItem(storageKey); + return allowedValues.find((value) => value === stored) ?? defaultValue; }); const selectSort = useCallback( From 77cbc8ca1fbca060ac2eb936fb1283ed15ad5e78 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:37:09 +0530 Subject: [PATCH 09/10] refactor: move the memory-to-album workflow into the utils layer The route was doing the memory lookup, the image selection, the duplicate-name check and the album creation itself. That work moves to album_util_create_from_memory, which raises a small set of domain errors the route translates into status codes, leaving it to handle HTTP only. Isolating it exposed an untested branch: the insert can still hit the unique constraint after the name check passes, and only a name that is taken on re-check means a conflict. Anything else has to surface rather than be reported as a duplicate. Both paths now have tests. The row helper in the album route tests builds the production AlbumRow instead of a look-alike dict, so a column added to the table fails there rather than drifting. --- backend/app/routes/albums.py | 43 +++++++------------ backend/app/utils/albums.py | 66 ++++++++++++++++++++++++++++ backend/tests/test_album_utils.py | 42 ++++++++++++++++++ backend/tests/test_albums.py | 71 ++++++++++++++++++------------- 4 files changed, 166 insertions(+), 56 deletions(-) create mode 100644 backend/app/utils/albums.py create mode 100644 backend/tests/test_album_utils.py diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index d6e9f471c..fdbed2378 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -1,5 +1,4 @@ from fastapi import APIRouter, HTTPException, status, Body, Path -import sqlite3 import uuid from app.schemas.album import ( GetAlbumsResponse, @@ -23,7 +22,6 @@ db_get_album_by_name, db_get_album, db_insert_album, - db_create_album_with_images, db_update_album, db_delete_album, db_get_album_images, @@ -33,7 +31,12 @@ db_get_album_cover_path, verify_album_password, ) -from app.database.memories import db_get_memory, db_get_memory_images +from app.utils.albums import ( + AlbumNameTakenError, + MemoryHasNoPhotosError, + MemoryNotFoundError, + album_util_create_from_memory, +) router = APIRouter() @@ -122,8 +125,9 @@ def create_album_from_memory( again under a different name. Any clips are left behind: album_images references images, and albums have no video support. """ - memory = db_get_memory(body.memory_id) - if not memory: + try: + result = album_util_create_from_memory(body.memory_id, body.name) + except MemoryNotFoundError as e: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( @@ -131,10 +135,8 @@ def create_album_from_memory( error="Memory Not Found", message="No memory exists with the provided ID.", ).model_dump(), - ) - - image_ids = [image["id"] for image in db_get_memory_images(body.memory_id)] - if not image_ids: + ) from e + except MemoryHasNoPhotosError as e: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( @@ -142,29 +144,16 @@ def create_album_from_memory( error="Empty Memory", message="This memory has no photos to convert.", ).model_dump(), - ) - - if db_get_album_by_name(body.name): - raise _album_exists(body.name) - - album_id = str(uuid.uuid4()) - try: - image_count = db_create_album_with_images( - album_id, body.name, memory.get("subtitle") or "", image_ids - ) - except sqlite3.IntegrityError as e: - # The name check above is not atomic. Re-check rather than assume a - # conflict: the same error covers an image that vanished mid-request. - if db_get_album_by_name(body.name): - raise _album_exists(body.name) from e - raise _internal_error(f"Failed to create album from memory: {e}") from e + ) from e + except AlbumNameTakenError as e: + raise _album_exists(body.name) from e except Exception as e: raise _internal_error(f"Failed to create album from memory: {e}") from e return CreateAlbumFromMemoryResponse( success=True, - message=f"Created album '{body.name}' with {image_count} photos", - data=CreateAlbumFromMemoryData(album_id=album_id, image_count=image_count), + message=f"Created album '{body.name}' with {result['image_count']} photos", + data=CreateAlbumFromMemoryData(**result), ) diff --git a/backend/app/utils/albums.py b/backend/app/utils/albums.py new file mode 100644 index 000000000..fe0e92986 --- /dev/null +++ b/backend/app/utils/albums.py @@ -0,0 +1,66 @@ +"""Album workflows that reach across more than one table.""" + +import sqlite3 +import uuid +from typing import TypedDict + +from app.database.albums import db_create_album_with_images, db_get_album_by_name +from app.database.memories import db_get_memory, db_get_memory_images + + +class AlbumFromMemoryResult(TypedDict): + album_id: str + image_count: int + + +class AlbumFromMemoryError(Exception): + """Base for the ways a conversion can legitimately fail.""" + + +class MemoryNotFoundError(AlbumFromMemoryError): + pass + + +class MemoryHasNoPhotosError(AlbumFromMemoryError): + pass + + +class AlbumNameTakenError(AlbumFromMemoryError): + pass + + +def album_util_create_from_memory(memory_id: str, name: str) -> AlbumFromMemoryResult: + """ + Copy a memory's photos into a new album. + + The memory itself is left untouched, so the same one can be converted + again under a different name. Any clips are left behind: album_images + references images, and albums have no video support. + + Raises AlbumFromMemoryError subclasses for the expected failures, so the + caller decides how to report them. + """ + memory = db_get_memory(memory_id) + if not memory: + raise MemoryNotFoundError(memory_id) + + image_ids = [image["id"] for image in db_get_memory_images(memory_id)] + if not image_ids: + raise MemoryHasNoPhotosError(memory_id) + + if db_get_album_by_name(name): + raise AlbumNameTakenError(name) + + album_id = str(uuid.uuid4()) + try: + image_count = db_create_album_with_images( + album_id, name, memory.get("subtitle") or "", image_ids + ) + except sqlite3.IntegrityError as e: + # The name check above is not atomic. Re-check rather than assume a + # conflict: the same error covers an image that vanished mid-request. + if db_get_album_by_name(name): + raise AlbumNameTakenError(name) from e + raise + + return AlbumFromMemoryResult(album_id=album_id, image_count=image_count) diff --git a/backend/tests/test_album_utils.py b/backend/tests/test_album_utils.py new file mode 100644 index 000000000..d133176e1 --- /dev/null +++ b/backend/tests/test_album_utils.py @@ -0,0 +1,42 @@ +import sqlite3 +from unittest.mock import patch + +import pytest + +from app.utils.albums import ( + AlbumNameTakenError, + album_util_create_from_memory, +) + + +class TestAlbumFromMemoryRace: + """ + The name check is not atomic, so the insert can still hit the UNIQUE + constraint. These cover the branch that decides what that meant. + """ + + def run_with_integrity_error(self, name_taken_on_recheck: bool): + with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( + "app.utils.albums.db_get_memory_images" + ) as mock_get_images, patch( + "app.utils.albums.db_get_album_by_name" + ) as mock_get_by_name, patch( + "app.utils.albums.db_create_album_with_images" + ) as mock_create: + mock_get_memory.return_value = {"memory_id": "mem-1", "subtitle": "July"} + mock_get_images.return_value = [{"id": "img-1"}] + # Free when first checked, then possibly taken by the time it fails. + recheck = {"album_id": "other"} if name_taken_on_recheck else None + mock_get_by_name.side_effect = [None, recheck] + mock_create.side_effect = sqlite3.IntegrityError("UNIQUE constraint failed") + + return album_util_create_from_memory("mem-1", "Paris 2022") + + def test_a_name_taken_mid_request_reads_as_a_conflict(self): + with pytest.raises(AlbumNameTakenError): + self.run_with_integrity_error(name_taken_on_recheck=True) + + def test_any_other_integrity_error_is_not_swallowed(self): + """A vanished image must not be reported as a duplicate name.""" + with pytest.raises(sqlite3.IntegrityError): + self.run_with_integrity_error(name_taken_on_recheck=False) diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index 116566279..89d37f9b5 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -6,6 +6,9 @@ from fastapi.testclient import TestClient from unittest.mock import patch import uuid +from typing import Any, Dict, Optional + +from app.database.albums import AlbumRow from app.routes import albums as albums_router sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -20,18 +23,28 @@ # ############################## -def album_row(album: dict, cover_image_path=None, created_at=None, updated_at=None): - """An albums row as the database helpers return it.""" - return { - "album_id": album["album_id"], - "album_name": album["album_name"], - "description": album["description"], - "is_locked": album["is_locked"], - "password_hash": album["password_hash"], - "cover_image_path": cover_image_path, - "created_at": created_at, - "updated_at": updated_at, - } +def album_row( + album: Dict[str, Any], + cover_image_path: Optional[str] = None, + created_at: Optional[str] = None, + updated_at: Optional[str] = None, +) -> AlbumRow: + """ + An albums row as the database helpers return it. + + Built as the production AlbumRow rather than a look-alike dict, so a + column added there fails here instead of drifting silently. + """ + return AlbumRow( + album_id=album["album_id"], + album_name=album["album_name"], + description=album["description"], + is_locked=album["is_locked"], + password_hash=album["password_hash"], + cover_image_path=cover_image_path, + created_at=created_at, + updated_at=updated_at, + ) @pytest.fixture @@ -550,12 +563,12 @@ class TestCreateAlbumFromMemory: """Test suite for converting a curated memory into an album.""" def test_create_album_from_memory_success(self, mock_memory, mock_memory_images): - with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( - "app.routes.albums.db_get_memory_images" + with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( + "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( - "app.routes.albums.db_get_album_by_name" + "app.utils.albums.db_get_album_by_name" ) as mock_get_by_name, patch( - "app.routes.albums.db_create_album_with_images" + "app.utils.albums.db_create_album_with_images" ) as mock_create: mock_get_memory.return_value = mock_memory mock_get_images.return_value = mock_memory_images @@ -592,7 +605,7 @@ def test_create_album_from_memory_success(self, mock_memory, mock_memory_images) ) def test_create_album_from_memory_rejects_blank_fields(self, payload): """A name of spaces satisfies min_length but is not a name.""" - with patch("app.routes.albums.db_create_album_with_images") as mock_create: + with patch("app.utils.albums.db_create_album_with_images") as mock_create: response = client.post("/albums/from-memory", json=payload) assert response.status_code == 422 @@ -601,12 +614,12 @@ def test_create_album_from_memory_rejects_blank_fields(self, payload): def test_create_album_from_memory_trims_the_name( self, mock_memory, mock_memory_images ): - with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( - "app.routes.albums.db_get_memory_images" + with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( + "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( - "app.routes.albums.db_get_album_by_name" + "app.utils.albums.db_get_album_by_name" ) as mock_get_by_name, patch( - "app.routes.albums.db_create_album_with_images" + "app.utils.albums.db_create_album_with_images" ) as mock_create: mock_get_memory.return_value = mock_memory mock_get_images.return_value = mock_memory_images @@ -622,7 +635,7 @@ def test_create_album_from_memory_trims_the_name( assert mock_create.call_args.args[1] == "Paris" def test_create_album_from_memory_not_found(self): - with patch("app.routes.albums.db_get_memory") as mock_get_memory: + with patch("app.utils.albums.db_get_memory") as mock_get_memory: mock_get_memory.return_value = None response = client.post( @@ -634,10 +647,10 @@ def test_create_album_from_memory_not_found(self): def test_create_album_from_empty_memory(self, mock_memory): """A memory with no photos cannot become an album.""" - with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( - "app.routes.albums.db_get_memory_images" + with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( + "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( - "app.routes.albums.db_create_album_with_images" + "app.utils.albums.db_create_album_with_images" ) as mock_create: mock_get_memory.return_value = mock_memory mock_get_images.return_value = [] @@ -653,12 +666,12 @@ def test_create_album_from_empty_memory(self, mock_memory): def test_create_album_from_memory_duplicate_name( self, mock_memory, mock_memory_images, mock_db_album ): - with patch("app.routes.albums.db_get_memory") as mock_get_memory, patch( - "app.routes.albums.db_get_memory_images" + with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( + "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( - "app.routes.albums.db_get_album_by_name" + "app.utils.albums.db_get_album_by_name" ) as mock_get_by_name, patch( - "app.routes.albums.db_create_album_with_images" + "app.utils.albums.db_create_album_with_images" ) as mock_create: mock_get_memory.return_value = mock_memory mock_get_images.return_value = mock_memory_images From 3ae563aaff138f45d48cbeea189b37e63502d346 Mon Sep 17 00:00:00 2001 From: ROHAN PANDEY <95585299+rohan-pandeyy@users.noreply.github.com> Date: Tue, 4 Aug 2026 23:37:17 +0530 Subject: [PATCH 10/10] test: annotate the album conversion tests Exact fixture parameter types and return annotations on the conversion tests and the two util tests, and the row helper takes a built-in dict generic. --- backend/tests/test_album_utils.py | 9 ++++++--- backend/tests/test_albums.py | 31 +++++++++++++++++++------------ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/backend/tests/test_album_utils.py b/backend/tests/test_album_utils.py index d133176e1..7357f1350 100644 --- a/backend/tests/test_album_utils.py +++ b/backend/tests/test_album_utils.py @@ -4,6 +4,7 @@ import pytest from app.utils.albums import ( + AlbumFromMemoryResult, AlbumNameTakenError, album_util_create_from_memory, ) @@ -15,7 +16,9 @@ class TestAlbumFromMemoryRace: constraint. These cover the branch that decides what that meant. """ - def run_with_integrity_error(self, name_taken_on_recheck: bool): + def run_with_integrity_error( + self, name_taken_on_recheck: bool + ) -> AlbumFromMemoryResult: with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( @@ -32,11 +35,11 @@ def run_with_integrity_error(self, name_taken_on_recheck: bool): return album_util_create_from_memory("mem-1", "Paris 2022") - def test_a_name_taken_mid_request_reads_as_a_conflict(self): + def test_a_name_taken_mid_request_reads_as_a_conflict(self) -> None: with pytest.raises(AlbumNameTakenError): self.run_with_integrity_error(name_taken_on_recheck=True) - def test_any_other_integrity_error_is_not_swallowed(self): + def test_any_other_integrity_error_is_not_swallowed(self) -> None: """A vanished image must not be reported as a duplicate name.""" with pytest.raises(sqlite3.IntegrityError): self.run_with_integrity_error(name_taken_on_recheck=False) diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index 89d37f9b5..3f1a524ff 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -6,7 +6,7 @@ from fastapi.testclient import TestClient from unittest.mock import patch import uuid -from typing import Any, Dict, Optional +from typing import Any, Optional from app.database.albums import AlbumRow from app.routes import albums as albums_router @@ -24,7 +24,7 @@ def album_row( - album: Dict[str, Any], + album: dict[str, Any], cover_image_path: Optional[str] = None, created_at: Optional[str] = None, updated_at: Optional[str] = None, @@ -59,7 +59,7 @@ def mock_db_album(): @pytest.fixture -def mock_memory(): +def mock_memory() -> dict[str, Any]: """A memories row as db_get_memory returns it, trimmed to what the route reads.""" return { "memory_id": str(uuid.uuid4()), @@ -69,7 +69,7 @@ def mock_memory(): @pytest.fixture -def mock_memory_images(): +def mock_memory_images() -> list[dict[str, Any]]: return [ {"id": str(uuid.uuid4()), "sort_order": 0}, {"id": str(uuid.uuid4()), "sort_order": 1}, @@ -562,7 +562,9 @@ def test_remove_multiple_images_from_album(self, mock_db_album): class TestCreateAlbumFromMemory: """Test suite for converting a curated memory into an album.""" - def test_create_album_from_memory_success(self, mock_memory, mock_memory_images): + def test_create_album_from_memory_success( + self, mock_memory: dict[str, Any], mock_memory_images: list[dict[str, Any]] + ) -> None: with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( @@ -603,7 +605,9 @@ def test_create_album_from_memory_success(self, mock_memory, mock_memory_images) {"memory_id": " ", "name": "Paris 2022"}, ], ) - def test_create_album_from_memory_rejects_blank_fields(self, payload): + def test_create_album_from_memory_rejects_blank_fields( + self, payload: dict[str, str] + ) -> None: """A name of spaces satisfies min_length but is not a name.""" with patch("app.utils.albums.db_create_album_with_images") as mock_create: response = client.post("/albums/from-memory", json=payload) @@ -612,8 +616,8 @@ def test_create_album_from_memory_rejects_blank_fields(self, payload): mock_create.assert_not_called() def test_create_album_from_memory_trims_the_name( - self, mock_memory, mock_memory_images - ): + self, mock_memory: dict[str, Any], mock_memory_images: list[dict[str, Any]] + ) -> None: with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch( @@ -634,7 +638,7 @@ def test_create_album_from_memory_trims_the_name( assert response.status_code == 200 assert mock_create.call_args.args[1] == "Paris" - def test_create_album_from_memory_not_found(self): + def test_create_album_from_memory_not_found(self) -> None: with patch("app.utils.albums.db_get_memory") as mock_get_memory: mock_get_memory.return_value = None @@ -645,7 +649,7 @@ def test_create_album_from_memory_not_found(self): assert response.status_code == 404 assert response.json()["detail"]["error"] == "Memory Not Found" - def test_create_album_from_empty_memory(self, mock_memory): + def test_create_album_from_empty_memory(self, mock_memory: dict[str, Any]) -> None: """A memory with no photos cannot become an album.""" with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( "app.utils.albums.db_get_memory_images" @@ -664,8 +668,11 @@ def test_create_album_from_empty_memory(self, mock_memory): mock_create.assert_not_called() def test_create_album_from_memory_duplicate_name( - self, mock_memory, mock_memory_images, mock_db_album - ): + self, + mock_memory: dict[str, Any], + mock_memory_images: list[dict[str, Any]], + mock_db_album: dict[str, Any], + ) -> None: with patch("app.utils.albums.db_get_memory") as mock_get_memory, patch( "app.utils.albums.db_get_memory_images" ) as mock_get_images, patch(