Delete folder or file from file group #744

ascobie · 2017-10-06T04:04:59Z

Only allow deletion from a storage container. Ignore files and folders on compute nodes.
Storage API only allows deleting blobs one at a time.

timotheeguerin · 2017-10-06T04:15:21Z

app/components/file/browse/blob-files-browser/blob-files-browser.component.ts

@@ -36,6 +40,7 @@ export class BlobFilesBrowserComponent implements OnChanges, OnDestroy {
        if (inputs.upload) {
            this.fileExplorerConfig = {
                canDropExternalFiles: Boolean(this.upload),
+                canDeleteFiles: true,


same as above Boolean(this.delete)

timotheeguerin · 2017-10-06T04:20:44Z

app/components/file/browse/blob-files-browser/blob-files-browser.component.ts

+        // TODO: Check with Tim as to whether i can chain 2 background tasks to run one after another.
+        // In which case most of this can be moved into DataContainerFilesComponent.
+        // task 1: get files to delete
+        // task 2: delete files


you could but I don't think thats a good idea, it would look weird. Maybe what you could do is have it send back an observable that resolve files instead of the actual files so the waiting can be done in the background task

but otherwise I think its all good like that for now until we make that generic FileSystem

timotheeguerin · 2017-10-06T04:25:22Z

app/components/file/browse/blob-files-browser/blob-files-browser.component.ts

+            yes: () => {
+                const listParams = { recursive: true, startswith: path };
+                const data = this.storageService.listBlobs(Promise.resolve(this.container), listParams);
+                return data.fetchAll().flatMap(() => data.items.take(1)).map((items) => {


i would not use .map here because it won't run unless you subscribe to it. I know this funciton is being passed up to the dialog which subscribe to it but it is a bit confusing to actually do state actions in the map. I would just have it
const obs = ...
obs.subscribe(...)
return obs;

timotheeguerin · 2017-10-06T04:25:39Z

app/components/data/details/data-container-files.component.ts

+
+        return this.backgroundTaskService.startTask(taskTitle, (task) => {
+            let deleted = 0;
+            let observable = Observable.interval(250).take(fileCount);


…ure/delete-group-files

timotheeguerin · 2017-10-13T01:26:46Z

app/components/data/details/data-container-files.component.ts

+                            task.progress.next(deleted / fileCount * 100);
+
+                            if (response && blobCache) {
+                                blobCache.deleteItem(files[i]);


shouldn't it be the deleteBlobIfexists that remove from the cache. I feel this would be better. THat way you don't have to do that everytime

on a .subscribe(() => { /* remove from cache */ })?

probably a very good idea and one that i am not sure why i didn't think of :)

timotheeguerin · 2017-10-13T01:27:21Z

app/components/data/details/data-container-files.component.ts

+                complete: () => {
+                    task.progress.next(100);
+                    // tslint:disable-next-line:max-line-length
+                    const message = `${deleted} files were successfully removed from the file group: ${this.container.name}`;


just a note this is where I think starting to think about i18n would be good. Long string like that don't look good in the code

for sure, and every time i see loads of UI text in the code i think the same thing.

timotheeguerin · 2017-10-13T01:27:39Z

app/components/data/details/data-container-files.component.ts

+                    task.progress.next(100);
+                    // tslint:disable-next-line:max-line-length
+                    const message = `${deleted} files were successfully removed from the file group: ${this.container.name}`;
+                    this.notificationService.success("Removed files from group", message, { persist: true });


do we really want to persist that one?

probably not, though i saw it was in a few other places. can remove.

yeah error and warning are persisted by default but info and success disapear.
I force it to displayed for stuff where you have actions that you might want to do

timotheeguerin · 2017-10-13T01:29:14Z

app/components/file/browse/blob-files-browser/blob-files-browser.component.ts

+    public handleDeleteEvent(event: FileDeleteEvent) {
+        const { path } = event;
+        this.dialogService.confirm(`Delete files`, {
+            description: event.isDirectory


can you make a description variable no to have this ternary condition inline

timotheeguerin · 2017-10-13T01:30:01Z

app/components/file/browse/blob-files-browser/blob-files-browser.component.ts

+            yes: () => {
+                const listParams = { recursive: true, startswith: path };
+                const data = this.storageService.listBlobs(this.container, listParams);
+                const obs = data.fetchAll().flatMap(() => data.items.take(1));


add .shareReplay(1) at the end so it doesn't trigger the flatMap for every subscription

timotheeguerin · 2017-10-13T01:31:01Z

app/services/file/file-navigator/file-navigator.ts

    }

    /**
     * Load the inital data
     */
    public init() {
        this._loadFilesInPath("");
+        if (this._cache) {
+            this._fileDeleted = this._cache.deleted.subscribe((key: string) => {


nitpick:_fileDeletedSub

timotheeguerin · 2017-10-13T03:37:12Z

app/components/data/details/data-container-files.component.ts

-            });
-
-            return observable;
+        // TODO: would like to get the deleted count from this call.


use next instead of complete and make the service return the number of files

do you mean the number of files(because you can just do files.length above) or the number of files that actually existsed and were deleted?

timotheeguerin · 2017-10-13T03:37:33Z

app/components/data/details/data-container-files.component.ts

+            error: (error) => {
+                // tslint:disable-next-line:max-line-length
+                const message = `Not all files were able to be removed from the file group: ${this.container.name}`;
+                this.notificationService.error("Error while deleting files", message, { persist: true });


don't need to specify presist true for error its the default

timotheeguerin · 2017-10-13T03:39:54Z

app/services/storage.service.ts

+            const observable = Observable.interval(100).take(fileCount);
+            observable.subscribe({
+                next: (i) => {
+                    deleted++;


isn't deleted the same as i?

yes, though i need to change it to be the actual number of deleted files.

but actually what do you expect to show if the number is different? Some files wasn't deleted successfully or some files were already deleted?

because of option 2 I don't really see the point of showing 23/26 files deleted. This feels more confusing to me and let you wonder why there is three that are not deleted(even though they actually are)

i expected i would show a warning that not all files were able to be deleted. but maybe i just shouldn't bother ....

set up deletion context menu item and trigger delete event

56f212c

ascobie added 💀 don't merge in progress labels Oct 6, 2017

ascobie self-assigned this Oct 6, 2017

ascobie requested a review from timotheeguerin October 6, 2017 04:05

timotheeguerin reviewed Oct 6, 2017

View reviewed changes

ascobie and others added 17 commits October 9, 2017 12:15

Merge branch 'master' into feature/delete-group-files

a2159d0

merge master into feature branch

73547e8

hook up delete, need hand with cache

150a321

remove container promise from storage service

1c0347d

checking in so i can look at Daves issue on master

b5ecd0d

Merge branch 'master' into feature/delete-group-files

f8383b2

working! but ugly!

52405a6

Fix delete

43ef437

Find

64664be

backgroup task changes, part cleanup

ea15f81

handle correct root directory

289cbf2

Merge branch 'master' of https://github.com/Azure/BatchLabs into feat…

a132e1e

…ure/delete-group-files

fix not loading root file

8a7eae9

Fix

155e7cb

fix backgroup mananger task

4ac648a

Merge branch 'master' into feature/delete-group-files

a209226

tests for delete node

2aadf67

ascobie removed 💀 don't merge in progress labels Oct 13, 2017

ascobie added this to the 0.8.0 milestone Oct 13, 2017

Merge branch 'master' into feature/delete-group-files

7f77c1e

timotheeguerin reviewed Oct 13, 2017

View reviewed changes

address feedback

06b76b6

timotheeguerin reviewed Oct 13, 2017

View reviewed changes

ascobie added 3 commits October 16, 2017 09:35

Merge branch 'master' into feature/delete-group-files

edee8f6

cleanup

965ef20

merge from master after storage-service conflict

0a336b4

ascobie merged commit 3c24947 into master Oct 17, 2017

ascobie deleted the feature/delete-group-files branch October 17, 2017 02:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete folder or file from file group #744

Delete folder or file from file group #744

ascobie commented Oct 6, 2017 •

edited

Loading

timotheeguerin Oct 6, 2017

timotheeguerin Oct 6, 2017

timotheeguerin Oct 6, 2017

timotheeguerin Oct 6, 2017

timotheeguerin Oct 6, 2017

timotheeguerin Oct 13, 2017

ascobie Oct 13, 2017

timotheeguerin Oct 13, 2017

ascobie Oct 13, 2017

timotheeguerin Oct 13, 2017

ascobie Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

timotheeguerin Oct 13, 2017

ascobie Oct 14, 2017

timotheeguerin Oct 14, 2017 •

edited

Loading

ascobie Oct 15, 2017

Delete folder or file from file group #744

Delete folder or file from file group #744

Conversation

ascobie commented Oct 6, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheeguerin Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascobie commented Oct 6, 2017 •

edited

Loading

timotheeguerin Oct 14, 2017 •

edited

Loading