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

Improving the file browser for importing metadata #3230

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

Andrea-Guevara
Copy link
Contributor

References

Description

Resolution of the following problem: the code may have tried to set the value of the file input field directly or was linked incorrectly to ngModel. This led to errors because browsers do not allow the value of a file field () to be set programmatically for security reasons.

Instructions for Reviewers

Changes made to the "ds-file-dropzone-no-uploader" component:

  • Removal of ngModel from the file input field.
  • Modification of "EventEmitter" in "@output() onFileAdded".
  • Correction of the onDrop and onDragOver methods making use of events (change, drop, dragover) to manipulate files and the state of the drag and drop area.
  • Addition of event.stopPropagation() to control the behavior of drag and drop events.
  • Creation of the handleFileInput method to correctly process file selection.
  • Modification of the "setFile" method.
  • Modified lines of code: 57, 86, 92, 96, 121.
  • Lines of code added: 88, 95, 110, 111, 112, 113, 114, 115, 116.

To reproduce:

  1. Log in to the repository as an administrator.
  2. Bring up the left sidebar menu, click Import and then click Metadata or Batch Import (ZIP).
  3. Try selecting a file from your machine via file browsing and check that it's working correctly.

@tdonohue tdonohue added bug component: administrative tools Related to the admin menu or tools 1 APPROVAL pull request only requires a single approval to merge component: file upload Relating to uploading files into DSpace port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Aug 1, 2024
Copy link
Member

@kshepherd kshepherd left a comment

Choose a reason for hiding this comment

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

+1 I have tested this and it works perfectly as intended, with no further console errors and both the standard file browser link and file dropzone functioning correctly. Merging immediately. Thanks @Andrea-Guevara !

@kshepherd
Copy link
Member

@Andrea-Guevara thanks for the fix! Great to have these new contributions from you. I have a few small tips that might help you for your next pull requests:

  • You don't need to list lines changed in your PR description - any reviewer will see the changes in the git diff or github 'files changed' overview.
  • Where possible, update or add tests in the relevant spec.ts test component -- we appear not to have full test coverage for the uploader component (see: uploader.component.spec.ts) currently so I wouldn't require it for this change, but that is something to keep an eye out for (I might open a new issue about this test coverage gap soon)
  • Your commit author appears as root <root@TI-03.OHB.LOCAL> -- I would recommend never doing normal userspace stuff as the root user. I'd also recommend setting up a good git configuration for your (non-root!) development user, so that you can be attributed properly for the contributions you are making. Have a look at the git history of the "main" branch to see how most people are setting their attributions (usually a combination of name and email address, but there are no strict rules on this, some people prefer to use aliases)

@kshepherd kshepherd merged commit e7f235a into DSpace:main Aug 3, 2024
13 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-8_x:

@tdonohue tdonohue added this to the 9.0 milestone Aug 5, 2024
@tdonohue tdonohue removed the port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release label Aug 5, 2024
@Andrea-Guevara
Copy link
Contributor Author

Good afternoon @kshepherd, I hope you're well! Thank you very much for your suggestions.

It would be really interesting to have a test file for the component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug component: administrative tools Related to the admin menu or tools component: file upload Relating to uploading files into DSpace
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Cannot use ordinary file browser/selector in Metadata Import (CSV)
4 participants