Bug #15685

File rename doesn't work, succeeds when no name is provided

Added by Eric Biagiotti over 1 year ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
10/22/2020
Due date:
% Done:

100%

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

Description

Renaming a file in a collection with a blank value reports a successful rename to the user even though file doesn't get renamed.


Subtasks

Task #16966: Review 15685-file-renaming-empty-nameResolvedPeter Amstutz


Related issues

Related to Arvados - Bug #16592: File renaming doesn't workResolved09/23/2020

Associated revisions

Revision a0d117b3
Added by Lucas Di Pentima 6 months ago

Merge branch '15685-file-renaming-empty-name'
Closes #15685

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz about 1 year ago

  • Release set to 20

#2 Updated by Peter Amstutz 11 months ago

  • Release changed from 20 to 31

#3 Updated by Peter Amstutz 11 months ago

  • Subject changed from File rename succeeds when no name is provided to File rename doesn't work, succeeds when no name is provided

#4 Updated by Peter Amstutz 7 months ago

  • Target version set to 2020-10-21 Sprint
  • Assigned To set to Daniel Kutyła

#5 Updated by Peter Amstutz 7 months ago

  • Target version changed from 2020-10-21 Sprint to 2020-11-04 Sprint

#6 Updated by Peter Amstutz 7 months ago

  • Assigned To changed from Daniel Kutyła to Lucas Di Pentima

#7 Updated by Lucas Di Pentima 7 months ago

  • Status changed from New to In Progress

#8 Updated by Lucas Di Pentima 7 months ago

Updates at arvados-workbench2|3af60f74 - branch 15685-file-renaming-empty-name
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/136/

  • File renaming was fixed in #16592.
  • Fixes webdav service's backend error response handling.
  • Adds integration tests for file renaming cases.

#9 Updated by Lucas Di Pentima 7 months ago

  • Related to Bug #16592: File renaming doesn't work added

#10 Updated by Lucas Di Pentima 6 months ago

  • Target version changed from 2020-11-04 Sprint to 2020-11-18

#11 Updated by Peter Amstutz 6 months ago

15685-file-renaming-empty-name @ commit:arvados-workflow2|3af60f74e1bb43c779d660bda187d780b677188e

I found a couple of edge cases:

  • I can enter a name of "&" which gets turned into "&amp;"
  • I can enter a name of " "

After renaming a file from a valid name either of the invalid names, I can't rename or delete the file:

Rename gets "Could not rename the file: Forbidden"

Delete gets "Could not remove file."

It needs to correctly handle or prevent these edge cases.

#12 Updated by Peter Amstutz 6 months ago

A couple other illegal names to check for are "." and ".."

#13 Updated by Lucas Di Pentima 6 months ago

Updates at arvados-workbench2|6528692d
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/168/

The edge cases were caused by missing file name decoding or encoding while working with the webdav service:
  • A file named '&' was displayed as '&amp;' because that's how it receives the data from keep-web, so I added a html decoding call.
  • When trying to rename the '&' the MOVE request were directed to '&amp;' instead.
  • A file name can legally be ' ' (whitespace) as per the Ruby SDK (see sdk/ruby/test/test_keep_manifest.rb starting at L307.
  • Expanded file renaming integeration tests.

An interesting side effect of using webdav to rename files is that we can rename a file to something like 'subdir/a_file' and it'll be moved to the 'subdir/' directory. I haven't written test cases for this yet, just in case it isn't a desirable behavior, and if that's the case, we should add validations on the rename form to avoid having '/' chars on the names (right now the validations are done on the backend, and I think that's a good thing, to avoid having duplicated code).

#14 Updated by Peter Amstutz 6 months ago

From chat:

  1. WB2 rename should allow editing the entire file path within the collection. This enables the user to move files into other directories of the collection.
  2. WB2 should not accept names with leading or trailing spaces (even though they are technically allowable on the backend).
    1. This should probably include a validation that splits on '/' and makes sure the path parts don't have leading or trailing spaces either.

#15 Updated by Lucas Di Pentima 6 months ago

Updates at arvados-workbench2|1b9616cd
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/174/

  • WB2 now shows the entire file path on the "rename file" dialog.
  • Added validations so that every path part cannot have leading/trailing whitespaces or be an empty string.
  • Added more integration tests checking these new behaviors.

#17 Updated by Peter Amstutz 6 months ago

15685-file-renaming-empty-name @ arvados-workbench2|1b9616cd4d120cdc473e457637287502fff531b8

Renaming works much better now, thank you! I can even rename invalid files with invalid names getting an error or strange behavior. Moving files into and out of subdirectories seems to work fine.

Moving files be

I found two user interaction bugs.

  1. If I open "Rename" and then enter an invalid name, I don't see an error until the text box loses focus. The "Ok" button remains disabled, but I have no feedback about what is wrong with the name until I try to click outside the text box (hitting enter doesn't do it). After the error appears once, then it appears/disappears immediately based on whether the contents of the text box is valid like I would expect to happen.
  2. The invalid file names "." and ".." are not flagged as errors, it allows you to hit "Ok" but then you get the non-specific error "Could not rename the file: Precondition Failed". Related, I can also include "." and ".." when renaming files, which seems to be interpreted as Unix path, which suggests you might be able to enter a filename like "../../c=collection_uuid/newfile" and it would move the file to another collection (if keep-web supports that.)

#18 Updated by Lucas Di Pentima 6 months ago

Updates at arvados-workbench2|4745e251
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/176/

  • Fixed both issues. The "validation on blur" was just a redux-form config setting.
  • Ajusted integration tests a bit.

#19 Updated by Peter Amstutz 6 months ago

Lucas Di Pentima wrote:

Updates at arvados-workbench2|4745e251
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/176/

  • Fixed both issues. The "validation on blur" was just a redux-form config setting.
  • Ajusted integration tests a bit.

Thanks, this LGTM!

#20 Updated by Anonymous 6 months ago

  • Status changed from In Progress to Resolved

#21 Updated by Peter Amstutz 6 months ago

  • Release changed from 31 to 36

Also available in: Atom PDF