From ccb39e674e2a971235a3ca7620dc27641962ea5a Mon Sep 17 00:00:00 2001 From: Alex <25013571+alexhb1@users.noreply.github.com> Date: Mon, 16 Feb 2026 12:27:56 +0000 Subject: [PATCH] Patch: Request retry, admin-level requests, and various fixes (#620) - Add request retry in the case of a download failure, admins will be prompted to attach a new file to the request - Add admin-level "add to requests" button in the release modal --- shelfmark/core/models.py | 1 + shelfmark/core/requests_service.py | 62 ++++++++++++- shelfmark/core/user_db.py | 3 + shelfmark/download/orchestrator.py | 6 ++ shelfmark/main.py | 48 +++++++--- src/frontend/src/App.tsx | 84 ++++++++++++++++- src/frontend/src/components/ReleaseModal.tsx | 31 +++++++ .../src/components/activity/ActivityCard.tsx | 46 +++++++--- .../components/activity/ActivitySidebar.tsx | 26 ++++++ .../components/activity/activityCardModel.ts | 9 +- .../components/activity/activityMappers.ts | 8 ++ .../components/settings/SettingsContent.tsx | 2 + .../src/components/settings/SettingsModal.tsx | 66 ++++++++------ .../customFields/UsersManagementField.tsx | 45 ++++++++-- .../components/settings/customFields/types.ts | 1 + .../settings/users/useUserMutations.ts | 10 +-- .../settings/users/useUsersFetch.ts | 14 +-- .../src/components/shared/Tooltip.tsx | 67 +++++++++++++- src/frontend/src/types/index.ts | 2 + tests/core/test_requests_service.py | 90 +++++++++++++++++++ 20 files changed, 550 insertions(+), 71 deletions(-) diff --git a/shelfmark/core/models.py b/shelfmark/core/models.py index dd9f7d0a..ce7befec 100644 --- a/shelfmark/core/models.py +++ b/shelfmark/core/models.py @@ -97,6 +97,7 @@ class DownloadTask: # User association (multi-user support) user_id: Optional[int] = None # DB user ID who queued this download username: Optional[str] = None # Username for {User} template variable + request_id: Optional[int] = None # Origin request ID when queued from request fulfilment # Runtime state priority: int = 0 diff --git a/shelfmark/core/requests_service.py b/shelfmark/core/requests_service.py index 63889d1b..95b20484 100644 --- a/shelfmark/core/requests_service.py +++ b/shelfmark/core/requests_service.py @@ -455,8 +455,11 @@ def fulfil_request( if requester is None: raise RequestServiceError("Requesting user not found", status_code=404) + queued_release_data = dict(selected_release_data) + queued_release_data["_request_id"] = request_id + success, error = queue_release( - selected_release_data, + queued_release_data, 0, user_id=request_row["user_id"], username=requester.get("username"), @@ -476,9 +479,66 @@ def fulfil_request( release_data=selected_release_data, delivery_state="queued", delivery_updated_at=_now_timestamp(), + last_failure_reason=None, admin_note=normalized_admin_note, reviewed_by=admin_user_id, reviewed_at=_now_timestamp(), ) except ValueError as exc: raise RequestServiceError(str(exc), status_code=409, code="stale_transition") from exc + + +def reopen_failed_request( + user_db: "UserDB", + *, + request_id: int, + failure_reason: str | None = None, +) -> dict[str, Any] | None: + """Reopen a failed fulfilled request so admins can re-approve with a new release.""" + normalized_failure_reason = None + if isinstance(failure_reason, str): + normalized_failure_reason = failure_reason.strip() or None + + with user_db._lock: + conn = user_db._connect() + try: + current_row = conn.execute( + "SELECT * FROM download_requests WHERE id = ?", + (request_id,), + ).fetchone() + current_request = user_db._parse_request_row(current_row) + if current_request is None: + return None + + if current_request.get("status") != "fulfilled": + return None + current_delivery_state = _existing_delivery_state(current_request) + # Terminal hook callbacks can run before delivery-state sync persists "error". + # Allow reopening fulfilled requests unless they are already complete. + if current_delivery_state == "complete": + return None + if current_delivery_state not in {"error", "cancelled"} and normalized_failure_reason is None: + return None + + conn.execute( + """ + UPDATE download_requests + SET status = 'pending', + delivery_state = 'none', + delivery_updated_at = NULL, + release_data = NULL, + last_failure_reason = ?, + reviewed_by = NULL, + reviewed_at = NULL + WHERE id = ? + """, + (normalized_failure_reason, request_id), + ) + updated_row = conn.execute( + "SELECT * FROM download_requests WHERE id = ?", + (request_id,), + ).fetchone() + conn.commit() + return user_db._parse_request_row(updated_row) + finally: + conn.close() diff --git a/shelfmark/core/user_db.py b/shelfmark/core/user_db.py index 506562bb..a59a1e00 100644 --- a/shelfmark/core/user_db.py +++ b/shelfmark/core/user_db.py @@ -200,6 +200,8 @@ class UserDB: ) if "delivery_updated_at" not in column_names: conn.execute("ALTER TABLE download_requests ADD COLUMN delivery_updated_at TIMESTAMP") + if "last_failure_reason" not in column_names: + conn.execute("ALTER TABLE download_requests ADD COLUMN last_failure_reason TEXT") conn.execute( """ @@ -614,6 +616,7 @@ class UserDB: "reviewed_at", "delivery_state", "delivery_updated_at", + "last_failure_reason", } def update_request( diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index bd5d2536..81f45bcd 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -178,6 +178,10 @@ def queue_release( try: source = release_data.get('source', 'direct_download') extra = release_data.get('extra', {}) + raw_request_id = release_data.get('_request_id') + request_id: Optional[int] = None + if isinstance(raw_request_id, int) and raw_request_id > 0: + request_id = raw_request_id # Get author, year, preview, and content_type from top-level (preferred) or extra (fallback) author = release_data.get('author') or extra.get('author') @@ -225,6 +229,7 @@ def queue_release( priority=priority, user_id=user_id, username=username, + request_id=request_id, ) if not book_queue.add(task): @@ -327,6 +332,7 @@ def _task_to_dict(task: DownloadTask) -> Dict[str, Any]: 'download_path': task.download_path, 'user_id': task.user_id, 'username': task.username, + 'request_id': task.request_id, } diff --git a/shelfmark/main.py b/shelfmark/main.py index 74bcd47f..2fedfada 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -46,6 +46,7 @@ from shelfmark.core.request_policy import ( resolve_policy_mode, ) from shelfmark.core.requests_service import ( + reopen_failed_request, sync_delivery_states_from_queue_status, ) from shelfmark.core.activity_service import ActivityService, build_download_item_key @@ -1080,9 +1081,6 @@ def _notify_admin_for_terminal_download_status(*, task_id: str, status: QueueSta def _record_download_terminal_snapshot(task_id: str, status: QueueStatus, task: Any) -> None: _notify_admin_for_terminal_download_status(task_id=task_id, status=status, task=task) - if activity_service is None: - return - final_status = _queue_status_to_final_activity_status(status) if final_status is None: return @@ -1129,19 +1127,45 @@ def _record_download_terminal_snapshot(task_id: str, status: QueueStatus, task: if linked_request is not None: snapshot["request"] = linked_request + if activity_service is not None: + try: + activity_service.record_terminal_snapshot( + user_id=owner_user_id, + item_type="download", + item_key=build_download_item_key(task_id), + origin=origin, + final_status=final_status, + snapshot=snapshot, + request_id=request_id, + source_id=task_id, + ) + except Exception as exc: + logger.warning("Failed to record terminal download snapshot for task %s: %s", task_id, exc) + + if user_db is None or linked_request is None or request_id is None or status != QueueStatus.ERROR: + return + + raw_error_message = getattr(task, "status_message", None) + fallback_reason = ( + raw_error_message.strip() + if isinstance(raw_error_message, str) and raw_error_message.strip() + else "Download failed" + ) try: - activity_service.record_terminal_snapshot( - user_id=owner_user_id, - item_type="download", - item_key=build_download_item_key(task_id), - origin=origin, - final_status=final_status, - snapshot=snapshot, + reopened_request = reopen_failed_request( + user_db, request_id=request_id, - source_id=task_id, + failure_reason=fallback_reason, ) + if reopened_request is not None: + _emit_request_update_events([reopened_request]) except Exception as exc: - logger.warning("Failed to record terminal download snapshot for task %s: %s", task_id, exc) + logger.warning( + "Failed to reopen request %s after terminal download error %s: %s", + request_id, + task_id, + exc, + ) def _task_owned_by_actor(task: Any, *, actor_user_id: int | None, actor_username: str | None) -> bool: diff --git a/src/frontend/src/App.tsx b/src/frontend/src/App.tsx index c21b5e8e..adee3e5a 100644 --- a/src/frontend/src/App.tsx +++ b/src/frontend/src/App.tsx @@ -226,6 +226,43 @@ function App() { socket, }); + const dismissedDownloadTaskIds = useMemo(() => { + const result = new Set(); + for (const key of dismissedActivityKeys) { + if (typeof key !== 'string' || !key.startsWith('download:')) { + continue; + } + const taskId = key.substring('download:'.length).trim(); + if (taskId) { + result.add(taskId); + } + } + return result; + }, [dismissedActivityKeys]); + + const isDownloadTaskDismissed = useCallback((taskId: string) => { + return dismissedDownloadTaskIds.has(taskId); + }, [dismissedDownloadTaskIds]); + + const statusForButtonState = useMemo(() => { + if (!currentStatus.complete || dismissedDownloadTaskIds.size === 0) { + return currentStatus; + } + + const filteredComplete = Object.fromEntries( + Object.entries(currentStatus.complete).filter(([taskId]) => !dismissedDownloadTaskIds.has(taskId)) + ) as Record; + + if (Object.keys(filteredComplete).length === Object.keys(currentStatus.complete).length) { + return currentStatus; + } + + return { + ...currentStatus, + complete: filteredComplete, + }; + }, [currentStatus, dismissedDownloadTaskIds]); + const showRequestsTab = useMemo(() => { if (requestRoleIsAdmin) { return true; @@ -947,6 +984,23 @@ function App() { [openRequestConfirmation, refreshRequestPolicy] ); + const handleReleaseBookRequest = useCallback( + async (book: Book, modalContentType: ContentType): Promise => { + void refreshRequestPolicy(); + const normalizedContentType = toContentType(modalContentType); + openRequestConfirmation({ + book_data: buildMetadataBookRequestData(book, normalizedContentType), + release_data: null, + context: { + source: '*', + content_type: normalizedContentType, + request_level: 'book', + }, + }); + }, + [openRequestConfirmation, refreshRequestPolicy] + ); + const handleReleaseModalPolicyRefresh = useCallback(() => { return refreshRequestPolicy({ force: true }); }, [refreshRequestPolicy]); @@ -1044,19 +1098,38 @@ function App() { const getDirectActionButtonState = useCallback( (bookId: string): ButtonStateInfo => { const baseState = getButtonState(bookId); + if (baseState.state === 'complete' && isDownloadTaskDismissed(bookId)) { + return applyDirectPolicyModeToButtonState( + { text: 'Download', state: 'download' }, + getDirectPolicyMode() + ); + } const mode = getDirectPolicyMode(); return applyDirectPolicyModeToButtonState(baseState, mode); }, - [getButtonState, getDirectPolicyMode] + [getButtonState, getDirectPolicyMode, isDownloadTaskDismissed] ); const getUniversalActionButtonState = useCallback( (bookId: string): ButtonStateInfo => { const baseState = getUniversalButtonState(bookId); + const trackedReleaseIds = bookToReleaseMap[bookId] || []; + const allTrackedReleasesDismissed = trackedReleaseIds.length > 0 && + trackedReleaseIds.every((releaseId) => isDownloadTaskDismissed(releaseId)); + + if ( + baseState.state === 'complete' && + (isDownloadTaskDismissed(bookId) || allTrackedReleasesDismissed) + ) { + return applyUniversalPolicyModeToButtonState( + { text: 'Get', state: 'download' }, + getUniversalDefaultPolicyMode() + ); + } const mode = getUniversalDefaultPolicyMode(); return applyUniversalPolicyModeToButtonState(baseState, mode); }, - [getUniversalButtonState, getUniversalDefaultPolicyMode] + [bookToReleaseMap, getUniversalButtonState, getUniversalDefaultPolicyMode, isDownloadTaskDismissed] ); const bookLanguages = config?.book_languages || DEFAULT_LANGUAGES; @@ -1263,6 +1336,11 @@ function App() { onClose={handleReleaseModalClose} onDownload={isBrowseFulfilMode ? handleBrowseFulfilDownload : handleReleaseDownload} onRequestRelease={isBrowseFulfilMode ? undefined : handleReleaseRequest} + onRequestBook={ + isBrowseFulfilMode || !requestRoleIsAdmin + ? undefined + : handleReleaseBookRequest + } getPolicyModeForSource={isBrowseFulfilMode ? () => 'download' : (source, ct) => getSourceMode(source, ct)} onPolicyRefresh={handleReleaseModalPolicyRefresh} supportedFormats={supportedFormats} @@ -1270,7 +1348,7 @@ function App() { contentType={activeReleaseContentType} defaultLanguages={defaultLanguageCodes} bookLanguages={bookLanguages} - currentStatus={currentStatus} + currentStatus={statusForButtonState} defaultReleaseSource={config?.default_release_source} onSearchSeries={isBrowseFulfilMode ? undefined : handleSearchSeries} /> diff --git a/src/frontend/src/components/ReleaseModal.tsx b/src/frontend/src/components/ReleaseModal.tsx index 9a19279f..52a43df5 100644 --- a/src/frontend/src/components/ReleaseModal.tsx +++ b/src/frontend/src/components/ReleaseModal.tsx @@ -194,6 +194,7 @@ interface ReleaseModalProps { onClose: () => void; onDownload: (book: Book, release: Release, contentType: ContentType) => Promise; onRequestRelease?: (book: Book, release: Release, contentType: ContentType) => Promise; + onRequestBook?: (book: Book, contentType: ContentType) => Promise; getPolicyModeForSource?: (source: string, contentType: ContentType) => RequestPolicyMode; onPolicyRefresh?: () => Promise; supportedFormats: string[]; @@ -622,6 +623,7 @@ export const ReleaseModal = ({ onClose, onDownload, onRequestRelease, + onRequestBook, getPolicyModeForSource, onPolicyRefresh, supportedFormats, @@ -638,6 +640,7 @@ export const ReleaseModal = ({ ? supportedAudiobookFormats : supportedFormats; const [isClosing, setIsClosing] = useState(false); + const [isRequestingBook, setIsRequestingBook] = useState(false); // Available sources from plugin registry const [availableSources, setAvailableSources] = useState([]); @@ -699,6 +702,19 @@ export const ReleaseModal = ({ }, 150); }, [onClose]); + const handleRequestBook = useCallback(async (): Promise => { + if (!book || !onRequestBook || isRequestingBook) { + return; + } + setIsRequestingBook(true); + try { + await onRequestBook(book, contentType); + handleClose(); + } finally { + setIsRequestingBook(false); + } + }, [book, onRequestBook, isRequestingBook, contentType, handleClose]); + // Handle ESC key useEffect(() => { const handleEscape = (e: KeyboardEvent) => { @@ -1511,6 +1527,21 @@ export const ReleaseModal = ({ )} + {onRequestBook && ( + + )} diff --git a/src/frontend/src/components/activity/ActivityCard.tsx b/src/frontend/src/components/activity/ActivityCard.tsx index 9bd656fb..00c0e0cc 100644 --- a/src/frontend/src/components/activity/ActivityCard.tsx +++ b/src/frontend/src/components/activity/ActivityCard.tsx @@ -86,7 +86,7 @@ const actionKey = (action: ActivityCardAction): string => { const actionUiConfig = ( action: ActivityCardAction -): { title: string; className: string; icon: 'cross' | 'check' | 'stop' } => { +): { title: string; className: string; icon: 'cross' | 'check' | 'stop' | 'retry' } => { switch (action.kind) { case 'download-remove': return { @@ -139,7 +139,7 @@ const actionUiConfig = ( } }; -const ActionIcon = ({ icon }: { icon: 'cross' | 'check' | 'stop' }) => { +const ActionIcon = ({ icon }: { icon: 'cross' | 'check' | 'stop' | 'retry' }) => { if (icon === 'stop') { return ( ); } + if (icon === 'retry') { + return ( + + ); + } return (