Skip to content

Conversation

@RaymondLuong3
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 commented Dec 15, 2025

Serval admins can look at the training data files that have been uploaded for a specific project and can download those files. This will help serval admins more quickly identify issues using training data.

image

This change is Reviewable

@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Dec 15, 2025
@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (45a667d) to head (6712bef).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../serval-administration/serval-project.component.ts 90.90% 1 Missing ⚠️
...ipture/ClientApp/src/xforge-common/file.service.ts 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3614   +/-   ##
=======================================
  Coverage   82.82%   82.82%           
=======================================
  Files         610      610           
  Lines       37420    37438   +18     
  Branches     6132     6160   +28     
=======================================
+ Hits        30992    31008   +16     
+ Misses       5494     5482   -12     
- Partials      934      948   +14     

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

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3651-download-training-files branch from 0816c20 to 046415e Compare December 15, 2025 22:15
@pmachapman pmachapman self-requested a review December 16, 2025 00:04
@pmachapman pmachapman self-assigned this Dec 16, 2025
Copy link
Collaborator

@pmachapman pmachapman 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 - just one comment when a training data file was originally uploaded from an Excel file

@pmachapman reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.ts line 338 at r1 (raw file):

    if (trainingData == null) return this.noticeService.show('File not found');

    this.fileService.onlineDownloadFile(FileType.TrainingData, trainingData.fileUrl, trainingData.title);

title is the original filename that was uploaded and wont end in .csv. All data is stored as CSV on our server (we do not store the original XLS or XLSX files if the user uploads an Excel file). Can you please have this set the file extension to ".csv" if the mimeType is text/csv?

Code quote:

this.fileService.onlineDownloadFile(FileType.TrainingData, trainingData.fileUrl, trainingData.title);

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3651-download-training-files branch from 51aa2e7 to d653b70 Compare January 2, 2026 19:50
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

@RaymondLuong3 made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.ts line 338 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

title is the original filename that was uploaded and wont end in .csv. All data is stored as CSV on our server (we do not store the original XLS or XLSX files if the user uploads an Excel file). Can you please have this set the file extension to ".csv" if the mimeType is text/csv?

Thanks for the suggestion. I put the logic into fileService.ts since it might be useful for the auto-detection of the mime type. Let me know if that makes sense?

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3651-download-training-files branch from d653b70 to 94e1822 Compare January 2, 2026 23:01
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 230 at r2 (raw file):

          // use the .csv extension explicitly if the MIME type is csv
          filename = filename.split('.')[0] + '.csv';
        }

This will return incorrect filenames for filenames like "test.data.xlsx". Something like this might be better?

if (blob.type === 'text/csv') {
  const extensionIndex: number = filename.lastIndexOf('.');
  filename = (extensionIndex !== -1 ? filename.substring(0, extensionIndex) : filename) + '.csv';
}

Code quote:

        if (blob.type === 'text/csv') {
          // use the .csv extension explicitly if the MIME type is csv
          filename = filename.split('.')[0] + '.csv';
        }

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3651-download-training-files branch from 94e1822 to 9cf3b55 Compare January 5, 2026 18:20
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

@RaymondLuong3 made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 230 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This will return incorrect filenames for filenames like "test.data.xlsx". Something like this might be better?

if (blob.type === 'text/csv') {
  const extensionIndex: number = filename.lastIndexOf('.');
  filename = (extensionIndex !== -1 ? filename.substring(0, extensionIndex) : filename) + '.csv';
}

You are right. How does this look?

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

@pmachapman reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3).

@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jan 5, 2026
@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3651-download-training-files branch from 9cf3b55 to 61acef9 Compare January 7, 2026 18:38
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman reviewed 2 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3).

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jan 8, 2026
@Nateowami Nateowami force-pushed the task/sf-3651-download-training-files branch from 296c702 to 6712bef Compare January 8, 2026 15:33
@Nateowami Nateowami enabled auto-merge (squash) January 8, 2026 15:34
@Nateowami Nateowami merged commit 98ec5cd into master Jan 8, 2026
21 checks passed
@Nateowami Nateowami deleted the task/sf-3651-download-training-files branch January 8, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing complete Testing of PR is complete and should no longer hold up merging of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants