Skip to content

Commit eb8930b

Browse files
slaemmerslaemmer
committed
UIFC-262 Add review changes
1 parent 58f385a commit eb8930b

6 files changed

Lines changed: 104 additions & 70 deletions

File tree

‎src/components/Filters/FilterFile/UploadFile/FileUploaderField.js‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ import { useState, useEffect } from 'react';
33
import { useIntl } from 'react-intl';
44

55
import FileUploaderFieldView from './FileUploaderFieldView';
6-
import { isFileSizeValid, createFileSizeErrorDetails } from '../../../../util/fileUtils';
7-
8-
const HTTP_STATUS_PAYLOAD_TOO_LARGE = 413;
6+
import { isFileSizeValid, createFileSizeErrorDetails, HTTP_STATUS_PAYLOAD_TOO_LARGE } from '../../../../util/fileUtils';
97

108
/**
119
* Helper function to create file size error message
@@ -33,6 +31,15 @@ const createFileSizeErrorMessage = (fileSize, intl) => {
3331
* @returns {Promise<string>} Error message
3432
*/
3533
const handlePayloadTooLargeError = async (response, file, intl) => {
34+
// Check Content-Length header first to avoid fetching large error message bodies
35+
const contentLength = response.headers.get('Content-Length');
36+
if (contentLength) {
37+
const length = parseInt(contentLength, 10);
38+
if (Number.isNaN(length) || length === 0 || length > 200) {
39+
return createFileSizeErrorMessage(file.size, intl);
40+
}
41+
}
42+
3643
try {
3744
const backendMessage = await response.text();
3845
// Sanitize backend message - only show if it's a simple size message
@@ -45,7 +52,7 @@ const handlePayloadTooLargeError = async (response, file, intl) => {
4552
);
4653
}
4754
} catch (error) {
48-
console.error(error); // eslint-disable-line no-console
55+
// Error fetching backend message - fall through to use formatted error
4956
}
5057

