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

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

Merged
merged 16 commits into from
Dec 11, 2024

Conversation

jalowe13
Copy link
Collaborator

@jalowe13 jalowe13 commented Nov 21, 2024

Overview

APCD For file upload failures, show response for 403 errors

Related

Changes

  • Added a reducer for setting a state to store the error message
  • Using that stored error message for the inline upload file modal

Testing steps to reproduce an error output in an isolated upload modal

server/portal/settings/settings_default.py:230

  1. Change showSubmissions to True

client/src/components/Submissions/Submissions.jsx:22
2. Comment out, then below it write

const submissionSystem = allSystems.find((s) => s.name === 'My Data (Work)')

client/src/components/Submissions/Submissions.jsx:128
3. Comment out, then below it write

const response ={is_submitter: true, isLoading: false,}
  1. Navigate to https://cep.test/workbench/data-submission and attempt to upload a file

UI

image

Notes

  • Must be implemented in Core Portal
  • Data file systems must be loaded at least once

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 8.33333% with 33 lines in your changes missing coverage. Please review.

Project coverage is 72.64%. Comparing base (764fcee) to head (5a5c8ea).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
client/src/redux/sagas/datafiles.sagas.js 0.00% 20 Missing ⚠️
client/src/redux/reducers/datafiles.reducers.js 11.11% 8 Missing ⚠️
...dModalListing/DataFilesUploadModalListingTable.jsx 16.66% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
- Coverage   72.68%   72.64%   -0.05%     
==========================================
  Files         534      534              
  Lines       33656    33681      +25     
  Branches     2989     2989              
==========================================
+ Hits        24464    24466       +2     
- Misses       8994     9017      +23     
  Partials      198      198              
Flag Coverage Δ
javascript 75.34% <8.33%> (-0.07%) ⬇️
unittests 60.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
client/src/components/Submissions/Submissions.jsx 49.03% <100.00%> (ø)
...dModalListing/DataFilesUploadModalListingTable.jsx 53.08% <16.66%> (-2.18%) ⬇️
client/src/redux/reducers/datafiles.reducers.js 33.25% <11.11%> (-0.47%) ⬇️
client/src/redux/sagas/datafiles.sagas.js 47.90% <0.00%> (-0.46%) ⬇️

@jalowe13 jalowe13 marked this pull request as ready for review November 25, 2024 16:10
@@ -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

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!

@jalowe13
Copy link
Collaborator Author

New screenshots added with changes from try catch for http and network errors

Copy link
Collaborator

@jmcmillenmusic jmcmillenmusic left a comment

Choose a reason for hiding this comment

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

LGTM! It throws an error as intended.

@chandra-tacc chandra-tacc merged commit 67a9c23 into main Dec 11, 2024
4 of 6 checks passed
@chandra-tacc chandra-tacc deleted the task/WP-505--APCD--File-Upload-Failures branch December 11, 2024 20:21
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