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

#384 - file download error message #387

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented May 10, 2024

This change is Reviewable

@johnml1135 johnml1135 requested a review from ddaspit May 10, 2024 20:37
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 70.77%. Comparing base (85d87f5) to head (dd70bc4).
Report is 1 commits behind head on main.

Files Patch % Lines
src/Serval.Client/Client.g.cs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
- Coverage   70.81%   70.77%   -0.04%     
==========================================
  Files         151      151              
  Lines        7172     7176       +4     
  Branches     1121     1123       +2     
==========================================
  Hits         5079     5079              
- Misses       1638     1642       +4     
  Partials      455      455              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Do we know why this error occurred in the first place?

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, all discussions resolved

Copy link
Collaborator Author

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

I believe there was a crash when uploading (or similar).

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @ddaspit)

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)


src/Serval.DataFiles/Controllers/DataFilesController.cs line 137 at r1 (raw file):

    /// <response code="403">The authenticated client cannot perform the operation or does not own the file</response>
    /// <response code="404">The file does not exist</response>
    /// <response code="500">(Likely cause) File should be on the server but is not present due to internal server error.  Please re-upload file.</response>

A 500 can be caused by any exception. I would prefer to make this more generic. Maybe something like:

The data file is corrupted. Please try reuploading or recreating the file.

@johnml1135
Copy link
Collaborator Author

src/Serval.DataFiles/Controllers/DataFilesController.cs line 137 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

A 500 can be caused by any exception. I would prefer to make this more generic. Maybe something like:

The data file is corrupted. Please try reuploading or recreating the file.

Done.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

@johnml1135 johnml1135 force-pushed the download_file_error_message branch from 8585c65 to 381e470 Compare May 22, 2024 14:55
@johnml1135 johnml1135 merged commit 1ac8c6d into main May 22, 2024
1 of 2 checks passed
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