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

feat: add file upload component and refactor fetch to work with formData (#2274) #4742

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

sknep
Copy link
Contributor

@sknep sknep commented Mar 5, 2025

Changes proposed in this pull request:

  • Adds a generic drag-and-drop FileUpload component (and unit tests) based on USWDS's File Upload to @shared, with onUpload, onCancel, and triggerOnMount props for use elsewhere
  • Adds a NewFileOrFolder component (and unit tests) that handles the create folder / upload file interactions with handlers passed in from the parent
  • Hooks up the upload file & create folder logic to the backend via useFileStorage, updates unit tests
  • Simplifies the mutations (particularly success and error messages) by abstracting the copypasta into reusable functions
  • Makes the useFileStorage hook responsible for clearing success/error messages after a given timeout, using state
  • Updates the nock helper to dynamically generate a proper response so we can test multiple pages without having a super long JS file of fake static data.
  • Refactors the fetch wrapper to pass in FormData properly according to the request headers, which is necessary for File uploads to work
  • no uploading state yet, and i'm waiting for the next PR to do dialog confirms, so everything is still using window.alert()

security considerations

  • aside from file size, the frontend does not limit what file types can be uploaded

throw new Error('Failed to fetch public files ' + (err?.message || ''));
},
});
const [uploadError, setUploadError] = useState(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apburnes i know it looks like a lot, but this handles dismissing the alerts (both error and success) after a set duration. I havent added a dismiss click yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok for this PR. Once we pickup the alert mgmt issue, we will pull this into a separate hook.

setFolderName('');
setShowFolderNameField(false);
} catch (error) {
setErrorMessage(error.message || 'Failed to create folder.');
Copy link
Contributor Author

@sknep sknep Mar 6, 2025

Choose a reason for hiding this comment

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

right now this is a little redundant. it will show the error in two places.
Screenshot 2025-03-06 at 5 55 03 PM
same for file upload
Screenshot 2025-03-06 at 5 54 14 PM

@sknep sknep force-pushed the file-storage-upload-2274 branch from ffee6ae to 84018e0 Compare March 7, 2025 17:50
@sknep sknep marked this pull request as ready for review March 7, 2025 17:57
@sknep sknep requested a review from a team March 10, 2025 15:41
@cloud-gov-pages-operations
Copy link
Contributor

🤖 This is an automated code coverage report

Total coverage (lines): 27.1%
Coverage diff: 5.02% 📈

Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

Looks good. In subsequent PR's we will refactor the alerting and I had a question about the request content type after we delete the headers for upload.

throw new Error('Failed to fetch public files ' + (err?.message || ''));
},
});
const [uploadError, setUploadError] = useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok for this PR. Once we pickup the alert mgmt issue, we will pull this into a separate hook.

if (baseConfigs.headers['content-type'] === 'multipart/form-data') {
requestBody = configs.body;
// browser must set this for FormData
delete baseConfigs.headers['content-type'];
Copy link
Contributor

Choose a reason for hiding this comment

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

After an upload, do the following requests have the content-type set to application/json?

Copy link
Contributor Author

@sknep sknep Mar 11, 2025

Choose a reason for hiding this comment

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

they'd get defaultHeaders again which includes 'content-type': 'application/json', unless they have been explicitly given 'content-type'='something-else'

FWIW i think these are supposed to be capitalized "Content-Type" but assumed yall had a reason for the lowercase

@sknep sknep force-pushed the file-storage-upload-2274 branch from 84018e0 to d55aae1 Compare March 11, 2025 17:10
@sknep sknep force-pushed the file-storage-upload-2274 branch from d55aae1 to 86c18e4 Compare March 11, 2025 20:40
@sknep sknep requested a review from drewbo March 11, 2025 20:42
@apburnes apburnes self-requested a review March 11, 2025 22:57
Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

💯 We went over next steps and outlined future issues to build on this upload functionality.

@apburnes apburnes merged commit 468cd78 into main Mar 12, 2025
7 of 8 checks passed
@apburnes apburnes deleted the file-storage-upload-2274 branch March 12, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants