feat: add confirmation dialog when deleting objects

[cdrage]

feat: add confirmation dialog when deleting objects

What does this PR do?

Adds confirmation dialogs to all critical components of Podman Desktop
where you are deleting a container, pod, deployment, service, etc.

This makes sure you do not accidently delete something (such as a
deployment) that could be critical infrastructure.

Screenshot / video of UI

Screen.Recording.2024-01-05.at.5.05.44.PM.mov

What issues does this PR fix or reference?

Closes #5444

How to test this PR?

Try to delete anything (volume, pod, container, etc.) and you will now
see a dialog.

Signed-off-by: Charlie Drage charlie@charliedrage.com

[cdrage]

TODO:

• Fix e2e tests (modal comes up to confirm deletion, need to click on it).

[deboer-tim]

I've been thinking about this for a while, and agree we need to do it. +1 to approach with ListItemButtonIcon where we can just add a confirmation attribute.

I think we've already heard from a couple users that don't always want this on for every object. I'd still say we do this first and be ready to have a discussion about a 'do not show again' and whether preference should be per object, per environment, etc. This approach would work, could always do something like export let confirm: boolean | string (preference key).

I don't love how the tests change, especially ones less related to the actions like *Details and *ListCompose. Instead of having to hook all message boxes to respond with 0 it feels like the tests should just say 'Say yes to confirmation dialog(s)'. For now maybe it would be better to at least put the message box mocking in a test helper util?

[cdrage]

I've been thinking about this for a while, and agree we need to do it. +1 to approach with ListItemButtonIcon where we can just add a confirmation attribute.

I think we've already heard from a couple users that don't always want this on for every object. I'd still say we do this first and be ready to have a discussion about a 'do not show again' and whether preference should be per object, per environment, etc. This approach would work, could always do something like export let confirm: boolean | string (preference key).

I don't love how the tests change, especially ones less related to the actions like *Details and *ListCompose. Instead of having to hook all message boxes to respond with 0 it feels like the tests should just say 'Say yes to confirmation dialog(s)'. For now maybe it would be better to at least put the message box mocking in a test helper util?

I agree!

Do we happen to already have a utility folder for svelte front-end-related tests? I wasn't able to find a helper file / directory that we could use. If not, I can throw it in a new folder.

@cbr7 @odockal I'm unsure what I'm doing wrong, but I'm trying to create a helper function to click the confirmation dialog for the e2e tests, if you're able to point out a fix that'd be awesome

[deboer-tim]

Tried this on volumes and services, it is working great. 👍🏼 Just one comment inline.

Nit: it says 'Do you want to delete volume", feels like it should say "Do you want to delete the volume" but this is not required.

[cdrage]

Tried this on volumes and services, it is working great. 👍🏼 Just one comment inline.

Nit: it says 'Do you want to delete volume", feels like it should say "Do you want to delete the volume" but this is not required.

We could update that in a separate PR as we'd have to update the titles in most places: https://github.com/containers/podman-desktop/pull/5445/files#diff-1fb3450fd535b8bfd2e357f46b03ba71a318831bcb449a065d509b27ebd53c05R16

[odockal]

@cdrage I am afraid that you have overwritten commit that fixed e2e tests, so it fails again.

[cdrage]

@cdrage I am afraid that you have overwritten commit that fixed e2e tests, so it fails again.

Ah sorry, do you happen to have your commit still available? For some reason with the rebase it did overwrite.

[odockal]

@cdrage Seems that we are all set for the merge?

[cdrage]

Yup 💯 just need a re-review!

[odockal]

I have had a chance to test the PR locally on Linux due to e2e tests updates, so it is +1 from me as well.

[deboer-tim]

LGTM

[cdrage]

Ready for a re-review 👍

Got e2e tests working thanks to help from @odockal was having difficulty why aria-label wasn't working.. then realized it was getting it from the wrong dialog.