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.

Sign up for GitHub

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

Task/WP-505: APCD File Upload Failures Logging Request #1013

Merged
merged 16 commits into from
Dec 11, 2024
Merged

Task/WP-505: APCD File Upload Failures Logging Request #1013

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4e2acb4
Adding error messages to upload modal ui
jalowe13 Nov 21, 2024
4514880
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Nov 21, 2024
147b20f
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Nov 22, 2024
fae17f1
response text for 403
jalowe13 Nov 22, 2024
cb765f5
remove logs and testing code
jalowe13 Nov 22, 2024
4129c40
additional testing reversal
jalowe13 Nov 22, 2024
3e5736a
console log removal
jalowe13 Nov 22, 2024
7d07fc6
500 errors for testing
jalowe13 Nov 25, 2024
aa2a5bf
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Nov 27, 2024
60117a9
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Nov 27, 2024
90aabb2
try catch for http and network errors
jalowe13 Nov 27, 2024
686a9e7
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Dec 4, 2024
a877589
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Dec 5, 2024
4921686
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Dec 6, 2024
9112323
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Dec 9, 2024
5a5c8ea
Merge branch 'main' into task/WP-505--APCD--File-Upload-Failures
jalowe13 Dec 10, 2024
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
8 changes: 7 additions & 1 deletion ...ataFiles/DataFilesModals/DataFilesUploadModalListing/DataFilesUploadModalListingTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,25 @@ import { LoadingSpinner, InlineMessage, Button } from '_common';
import { FileLengthCell } from '../../DataFilesListing/DataFilesListingCells';
import { useUpload } from 'hooks/datafiles/mutations';
import styles from './DataFilesUploadModalListingTable.module.scss';
import { useSelector } from 'react-redux';

const DataFilesUploadStatus = ({ i, removeCallback, rejectedFiles }) => {
if (rejectedFiles.filter((f) => f.id === i).length > 0) {
return <InlineMessage type="error">Exceeds File Size Limit</InlineMessage>;
}
const errorMessage = useSelector((state) => state.files.error.message);
const status = useUpload().status[i];
switch (status) {
case 'UPLOADING':
return <LoadingSpinner placement="inline" />;
case 'SUCCESS':
return <span className="badge badge-success">SUCCESS</span>;
case 'ERROR':
return <InlineMessage type="error">Upload Failed</InlineMessage>;
return (
<InlineMessage type="error">
Upload Failed: {errorMessage}
</InlineMessage>
);
default:
return (
<Button type="link" onClick={() => removeCallback(i)}>
Expand Down
2 changes: 1 addition & 1 deletion client/src/components/Submissions/Submissions.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ const Submissions = () => {

const useSubmitterRole = () => {
const query = useQuery({
queryKey: 'submitter-role',
queryKey: ['submitter-role'],
queryFn: getSubmitterRole,
});
return query;
Expand Down
9 changes: 9 additions & 0 deletions client/src/redux/reducers/datafiles.reducers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export const initialFilesState = {
error: {
FilesListing: false,
modal: false,
message: '',
},
listing: {
FilesListing: [],
Expand Down Expand Up @@ -331,6 +332,14 @@ export function files(state = initialFilesState, action) {
[action.payload.section]: setValue,
},
};
case 'DATA_FILES_SET_ERROR':
return {
...state,
error: {
...state.error,
message: action.payload.message,
},
};
case 'DATA_FILES_SET_LOADING':
return {
...state,
Expand Down
8 changes: 8 additions & 0 deletions client/src/redux/sagas/datafiles.sagas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ export async function uploadFileUtil(api, scheme, system, path, file) {
body: formData,
});
if (!request.ok) {
if (request.status === 403 || request.status === 500) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put a try catch around line 488 to line 498 to do the following? A frequent issue we see is the client side network is blocking file uploads (firewall settings), since the firewall is blocking it, sometimes, it might not even be a http response.

try {
    const request = await fetch(url, {
      method: 'POST',
      headers: { 'X-CSRFToken': Cookies.get('csrftoken') },
      credentials: 'same-origin',
      body: formData,
    });

    if (!request.ok) {
      throw new Error(`HTTP error: ${request.status}`);
    }

    return request;

  } catch (error) {
    if (error instanceof TypeError) {
      throw new Error('Network error: The file upload was blocked.');
    }

    throw error;
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

After seeing your screenshot on error message, I'm not sure how much that error is going to help or cause more problems. We want to know the user to know if file upload is blocked vs server http error.

May be a simpler error message:

  1. http response code - put the response code and say file upload server failed with response code: {}
  2. when there is no response code, then show the message TypeError catch.

Copy link
Collaborator Author

@jalowe13 jalowe13 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After seeing your screenshot on error message, I'm not sure how much that error is going to help or cause more problems. We want to know the user to know if file upload is blocked vs server http error.

May be a simpler error message:

  1. http response code - put the response code and say file upload server failed with response code: {}
  2. when there is no response code, then show the message TypeError catch.

@chandra-tacc
This is a great suggestion. It would be best to test for both types of occurrences when a file is uploaded. One where the firewall is blocking the request (the catch variant) and the other for when the POST request is successful. I'll edit the success message down as well as to not overcomplicate things. Thank you for the thorough review Chandra!

const responseText = await request.text();
throw new Error(responseText);
}
throw new Error(request.status);
}
return request;
Expand Down Expand Up @@ -548,6 +552,10 @@ export function* uploadFile(api, scheme, system, path, file, index) {
type: 'DATA_FILES_SET_OPERATION_STATUS_BY_KEY',
payload: { status: 'ERROR', key: index, operation: 'upload' },
});
yield put({
type: 'DATA_FILES_SET_ERROR',
payload: { message: e.toString() },
});
return 'ERR';
}
return 'SUCCESS';
Expand Down