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

Feature/dls user upload #1596 #1597

Draft
wants to merge 63 commits into
base: develop
Choose a base branch
from
Draft

Conversation

kaperoo
Copy link
Contributor

@kaperoo kaperoo commented Nov 29, 2023

DLS User Upload

This PR adds upload functionality to DLS tables (and cards) via Alex's upload server and Uppy framework.

Added 2 new components:

  • The UploadButton used to open the upload dialog
    image
    The upload button exists in 3 variants: text (in datafiles table), short text (for cards) and an icon.

  • The UploadDialog
    image
    The UploadDialog has 2 variants, one for uploading datasets with name and description textfields, and one with just 'upload' field for the upload of datafiles.

Testing instructions

Created 2 new unit test suites:

  • uploadButton.component.test.tsx
  • uploadDialog.component.test.tsx
    And one e2e:
  • dlsUpload.cy.ts

Also, updated DLS table/card component tests to check for the upload buttons.

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

connect to #1596

@kaperoo kaperoo linked an issue Nov 29, 2023 that may be closed by this pull request
1 task
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

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

Project coverage is 96.16%. Comparing base (794f26e) to head (28ea293).
Report is 11 commits behind head on develop.

Files Patch % Lines
...ateway-common/src/views/uploadDialog.component.tsx 83.92% 9 Missing ⚠️
...ateway-common/src/views/uploadButton.component.tsx 94.44% 2 Missing ⚠️
packages/datagateway-common/src/api/datasets.tsx 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1597      +/-   ##
===========================================
+ Coverage    96.08%   96.16%   +0.07%     
===========================================
  Files          170      172       +2     
  Lines         7209     7454     +245     
  Branches      2260     2387     +127     
===========================================
+ Hits          6927     7168     +241     
- Misses         262      266       +4     
  Partials        20       20              
Flag Coverage Δ
common 95.05% <88.11%> (-0.19%) ⬇️
dataview 97.39% <100.00%> (+0.43%) ⬆️
download 95.99% <ø> (ø)
search 96.55% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@moonraker595
Copy link
Contributor

Hi, I'm going to add some comments here while I integrate the FE back into a containerised environment.

It would be good to have the upload URL set as an environment variable. At the moment I believe its hard coded to http://127.0.0.1:8181/upload/

Something like:

datagateway:
    image: harbor.stfc.ac.uk/datagateway/plugins:pr-1597
    container_name: datgateway_container
    environment:
      - DOWNLOAD_API_URL=https://scigateway-preprod.esc.rl.ac.uk:8181/topcat
      - ICAT_URL=https://icat_payara_container:8181/icat
      - IDS_URL=https://scigateway-preprod.esc.rl.ac.uk:8181/ids
      - UPLOAD_URL=https://whatever_my_upload_backend_is:8080/

@kaperoo
Copy link
Contributor Author

kaperoo commented Feb 20, 2024

uploadUrl should now be set as an environment variable @moonraker595.

@kaperoo
Copy link
Contributor Author

kaperoo commented Apr 10, 2024

A couple of changes and questions:

  1. I have updated the list of restricted file types.
    If there are more restricted file types in the future, maybe we could use the allowedFileTypes from uppy and only specify the ones we allow?

  2. We now use the /commit endpoint after a successful upload.
    If we upload multiple files and some fail, should we only commit the successful ones or none at all?

  3. Added checks for dataset name and duplicate files in the upload queue.

  4. We can easily add the Last Modified date of the file, but the Creation Date may not be possible.
    If I were to send the LM date, should I do it in the file meta or in the call to /commit?

@moonraker595
Copy link
Contributor

moonraker595 commented Apr 10, 2024 via email

@moonraker595
Copy link
Contributor

A couple of changes and questions:

1. I have updated the list of restricted file types.
   If there are more restricted file types in the future, maybe we could use the allowedFileTypes from uppy and only specify the ones we allow?

2. We now use the `/commit` endpoint after a successful upload.
   If we upload multiple files and some fail, should we only commit the successful ones or none at all?

3. Added checks for dataset name and duplicate files in the upload queue.

4. We can easily add the _Last Modified_ date of the file, but the _Creation Date_ may [not](https://stackoverflow.com/questions/27800376/how-to-get-file-creation-date-on-browser-using-javascript-or-jquery) be possible.
   If I were to send the LM date, should I do it in the file meta or in the call to `/commit`?

Have these changes been pushed to harbor? I'm not seeing the call to /commit

@moonraker595
Copy link
Contributor

moonraker595 commented Apr 17, 2024

Hi, thanks for this, I've deployed the latest changes to pre-prod which seems to be working well. 👍
A couple of observations/reminders:

  • I'm not seeing a refresh in the list of datafiles after the upload has closed, this could be due to other network errors happening in the background but I thought I'd flag it
    Apr-17-2024 14-23-30
  • Allowed file types; This would be great so we can just ask diamond for a list
  • We should definitely be throwing the whole upload out if the call to /commit fails. I noticed this when i was implementing the file mod date. During testing the call would fail but everything in uppy was green, except for a 400 status that would pop up and disappear. Is there a way to force uppy to fail should the call to /commit fail despite the upload being successful?
    I've managed to capture a gif of this happening:
    Apr-17-2024 15-18-55

@kaperoo
Copy link
Contributor Author

kaperoo commented Apr 19, 2024

Hi, thanks for your comment @moonraker595 .

We've moved the call to /commit to the upload post-processing. This means that the files will not "turn green" until the /commit call has been successful. This also fixes the table refresh issue.

Because of this (but mostly because of another annoying quirk of uppy), we need to make separate commit calls for the initial upload and (if some files failed) for each successful retry. To do this, we need the response from the initial call to include the id of the created dataset.
Currently we have made this work by slicing the id from the response message.

@moonraker595
Copy link
Contributor

moonraker595 commented Apr 22, 2024

Thanks @kaperoo. The most recent change to the backend code (currently going through review) doesn't return any dataset ids, just the new datafile ids that were created ( i didn't think you needed the dataset ids at the time). So:

  • do you want me to return the dataset id if one was created? So something like:
    { "datasetId": 12345, "datafiles": [ 1, 2, 3 ] }
    or { "datafiles": [ 1, 2, 3 ] } for when no dataset has been created
  • Or i can split out the commit endpoint into 2 if thats easier? So one to create a dataset and one to commit, as it was before.

@kaperoo
Copy link
Contributor Author

kaperoo commented Apr 22, 2024

I don't think we need a separate endpoint for that. The first option you mentioned sounds great.
If it is easier for you, you can always return the existing/newly created datasetId, but we only need it for failed dataset upload retries.

Thanks

@moonraker595
Copy link
Contributor

moonraker595 commented Apr 22, 2024

Hi, this has been implemented and should be in the latest image in harbor (:main). Note : It'll always return the datasetId. The datafiles will always be in an array, even if there's one: { "datasetId": 12345, "datafiles": [ 1 ] }

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.

Add upload functionality to DLS tables
4 participants