Fix deletion of selected files in trashbin #30993

sharidas · 2018-04-03T13:18:28Z

When files were selected all, and later
selective files were opted for deletion,
the deletion was not happening. The reason
for this behaviour is filenames are passed
through ajax as seen in ui. Where as that
is not the case. This change helps to fix it.

Signed-off-by: Sujith H [email protected]

Description

The filenames passed in the ajax were the files seen in UI. But that is not the case with trashbin files. This change fixes the issue.

Related Issue

#30942

Motivation and Context

The filenames passed in the ajax were the files seen in UI. But that is not the case with trashbin files. This change fixes the issue.

How Has This Been Tested?

Create 2 files. And delete them.
Select all and then change the decision to select only one. Delete the file. The file is deleted.
The same experiment is repeated with multiple files. And the files were deleted.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2018-04-03T13:21:47Z

apps/files_trashbin/js/filelist.js

+				/*
+				The getSelectedFiles gives the file name as it is seen in UI.
+				But the fact is filenames are storead as filename.dxxxxx. This
+				change helps to extract exact filename.


the filenames are stored like this because you can have multiple files in trashbin with the same name from different deletions. we need to use the suffixed name when talking to the server so I doubt that this approach can work correctly

codecov · 2018-04-03T14:15:21Z

Codecov Report

Merging #30993 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #30993   +/-   ##
=========================================
  Coverage     62.91%   62.91%           
  Complexity    18424    18424           
=========================================
  Files          1156     1156           
  Lines         69184    69184           
  Branches       1260     1260           
=========================================
  Hits          43527    43527           
  Misses        25288    25288           
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.12% <ø> (ø)`	`18424 <ø> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b08fce1...c1b1a0c. Read the comment docs.

sharidas · 2018-04-03T17:41:16Z

apps/files_trashbin/js/filelist.js

+					filesFromAttribute.push(trSelectedFiles[i].getAttribute('data-file'));
+				}
+				if (files !== filesFromAttribute) {
+					files = filesFromAttribute;


@PVince81 This change gives the suffixed name i.e if the file name is a.txt, then the data passed to server will be of the form a.txt.dxxxxxxx. While testing with single and multiple files, I was able to see files getting deleted from trashbin. Also verified files getting deleted from files_trashbin/files. Yes you are right, the issue was caused because suffixed filenames were not passed to the server.

I see, makes sense.

PVince81 · 2018-04-03T19:33:25Z

next up is adjusting unit tests.

you can run them with make test-js-debug and if you need the browser for stepping with a debugger, open "http://localhost:9876/debug.html" (white page) and open the browser console

phil-davis · 2018-04-04T01:58:54Z

Selected delete in trashbin works with this code (manual test).

Selected restore does not work. I expect the same sort of code changes are needed for the code that runs when the "Restore" button is clicked.

(also see recent comment in issue - #30942 (comment) )

sharidas · 2018-04-06T06:41:30Z

I have updated the change for restore issue. Will be looking into the unit tests.

phil-davis · 2018-04-06T10:35:59Z

@sharidas webUI tests for this are available. See the 2nd commit in #31024

The 2 new scenarios fail against current master, and pass when your code is included - good.

Please include those tests in this PR. You will need to rebase this PR up to current master (to get the recent webUI test methods that are used). Then cherry-pick that commit c736a07 into this PR and push it up to GitHub.

phil-davis · 2018-04-06T10:57:51Z

I realized that I can add the test commit myself to this PR, so I have done that.

phil-davis · 2018-04-06T11:26:37Z

@PVince81 unit tests and webUI acceptance tests have been added here.
Ready for review.

PVince81 · 2018-04-06T11:48:10Z

apps/files_trashbin/js/filelist.js

+				for (var i = 0; i < trSelectedFiles.length; i++) {
+					filesFromAttribute.push(trSelectedFiles[i].getAttribute('data-file'));
+				}
+				if (files !== filesFromAttribute) {


I'm not sure this comparison is accurate as it might be comparing only the pointer reference and not the contents. Please use a clearer approach for this check.

PVince81 · 2018-04-06T11:48:39Z

apps/files_trashbin/js/filelist.js

+				for (var i = 0; i < trSelectedFiles.length; i++) {
+					filesFromAttribute.push(trSelectedFiles[i].getAttribute('data-file'));
+				}
+				if (files !== filesFromAttribute) {


duplicate code from above, please move to common method

phil-davis · 2018-04-06T12:54:24Z

@PVince81 I refactored that. And also, _onClickDeleteSelected and _onClickRestoreSelected are specifically in the trashbin JS code, and they are only called to work on selected files, so they can always work on the data-file attribute. There is no need to ever do _.pluck(this.getSelectedFiles(), 'name') and so that removes the need for any comparison of the 2 things (that you commented above.

sharidas · 2018-04-06T13:33:27Z

@phil-davis Even I was having same thought when asked to move the changes to common function. The _pluck(this.getSelectedFiles(), 'name' can be safely removed and no more comparison required.

PVince81 · 2018-04-06T15:05:27Z

apps/files_trashbin/js/filelist.js

@@ -171,7 +186,7 @@
 				};
 			}
 			else {
-				files = _.pluck(this.getSelectedFiles(), 'name');


Now that I understand this PR better, I'm a bit puzzled that the "name" attribute doesn't contain the combined name because the name displayed in the UI is supposed to be "displayName". Maybe that's another bug or regression.

An alternative and potentially better fix would be to adjust this class' elementToFile() to add another attribute on the object containing the combined name.

I did acceptance and JS tests here, and just did a bit of refactoring of the JavaScript code because I thought it looked easy enough and would get this PR finished.

Now that there are classes and attributes and objects to be massaged, I will leave it to someone with real JavaScript coding skills :) @sharidas ?

At least there is a good set of JS unit tests and webUI acceptance tests, so you now easily know if a refactoring breaks some "important" behavior.

The name attribute does contain the compound name (tested on master):

and the "data-file" does contain the compound name, so I don't see the difference that the fix brings in regard to the name:

@sharidas was there something else ?

The problem happens when user select all and the unselect(s) from the list. In my case I tried select all.

The files I had in the trash were a.txt and b.txt. Now I unselected b.txt. Next tried to delete a.txt. Below is the screenshot where name has only a.txt:

Without selecting select all, and if I select specifically a file: a.txt the name becomes:

I guess @PVince81 the second option, right?

hmm, then maybe the selection logic in "apps/files/js/filelist.js" has a general bug and we should rather fix it there instead of working it around in the trashbin list

this will need more debugging in the selection events

PVince81 · 2018-04-06T15:29:33Z

tests/acceptance/features/webUITrashbin/restore.feature

@@ -50,6 +50,29 @@ So that I can recover accidentally deleted files/folders in ownCloud
 		But the file "data.zip" should not be listed in the files page on the webUI
 		And the folder "simple-folder" should not be listed in the files page on the webUI

+	Scenario: Select all except for some trashbin files and restore them in a batch


@phil-davis can you add a case where you delete two entries with the same name ?
basically create a folder "test", then deleted it, then another "test", delete it. they'd appear as two entries.
internally they have names with the timestamp appended (test.dXXXXXXX) but visually they look the same

That will take more work. We have realized this before, but not yet done code to handle it in webUI tests. At the moment the webUI tests do things to file rows (on file page, favorites page, trashbin...) by finding the row with the matching displayed file name.

On favorites and trashbin you can have multiple files in the list with the same name. Currently we have not made code that lets you refer to "the second file called lorem.txt". And then it needs a way to say you want "the lorem.txt that came from folder f1", and that would look in the hover text on trashbin or favorites to see what the "real" location of the file is (which is something that users can do to distinguish which is the file they want to action).

That is not going to happen overnight. i raised issue #31038

sharidas · 2018-04-09T07:41:13Z

Searched till 9.1.1 version. And this is broken in version 9.1.1.

PVince81 · 2018-04-09T12:53:20Z

so version 9.1.0 was fine ? any changes ? git bisect will tell you

sharidas · 2018-04-09T15:21:39Z

Didn't bisected further. But I have my new finding as follows:
When the page loads it populates the files based on an ajax request
http://localhost/index.php/apps/files_trashbin/ajax/list.php?dir=%2F&sort=mtime&sortdirection=desc
The return of the list.php https://github.com/owncloud/core/blob/master/apps/files_trashbin/ajax/list.php provides file name without extension. So when exact filenames are getting populated, in the file listed in UI, would also cause not to get the real file name with extension .dxxxx.
Hence I made a change in the master branch locally, to have another field lets say realname. And then just modifying the call to _.pluck() by giving argument as realname. Even that helps solve the issue.
With the code flow I don't suspect the getSelectedFiles() in the filelist.js from trashbin app. I still believe without breaking the code flow, we can introduce a new field, realname which would help the UI know what's the real name of the file in FS and what is the name to display. I will post the PR updated. Which would help to understand what I am trying to convey.

PVince81 · 2018-04-09T15:28:21Z

Introducing "realname" is likely just a workaround for the real issue.
I suspect that the real issue is inside of getSelectedFiles() and the selection logic. So while it might work it around here it might defer the issue to a later point.

sharidas · 2018-06-06T08:20:57Z

apps/files_trashbin/js/filelist.js

+		/**
+		 * Event handler for when selecting/deselecting all files
+		 */
+		_onClickSelectAll: function(e) {


Override selectAll in trashbin. So The only difference between files/js/filelist.js and this filelist.js is from https://github.com/owncloud/core/pull/30993/files#diff-5016b1abbde02ec31bb639507f64d604R170 to https://github.com/owncloud/core/pull/30993/files#diff-5016b1abbde02ec31bb639507f64d604R176
i.e, addition of line:

this.files[i].name = this.files[i].name + '.d' + Math.floor(this.files[i].mtime/1000);

looks better, need to find a way to prevent that much code duplication, could be done by introducing a new smaller method that can be overridden

PVince81

seems we're getting closer and now I understand the actual problem better

PVince81 · 2018-06-06T16:50:05Z

apps/files_trashbin/js/filelist.js

+					name of the files, i.e make the filenames real so that
+					its appended with '.d' + mtime/1000
+					*/
+					this.files[i].name = this.files[i].name + '.d' +


not good as this would overwrite the existing data structure contents and will cause side effects

you need to make a copy of the file data, only replace the name there, not in the original array

PVince81 · 2018-06-06T16:50:41Z

apps/files_trashbin/js/filelist.js

+		/**
+		 * Event handler for when selecting/deselecting all files
+		 */
+		_onClickSelectAll: function(e) {


looks better, need to find a way to prevent that much code duplication, could be done by introducing a new smaller method that can be overridden

sharidas · 2018-06-08T05:55:58Z

apps/files_trashbin/js/filelist.js

+			Any change to trashbinFiles will not have any change to this.files
+			 */
+			var trashbinFiles = [];
+			this.files.forEach((v, i) => {


Do we have a better way to achieve a copy of this.file update the name or does this look ok? Got this snippet while searching. This change does achieve the goal to not update this.files.

PVince81 · 2018-06-08T07:16:35Z

apps/files/js/filelist.js

@@ -704,15 +704,16 @@
 		/**
 		 * Event handler for when selecting/deselecting all files
 		 */
-		_onClickSelectAll: function(e) {
+		_onClickSelectAll: function(e, modifiedFiles) {


Beware, the function is called from a jQuery event and I'm not sure whether the second param could have arbitrary values. Please double check.

Please update the JSDoc of the function

PVince81

Solution looks great, some small tweak, see comment :-)

phil-davis · 2018-06-08T07:46:21Z

@PVince81 @sharidas time to merge and backport?
It will be nice to have this little fix in 10.0.9

sharidas · 2018-06-08T09:18:14Z

apps/files/js/filelist.js

+			 * Check if there are no arbitrary keys that got into
+			 * modifiedFiles, which can cause irregular behaviours.
+			 */
+			if (modifiedFiles && (modifiedFiles.length === this.files.length)) {


Tried to verify at least the keys in modifiedFiles are same as this.files. Hope this check should be sufficient. If the keys are not same then immediate fall back to this.files is made at https://github.com/owncloud/core/pull/30993/files#diff-dafefd9519924bec4efa9f5af4af8e68R729

comparing length doesn't guarantee that both arrays aren't the same and this approach looks like a hack

please find a more elegant way to solve this

many changes, need re-review

The file selection process provides the result which has file name that doesn't match with the same in the filesystem. This is with reference to the trashbin. So lets say user tries to selectall, the value in this.files doesn't give the name which can be passed to the ajax request to delete or undelete for later actions. So the purpose of this change is to check if the selector is from trashbin, if so adjust the filename as filename + '.d' + mtime/1000. Signed-off-by: Sujith H <[email protected]>

Signed-off-by: Phil Davis <[email protected]>

sharidas · 2018-06-08T14:38:45Z

Backport PR: #31700

PVince81

👍

lock · 2019-07-31T06:02:44Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

sharidas added the 3 - To Review label Apr 3, 2018

sharidas self-assigned this Apr 3, 2018

sharidas requested a review from PVince81 April 3, 2018 13:18

PVince81 reviewed Apr 3, 2018

View reviewed changes

sharidas commented Apr 3, 2018

View reviewed changes

sharidas force-pushed the trashbin-fix branch from d0a2331 to 673664d Compare April 6, 2018 06:38

phil-davis force-pushed the trashbin-fix branch from 673664d to 3806383 Compare April 6, 2018 10:56

phil-davis added the app:files_trashbin label Apr 6, 2018

phil-davis added this to the development milestone Apr 6, 2018

phil-davis added the backport-request label Apr 6, 2018

phil-davis mentioned this pull request Apr 6, 2018

[test] Trashbin fix UI tests #31024

Closed

PVince81 reviewed Apr 6, 2018

View reviewed changes

phil-davis mentioned this pull request Apr 6, 2018

Enhance webUI acceptance tests to handle multiple files with the same name #31038

Closed

2 tasks

sharidas force-pushed the trashbin-fix branch from 045e0de to f10dee8 Compare April 9, 2018 15:21

sharidas removed the status/STALE label Jun 6, 2018

sharidas force-pushed the trashbin-fix branch from ee848eb to 78dcf6e Compare June 6, 2018 08:15

sharidas commented Jun 6, 2018

View reviewed changes

PVince81 suggested changes Jun 6, 2018

View reviewed changes

sharidas force-pushed the trashbin-fix branch 2 times, most recently from 09af122 to 4e6423b Compare June 7, 2018 16:50

sharidas commented Jun 8, 2018

View reviewed changes

sharidas force-pushed the trashbin-fix branch from 4e6423b to 98d1c34 Compare June 8, 2018 06:56

PVince81 reviewed Jun 8, 2018

View reviewed changes

PVince81 previously approved these changes Jun 8, 2018

View reviewed changes

sharidas force-pushed the trashbin-fix branch from 98d1c34 to f879f4d Compare June 8, 2018 09:16

sharidas commented Jun 8, 2018

View reviewed changes

sharidas and others added 3 commits June 8, 2018 20:01

webUI trashbin tests for partial unselection

48ea43e

Signed-off-by: Phil Davis <[email protected]>

Unit tests for trashbin partial unselected delete and restore

c1b1a0c

Signed-off-by: Phil Davis <[email protected]>

sharidas force-pushed the trashbin-fix branch from f879f4d to c1b1a0c Compare June 8, 2018 14:32

sharidas mentioned this pull request Jun 8, 2018

[stable10] Fix deletion of selected files in trashbin #31700

Merged

12 tasks

sharidas removed the backport-request label Jun 8, 2018

PVince81 approved these changes Jun 8, 2018

View reviewed changes

phil-davis merged commit a46b5e6 into master Jun 8, 2018

phil-davis deleted the trashbin-fix branch June 8, 2018 15:09

sharidas mentioned this pull request Jun 11, 2018

Trashbin: cannot permanently delete files with partial unselection. #30942

Closed

PVince81 modified the milestones: development, QA Jun 13, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Fix deletion of selected files in trashbin #30993

Fix deletion of selected files in trashbin #30993

Conversation

sharidas commented Apr 3, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 3, 2018

phil-davis commented Apr 4, 2018 • edited Loading

sharidas commented Apr 6, 2018

phil-davis commented Apr 6, 2018

phil-davis commented Apr 6, 2018

phil-davis commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-davis commented Apr 6, 2018

sharidas commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-davis Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

sharidas commented Apr 9, 2018

PVince81 commented Apr 9, 2018

sharidas commented Apr 9, 2018

PVince81 commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

phil-davis commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Jun 8, 2018

PVince81 left a comment

Choose a reason for hiding this comment

lock bot commented Jul 31, 2019

codecov bot commented Apr 3, 2018 •

edited

Loading

phil-davis commented Apr 4, 2018 •

edited

Loading

phil-davis Apr 6, 2018 •

edited

Loading