5158
return createFileSizeErrorMessage(file.size, intl);
@@ -103,11 +110,10 @@ const FileUploaderField = ({
103110
throw new Error(errorMessage);
104111
} else {
105112
throw new Error(
106-
intl.formatMessage({ id: 'ui-finc-select.filter.file.uploadError.network' })
113+
intl.formatMessage({ id: 'ui-finc-select.filter.file.uploadError' })
107114
);
108115
}
109116
} catch (err) {
110-
console.error(err); // eslint-disable-line no-console
111117
setError(err.message);
112118
setFile({});
113119
} finally {
@@ -130,7 +136,7 @@ const FileUploaderField = ({
130136
FileUploaderField.propTypes = {
131137
input: PropTypes.shape({
132138
onChange: PropTypes.func.isRequired,
133-
value: PropTypes.string,
139+
value: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
134140
}).isRequired,
135141
meta: PropTypes.object,
136142
onUploadFile: PropTypes.func.isRequired,

‎src/components/Filters/FilterFile/UploadFile/FileUploaderField.test.js‎

Lines changed: 70 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { MemoryRouter } from 'react-router-dom';
22
import { Form, Field } from 'react-final-form';
3+
import { act } from 'react';
34

4-
import { waitFor } from '@folio/jest-config-stripes/testing-library/react';
5+
import { waitFor, screen } from '@folio/jest-config-stripes/testing-library/react';
56
import { StripesContext, useStripes } from '@folio/stripes/core';
67

78
import FileUploaderField from './FileUploaderField';
@@ -60,33 +61,30 @@ describe('FileUploaderField', () => {
6061

6162
describe('Validation error timing', () => {
6263
it('should not show validation error when untouched', () => {
63-
const { container } = renderComponent(stripes, {
64+
renderComponent(stripes, {
6465
touched: false,
6566
error: 'Required',
6667
});
6768

68-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
69-
expect(errorElement).not.toBeInTheDocument();
69+
expect(screen.queryByText('Required')).not.toBeInTheDocument();
7070
});
7171

7272
it('should show validation error when touched', () => {
73-
const { container } = renderComponent(stripes, {
73+
renderComponent(stripes, {
7474
touched: true,
7575
error: 'Required',
7676
});
7777

78-
const errorElement = container.querySelector('.errorMessage');
79-
expect(errorElement).not.toHaveAttribute('hidden');
78+
expect(screen.getByText('Required')).toBeInTheDocument();
8079
});
8180

8281
it('should show validation error when submit failed', () => {
83-
const { container } = renderComponent(stripes, {
82+
renderComponent(stripes, {
8483
submitFailed: true,
8584
error: 'Required',
8685
});
8786

88-
const errorElement = container.querySelector('.errorMessage');
89-
expect(errorElement).not.toHaveAttribute('hidden');
87+
expect(screen.getByText('Required')).toBeInTheDocument();
9088
});
9189
});
9290

@@ -103,7 +101,9 @@ describe('FileUploaderField', () => {
103101
text: () => Promise.resolve('file-id-123'),
104102
});
105103

106-
await mockOnDrop([smallFile]);
104+
await act(async () => {
105+
await mockOnDrop([smallFile]);
106+
});
107107

108108
expect(mockOnUploadFile).toHaveBeenCalledWith(smallFile);
109109
await waitFor(() => {
@@ -122,7 +122,9 @@ describe('FileUploaderField', () => {
122122
text: () => Promise.resolve('file-id-456'),
123123
});
124124

125-
await mockOnDrop([maxFile]);
125+
await act(async () => {
126+
await mockOnDrop([maxFile]);
127+
});
126128

127129
expect(mockOnUploadFile).toHaveBeenCalledWith(maxFile);
128130
await waitFor(() => {
@@ -131,85 +133,92 @@ describe('FileUploaderField', () => {
131133
});
132134

133135
it('should reject files over the limit', async () => {
134-
const { container } = renderComponent(stripes);
136+
renderComponent(stripes);
135137

136138
const largeFile = new File(['test'], 'large.txt');
137139
Object.defineProperty(largeFile, 'size', { value: MAX_FILE_SIZE_BYTES + 1 });
138140

139-
await mockOnDrop([largeFile]);
141+
await act(async () => {
142+
await mockOnDrop([largeFile]);
143+
});
140144

141145
expect(mockOnUploadFile).not.toHaveBeenCalled();
142146

143147
await waitFor(() => {
144-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
145-
expect(errorElement).toBeInTheDocument();
148+
expect(screen.getByText(/exceeds the maximum allowed size/i)).toBeInTheDocument();
146149
});
147150
});
148151
});
149152

150153
describe('Upload error handling', () => {
151154
it('should handle 413 with valid backend message', async () => {
152-
const { container } = renderComponent(stripes);
155+
renderComponent(stripes);
153156

154157
const file = new File(['test'], 'test.txt');
155158

156159
mockOnUploadFile.mockResolvedValue({
157160
ok: false,
158161
status: 413,
162+
headers: new Headers({ 'Content-Length': '25' }),
159163
text: () => Promise.resolve('File size limit exceeded'),
160164
});
161165

162-
await mockOnDrop([file]);
166+
await act(async () => {
167+
await mockOnDrop([file]);
168+
});
163169

164170
await waitFor(() => {
165-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
166-
expect(errorElement).toBeInTheDocument();
171+
expect(screen.getByText(/Server rejected the file/i)).toBeInTheDocument();
167172
});
168173
});
169174

170175
it('should sanitize HTML in backend messages', async () => {
171-
const { container } = renderComponent(stripes);
176+
renderComponent(stripes);
172177

173178
const file = new File(['test'], 'test.txt');
174179

175180
mockOnUploadFile.mockResolvedValue({
176181
ok: false,
177182
status: 413,
183+
headers: new Headers({ 'Content-Length': '29' }),
178184
text: () => Promise.resolve('<script>alert("xss")</script>'),
179185
});
180186

181-
await mockOnDrop([file]);
187+
await act(async () => {
188+
await mockOnDrop([file]);
189+
});
182190

183191
await waitFor(() => {
184-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
185-
expect(errorElement).toBeInTheDocument();
186-
expect(errorElement.textContent).not.toContain('<script>');
192+
expect(screen.queryByText(/<script>/)).not.toBeInTheDocument();
193+
expect(screen.getByText(/exceeds the maximum allowed size/i)).toBeInTheDocument();
187194
});
188195
});
189196

190197
it('should reject overly long backend messages', async () => {
191-
const { container } = renderComponent(stripes);
198+
renderComponent(stripes);
192199

193200
const file = new File(['test'], 'test.txt');
194201
const longMessage = 'a'.repeat(250);
195202

196203
mockOnUploadFile.mockResolvedValue({
197204
ok: false,
198205
status: 413,
206+
headers: new Headers({ 'Content-Length': '250' }),
199207
text: () => Promise.resolve(longMessage),
200208
});
201209

202-
await mockOnDrop([file]);
210+
await act(async () => {
211+
await mockOnDrop([file]);
212+
});
203213

204214
await waitFor(() => {
205-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
206-
expect(errorElement).toBeInTheDocument();
207-
expect(errorElement.textContent).not.toContain(longMessage);
215+
expect(screen.queryByText(longMessage)).not.toBeInTheDocument();
216+
expect(screen.getByText(/exceeds the maximum allowed size/i)).toBeInTheDocument();
208217
});
209218
});
210219

211220
it('should handle network errors', async () => {
212-
const { container } = renderComponent(stripes);
221+
renderComponent(stripes);
213222

214223
const file = new File(['test'], 'test.txt');
215224

@@ -218,45 +227,49 @@ describe('FileUploaderField', () => {
218227
status: 500,
219228
});
220229

221-
await mockOnDrop([file]);
230+
await act(async () => {
231+
await mockOnDrop([file]);
232+
});
222233

223234
await waitFor(() => {
224-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
225-
expect(errorElement).toBeInTheDocument();
235+
expect(screen.getByText(/An error occurred during upload/i)).toBeInTheDocument();
226236
});
227237
});
228238

229239
it('should handle rejected promises', async () => {
230-
const { container } = renderComponent(stripes);
240+
renderComponent(stripes);
231241

232242
const file = new File(['test'], 'test.txt');
233243

234244
mockOnUploadFile.mockRejectedValue(new Error('Network failure'));
235245

236-
await mockOnDrop([file]);
246+
await act(async () => {
247+
await mockOnDrop([file]);
248+
});
237249

238250
await waitFor(() => {
239-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
240-
expect(errorElement).toBeInTheDocument();
251+
expect(screen.getByText('Network failure')).toBeInTheDocument();
241252
});
242253
});
243254

244255
it('should handle response.text() failure', async () => {
245-
const { container } = renderComponent(stripes);
256+
renderComponent(stripes);
246257

247258
const file = new File(['test'], 'test.txt');
248259

249260
mockOnUploadFile.mockResolvedValue({
250261
ok: false,
251262
status: 413,
263+
headers: new Headers({ 'Content-Length': '11' }),
252264
text: () => Promise.reject(new Error('Read failed')),
253265
});
254266

255-
await mockOnDrop([file]);
267+
await act(async () => {
268+
await mockOnDrop([file]);
269+
});
256270

257271
await waitFor(() => {
258-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
259-
expect(errorElement).toBeInTheDocument();
272+
expect(screen.getByText(/exceeds the maximum allowed size/i)).toBeInTheDocument();
260273
});
261274
});
262275
});
@@ -268,7 +281,9 @@ describe('FileUploaderField', () => {
268281
const file1 = new File(['test1'], 'test1.txt');
269282
const file2 = new File(['test2'], 'test2.txt');
270283

271-
await mockOnDrop([file1, file2]);
284+
await act(async () => {
285+
await mockOnDrop([file1, file2]);
286+
});
272287

273288
expect(mockOnUploadFile).not.toHaveBeenCalled();
274289
});
@@ -284,23 +299,26 @@ describe('FileUploaderField', () => {
284299
text: () => Promise.resolve('empty-file-id'),
285300
});
286301

287-
await mockOnDrop([file]);
302+
await act(async () => {
303+
await mockOnDrop([file]);
304+
});
288305

289306
expect(mockOnUploadFile).toHaveBeenCalledWith(file);
290307
});
291308

292309
it('should clear errors on successful upload', async () => {
293-
const { container } = renderComponent(stripes);
310+
renderComponent(stripes);
294311

295312
// First, trigger an error
296313
const largeFile = new File(['test'], 'large.txt');
297314
Object.defineProperty(largeFile, 'size', { value: MAX_FILE_SIZE_BYTES + 1 });
298315

299-
await mockOnDrop([largeFile]);
316+
await act(async () => {
317+
await mockOnDrop([largeFile]);
318+
});
300319

301320
await waitFor(() => {
302-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
303-
expect(errorElement).toBeInTheDocument();
321+
expect(screen.getByText(/exceeds the maximum allowed size/i)).toBeInTheDocument();
304322
});
305323

306324
// Now upload a valid file
@@ -312,11 +330,12 @@ describe('FileUploaderField', () => {
312330
text: () => Promise.resolve('valid-file-id'),
313331
});
314332

315-
await mockOnDrop([validFile]);
333+
await act(async () => {
334+
await mockOnDrop([validFile]);
335+
});
316336

317337
await waitFor(() => {
318-
const errorElement = container.querySelector('.errorMessage:not([hidden])');
319-
expect(errorElement).not.toBeInTheDocument();
338+
expect(screen.queryByText(/exceeds the maximum allowed size/i)).not.toBeInTheDocument();
320339
});
321340
});
322341
});

‎src/components/Filters/FilterForm.test.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ describe('FilterForm', () => {
7070
});
7171

7272
test('should display accordions', () => {
73-
expect(screen.getByRole('button', { name: 'Icon General' })).toBeInTheDocument();
74-
expect(screen.getByRole('button', { name: 'Icon File' })).toBeInTheDocument();
75-
expect(screen.getByRole('button', { name: 'Icon Metadata collections' })).toBeInTheDocument();
73+
expect(screen.getByRole('button', { name: 'General' })).toBeInTheDocument();
74+
expect(screen.getByRole('button', { name: 'File' })).toBeInTheDocument();
75+
expect(screen.getByRole('button', { name: 'Metadata collections' })).toBeInTheDocument();
7676
});
7777

7878
test('should display all fields', () => {

0 commit comments

Comments
 (0)