Project

General

Profile

Actions

Bug #18834

closed

[Workbench2] uploading a file into a subdirectory of a collection does not work, file always goes into the root of the collection

Added by Ward Vandewege 4 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
03/30/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Subtasks 1 (0 open1 closed)

Arvados - Task #18880: Review 18834-uploading-a-file-into-a-subdirectory-of-a-collection-does-not-workResolvedDaniel Kutyła03/30/2022

Actions
Actions #1

Updated by Peter Amstutz 4 months ago

  • Target version changed from 2022-03-30 Sprint to 2022-03-16 sprint
  • Assigned To set to Daniel Kutyła
Actions #2

Updated by Peter Amstutz 4 months ago

  • Target version changed from 2022-03-16 sprint to 2022-03-30 Sprint
Actions #3

Updated by Daniel Kutyła 3 months ago

  • Status changed from New to In Progress
Actions #4

Updated by Daniel Kutyła 3 months ago

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/992852b9506bd3092bd052c681afca966478f88b
Test run: developer-tests-workbench2: #624
Branch: 18834-uploading-a-file-into-a-subdirectory-of-a-collection-does-not-work

Fixed upload to a subdirectory

Actions #5

Updated by Daniel Kutyła 3 months ago

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/44d370d6e0e9e91dbc579e7d990667d52ad67b17
Test run: developer-tests-workbench2: developer-tests-workbench2: #626
Branch: 18834-uploading-a-file-into-a-subdirectory-of-a-collection-does-not-work

Fixed failing unit tests

Actions #6

Updated by Peter Amstutz 3 months ago

  • Target version changed from 2022-03-30 Sprint to 2022-04-13 Sprint
Actions #7

Updated by Lucas Di Pentima 3 months ago

Reviewing arvados-workbench2|c8a07de - developer-tests-workbench2: #631

  • At file collection-service.ts:
    • Methods uploadFiles()/uploadFile(): Instead of having a targetLocation param that completely overrides both collectionUuid and file.name for the final fileURL construction, I think it would be more dev-friendly to add a param called something like destPath with '/' as a default value, so that it's used as another component of the fileURL building code. What do you think? Callers that don't specify it will get their uploads to go to the collection's root dir, and others will be able to specify where exactly files should be uploaded, making the code more readable.
  • Could you add some unit testing on uploadFiles()/uploadFile()? We should be trying to improve the testing coverage whenever we can.
  • What's the fix at arvados-workbench2|44d370d about? Seems unrelated to this branch.
  • The new cypress test code doesn't seem to confirm that the file was really uploaded to the correct "subdir/", it just confirms that it was uploaded. Could you add an assertion on that too? One easy way to do it is to re-request the collection's record and check its manifest data.
Actions #8

Updated by Daniel Kutyła 3 months ago

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/14b107de3fe4c29f4a37f1958bad99e2b82d2b24
Test run: developer-tests-workbench2: developer-tests-workbench2: #644
Branch: 18834-uploading-a-file-into-a-subdirectory-of-a-collection-does-not-work

Added unit tests, made cypress test more strict

Actions #9

Updated by Lucas Di Pentima 3 months ago

Just one observation:

  • One of the new unit tests reveals that the code is issuing an upload request with double-slashes (file src/services/collection-service/collection-service.test.ts:L125). Could you fix the code (and update the test assertion) so that it doesn't do that?

Other than that, LGTM.

Actions #10

Updated by Daniel Kutyła 3 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions #11

Updated by Peter Amstutz about 1 month ago

  • Release set to 51
Actions

Also available in: Atom PDF