Bug #17337
closedFiles not visible in Arvados Workbench 2
Added by Daniel Kutyła over 3 years ago. Updated over 3 years ago.
Description
Yesterday I uploaded some FASTA and BAM files to a collection in Arvados and the files are not visible in the interface. On top it is written that the collection contains 10 files, but in the file list only 2 files are shown. When I switch to Arvados Workbench 1 then all the files are visible in the file list normally.
Side note: "can it be due to the # in the file name?"
Related issues
Updated by Peter Amstutz over 3 years ago
- Target version set to 2021-02-17 sprint
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-02-17 sprint to 2021-03-03 sprint
Updated by Lucas Di Pentima over 3 years ago
I've just tried to reproduce the issue and I was able to see two problems:
- A file including a
#
char doesn't show on wb2 (it does on wb1). - Renaming a preexisting file to include a
#
char will truncate its name up to that char.
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/bf112da6d20b47e94de5d486d943edc66b3e727f
Test run: developer-tests-workbench2: #300
Added uri encoding
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
Some comments:
- On file src/services/collection-service/collection-service-files-response.test.ts a test was added for
getFileFullPath()
which wasn't changed in this branch. Can you also add a test forextractFilesData()
? - Now files with whitespaces on their names aren't displayed on the collection's files panel. Please write a test for this.
- Please check your IDE settings, as it's modifying the indentation on the majority of the cypress tests files, and it's making me nearly impossible to understand if you added something or if it's just all the file that got re-indented. (please explain here as a comment what did you change on
cypress/integration/collection.spec.js
) - Manually trying to upload a file with a whitespace or renaming a file by adding a whitespase makes the file disappear from the listing, I'm not sure why the 'renames a file using valid names' cypress tests aren't failing.
- As you can see on developer-tests-workbench2: #300
/console the
collection.spec.js
file only ran 1 test case, don't know what happened with the rest.
- As you can see on developer-tests-workbench2: #300
- Please check your IDE settings, as it's modifying the indentation on the majority of the cypress tests files, and it's making me nearly impossible to understand if you added something or if it's just all the file that got re-indented. (please explain here as a comment what did you change on
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/eadb2e4a0005b89cb2ca1977bb6f53652e911249
Test run: developer-tests-workbench2: #301
// small test fix below
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/6a6a0ad3791c3d93f9881db8dc5bb30827af0898
Test run: developer-tests-workbench2: #302
Fixed whitespace issue added tests
branch: 17337-files-not-visible-in-arvados
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-03-03 sprint to 2021-03-17 sprint
Updated by Peter Amstutz over 3 years ago
- Related to Bug #17422: Determine if keep-web correctly handles '#' in path added
Updated by Lucas Di Pentima over 3 years ago
- On file
cypress/integration/collection.spec.js
it was added{force: true}
on somerightclick()
call (line 141), can you explain why this is needed on some and not others? (for example, line 374) - As per Cypress' documentation,force: true
disables element actionability checks, is this always desirable, or in certain situations? Asking mostly to learn about Cypress testing strategies for future tests. - The "
should display all files within the collection even with the # sign within the file name
" test case seems to be superfluous, as you can add name transitions on the "renames a file using valid names
" test case (filecypress/integration/collection.spec.js
line 190) avoiding adding a new test case with its set-up time cost. - There're still some cases where this isn't working:
- The 2nd case described at #note-4 is still happening.
- Also when a file has # chars and whitespaces, it disappears from the listings.
- When trying to rename a file with trailing # sign it gets a 412 error. Checking the network inspector it seems that the Destination request header is wrong.
- I've also found similar cases with the
?
sign. - I've added the test cases on cypress so you can make them pass. (you can use wb1 to do the manual testing -- you'll need to click on the lock button before renaming files on wb1)
- Could you write some unit tests for
escapeHashIfRequired()
?
Updated by Lucas Di Pentima over 3 years ago
I’m reading http://xkr.us/articles/javascript/encode-compare/ and it says encodeURIComponent() will encode / and we already allow slashes on ‘Rename’ to move files to a subdir, so take that into account.
Updated by Lucas Di Pentima over 3 years ago
After our call about this issue, I gave it a go on the encodeURI()
vs encodeURIComponent()
differences:
diff --git a/src/services/collection-service/collection-service.ts b/src/services/collection-service/collection-service.ts
index c46c3e27..05f00805 100644
--- a/src/services/collection-service/collection-service.ts
+++ b/src/services/collection-service/collection-service.ts
@@ -76,9 +76,10 @@ export class CollectionService extends TrashableResourceService<CollectionResour
}
async moveFile(collectionUuid: string, oldPath: string, newPath: string) {
+ const encodedNewPath = newPath.split('/').map((c) => encodeURIComponent(c)).join('/');
await this.webdavClient.move(
`c=${collectionUuid}${oldPath}`,
- `c=${collectionUuid}${encodeURI(newPath)}`
+ `c=${collectionUuid}/${encodedNewPath}`
);
await this.update(collectionUuid, { preserveVersion: true });
}
That seems to fix the renaming issue. The other problem is making wb2 being able to properly render a file with a #
char on its name. I've found that a similar problems lies on the file src/services/collection-service/collection-service-files-response.ts
, inside extractFilesData()
, the getTagValue()
function used to get the file's url won't decode the #
chars and leave them as %23
, so I think a solution similar to the above diff would be necessary.
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-03-17 sprint to 2021-03-31 sprint
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/8297f0f273e326e64145a48266805b0b3d073c32
Test run: developer-tests-workbench2: #340
Added custom encoding functions with tests
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
- An
only()
call wasn't removed from the cypress collections test suite. - The above issue masked a failing cypress test called 'renames a file to a different directory'
Updated by Lucas Di Pentima over 3 years ago
I've pushed arvados-workbench2|ef1017fb adding an additional failing file rename case: %
chars.
Updated by Tom Clegg over 3 years ago
In src/common/url.ts, surely this
encodeURIComponent(path.replace(/%2F/g, '/'))
was meant to be
encodeURIComponent(path).replace(/%2F/g, '/')
...and this
decodeURIComponent(path.replace(/\//g, '%2F'));
could be written more simply as
decodeURIComponent(path);
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/f5f72a4ee9b00aab5492f8991677b6503a6f2ac3
Test run: developer-tests-workbench2: #346
Added % sign handling within the filename
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
Test re-run at: developer-tests-workbench2: #347
Updated by Lucas Di Pentima over 3 years ago
- At file
src/common/url.test.ts
- Lines 33 & 47: Test suites with equal names are confusing.
- Lines 34 & 48: I think those tests need better naming.
- Sorry I didn't got this other case earlier, but: wb2 doesn't seem to list files with
%2F
on their names. I've added a test case for that on arvados-workbench2|3ecdcece - At file
src/common/url.ts
lines 24 & 32: is there a reason to catch & ignore errors on these cases? If so, could we add test cases for them?
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/bde7ca868a0c201544476da6c049a98c1188dde9
Test run: developer-tests-workbench2: #351
Added %2F handling, changed the way encode and decode is performed to avoid issues with the '/' characters
branch: 17337-files-not-visible-in-arvados
Updated by Daniel Kutyła over 3 years ago
Test re-run at: developer-tests-workbench2: #352
Updated by Lucas Di Pentima over 3 years ago
I've found some more file name displaying issues. It seems that string literals like %22
get decoded when displaying file names and also this incorrectly decoded file name is used as a source path when the user tries to change the file name to another one.
I've added some cases in Cypress at arvados-workbench2|5436892f
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/5642c48285c296aa3b77760ed5d1431ccdb0f2f3
Test run: developer-tests-workbench2: #356
Added more tests to handle edge cases, fixed double decoding issue
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
Please check why unit tests are failing at developer-tests-workbench2: #356 /console. Thanks!
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/fa8284f51d4022cd36e9db5dd3534b45f1f6b76f
Test run: developer-tests-workbench2: #357
Fixed failing unit test
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
I feel we're almost there! :)
- When I try to download a file with a name that includes a literal string
%20
(for example), workbench2 doesn't encode it as%2520
so the resulting download URL points to an inexistent file, getting a 404. - I've updated the
src/services/collection-service/collection-service-files-response.test.ts
file to coalesce the 2 tests into one with better XML indentation and more test cases, including the one I mentioned yesterday: the double-encoded file name. In those test cases is evident that the "expected url" is the decoded form of the URL, that then needs to be re-encoded to be able to use it, this may be the reason of the issue described on the previous bulletpoint.
Updated by Peter Amstutz over 3 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-03-31 sprint to 2021-04-14 sprint
Updated by Daniel Kutyła over 3 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/5a7d3c08744413b659fd24be59992fc1daa486e5
Test run: developer-tests-workbench2: #360
Added more unit tests and handling for non usual directory names
branch: 17337-files-not-visible-in-arvados
Updated by Lucas Di Pentima over 3 years ago
Just 2 small comments before merging:
- File
src/services/collection-service/collection-service-files-response.ts
lines 33-36: I believe this is used to remove any trailing slashes on the url, correct? If that's the case, wouldn't you think that it could be simpler to add a.replace(/\/$/, '')
call instead? We already make several replacements to the url, so it would be convenient for code consistency's sake. - File
src/services/collection-service/collection-service-files-response.ts
lines 46 & 50: Is there a reason to use the deprecatedunescape()
function instead ofdecodeURI()
ordecodeURIComponent()
? https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/unescape
With those fixed, it LGTM!
Updated by Daniel Kutyła over 3 years ago
- lines 33-36 removes last item from the url to get the parent dir and since directories ends with / first pop for them will return empty string that is why there is a second one
Updated by Daniel Kutyła over 3 years ago
- % Done changed from 0 to 100
- Status changed from In Progress to Resolved
Applied in changeset arvados-workbench2|5385afcada8666051658c6889c83848702497759.