Arvados

Branch 3821-collection-file-manage @ 07e060d

• Added a "Remove selected files" option to the Selection dropdown collection#show page. This allows multiple files to be selected and removed from a collection per request

• Added trash icon(s) to collection file row(s) in collection#show page. This allows removal of that particular file (row) from collection

• Added pencil icon(s) to collection file row(s) in collection#show page. This allows renaming of that particular file in the collection. This is implemented using the current "render_editable_attribute" with appropriate enhancements made to this method. The editable input box displays the full path of the file. The user is allowed to: just change the file name or move it to a different existing subdirectory or even to an entirely new subdirectory

• Added controller tests as well as (minimal required) integration tests

Test run @ https://ci.curoverse.com/job/developer-run-tests/215/

Some comments:

• File apps/workbench/config/routes.rb:
  • Related detail: Could you update the rest of the routes so that instead of using the “:on => :member” style, use the newer “on: :member” for consistency?
• File apps/workbench/test/controllers/collections_controller_test.rb:
  • Lines 644, 669, 693: Instead of asserting against a previously created string variable, wouldn’t be better to prove that the newly created collection’s manifest includes 0:0:file1?
  • Line 683: I think that assert_match is proving that ./file1 & ./file2 exist, but doesn’t prove that ./dir1/ is missing, maybe a $ is missing at the end of the regexp?
• The pencil icon’s tooltip says “edit”, do you think it would be better to say “rename”?
• Renaming a file the same as another file, makes that other file to be deleted. Is that on purpose? If so, maybe we can ask the user for confirmation.

Could you update the rest of the routes so that instead of using the “:on => :member” style, use the newer “on: :member” for consistency?

It would be desirable that "all" references are updated and thoroughly tested. So, didn't want to go there at this time. Will note it for next time around.

File apps/workbench/test/controllers/collections_controller_test.rb: Lines 644, 669, 693: Instead of asserting against a previously created string variable, wouldn’t be better to prove that the newly created collection’s manifest includes 0:0:file1?

Good catch. Updated the assertions

Line 683: I think that assert_match is proving that ./file1 & ./file2 exist, but doesn’t prove that ./dir1/ is missing, maybe a $ is missing at the end of the regexp?

Added the missing $ and also added another assertion fir "dir1".

The pencil icon’s tooltip says “edit”, do you think it would be better to say “rename”?
Renaming a file the same as another file, makes that other file to be deleted. Is that on purpose? If so, maybe we can ask the user for confirmation.

I improved the tooltip on the pencil icon in detailed now. It now says the user can change the file path (name or dir or both) and may not use duplicate names.

Thanks.

LGTM, please merge.