diff --git a/api/services/knowledge_fs/data_facade.py b/api/services/knowledge_fs/data_facade.py index 740baa15c63..3f3a2d82569 100644 --- a/api/services/knowledge_fs/data_facade.py +++ b/api/services/knowledge_fs/data_facade.py @@ -642,24 +642,35 @@ class KnowledgeFSDataFacade: def prepare_logical_document_download( self, *, tenant_id: str, account_id: str, control_space_id: str, document_id: str ) -> KnowledgeFSDocumentDownloadDescriptor: - """Resolve the readable active revision of a logical document to its stored asset.""" + """Resolve a logical document's active or latest failed revision to its stored asset.""" logical_document = self.get_logical_document( tenant_id=tenant_id, account_id=account_id, control_space_id=control_space_id, document_id=document_id, ) - active = logical_document.active - if active is None: - raise KnowledgeFSProductResourceNotFoundError("Logical document has no active revision") + downloadable_revision = logical_document.active + if downloadable_revision is None: + if logical_document.status != "failed": + raise KnowledgeFSProductResourceNotFoundError("Logical document has no downloadable revision") + revisions = self.list_document_revisions( + tenant_id=tenant_id, + account_id=account_id, + control_space_id=control_space_id, + document_id=document_id, + limit=1, + ) + downloadable_revision = revisions.data[0] if revisions.data else None + if downloadable_revision is None or downloadable_revision.state != "failed": + raise KnowledgeFSProductResourceNotFoundError("Logical document has no downloadable revision") asset = self.get_document( tenant_id=tenant_id, account_id=account_id, control_space_id=control_space_id, - document_id=active.document_asset_id, + document_id=downloadable_revision.document_asset_id, ) - if asset.version != active.document_asset_version: - raise KnowledgeFSProductResourceNotFoundError("Logical document active asset version is unavailable") + if asset.version != downloadable_revision.document_asset_version: + raise KnowledgeFSProductResourceNotFoundError("Logical document revision asset version is unavailable") return KnowledgeFSDocumentDownloadDescriptor( document_id=logical_document.id, filename=asset.filename, @@ -971,6 +982,7 @@ class KnowledgeFSDataFacade: control_space_id: str, document_id: str, cursor: str | None = None, + limit: int | None = None, ) -> KnowledgeFSDocumentRevisionListResponse: raw = self._interactive_child( tenant_id=tenant_id, @@ -979,7 +991,7 @@ class KnowledgeFSDataFacade: operation_id="listDocumentRevisions", resource_id=document_id, path_parameters=(("documentId", document_id),), - query=(("cursor", cursor),) if cursor else (), + query=_knowledge_fs_query(("cursor", cursor), ("limit", limit)), ) return KnowledgeFSDocumentRevisionListResponse.model_validate(raw) diff --git a/api/services/knowledge_fs/product_dto.py b/api/services/knowledge_fs/product_dto.py index 885755506a7..fed1e2e5f28 100644 --- a/api/services/knowledge_fs/product_dto.py +++ b/api/services/knowledge_fs/product_dto.py @@ -1418,7 +1418,7 @@ class KnowledgeFSDocumentListResponse(ResponseModel): class KnowledgeFSDocumentDownloadDescriptor(ResponseModel): - """Trusted, internal description of the active object behind a logical document.""" + """Trusted, internal description of an active or latest failed downloadable revision asset.""" document_id: str filename: str diff --git a/api/tests/unit_tests/services/test_knowledge_fs_data_facade.py b/api/tests/unit_tests/services/test_knowledge_fs_data_facade.py index 757d99e191f..d5d29c2437b 100644 --- a/api/tests/unit_tests/services/test_knowledge_fs_data_facade.py +++ b/api/tests/unit_tests/services/test_knowledge_fs_data_facade.py @@ -1942,7 +1942,7 @@ def test_logical_document_delete_preserves_initial_row_version() -> None: "list_document_revisions", "KnowledgeFSDocumentRevisionListResponse", "listDocumentRevisions", - {"document_id": "document-1", "cursor": "cursor-1"}, + {"document_id": "document-1", "cursor": "cursor-1", "limit": 1}, "document-1", ), ( @@ -2272,6 +2272,8 @@ def test_facade_public_methods_preserve_the_registered_operation_and_child_bindi assert delegated.call_args.kwargs["resource_id"] == child_resource_id if operation_id == "listBackgroundTasks": assert delegated.call_args.kwargs["query"] == (("limit", "25"), ("cursor", "cursor-1")) + if operation_id == "listDocumentRevisions": + assert delegated.call_args.kwargs["query"] == (("cursor", "cursor-1"), ("limit", "1")) if operation_id in {"listGoldenQuestions", "listQualityBadCases"}: assert delegated.call_args.kwargs["query"] == (("limit", "25"), ("cursor", "cursor-1")) if operation_id == "createQualityReplay": diff --git a/api/tests/unit_tests/services/test_knowledge_fs_download_facade.py b/api/tests/unit_tests/services/test_knowledge_fs_download_facade.py index 3a4f65d79cb..3ee40e37fb5 100644 --- a/api/tests/unit_tests/services/test_knowledge_fs_download_facade.py +++ b/api/tests/unit_tests/services/test_knowledge_fs_download_facade.py @@ -1,6 +1,7 @@ from __future__ import annotations from datetime import UTC, datetime +from typing import Literal from unittest.mock import MagicMock import pytest @@ -8,13 +9,18 @@ import pytest from services.knowledge_fs.data_facade import KnowledgeFSDataFacade from services.knowledge_fs.product_dto import ( KnowledgeFSDocumentResponse, + KnowledgeFSDocumentRevisionListResponse, KnowledgeFSDocumentRevisionResponse, KnowledgeFSLogicalDocumentResponse, ) from services.knowledge_fs.product_remote import KnowledgeFSProductResourceNotFoundError -def logical_document(*, active: KnowledgeFSDocumentRevisionResponse | None): +def logical_document( + *, + active: KnowledgeFSDocumentRevisionResponse | None, + status: Literal["deleting", "failed", "pending", "ready"] = "ready", +): return KnowledgeFSLogicalDocumentResponse( active=active, active_revision=active.revision if active else None, @@ -23,16 +29,18 @@ def logical_document(*, active: KnowledgeFSDocumentRevisionResponse | None): id="logical-1", knowledge_space_id="space-1", row_version=1, - status="ready", + status=status, title="Document", updated_at=datetime.now(UTC), user_metadata={}, ) -def revision() -> KnowledgeFSDocumentRevisionResponse: +def revision( + *, state: Literal["active", "candidate", "failed", "superseded"] = "active" +) -> KnowledgeFSDocumentRevisionResponse: return KnowledgeFSDocumentRevisionResponse( - activated_at=datetime.now(UTC), + activated_at=datetime.now(UTC) if state == "active" else None, content_hash="a" * 64, created_at=datetime.now(UTC), document_asset_id="asset-1", @@ -42,7 +50,7 @@ def revision() -> KnowledgeFSDocumentRevisionResponse: mime_type="text/markdown", revision=3, size_bytes=4, - state="active", + state=state, ) @@ -77,6 +85,7 @@ def test_prepare_logical_document_download_resolves_active_asset() -> None: assert result.document_id == "logical-1" assert result.object_key == "tenant/spaces/space-1/documents/page.md" + facade.list_document_revisions.assert_not_called() facade.get_document.assert_called_once_with( tenant_id="tenant-1", account_id="account-1", @@ -85,9 +94,37 @@ def test_prepare_logical_document_download_resolves_active_asset() -> None: ) -def test_prepare_logical_document_download_rejects_document_without_active_revision() -> None: +def test_prepare_logical_document_download_resolves_latest_failed_revision_without_active_revision() -> None: facade = MagicMock(spec=KnowledgeFSDataFacade) - facade.get_logical_document.return_value = logical_document(active=None) + facade.get_logical_document.return_value = logical_document(active=None, status="failed") + facade.list_document_revisions.return_value = KnowledgeFSDocumentRevisionListResponse( + data=[revision(state="failed")] + ) + facade.get_document.return_value = asset() + + result = KnowledgeFSDataFacade.prepare_logical_document_download( + facade, + tenant_id="tenant-1", + account_id="account-1", + control_space_id="control-1", + document_id="logical-1", + ) + + assert result.document_id == "logical-1" + assert result.object_key == "tenant/spaces/space-1/documents/page.md" + facade.list_document_revisions.assert_called_once_with( + tenant_id="tenant-1", + account_id="account-1", + control_space_id="control-1", + document_id="logical-1", + limit=1, + ) + + +def test_prepare_logical_document_download_rejects_document_without_visible_revision() -> None: + facade = MagicMock(spec=KnowledgeFSDataFacade) + facade.get_logical_document.return_value = logical_document(active=None, status="failed") + facade.list_document_revisions.return_value = KnowledgeFSDocumentRevisionListResponse(data=[]) with pytest.raises(KnowledgeFSProductResourceNotFoundError): KnowledgeFSDataFacade.prepare_logical_document_download( @@ -97,3 +134,39 @@ def test_prepare_logical_document_download_rejects_document_without_active_revis control_space_id="control-1", document_id="logical-1", ) + + +def test_prepare_logical_document_download_rejects_latest_candidate_revision() -> None: + facade = MagicMock(spec=KnowledgeFSDataFacade) + facade.get_logical_document.return_value = logical_document(active=None, status="failed") + facade.list_document_revisions.return_value = KnowledgeFSDocumentRevisionListResponse( + data=[revision(state="candidate")] + ) + + with pytest.raises(KnowledgeFSProductResourceNotFoundError): + KnowledgeFSDataFacade.prepare_logical_document_download( + facade, + tenant_id="tenant-1", + account_id="account-1", + control_space_id="control-1", + document_id="logical-1", + ) + + facade.get_document.assert_not_called() + + +def test_prepare_logical_document_download_rejects_pending_document_without_active_revision() -> None: + facade = MagicMock(spec=KnowledgeFSDataFacade) + facade.get_logical_document.return_value = logical_document(active=None, status="pending") + + with pytest.raises(KnowledgeFSProductResourceNotFoundError): + KnowledgeFSDataFacade.prepare_logical_document_download( + facade, + tenant_id="tenant-1", + account_id="account-1", + control_space_id="control-1", + document_id="logical-1", + ) + + facade.list_document_revisions.assert_not_called() + facade.get_document.assert_not_called() diff --git a/web/features/new-rag/__tests__/documents-page.spec.tsx b/web/features/new-rag/__tests__/documents-page.spec.tsx index 23b7b9d453e..520761880b4 100644 --- a/web/features/new-rag/__tests__/documents-page.spec.tsx +++ b/web/features/new-rag/__tests__/documents-page.spec.tsx @@ -1147,6 +1147,122 @@ describe('DocumentsPage', () => { expect(downloadBlobMock).toHaveBeenCalledWith({ data: file, fileName: 'source-report.md' }) }) + it('downloads a failed document without an active revision from the document action menu', async () => { + const user = userEvent.setup() + documentsQuery.data = { + pages: [ + { + items: [ + document({ + active: null, + activeRevision: undefined, + id: 'failed-report', + status: 'failed', + title: 'Failed report.pdf', + }), + ], + }, + ], + } + tasksQuery.data = { + pages: [ + { + items: [task({ documentId: 'failed-report', documentRevision: 1, state: 'failed' })], + }, + ], + } + const file = new File(['failed report'], 'failed-report.pdf', { type: 'application/pdf' }) + downloadDocumentMutation.mockResolvedValue(file) + + render() + await user.click(screen.getByRole('button', { name: /dataset\.newKnowledge\.documentActions/ })) + const download = await screen.findByRole('menuitem', { + name: 'dataset.newKnowledge.downloadDocuments', + }) + expect(download).toBeEnabled() + await user.click(download) + + expect(downloadDocumentMutation).toHaveBeenCalledWith({ + params: { control_space_id: 'space-1', document_id: 'failed-report' }, + }) + expect(downloadBlobMock).toHaveBeenCalledWith({ data: file, fileName: 'failed-report.pdf' }) + }) + + it('keeps downloads disabled when a pending document is displayed as failed by a canceled task', async () => { + const user = userEvent.setup() + documentsQuery.data = { + pages: [ + { + items: [ + document({ + active: null, + activeRevision: undefined, + id: 'canceled-report', + status: 'pending', + title: 'Canceled report.pdf', + }), + ], + }, + ], + } + tasksQuery.data = { + pages: [ + { + items: [task({ documentId: 'canceled-report', documentRevision: 1, state: 'canceled' })], + }, + ], + } + + render() + await user.click(screen.getByRole('checkbox', { name: 'Canceled report.pdf' })) + const bulkActions = screen.getByRole('group', { + name: 'dataset.newKnowledge.bulkDocumentActions', + }) + expect( + within(bulkActions).getByRole('button', { + name: 'dataset.newKnowledge.downloadDocuments', + }), + ).toBeDisabled() + + await user.click(screen.getByRole('button', { name: /dataset\.newKnowledge\.documentActions/ })) + expect( + await screen.findByRole('menuitem', { + name: 'dataset.newKnowledge.downloadDocuments', + }), + ).toHaveAttribute('aria-disabled', 'true') + expect(downloadDocumentMutation).not.toHaveBeenCalled() + expect(downloadDocumentsMutation).not.toHaveBeenCalled() + }) + + it('keeps row and selected-document downloads disabled until task status loads', async () => { + const user = userEvent.setup() + documentsQuery.data = { + pages: [{ items: [document({ id: 'report', title: 'Report.pdf' })] }], + } + + const rendered = render() + await user.click(screen.getByRole('checkbox', { name: 'Report.pdf' })) + + tasksQuery.data = undefined + tasksQuery.isPending = true + rendered.rerender() + + const bulkActions = screen.getByRole('group', { + name: 'dataset.newKnowledge.bulkDocumentActions', + }) + expect( + within(bulkActions).getByRole('button', { + name: 'dataset.newKnowledge.downloadDocuments', + }), + ).toBeDisabled() + await user.click(screen.getByRole('button', { name: /dataset\.newKnowledge\.documentActions/ })) + expect( + await screen.findByRole('menuitem', { + name: 'dataset.newKnowledge.downloadDocuments', + }), + ).toHaveAttribute('aria-disabled', 'true') + }) + it('creates a metadata field without scanning or rewriting documents', async () => { const user = userEvent.setup() @@ -3519,7 +3635,57 @@ describe('DocumentsPage', () => { }) }) - it('disables bulk download when any selected document has no active revision', async () => { + it('downloads selected failed documents without active revisions as a ZIP archive', async () => { + const user = userEvent.setup() + documentsQuery.data = { + pages: [ + { + items: [ + document({ + active: null, + activeRevision: undefined, + id: 'failed', + status: 'failed', + title: 'Failed.pdf', + }), + ], + }, + ], + } + tasksQuery.data = { + pages: [ + { + items: [task({ documentId: 'failed', documentRevision: 1, state: 'failed' })], + }, + ], + } + const archive = new File(['documents'], 'failed-documents.zip', { + type: 'application/zip', + }) + downloadDocumentsMutation.mockResolvedValue(archive) + + render() + await user.click(screen.getByRole('checkbox', { name: 'Failed.pdf' })) + const actions = screen.getByRole('group', { + name: 'dataset.newKnowledge.bulkDocumentActions', + }) + const download = within(actions).getByRole('button', { + name: 'dataset.newKnowledge.downloadDocuments', + }) + expect(download).toBeEnabled() + await user.click(download) + + expect(downloadDocumentsMutation).toHaveBeenCalledWith({ + body: { document_ids: ['failed'] }, + params: { control_space_id: 'space-1' }, + }) + expect(downloadBlobMock).toHaveBeenCalledWith({ + data: archive, + fileName: 'failed-documents.zip', + }) + }) + + it('disables bulk download when any selected document is pending', async () => { const user = userEvent.setup() documentsQuery.data = { pages: [ @@ -3530,6 +3696,7 @@ describe('DocumentsPage', () => { active: null, activeRevision: undefined, id: 'pending', + status: 'pending', title: 'Pending.pdf', }), ], diff --git a/web/features/new-rag/document-list.tsx b/web/features/new-rag/document-list.tsx index 4e41b72dcf7..6052ba7d2aa 100644 --- a/web/features/new-rag/document-list.tsx +++ b/web/features/new-rag/document-list.tsx @@ -197,6 +197,7 @@ const DocumentRow = memo( sourcePending, status, statusPending, + tasksPending, }: { canDownload: boolean document: LogicalDocument @@ -219,6 +220,7 @@ const DocumentRow = memo( sourcePending: boolean status: DocumentDisplayStatus statusPending: boolean + tasksPending: boolean }) => { const { t } = useTranslation('dataset') const { t: tCommon } = useTranslation('common') @@ -295,7 +297,7 @@ const DocumentRow = memo( canEdit={!selectionDisabled} documentEnabled={document.enabled} documentTitle={document.title} - downloadDisabled={!document.active || !documentCanDownload(status)} + downloadDisabled={tasksPending || !documentCanDownload(document, status)} onDownload={() => onDownload(document.id)} onRemove={() => onRemove(document.id)} onRename={(title) => onRename(document.id, title)} @@ -659,6 +661,7 @@ export function DocumentsList({ tasksPending || (statusPending && document.sourceId && !sourceNames.has(document.sourceId)), )} + tasksPending={tasksPending} /> ))} diff --git a/web/features/new-rag/document-model.ts b/web/features/new-rag/document-model.ts index b9102c91346..8dbbd204506 100644 --- a/web/features/new-rag/document-model.ts +++ b/web/features/new-rag/document-model.ts @@ -2,8 +2,9 @@ import type { BackgroundTask, DocumentProcessingTask, LogicalDocument } from './ export type DocumentDisplayStatus = 'ready' | 'queued' | 'processing' | 'failed' | 'disabled' -export function documentCanDownload(status: DocumentDisplayStatus) { - return status !== 'queued' && status !== 'processing' +export function documentCanDownload(document: LogicalDocument, status: DocumentDisplayStatus) { + const hasDownloadableRevision = Boolean(document.active) || document.status === 'failed' + return hasDownloadableRevision && status !== 'queued' && status !== 'processing' } export function documentCanReindex(status: DocumentDisplayStatus) { diff --git a/web/features/new-rag/documents-page.tsx b/web/features/new-rag/documents-page.tsx index 6687c8c2f16..5950bacbb26 100644 --- a/web/features/new-rag/documents-page.tsx +++ b/web/features/new-rag/documents-page.tsx @@ -810,6 +810,7 @@ export function DocumentsPage({ knowledgeSpaceId }: { knowledgeSpaceId: string } ), [documentStatuses, documents, t, taskByDocument], ) + const taskResultsIncomplete = Boolean(!tasksQuery.data || tasksQuery.isPending) const filterActive = filter !== 'all' || Boolean(search.trim()) const availableDocumentIds = useMemo( () => @@ -851,12 +852,15 @@ export function DocumentsPage({ knowledgeSpaceId }: { knowledgeSpaceId: string } const downloadableSelectedDocumentIds = useMemo(() => { if ( bulkSelectionInvalid || - selectedDocuments.some((document) => !document.active) || - selectedDocumentStatuses.some((status) => !documentCanDownload(status)) + taskResultsIncomplete || + selectedDocuments.some((document) => { + const status = documentStatuses.get(document.id) + return !status || !documentCanDownload(document, status) + }) ) return [] return selectedDocuments.map((document) => document.id) - }, [bulkSelectionInvalid, selectedDocuments, selectedDocumentStatuses]) + }, [bulkSelectionInvalid, documentStatuses, selectedDocuments, taskResultsIncomplete]) const filteredDocuments = useMemo(() => { const normalizedSearch = search.trim().toLocaleLowerCase() return documents.filter((document) => { @@ -895,7 +899,6 @@ export function DocumentsPage({ knowledgeSpaceId }: { knowledgeSpaceId: string } (sourcesQuery.error && sourcesQuery.data) || sourcesQuery.isFetchNextPageError, ) - const taskResultsIncomplete = Boolean(!tasksQuery.data || tasksQuery.isPending) const sourceResultsIncomplete = Boolean( !sourcesQuery.data || sourcesQuery.isPending || @@ -1931,7 +1934,13 @@ export function DocumentsPage({ knowledgeSpaceId }: { knowledgeSpaceId: string } if (!canDownload || documentActionPendingRef.current) return false const currentDocument = documents.find((document) => document.id === documentId) const status = documentStatuses.get(documentId) - if (!currentDocument?.active || !status || !documentCanDownload(status)) return false + if ( + taskResultsIncomplete || + !currentDocument || + !status || + !documentCanDownload(currentDocument, status) + ) + return false documentActionPendingRef.current = true setPendingDocumentAction({ action: 'download', documentId }) try { @@ -1957,7 +1966,7 @@ export function DocumentsPage({ knowledgeSpaceId }: { knowledgeSpaceId: string } setPendingDocumentAction(undefined) } }, - [canDownload, documentStatuses, documents, knowledgeSpaceId, tCommon], + [canDownload, documentStatuses, documents, knowledgeSpaceId, taskResultsIncomplete, tCommon], ) const handleToggleDocumentAvailability = useCallback(