From f52b47f6df33d0dd86e2bb8a41b6f25606263532 Mon Sep 17 00:00:00 2001 From: mohamad Date: Thu, 8 May 2025 22:34:07 +0200 Subject: [PATCH] Refactor CRUD operations in group, item, and list modules to remove unnecessary transaction context; enhance error handling and improve code readability. Update API endpoint for OCR processing in configuration and add confirmation dialogs for item actions in ListDetailPage. --- be/app/crud/group.py | 95 ++++++++++++------------- be/app/crud/item.py | 18 +++-- be/app/crud/list.py | 122 +++++++++++++++----------------- fe/src/config/api-config.ts | 2 +- fe/src/pages/ListDetailPage.vue | 118 +++++++++++++++++++++++++++++- 5 files changed, 227 insertions(+), 128 deletions(-) diff --git a/be/app/crud/group.py b/be/app/crud/group.py index c793521..8a6ebc6 100644 --- a/be/app/crud/group.py +++ b/be/app/crud/group.py @@ -4,6 +4,7 @@ from sqlalchemy.future import select from sqlalchemy.orm import selectinload # For eager loading members from sqlalchemy.exc import SQLAlchemyError, IntegrityError, OperationalError from typing import Optional, List +from sqlalchemy import func from app.models import User as UserModel, Group as GroupModel, UserGroup as UserGroupModel from app.schemas.group import GroupCreate @@ -47,14 +48,13 @@ async def create_group(db: AsyncSession, group_in: GroupCreate, creator_id: int) async def get_user_groups(db: AsyncSession, user_id: int) -> List[GroupModel]: """Gets all groups a user is a member of.""" try: - async with db.begin(): - result = await db.execute( - select(GroupModel) - .join(UserGroupModel) - .where(UserGroupModel.user_id == user_id) - .options(selectinload(GroupModel.member_associations)) - ) - return result.scalars().all() + result = await db.execute( + select(GroupModel) + .join(UserGroupModel) + .where(UserGroupModel.user_id == user_id) + .options(selectinload(GroupModel.member_associations)) + ) + return result.scalars().all() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -63,15 +63,14 @@ async def get_user_groups(db: AsyncSession, user_id: int) -> List[GroupModel]: async def get_group_by_id(db: AsyncSession, group_id: int) -> Optional[GroupModel]: """Gets a single group by its ID, optionally loading members.""" try: - async with db.begin(): - result = await db.execute( - select(GroupModel) - .where(GroupModel.id == group_id) - .options( - selectinload(GroupModel.member_associations).selectinload(UserGroupModel.user) - ) + result = await db.execute( + select(GroupModel) + .where(GroupModel.id == group_id) + .options( + selectinload(GroupModel.member_associations).selectinload(UserGroupModel.user) ) - return result.scalars().first() + ) + return result.scalars().first() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -80,13 +79,12 @@ async def get_group_by_id(db: AsyncSession, group_id: int) -> Optional[GroupMode async def is_user_member(db: AsyncSession, group_id: int, user_id: int) -> bool: """Checks if a user is a member of a specific group.""" try: - async with db.begin(): - result = await db.execute( - select(UserGroupModel.id) - .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) - .limit(1) - ) - return result.scalar_one_or_none() is not None + result = await db.execute( + select(UserGroupModel.id) + .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) + .limit(1) + ) + return result.scalar_one_or_none() is not None except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -95,12 +93,11 @@ async def is_user_member(db: AsyncSession, group_id: int, user_id: int) -> bool: async def get_user_role_in_group(db: AsyncSession, group_id: int, user_id: int) -> Optional[UserRoleEnum]: """Gets the role of a user in a specific group.""" try: - async with db.begin(): - result = await db.execute( - select(UserGroupModel.role) - .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) - ) - return result.scalar_one_or_none() + result = await db.execute( + select(UserGroupModel.role) + .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) + ) + return result.scalar_one_or_none() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -146,11 +143,10 @@ async def remove_user_from_group(db: AsyncSession, group_id: int, user_id: int) async def get_group_member_count(db: AsyncSession, group_id: int) -> int: """Counts the number of members in a group.""" try: - async with db.begin(): - result = await db.execute( - select(func.count(UserGroupModel.id)).where(UserGroupModel.group_id == group_id) - ) - return result.scalar_one() + result = await db.execute( + select(func.count(UserGroupModel.id)).where(UserGroupModel.group_id == group_id) + ) + return result.scalar_one() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -170,22 +166,21 @@ async def check_group_membership( GroupMembershipError: If the user_id is not a member of the group. """ try: - async with db.begin(): - # Check group existence first - group_exists = await db.get(GroupModel, group_id) - if not group_exists: - raise GroupNotFoundError(group_id) - - # Check membership - membership = await db.execute( - select(UserGroupModel.id) - .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) - .limit(1) - ) - if membership.scalar_one_or_none() is None: - raise GroupMembershipError(group_id, action=action) - # If we reach here, the user is a member - return None + # Check group existence first + group_exists = await db.get(GroupModel, group_id) + if not group_exists: + raise GroupNotFoundError(group_id) + + # Check membership + membership = await db.execute( + select(UserGroupModel.id) + .where(UserGroupModel.group_id == group_id, UserGroupModel.user_id == user_id) + .limit(1) + ) + if membership.scalar_one_or_none() is None: + raise GroupMembershipError(group_id, action=action) + # If we reach here, the user is a member + return None except GroupNotFoundError: # Re-raise specific errors raise except GroupMembershipError: diff --git a/be/app/crud/item.py b/be/app/crud/item.py index 16ec082..19b6969 100644 --- a/be/app/crud/item.py +++ b/be/app/crud/item.py @@ -43,13 +43,12 @@ async def create_item(db: AsyncSession, item_in: ItemCreate, list_id: int, user_ async def get_items_by_list_id(db: AsyncSession, list_id: int) -> PyList[ItemModel]: """Gets all items belonging to a specific list, ordered by creation time.""" try: - async with db.begin(): - result = await db.execute( - select(ItemModel) - .where(ItemModel.list_id == list_id) - .order_by(ItemModel.created_at.asc()) # Or desc() if preferred - ) - return result.scalars().all() + result = await db.execute( + select(ItemModel) + .where(ItemModel.list_id == list_id) + .order_by(ItemModel.created_at.asc()) # Or desc() if preferred + ) + return result.scalars().all() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -58,9 +57,8 @@ async def get_items_by_list_id(db: AsyncSession, list_id: int) -> PyList[ItemMod async def get_item_by_id(db: AsyncSession, item_id: int) -> Optional[ItemModel]: """Gets a single item by its ID.""" try: - async with db.begin(): - result = await db.execute(select(ItemModel).where(ItemModel.id == item_id)) - return result.scalars().first() + result = await db.execute(select(ItemModel).where(ItemModel.id == item_id)) + return result.scalars().first() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: diff --git a/be/app/crud/list.py b/be/app/crud/list.py index f289662..7e3655e 100644 --- a/be/app/crud/list.py +++ b/be/app/crud/list.py @@ -45,45 +45,41 @@ async def create_list(db: AsyncSession, list_in: ListCreate, creator_id: int) -> async def get_lists_for_user(db: AsyncSession, user_id: int) -> PyList[ListModel]: """Gets all lists accessible by a user.""" try: - async with db.begin(): - group_ids_result = await db.execute( - select(UserGroupModel.group_id).where(UserGroupModel.user_id == user_id) - ) - user_group_ids = group_ids_result.scalars().all() + group_ids_result = await db.execute( + select(UserGroupModel.group_id).where(UserGroupModel.user_id == user_id) + ) + user_group_ids = group_ids_result.scalars().all() - # Build conditions for the OR clause dynamically - conditions = [ - and_(ListModel.created_by_id == user_id, ListModel.group_id.is_(None)) - ] - if user_group_ids: # Only add the IN clause if there are group IDs - conditions.append(ListModel.group_id.in_(user_group_ids)) + # Build conditions for the OR clause dynamically + conditions = [ + and_(ListModel.created_by_id == user_id, ListModel.group_id.is_(None)) + ] + if user_group_ids: # Only add the IN clause if there are group IDs + conditions.append(ListModel.group_id.in_(user_group_ids)) - query = select(ListModel).where(or_(*conditions)).order_by(ListModel.updated_at.desc()) + query = select(ListModel).where(or_(*conditions)).order_by(ListModel.updated_at.desc()) - result = await db.execute(query) - return result.scalars().all() + result = await db.execute(query) + return result.scalars().all() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: - # It would be helpful to log the original error e here for more detailed debugging - # For example: logger.error(f"SQLAlchemyError in get_lists_for_user: {type(e).__name__} - {str(e)}") raise DatabaseQueryError(f"Failed to query user lists: {str(e)}") async def get_list_by_id(db: AsyncSession, list_id: int, load_items: bool = False) -> Optional[ListModel]: """Gets a single list by ID, optionally loading its items.""" try: - async with db.begin(): - query = select(ListModel).where(ListModel.id == list_id) - if load_items: - query = query.options( - selectinload(ListModel.items) - .options( - joinedload(ItemModel.added_by_user), - joinedload(ItemModel.completed_by_user) - ) + query = select(ListModel).where(ListModel.id == list_id) + if load_items: + query = query.options( + selectinload(ListModel.items) + .options( + joinedload(ItemModel.added_by_user), + joinedload(ItemModel.completed_by_user) ) - result = await db.execute(query) - return result.scalars().first() + ) + result = await db.execute(query) + return result.scalars().first() except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -139,29 +135,28 @@ async def delete_list(db: AsyncSession, list_db: ListModel) -> None: async def check_list_permission(db: AsyncSession, list_id: int, user_id: int, require_creator: bool = False) -> ListModel: """Fetches a list and verifies user permission.""" try: - async with db.begin(): - list_db = await get_list_by_id(db, list_id=list_id, load_items=True) - if not list_db: - raise ListNotFoundError(list_id) + list_db = await get_list_by_id(db, list_id=list_id, load_items=True) + if not list_db: + raise ListNotFoundError(list_id) - is_creator = list_db.created_by_id == user_id + is_creator = list_db.created_by_id == user_id - if require_creator: - if not is_creator: - raise ListCreatorRequiredError(list_id, "access") - return list_db + if require_creator: + if not is_creator: + raise ListCreatorRequiredError(list_id, "access") + return list_db - if is_creator: - return list_db + if is_creator: + return list_db - if list_db.group_id: - from app.crud.group import is_user_member - is_member = await is_user_member(db, group_id=list_db.group_id, user_id=user_id) - if not is_member: - raise ListPermissionError(list_id) - return list_db - else: + if list_db.group_id: + from app.crud.group import is_user_member + is_member = await is_user_member(db, group_id=list_db.group_id, user_id=user_id) + if not is_member: raise ListPermissionError(list_id) + return list_db + else: + raise ListPermissionError(list_id) except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: @@ -170,29 +165,28 @@ async def check_list_permission(db: AsyncSession, list_id: int, user_id: int, re async def get_list_status(db: AsyncSession, list_id: int) -> ListStatus: """Gets the update timestamps and item count for a list.""" try: - async with db.begin(): - list_query = select(ListModel.updated_at).where(ListModel.id == list_id) - list_result = await db.execute(list_query) - list_updated_at = list_result.scalar_one_or_none() + list_query = select(ListModel.updated_at).where(ListModel.id == list_id) + list_result = await db.execute(list_query) + list_updated_at = list_result.scalar_one_or_none() - if list_updated_at is None: - raise ListNotFoundError(list_id) + if list_updated_at is None: + raise ListNotFoundError(list_id) - item_status_query = ( - select( - sql_func.max(ItemModel.updated_at).label("latest_item_updated_at"), - sql_func.count(ItemModel.id).label("item_count") - ) - .where(ItemModel.list_id == list_id) + item_status_query = ( + select( + sql_func.max(ItemModel.updated_at).label("latest_item_updated_at"), + sql_func.count(ItemModel.id).label("item_count") ) - item_result = await db.execute(item_status_query) - item_status = item_result.first() + .where(ItemModel.list_id == list_id) + ) + item_result = await db.execute(item_status_query) + item_status = item_result.first() - return ListStatus( - list_updated_at=list_updated_at, - latest_item_updated_at=item_status.latest_item_updated_at if item_status else None, - item_count=item_status.item_count if item_status else 0 - ) + return ListStatus( + list_updated_at=list_updated_at, + latest_item_updated_at=item_status.latest_item_updated_at if item_status else None, + item_count=item_status.item_count if item_status else 0 + ) except OperationalError as e: raise DatabaseConnectionError(f"Failed to connect to database: {str(e)}") except SQLAlchemyError as e: diff --git a/fe/src/config/api-config.ts b/fe/src/config/api-config.ts index bd9c287..76b3d23 100644 --- a/fe/src/config/api-config.ts +++ b/fe/src/config/api-config.ts @@ -73,7 +73,7 @@ export const API_ENDPOINTS = { // OCR OCR: { - PROCESS: '/ocr/process', + PROCESS: '/ocr/extract-items', STATUS: (jobId: string) => `/ocr/status/${jobId}`, RESULT: (jobId: string) => `/ocr/result/${jobId}`, BATCH: '/ocr/batch', diff --git a/fe/src/pages/ListDetailPage.vue b/fe/src/pages/ListDetailPage.vue index fb45129..b6479bf 100644 --- a/fe/src/pages/ListDetailPage.vue +++ b/fe/src/pages/ListDetailPage.vue @@ -105,6 +105,8 @@ label="Item Name" :rules="[(val) => !!val || 'Name is required']" outlined + ref="itemNameInput" + @keydown.enter.prevent="onAddItem" />
@@ -114,6 +116,7 @@ label="Quantity (optional)" outlined min="1" + @keydown.enter.prevent="onAddItem" />
@@ -142,8 +145,9 @@ @@ -152,9 +156,36 @@ Quantity: {{ item.quantity }} + + + + + + + + + + {{ confirmDialogMessage }} + + + + + + + + @@ -172,6 +203,7 @@ interface Item { version: number; updating?: boolean; updated_at: string; + deleting?: boolean; } interface List { @@ -198,7 +230,7 @@ const list = ref({ const loading = ref(true); const error = ref(null); const addingItem = ref(false); -const pollingInterval = ref(undefined); +const pollingInterval = ref | undefined>(undefined); const lastListUpdate = ref(null); const lastItemUpdate = ref(null); @@ -214,6 +246,14 @@ const ocrItems = ref<{ name: string }[]>([]); const addingOcrItems = ref(false); const ocrError = ref(null); +// Add new refs for confirmation dialog +const showConfirmDialog = ref(false); +const confirmDialogMessage = ref(''); +const pendingAction = ref<(() => Promise) | null>(null); + +// Add ref for item name input +const itemNameInput = ref<{ focus: () => void } | null>(null); + const fetchListDetails = async () => { loading.value = true; error.value = null; @@ -277,7 +317,13 @@ const stopPolling = () => { }; const onAddItem = async () => { - if (!newItem.value.name) return; + if (!newItem.value.name) { + $q.notify({ + type: 'warning', + message: 'Please enter an item name', + }); + return; + } addingItem.value = true; try { @@ -287,6 +333,8 @@ const onAddItem = async () => { ); list.value.items.push(response.data as Item); newItem.value = { name: '' }; + // Focus the input for the next item + itemNameInput.value?.focus(); } catch (err: unknown) { $q.notify({ type: 'negative', @@ -395,14 +443,68 @@ const addOcrItems = async () => { } }; +// Add keyboard shortcut handler +const handleKeyPress = (event: KeyboardEvent) => { + // Focus item name input when pressing 'n' + if (event.key === 'n' && !event.ctrlKey && !event.metaKey) { + event.preventDefault(); + itemNameInput.value?.focus(); + } +}; + +// Add confirmation dialog handlers +const confirmUpdateItem = (item: Item) => { + confirmDialogMessage.value = `Are you sure you want to mark "${item.name}" as ${item.is_complete ? 'complete' : 'incomplete'}?`; + pendingAction.value = () => updateItem(item); + showConfirmDialog.value = true; +}; + +const confirmDeleteItem = (item: Item) => { + confirmDialogMessage.value = `Are you sure you want to delete "${item.name}"?`; + pendingAction.value = () => deleteItem(item); + showConfirmDialog.value = true; +}; + +const handleConfirmedAction = async () => { + if (pendingAction.value) { + await pendingAction.value(); + pendingAction.value = null; + } + showConfirmDialog.value = false; +}; + +// Add delete item function +const deleteItem = async (item: Item) => { + item.deleting = true; + try { + await apiClient.delete( + API_ENDPOINTS.LISTS.ITEM(list.value.id.toString(), item.id.toString()) + ); + const index = list.value.items.findIndex((i) => i.id === item.id); + if (index !== -1) { + list.value.items.splice(index, 1); + } + } catch (err: unknown) { + $q.notify({ + type: 'negative', + message: (err as Error).message || 'Failed to delete item', + }); + } finally { + item.deleting = false; + } +}; + +// Add keyboard event listeners onMounted(() => { void fetchListDetails().then(() => { startPolling(); }); + window.addEventListener('keydown', handleKeyPress); }); onUnmounted(() => { stopPolling(); + window.removeEventListener('keydown', handleKeyPress); }); @@ -411,4 +513,14 @@ onUnmounted(() => { text-decoration: line-through; opacity: 0.7; } + +/* Add transition for item updates */ +.q-item { + transition: all 0.3s ease; +} + +.q-item.updating { + opacity: 0.7; + pointer-events: none; +}