Arvados

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.

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

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 for extractFilesData()?
• 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.

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

• On file cypress/integration/collection.spec.js it was added {force: true} on some rightclick() 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 (file cypress/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()?

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.

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.

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

• 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'

I've pushed arvados-workbench2|ef1017fb adding an additional failing file rename case: % chars.

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);

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

Test re-run at: developer-tests-workbench2: #347

• 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?

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

Test re-run at: developer-tests-workbench2: #352

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

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

Please check why unit tests are failing at developer-tests-workbench2: #356 /console. Thanks!

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

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.

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

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 deprecated unescape() function instead of decodeURI() or decodeURIComponent()? https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/unescape

With those fixed, it LGTM!

• 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