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

UI: Handle Empty File Uploads in FileUploader #1802

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

Samk13
Copy link
Member

@Samk13 Samk13 commented Aug 27, 2024

❤️ Thank you for your contribution!

Description

upload-empty-files

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

@Samk13 Samk13 changed the title WIP: handle empty files and upload non-empty files WIP: handle upload empty files Aug 27, 2024
@Samk13 Samk13 force-pushed the fix-empty-files-error branch 4 times, most recently from 400d2c8 to d94136e Compare August 28, 2024 10:50
@Samk13 Samk13 changed the title WIP: handle upload empty files UI: Handle empty file uploads in FileUploader Aug 28, 2024
@Samk13 Samk13 changed the title UI: Handle empty file uploads in FileUploader UI: Handle Empty File Uploads in FileUploader Aug 28, 2024
Samk13 added a commit to Samk13/invenio-app-rdm that referenced this pull request Aug 28, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
@Samk13 Samk13 force-pushed the fix-empty-files-error branch 5 times, most recently from f50bb40 to 2bc83b4 Compare August 29, 2024 15:19
@Samk13 Samk13 marked this pull request as ready for review August 29, 2024 15:19
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! Just a few comments to make it a bit more readable!

Comment on lines 84 to 85
const emptyFiles = acceptedFiles.filter((file) => file.size === 0);
const nonEmptyFiles = acceptedFiles.filter((file) => file.size > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor optimization... we ended up having 3 times filter, maybe we can loop once?

Suggested change
const emptyFiles = acceptedFiles.filter((file) => file.size === 0);
const nonEmptyFiles = acceptedFiles.filter((file) => file.size > 0);
const filesNames = _map(filesList, "name");
let hasEmptyFiles = false;
let hasNonEmptyFiles = false;
let hasDuplicatedFiles = false;
let nonEmptyFiles = [];
for (const file of acceptedFiles) {
if (file.size === 0) {
hasEmptyFiles = true;
} else {
hasNonEmptyFiles = true;
nonEmptyFiles.push(file);
}
if (filesNames.includes(file.name)) {
hasDuplicatedFiles = true;
}
}

Copy link
Member Author

@Samk13 Samk13 Sep 13, 2024

Choose a reason for hiding this comment

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

I opted to use reduce to accumulate duplicate, empty, and non-empty files in a single pass. This allows us to track file names for warnings while maintaining efficiency and flexibility, as booleans alone can't e.g track file names.
Let me know if this works or if there's a better approach.

Samk13 added a commit to Samk13/invenio-app-rdm that referenced this pull request Sep 13, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
@Samk13 Samk13 force-pushed the fix-empty-files-error branch 2 times, most recently from 2b84f65 to 80bde7a Compare September 14, 2024 00:10
Samk13 added a commit to Samk13/invenio-app-rdm that referenced this pull request Sep 14, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
Samk13 added a commit to Samk13/invenio-app-rdm that referenced this pull request Sep 14, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
* Display a warning for empty files, indicating they won't be included and listing file names.
* Feature controlled by `records-resources-allow-empty-files` config value.
* Continue uploading other files while showing the warning message.
* Add the missing translation tags.
* Refactor the logic to display multiple warning messages, while allowing other files to upload, except for case where exceeding the file size limit.
@Samk13
Copy link
Member Author

Samk13 commented Sep 14, 2024

@ntarocco Since you wanted to improve readability, I've added a separate commit where I refactored the component. Now, the uploader continues uploading the rest of the files while showing warnings for duplicate files or empty files (was not possible before). I've also added other missing translations and small fixes.
So you can review this commit separately.
Screenshot from 2024-09-14 21-57-04

Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

Fantastic improvement and very nice and readable implementation 👏🏼

@ntarocco ntarocco merged commit 6d226ce into inveniosoftware:master Sep 19, 2024
4 checks passed
ntarocco pushed a commit to inveniosoftware/invenio-app-rdm that referenced this pull request Sep 19, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
Samk13 added a commit to inveniosoftware/invenio-app-rdm that referenced this pull request Sep 19, 2024
* Expose `RECORDS_RESOURCES_ALLOW_EMPTY_FILES` for UI control
* Related to: inveniosoftware/invenio-rdm-records#1802
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.

Backend errors not shown in record file upload screen and failed files cannot remove
3 participants