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

[MPDX-8440] Remove TNT import file upload restriction #1180

Merged
merged 2 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
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
89 changes: 0 additions & 89 deletions pages/api/uploads/upload-tnt-connect-import.page.ts

This file was deleted.

48 changes: 0 additions & 48 deletions pages/api/uploads/upload-tnt-connect-import.test.ts

This file was deleted.

14 changes: 7 additions & 7 deletions src/components/Tool/TntConnect/TntConnect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { LoadingSpinner } from 'src/components/Settings/Organization/LoadingSpin
import { ContactTagInput } from 'src/components/Tags/Tags';
import Modal from 'src/components/common/Modal/Modal';
import useGetAppSettings from 'src/hooks/useGetAppSettings';
import { useRequiredSession } from 'src/hooks/useRequiredSession';
import theme from 'src/theme';
import { ToolsGridContainer } from '../styledComponents';
import { uploadTnt, validateTnt } from './uploads/uploadTntConnect';
Expand Down Expand Up @@ -97,6 +98,7 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
const { appName } = useGetAppSettings();
const { t } = useTranslation();
const { enqueueSnackbar } = useSnackbar();
const { apiToken } = useRequiredSession();
const [showModal, setShowModal] = useState(false);
const [loading, setLoading] = useState(false);
const [tntFile, setTntFile] = useState<{
Expand Down Expand Up @@ -125,6 +127,7 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
if (file) {
try {
await uploadTnt({
apiToken,
override: attributes.override,
selectedTags: attributes.selectedTags,
file,
Expand Down Expand Up @@ -152,9 +155,9 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
const handleFileChange: React.ChangeEventHandler<HTMLInputElement> = (
event,
) => {
const f = event.target.files?.[0];
if (f) {
updateTntFile(f);
const file = event.target.files?.[0];
if (file) {
updateTntFile(file);
}
};

Expand All @@ -165,6 +168,7 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
}
};
}, [tntFile]);

const updateTntFile = (file: File) => {
const validationResult = validateTnt({ file, t });
if (!validationResult.success) {
Expand All @@ -174,10 +178,6 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
return;
}

if (tntFile) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old blob will already be released by the useEffect cleanup function on line 164.

// Release the previous file blob
URL.revokeObjectURL(tntFile.blobUrl);
}
setTntFile({ file, blobUrl: URL.createObjectURL(file) });
};

Expand Down
35 changes: 13 additions & 22 deletions src/components/Tool/TntConnect/uploads/uploadTntConnect.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { session } from '__tests__/fixtures/session';
import { uploadTnt } from './uploadTntConnect';

const apiToken = session.user.apiToken;

describe('uploadTnt', () => {
const fetch = jest.fn().mockResolvedValue({
json: () => Promise.resolve({ success: true }),
ok: true,
});
beforeEach(() => {
window.fetch = fetch;
Expand All @@ -16,16 +19,14 @@ describe('uploadTnt', () => {
const textFile = new File(['contents'], 'file.txt', {
type: 'text/plain',
});
const largeFile = new File([new ArrayBuffer(2_000_000)], 'large.xml', {
type: 'text/xml',
});

const selectedTags = ['test', 'tag1'];
const accountListId = '1234';

it('uploads the image', () => {
it('uploads the file', () => {
return expect(
uploadTnt({
apiToken,
override: 'false',
selectedTags,
accountListId,
Expand All @@ -38,6 +39,7 @@ describe('uploadTnt', () => {
it('rejects files that are not xml files', () => {
return expect(
uploadTnt({
apiToken,
override: 'false',
selectedTags,
accountListId,
Expand All @@ -47,40 +49,29 @@ describe('uploadTnt', () => {
).rejects.toThrow('Cannot upload file: file must be a xml file');
});

it('rejects files that are too large', () => {
return expect(
uploadTnt({
override: 'false',
selectedTags,
accountListId,
file: largeFile,
t,
}),
).rejects.toThrow('Cannot upload file: file size cannot exceed 1MB');
});

it('handles server errors', () => {
it('handles network errors', () => {
fetch.mockRejectedValue(new Error('Network error'));

return expect(
uploadTnt({
apiToken,
override: 'false',
selectedTags,
accountListId,
file,
t,
}),
).rejects.toThrow('Cannot upload file: server error');
).rejects.toThrow('Cannot upload file: network error');
});

it('handles success being false', () => {
const fetch = jest.fn().mockResolvedValue({
json: () => Promise.resolve({ success: false }),
fetch.mockResolvedValue({
ok: false,
});
window.fetch = fetch;

return expect(
uploadTnt({
apiToken,
override: 'false',
selectedTags,
accountListId,
Expand Down
40 changes: 19 additions & 21 deletions src/components/Tool/TntConnect/uploads/uploadTntConnect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,19 @@ export const validateTnt = ({
message: t('Cannot upload file: file must be a xml file'),
};
}
// The /api lambda appears to truncate the source body at 2^20 bytes
// Conservatively set the limit at 1MB, which is a little lower than 1MiB because of the
// overhead of encoding multipart/form-data and the other fields in the POST body
if (file.size > 1_000_000) {
return {
success: false,
message: t('Cannot upload file: file size cannot exceed 1MB'),
};
}

return { success: true };
};

export const uploadTnt = async ({
apiToken,
selectedTags,
override,
file,
t,
accountListId,
}: {
apiToken: string;
selectedTags: string[];
override: string;
file: File;
Expand All @@ -45,19 +38,24 @@ export const uploadTnt = async ({
}

const form = new FormData();
form.append('override', override);
form.append('tag_list', selectedTags.join(','));
form.append('file', file);
form.append('accountListId', accountListId);
form.append('data[type]', 'imports');
form.append('data[attributes][override]', override);
form.append('data[attributes][tag_list]', selectedTags.join(','));
form.append('data[attributes][file]', file);

const res = await fetch(`/api/uploads/upload-tnt-connect-import`, {
method: 'POST',
body: form,
}).catch((err) => {
throw new Error(t('Cannot upload file: server error') + err);
const res = await fetch(
`${process.env.REST_API_URL}account_lists/${accountListId}/imports/tnt`,
{
method: 'POST',
headers: {
authorization: `Bearer ${apiToken}`,
},
body: form,
},
).catch(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch throws if there was a network error such that it couldn't make the request it all. It resolves to a response with ok: false if there was an HTTP error (i.e. 4xx or 5xx status code). That's why I switched this error message to be more accurate.

throw new Error(t('Cannot upload file: network error'));
});
const data: { success: boolean } = await res.json();
if (!data.success) {
throw new Error(t('Cannot upload file: server not successful') + data);
if (!res.ok) {
throw new Error(t('Cannot upload file: server not successful'));
}
};
Loading