Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(file-uploader): fix duplicate upload requests #5818

Merged
merged 6 commits into from
Sep 21, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tough-moons-march.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@aws-amplify/ui-react-storage": patch
---

fix(file-uploader): fix duplicate upload requests
6 changes: 3 additions & 3 deletions packages/react-storage/jest.config.ts
Original file line number Diff line number Diff line change
@@ -12,9 +12,9 @@ const config: Config = {
coverageThreshold: {
global: {
branches: 87,
functions: 90,
lines: 94,
statements: 95,
functions: 86.5,
lines: 93.5,
statements: 94,
},
},
moduleNameMapper: { '^uuid$': '<rootDir>/../../node_modules/uuid' },
Original file line number Diff line number Diff line change
@@ -111,7 +111,6 @@ const FileUploaderBase = React.forwardRef(function FileUploader(
files,
removeUpload,
queueFiles,
setProcessedKey,
setUploadingFile,
setUploadPaused,
setUploadProgress,
@@ -149,7 +148,6 @@ const FileUploaderBase = React.forwardRef(function FileUploader(
onUploadError,
onUploadSuccess,
onUploadStart,
onProcessFileSuccess: setProcessedKey,
setUploadingFile,
setUploadProgress,
setUploadSuccess,
@@ -205,8 +203,7 @@ const FileUploaderBase = React.forwardRef(function FileUploader(
if (typeof onFileRemove === 'function') {
const file = files.find((file) => file.id === id);
if (file) {
// return `processedKey` if available and `processFile` is provided
const key = (processFile && file?.processedKey) ?? file.key;
const key = file.resolvedKey ?? file.key;
onFileRemove({ key });
}
}
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ import {
FileUploader,
MISSING_REQUIRED_PROPS_MESSAGE,
ACCESS_LEVEL_DEPRECATION_MESSAGE,
ACCESS_LEVEL_WITH_PATH_CALLBACK_MESSAGE,
} from '../FileUploader';
import { FileUploaderProps, FileUploaderHandle, FileStatus } from '../types';
import { defaultFileUploaderDisplayText } from '../utils';
@@ -28,7 +29,10 @@ const uploadDataSpy = jest
pause: jest.fn(),
resume: jest.fn(),
state: 'SUCCESS',
result: Promise.resolve({ key: input.key, data: input.data }),
result: Promise.resolve({
key: (input as { path?: string })?.path ?? input.key,
data: input.data,
}),
}));

const fileUploaderProps: FileUploaderProps = {
@@ -155,6 +159,22 @@ describe('FileUploader', () => {
expect(getByText('Select Files')).toBeVisible();
});

it('displays error message when provided with `path` callback and `accessLevel` props', () => {
// turn off error logging in console for test output
jest.spyOn(console, 'error').mockImplementation();

expect(() =>
render(
// @ts-expect-error providign callback `path` and `accessLevel` are disallowd by TS
<FileUploader
accessLevel="private"
path={() => 'path'}
maxFileCount={12}
/>
)
).toThrow(ACCESS_LEVEL_WITH_PATH_CALLBACK_MESSAGE);
});

it('displays error message when file exceeds max file size', () => {
const maxFileSize = 0;
const { getByText } = render(
@@ -324,7 +344,7 @@ describe('FileUploader', () => {
});
});

it('provides the processed file key on a remove file event after upload when processFile is provided', async () => {
it('provides the resolved file key on a remove file event after upload when processFile is provided', async () => {
const onFileRemove = jest.fn();

const processedKey = 'processedKey';
@@ -369,6 +389,7 @@ describe('FileUploader', () => {

it('provides the processed file key on a remove file event after upload when processFile is provided with a path function', async () => {
const onFileRemove = jest.fn();
const path = () => 'my-path';

const processedKey = 'processedKey';
const processFile: FileUploaderProps['processFile'] = (input) => ({
@@ -381,7 +402,7 @@ describe('FileUploader', () => {
{...fileUploaderProps}
onFileRemove={onFileRemove}
processFile={processFile}
path={() => 'my-path'}
path={path}
accessLevel={undefined}
/>
);
@@ -398,17 +419,19 @@ describe('FileUploader', () => {
// Wait for the file to be uploaded
await waitFor(() => {
expect(uploadDataSpy).toHaveBeenCalled();
});

const removeButton = getByTestId(
container,
'storage-manager-remove-button'
);
expect(removeButton).toBeDefined();
const removeButton = getByTestId(
container,
'storage-manager-remove-button'
);
expect(removeButton).toBeDefined();

fireEvent.click(removeButton);
fireEvent.click(removeButton);

expect(onFileRemove).toHaveBeenCalledTimes(1);
expect(onFileRemove).toHaveBeenCalledWith({ key: processedKey });
expect(onFileRemove).toHaveBeenCalledTimes(1);
expect(onFileRemove).toHaveBeenCalledWith({
key: `${path()}processedKey`,
});
});

Original file line number Diff line number Diff line change
@@ -310,7 +310,7 @@ describe('fileUploaderStateReducer', () => {
expect(result.current.state.files).toEqual([file]);
});

it('updates the key of a target file on SET_PROCESSED_FILE_KEY', () => {
it('updates the resolvedKey of a target file on SET_STATUS_SUCESS', () => {
const file: StorageFile = {
id: imageFile.name,
file: imageFile,
@@ -328,18 +328,19 @@ describe('fileUploaderStateReducer', () => {
return { state, dispatch };
});

const processedKey = `processed-${imageFile.name}`;
const resolvedKey = `processed-${imageFile.name}`;
const action: Action = {
type: FileUploaderActionTypes.SET_PROCESSED_FILE_KEY,
type: FileUploaderActionTypes.SET_STATUS_UPLOADED,
id: imageFile.name,
processedKey,
resolvedKey,
status: FileStatus.UPLOADED,
};

expect(result.current.state.files[0].processedKey).toBeUndefined();
expect(result.current.state.files[0].resolvedKey).toBeUndefined();

act(() => result.current.dispatch(action));

expect(result.current.state.files[0].processedKey).toBe(processedKey);
expect(result.current.state.files[0].resolvedKey).toBe(resolvedKey);
});

it('should only change added files to queued in QUEUE_FILES action', () => {
Original file line number Diff line number Diff line change
@@ -124,6 +124,7 @@ describe('useUploadFiles', () => {
act(() =>
result.current.setUploadSuccess({
id: 'file1',
resolvedKey: 'file1',
})
);

@@ -166,18 +167,6 @@ describe('useUploadFiles', () => {
expect(result.current.files.length).toBe(1);
});

it('should update a target file key', () => {
const { result } = renderHook(() => useFileUploader(defaultFiles));

const processedKey = 'processedKey';

expect(result.current.files[0].processedKey).toBeUndefined();

act(() => result.current.setProcessedKey({ id: 'file1', processedKey }));

expect(result.current.files[0].processedKey).toBe(processedKey);
});

describe('defaultFiles', () => {
it('should handle good defaultFiles', () => {
const { result } = renderHook(() =>
Original file line number Diff line number Diff line change
@@ -22,14 +22,6 @@ export const queueFilesAction = (): Action => ({
type: FileUploaderActionTypes.QUEUE_FILES,
});

export const setProcessedKeyAction = (input: {
id: string;
processedKey: string;
}): Action => ({
...input,
type: FileUploaderActionTypes.SET_PROCESSED_FILE_KEY,
});

export const setUploadingFileAction = ({
id,
uploadTask,
@@ -63,6 +55,19 @@ export const setUploadStatusAction = ({
status,
});

export const setUploadSuccessAction = ({
id,
resolvedKey,
}: {
id: string;
resolvedKey: string;
}): Action => ({
type: FileUploaderActionTypes.SET_STATUS_UPLOADED,
id,
resolvedKey,
status: FileStatus.UPLOADED,
});

export const removeUploadAction = ({ id }: { id: string }): Action => ({
type: FileUploaderActionTypes.REMOVE_UPLOAD,
id,
Original file line number Diff line number Diff line change
@@ -46,21 +46,19 @@ export function fileUploaderStateReducer(
case FileUploaderActionTypes.QUEUE_FILES: {
const { files } = state;

const newFiles = files.reduce<StorageFiles>((files, currentFile) => {
return [
const newFiles = files.reduce<StorageFiles>(
(files, currentFile) => [
...files,
{
...currentFile,
...(currentFile.status === FileStatus.ADDED
? { status: FileStatus.QUEUED }
: {}),
},
];
}, []);
return {
...state,
files: newFiles,
};
],
[]
);
return { ...state, files: newFiles };
}
case FileUploaderActionTypes.SET_STATUS_UPLOADING: {
const { id, uploadTask } = action;
@@ -72,11 +70,9 @@ export function fileUploaderStateReducer(

return { ...state, files };
}
case FileUploaderActionTypes.SET_PROCESSED_FILE_KEY: {
const { processedKey, id } = action;
const files = updateFiles(state.files, { processedKey, id });

return { files };
case FileUploaderActionTypes.SET_STATUS_UPLOADED: {
const files = updateFiles(state.files, action);
return { ...state, files };
}
case FileUploaderActionTypes.SET_UPLOAD_PROGRESS: {
const { id, progress } = action;
@@ -95,16 +91,11 @@ export function fileUploaderStateReducer(
const { files } = state;

const newFiles = files.reduce<StorageFiles>((files, currentFile) => {
if (currentFile.id === id) {
// remove by not returning currentFile
return [...files];
}
return [...files, currentFile];
// remove by not returning currentFile
return currentFile.id === id ? [...files] : [...files, currentFile];
}, []);
return {
...state,
files: newFiles,
};

return { ...state, files: newFiles };
}
}
}
Original file line number Diff line number Diff line change
@@ -6,14 +6,15 @@ export interface UseFileUploaderState {
}

export enum FileUploaderActionTypes {
ADD_FILES = 'ADD_FILES',
CLEAR_FILES = 'CLEAR_FILES',
QUEUE_FILES = 'QUEUE_FILES',
SET_STATUS = 'SET_STATUS',
SET_PROCESSED_FILE_KEY = 'SET_PROCESSED_FILE_KEY',
SET_STATUS_UPLOADING = 'SET_STATUS_UPLOADING',
SET_UPLOAD_PROGRESS = 'SET_UPLOAD_PROGRESS',
REMOVE_UPLOAD = 'REMOVE_UPLOAD',
ADD_FILES,
CLEAR_FILES,
QUEUE_FILES,
REMOVE_UPLOAD,
SET_STATUS,
SET_PROCESSED_FILE_KEY,
SET_STATUS_UPLOADED,
SET_STATUS_UPLOADING,
SET_UPLOAD_PROGRESS,
}

export type GetFileErrorMessage = (file: File) => string;
@@ -47,9 +48,10 @@ export type Action =
progress: number;
}
| {
type: FileUploaderActionTypes.SET_PROCESSED_FILE_KEY;
type: FileUploaderActionTypes.SET_STATUS_UPLOADED;
id: string;
processedKey: string;
resolvedKey: string;
status: FileStatus.UPLOADED;
}
| {
type: FileUploaderActionTypes.REMOVE_UPLOAD;
Loading