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

Jump to bottom

[Document Repository] Upload multiple files at once #8289

Merged
merged 10 commits into from Apr 5, 2023
Merged

[Document Repository] Upload multiple files at once #8289

merged 10 commits into from Apr 5, 2023

Conversation

jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Dec 19, 2022 •
edited

Brief summary of changes

  • <FileElement> now supports the 'multiple' attribute.
  • The document_repository module has been modified to support multiple uploaded files.
  • Some visual feedback has been added (error reporting + uploading status).

Testing instructions

  • Ensure that another module using <FileElement> still works as intended.
  • Try with different files, file types and folder permissions to ensure proper error reporting.

Related issue: #8218

@jeffersoncasimir jeffersoncasimir added the Feature PR or issue introducing/requiring at least one new feature label Dec 19, 2022
@christinerogers
Copy link
Contributor

tagging Santiago for review of the feature - i think the issue originated from his request? If it was another project/person requesting, feel free to pass this on

@laemtl laemtl added this to the 25.0.0 milestone Feb 28, 2023
@christinerogers
Copy link
Contributor

resolves #8218

@christinerogers
Copy link
Contributor

this PR needs review @miladheshmati @SantiagoTG

@laemtl this is still on the roadmap for 25?

@miladheshmati
Copy link
Contributor

this PR needs review @miladheshmati @SantiagoTG

@laemtl this is still on the roadmap for 25?

@christinerogers, I checked the code, the code seems fine, however, due to some technical problems on my VM, I was not able to test it there.

@laemtl
Copy link
Contributor

laemtl commented Mar 21, 2023

@christinerogers Yes it is still on the roadmap.

@christinerogers
Copy link
Contributor

@miladheshmati any more success testing it this week?

@miladheshmati
Copy link
Contributor

@miladheshmati any more success testing it this week?

There was challenges with my VM but thanks to @CamilleBeau, it is solved now and I was able to test it today

@miladheshmati miladheshmati added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 25, 2023
@miladheshmati
Copy link
Contributor

miladheshmati commented Mar 25, 2023 •
edited

One small issue is that after the previous attempt to upload, the drop-down for the "Site" field seems to have issues. It cannot be changed.

Screenshot 2023-03-25 at 9 30 26 AM

@miladheshmati
Copy link
Contributor

Other than that, uploading multiple files works fine.

Screenshot 2023-03-25 at 9 47 28 AM

@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Mar 28, 2023 •
edited

One small issue is that after the previous attempt to upload, the drop-down for the "Site" field seems to have issues. It cannot be changed.

Thanks for the review! The issue you are experiencing was not introduced by this PR and it is because the 'Site' element is not a <SelectElement> like the fields above and below, but a <SearchableDropdown> element. You will notice that you can also type in this field to narrow the options, and when you delete the text in the field, all options are available again.

driusan
driusan reviewed Mar 28, 2023
View reviewed changes
modules/document_repository/jsx/uploadForm.js Outdated
body: formObject,
})
.then((resp) => {
console.error(resp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is resp always unconditionally logged as an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why that was done but it was not introduced by this PR. It seems to be a forgotten debugging snippet.

Copy link
Contributor

@laemtl laemtl Mar 28, 2023 •
edited

Choose a reason for hiding this comment

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

This can be moved in the logic where the resp returns an error (else case ligne 222)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Done.

@laemtl laemtl added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 4, 2023
@laemtl laemtl changed the base branch from main to 25.0-release April 4, 2023 15:13
@laemtl laemtl removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 4, 2023
@driusan driusan merged commit 8a70e3e into aces:25.0-release Apr 5, 2023
12 checks passed
cmadjar pushed a commit to cmadjar/Loris that referenced this pull request Apr 11, 2023
@jeffersoncasimir @cmadjar
[Document Repository] Upload multiple files at once (aces#8289)
0f31cb9 
- <FileElement> now supports the 'multiple' attribute.
- The document_repository module has been modified to support multiple uploaded files.
- Some visual feedback has been added (error reporting + uploading status).

Resolves aces#8218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@laemtl laemtl laemtl left review comments

@SantiagoTG SantiagoTG Awaiting requested review from SantiagoTG

Assignees

@miladheshmati miladheshmati

Labels
Feature PR or issue introducing/requiring at least one new feature Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jeffersoncasimir @christinerogers @miladheshmati @laemtl @driusan