diff --git a/specifyweb/backend/workbench/upload/upload_attachments.py b/specifyweb/backend/workbench/upload/upload_attachments.py index 0fbb0d7c77b..410647cd0b0 100644 --- a/specifyweb/backend/workbench/upload/upload_attachments.py +++ b/specifyweb/backend/workbench/upload/upload_attachments.py @@ -20,6 +20,16 @@ BASE_TABLE_NAME = "baseTable" ATTACHMENTS_COLUMN = "_UPLOADED_ATTACHMENTS" +def serialize_attachment_field_value(field: str, value) -> str: + if field == "ispublic": + if value is None: + return "false" + return "true" if bool(value) else "false" + return "" if value is None else str(value) + +def attachment_field_default(field: str) -> str: + return "false" if field == "ispublic" else "" + def get_attachments(row: Row): if has_attachments(row): return json.loads(cast(str, row.get(ATTACHMENTS_COLUMN))) @@ -91,7 +101,10 @@ def add_attachments_to_plan( new_row[f"_ATTACHMENT_ORDINAL_{index}"] = str(spdatasetattachment.ordinal) for field in attachment_fields_to_copy: - new_row[f"_ATTACHMENT_{field.upper()}_{index}"] = str(getattr(spdatasetattachment.attachment, field) or "") + new_row[f"_ATTACHMENT_{field.upper()}_{index}"] = serialize_attachment_field_value( + field, + getattr(spdatasetattachment.attachment, field, None), + ) # Inject attachment tables into upload plan table_name = attachment["table"] @@ -121,7 +134,7 @@ def add_attachments_to_plan( column=f"_ATTACHMENT_{field.upper()}_{index}", matchBehavior="ignoreNever", nullAllowed=True, - default="0" + default=attachment_field_default(field) ) attachment_uploadable = UploadTable( name="Attachment", diff --git a/specifyweb/frontend/js_src/lib/components/Attachments/__tests__/uploadFile.test.ts b/specifyweb/frontend/js_src/lib/components/Attachments/__tests__/uploadFile.test.ts index 93767e7c75d..8eddbdf7907 100644 --- a/specifyweb/frontend/js_src/lib/components/Attachments/__tests__/uploadFile.test.ts +++ b/specifyweb/frontend/js_src/lib/components/Attachments/__tests__/uploadFile.test.ts @@ -7,25 +7,47 @@ import { uploadFile } from '../attachments'; requireContext(); describe('uploadFile', () => { - const xhrMock = { - open: jest.fn(), - send: jest.fn(), - // Immediately call the callback - addEventListener, - readyState: 4, - status: Http.OK as number, // Need to loosen up the type. - _error: undefined as unknown, + type EventName = 'abort' | 'error' | 'readystatechange' | 'timeout'; + type MockXhr = { + readonly open: jest.Mock; + readonly send: jest.Mock; + readonly addEventListener: jest.Mock; + readonly removeEventListener: jest.Mock; + readonly upload: { + readonly addEventListener: jest.Mock; + }; + readonly readyState: number; + readonly status: number; + readonly responseText: string; + readonly timeout: number; }; - // Functions get hoisted - function addEventListener(_: string, callback: () => void) { - try { - callback(); - } catch (error) { - console.log('got here!'); - xhrMock._error = error; - } - } + let nextEvent: EventName = 'readystatechange'; + let xhrMock: MockXhr; + + const makeXhrMock = (): MockXhr => { + const listeners: Partial void>> = {}; + + return { + open: jest.fn(), + send: jest.fn((..._args: readonly unknown[]) => listeners[nextEvent]?.()), + addEventListener: jest.fn((eventName: EventName, callback: () => void) => { + listeners[eventName] = callback; + }), + removeEventListener: jest.fn( + (eventName: EventName, callback: () => void) => { + if (listeners[eventName] === callback) listeners[eventName] = undefined; + } + ), + upload: { + addEventListener: jest.fn(), + }, + readyState: 4, + status: Http.OK, + responseText: '', + timeout: 0, + }; + }; let previousImplementation: typeof window.XMLHttpRequest; let previousTelemetry: typeof globalThis.performance.getEntries; @@ -33,15 +55,9 @@ describe('uploadFile', () => { beforeEach(() => { previousImplementation = window.XMLHttpRequest; previousTelemetry = globalThis.performance.getEntries; + xhrMock = makeXhrMock(); - xhrMock.open.mockClear(); - xhrMock.send.mockClear(); - /* - * This is done like this so that a test can alter the return code - * to test errors. - */ - xhrMock.status = Http.OK; - xhrMock._error = undefined; + nextEvent = 'readystatechange'; // @ts-expect-error jest.spyOn(window, 'XMLHttpRequest').mockImplementation(() => xhrMock); @@ -79,9 +95,10 @@ describe('uploadFile', () => { 'POST', attachmentSettings.write ); + expect(xhrMock.timeout).toBe(30 * 60 * 1000); expect(xhrMock.send).toHaveBeenCalledTimes(1); - const formData: FormData = xhrMock.send.mock.calls.at(-1)[0]; + const formData = xhrMock.send.mock.calls.at(-1)?.[0] as FormData; expect(formData.get('token')).toBe(testToken); expect(formData.get('store')).toBe(testAttachmentLocation); @@ -117,7 +134,7 @@ describe('uploadFile', () => { ); expect(xhrMock.send).toHaveBeenCalledTimes(1); - const formData: FormData = xhrMock.send.mock.calls.at(-1)[0]; + const formData = xhrMock.send.mock.calls.at(-1)?.[0] as FormData; expect(formData.get('token')).toBe(newToken); expect(formData.get('store')).toBe(newLocation); @@ -130,4 +147,17 @@ describe('uploadFile', () => { expect(attachment?.get('origFilename')).toBe(testFileName); expect(attachment?.get('title')).toBe(testFileName); }); + + test('fails when upload request times out', async () => { + nextEvent = 'timeout'; + const consoleError = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + + await expect(uploadFile({ file: testFile })).rejects.toThrow( + 'Invalid response code 0.' + ); + expect(consoleError).toHaveBeenCalled(); + consoleError.mockRestore(); + }); }); diff --git a/specifyweb/frontend/js_src/lib/components/Attachments/attachments.ts b/specifyweb/frontend/js_src/lib/components/Attachments/attachments.ts index ade149cf7f8..849fda5e0f8 100644 --- a/specifyweb/frontend/js_src/lib/components/Attachments/attachments.ts +++ b/specifyweb/frontend/js_src/lib/components/Attachments/attachments.ts @@ -50,6 +50,7 @@ export const attachmentSettingsPromise = load>( }); export const attachmentsAvailable = (): boolean => typeof settings === 'object'; +const uploadTimeoutMilliseconds = 30 * 60 * 1000; /* * This function is useful when testing functions that depend on the settings. @@ -264,33 +265,90 @@ export async function uploadFile({ handleProgress(event.lengthComputable ? event.loaded / event.total : true) ); } - xhr.open('POST', settings.write); - xhr.send(formData); const DONE = 4; - await new Promise((resolve, reject) => - xhr.addEventListener('readystatechange', () => { - if (xhr.readyState === DONE) + xhr.open('POST', settings.write); + xhr.timeout = uploadTimeoutMilliseconds; + await new Promise((resolve, reject) => { + let settled = false; + + const handleReadyStateChange = () => { + if (xhr.readyState !== DONE) return; + settle(() => { try { - resolve( - handleAjaxResponse({ - expectedErrors: [], - accept: undefined, - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions - response: { - ok: xhr.status === Http.OK, - status: xhr.status, - url: settings!.write, - } as Response, - - errorMode: strict ? 'visible' : 'silent', - text: xhr.responseText, - }) - ); + handleAjaxResponse({ + expectedErrors: [], + accept: undefined, + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + response: { + ok: xhr.status === Http.OK, + status: xhr.status, + url: settings!.write, + } as Response, + errorMode: strict ? 'visible' : 'silent', + text: xhr.responseText, + }); + resolve(); } catch (error) { reject(error); } - }) - ); + }); + }; + + const handleFailureWithoutResponse = (message: string) => { + settle(() => { + try { + handleAjaxResponse({ + expectedErrors: [], + accept: undefined, + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + response: { + ok: false, + status: 0, + url: settings!.write, + } as Response, + errorMode: strict ? 'visible' : 'silent', + text: message, + }); + reject(new Error(message)); + } catch (error) { + reject(error); + } + }); + }; + + const settle = (callback: () => void) => { + if (settled) return; + settled = true; + cleanup(); + callback(); + }; + + const handleError = () => + handleFailureWithoutResponse( + 'Upload failed before receiving a response.' + ); + const handleAbort = () => + handleFailureWithoutResponse( + 'Upload was aborted before receiving a response.' + ); + const handleTimeout = () => + handleFailureWithoutResponse( + `Upload timed out after ${uploadTimeoutMilliseconds} milliseconds.` + ); + + const cleanup = () => { + xhr.removeEventListener('readystatechange', handleReadyStateChange); + xhr.removeEventListener('error', handleError); + xhr.removeEventListener('abort', handleAbort); + xhr.removeEventListener('timeout', handleTimeout); + }; + + xhr.addEventListener('readystatechange', handleReadyStateChange); + xhr.addEventListener('error', handleError); + xhr.addEventListener('abort', handleAbort); + xhr.addEventListener('timeout', handleTimeout); + xhr.send(formData); + }); return new tables.Attachment.Resource({ attachmentlocation: data.attachmentLocation, diff --git a/specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Upload.tsx b/specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Upload.tsx index e9b9bd05253..4351d80fb5d 100644 --- a/specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Upload.tsx +++ b/specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Upload.tsx @@ -142,6 +142,11 @@ export function AttachmentUpload({ >('main'); const loading = React.useContext(LoadingContext); + const [attachmentIsPublicDefault] = collectionPreferences.use( + 'general', + 'attachments', + 'attachment.is_public_default' + ); React.useEffect(() => { if (upload !== 'confirmed' || baseTableName === undefined) return; @@ -174,8 +179,9 @@ export function AttachmentUpload({ uploadAttachmentSpec: uploadable.uploadTokenSpec, dryRun, triggerRetry, + attachmentIsPublicDefault, }), - [baseTableName] + [baseTableName, attachmentIsPublicDefault] ); const [uploadedCount, setUploadedCount] = React.useState( undefined @@ -282,8 +288,10 @@ async function uploadFileWrapped({ uploadAttachmentSpec, dryRun, triggerRetry, + attachmentIsPublicDefault = false, }: WrappedActionProps & { readonly uploadAttachmentSpec?: UploadAttachmentSpec; + readonly attachmentIsPublicDefault?: boolean; }): Promise { const getUploadableCommited = ({ status, @@ -329,12 +337,6 @@ async function uploadFileWrapped({ if (dryRun) return getUploadableCommited({ status: record }); - const [attachmentIsPublicDefault] = collectionPreferences.use( - 'general', - 'attachments', - 'attachment.is_public_default' - ); - /* * TODO: Make this smarter if it causes performance problems. * Fetch multiple tokens at once diff --git a/specifyweb/frontend/js_src/lib/components/WbImportAttachments/index.tsx b/specifyweb/frontend/js_src/lib/components/WbImportAttachments/index.tsx index 54abf29e444..aeee2eb53b1 100644 --- a/specifyweb/frontend/js_src/lib/components/WbImportAttachments/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/WbImportAttachments/index.tsx @@ -68,14 +68,9 @@ export function WbImportAttachmentsView(): JSX.Element { function uploadFiles( files: RA, - handleProgress: (progress: (progress: number | undefined) => number) => void + handleProgress: (progress: (progress: number | undefined) => number) => void, + attachmentIsPublicDefault: boolean ): RA>> { - const [attachmentIsPublicDefault] = collectionPreferences.use( - 'general', - 'attachments', - 'attachment.is_public_default' - ); - return files.map(async (file) => uploadFile({ file, attachmentIsPublicDefault }) .then(async (attachment) => @@ -126,6 +121,11 @@ async function saveDataSetAttachments( function FilesPicked({ files }: { readonly files: RA }): JSX.Element { const navigate = useNavigate(); + const [attachmentIsPublicDefault] = collectionPreferences.use( + 'general', + 'attachments', + 'attachment.is_public_default' + ); const [fileUploadProgress, setFileUploadProgress] = React.useState< number | undefined >(undefined); @@ -137,7 +137,12 @@ function FilesPicked({ files }: { readonly files: RA }): JSX.Element { ): Promise => { setFileUploadProgress(0); - return Promise.all(uploadFiles(files, setFileUploadProgress)) // Upload all selected files/attachments + return Promise.resolve() + .then(() => + Promise.all( + uploadFiles(files, setFileUploadProgress, attachmentIsPublicDefault) + ) + ) // Upload all selected files/attachments .then(async (attachments) => f .all({ diff --git a/specifyweb/frontend/js_src/lib/components/WorkBench/WbValidation.tsx b/specifyweb/frontend/js_src/lib/components/WorkBench/WbValidation.tsx index 7c47f853114..a723ca61655 100644 --- a/specifyweb/frontend/js_src/lib/components/WorkBench/WbValidation.tsx +++ b/specifyweb/frontend/js_src/lib/components/WorkBench/WbValidation.tsx @@ -11,6 +11,7 @@ import { formatToManyIndex, formatTreeRank, } from '../WbPlanView/mappingHelpers'; +import { ATTACHMENTS_COLUMN } from './attachmentHelpers'; import type { WbCellCounts, WbMeta } from './CellMeta'; import type { UploadResult } from './resultsParser'; import { @@ -216,16 +217,31 @@ export class WbValidation { initialColumns: RA, inferColumnsCallback: (() => RA) | undefined = undefined ): RA { + const resolvePhysicalColumns = (columns: RA): RA => + columns + .map((column) => this.workbench.dataset.columns.indexOf(column)) + .filter((physicalCol) => physicalCol !== -1); + + const normalizeColumns = (columns: RA): RA => + columns.map((column) => + column.startsWith('_ATTACHMENT_') && + this.workbench.dataset.columns.includes(ATTACHMENTS_COLUMN) + ? ATTACHMENTS_COLUMN + : column + ); + // See https://github.com/specify/specify7/issues/810 - let columns: RA = initialColumns.filter(Boolean); - if (typeof inferColumnsCallback === 'function') { - if (columns.length === 0) columns = inferColumnsCallback(); - if (columns.length === 0) columns = this.workbench.dataset.columns; + let columns: RA = normalizeColumns(initialColumns.filter(Boolean)); + let physicalColumns = resolvePhysicalColumns(columns); + + if (typeof inferColumnsCallback === 'function' && physicalColumns.length === 0) { + columns = normalizeColumns(inferColumnsCallback()); + physicalColumns = resolvePhysicalColumns(columns); + if (physicalColumns.length === 0) + physicalColumns = resolvePhysicalColumns(this.workbench.dataset.columns); } - // Convert to physicalCol and filter out unknown columns - return columns - .map((column) => this.workbench.dataset.columns.indexOf(column)) - .filter((physicalCol) => physicalCol !== -1); + + return physicalColumns; } private applyRowValidationResults(