Skip to content

Conversation

@adfost
Copy link
Contributor

@adfost adfost commented Jul 8, 2021

Delete multiple objects using selectors on object list

Screen Shot 2021-07-08 at 12 44 02 PM

@dvaldivia dvaldivia requested review from Alevsk, bexsoft, dvaldivia and jinapurapu and removed request for bexsoft July 9, 2021 16:47
@dvaldivia
Copy link
Collaborator

When the button for Delete Selected becomes enabled, the icon keeps it's original color so it can't be seen, it should flip to white, same for the font,

Screen Shot 2021-07-09 at 9 51 40 AM

After deleting an object, the button doesn't reset to it's original state

Screen Shot 2021-07-09 at 9 52 44 AM

@adfost adfost force-pushed the delete_multiple_buckets branch 2 times, most recently from d4eae3d to a8f96ff Compare July 10, 2021 01:43
dvaldivia
dvaldivia previously approved these changes Jul 14, 2021
Copy link
Collaborator

@dvaldivia dvaldivia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testd. lgtm

Comment on lines 100 to 102
for (var i = 0; i < selectedObjects.length; i++) {
removeRecord(i);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will trigger multiple delete object calls, and the removeRecord inside has

        setDeleteLoading(false);
        closeDeleteModalAndRefresh(true);

that will be called multiple times as well, @adfost its better to implement a new endpoint called bulkDelete that receive a list of object IDs and then perform the delete operations on the backend, once all objects are deleted you return operation success to the frontend, that way you will have only 1 call instead of multiple ones

@adfost adfost force-pushed the delete_multiple_buckets branch 2 times, most recently from f0a1779 to 283e74a Compare July 15, 2021 17:18
api
.invoke(
"DELETE",
`/api/v1/buckets/${selectedBucket}/delete-multiple?path=${selectedObjects}&recursive=${recursive}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are sending multiple paths to delete, I think it's better so send them in the body of the request as a string array instead of sending them in the query. This may cause troubles in the future.

Please change API accordingly

Suggested change
`/api/v1/buckets/${selectedBucket}/delete-multiple?path=${selectedObjects}&recursive=${recursive}`
`/api/v1/buckets/${selectedBucket}/delete-multiple?recursive=${recursive}`, {path: selectedObjects}

Comment on lines 882 to 892
<Button
variant="contained"
color="primary"
startIcon={<DeleteIcon />}
onClick={() => {
setDeleteMultipleOpen(true);
}}
disabled={selectedObjects.length === 0}
>
Delete Selected
</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the position of this button as this is not the main action that we want to promote. Please change it before the create folder button:

125111859-452c0e80-e09b-11eb-8f6d-8a6242dc2a02

bexsoft
bexsoft previously approved these changes Jul 19, 2021
@adfost adfost force-pushed the delete_multiple_buckets branch from 109bd9c to 9bb6b00 Compare July 20, 2021 21:40
dvaldivia
dvaldivia previously approved these changes Jul 20, 2021
Copy link
Collaborator

@dvaldivia dvaldivia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. LGTM

Alevsk
Alevsk previously approved these changes Jul 21, 2021
dvaldivia
dvaldivia previously approved these changes Jul 21, 2021
@dvaldivia dvaldivia dismissed stale reviews from Alevsk and themself via da0ff96 July 21, 2021 22:45
@dvaldivia dvaldivia merged commit 2e1e4e4 into minio:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants