aqueduct icon indicating copy to clipboard operation
aqueduct copied to clipboard

Eng 1519 add delete integration functionality to UI

Open eunice-chan opened this issue 1 year ago • 7 comments

Describe your changes and why you are making these changes

Allow the user to delete integrations from the frontend. This isn't possible if there are workflows using the integration.

Demo

Tweaked spacing for Workflow heading so it isn't so close to the preview. Added a message saying there are no workflows when len(workflows) === 0.

This is how backend errors are surfaced: (actually it says "Delete integration failed with error:") integration_deletion_error_example

The button is disabled if the integration is use (len(workflows) > 0).

https://user-images.githubusercontent.com/30596854/183769074-1723ed7b-da2f-481e-b2fe-37a075826fa5.mov

This is how a successful integration deletion looks.

https://user-images.githubusercontent.com/30596854/183769090-7be3ebdb-a530-4bf7-9ca9-796874ec725b.mov

This is the same as above, but I added a delay to the success toast so you can see what the screen looks like when the backend is still processing.

https://user-images.githubusercontent.com/30596854/183769087-f72febb8-1eca-4911-b50b-2ae9aa6fb5cb.mov

Related issue number (if any)

Eng 1519

Checklist before requesting a review

  • [x] I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • [x] I have performed a self-review of my code.
  • [x] I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • [N/A] If this is a new feature, I have added unit tests and integration tests.
  • [N/A] I have run the integration tests locally and they are passing.
  • [x] I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • [x] All features on the UI continue to work correctly.
  • [x] Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

eunice-chan avatar Aug 10 '22 15:08 eunice-chan

@eunice-chan, this looks great! Two small things:

  • Let's add a modal confirming that the user actually wants to delete the integration rather than immediately deleting it. Doesn't need to be anything fancy, but we just want to double check.
  • Let's hardcode a check disabling deletion for the demo DB for now.

vsreekanti avatar Aug 10 '22 17:08 vsreekanti

The code looks good. @vsreekanti @agiron123 I would let you get a final pass as long as the UX looks to you.

Regarding UI, I agree that we should have a modal to let the user confirm. Another nice to have is some tooltip when the user hover on a disabled delete integration button, to inform the user there are still associated workflows.

likawind avatar Aug 12 '22 20:08 likawind

@likawind A disabled button isn't able to support a tooltip (https://stackoverflow.com/questions/61115913/is-it-possible-to-render-a-tooltip-on-a-disabled-mui-button-within-a-buttongroup). Definitely agree it would be useful to include an explanation somewhere as to why the button is disabled.

eunice-chan avatar Aug 12 '22 20:08 eunice-chan

Great work here @eunice-chan!

@vsreekanti @likawind I met with @eunice-chan earlier last week about this UI and we discussed the toast messages that are shown when the user deletes the integration.

I agree that we should show a confirmation dialog before actually running the request to delete the integration. If we do that, then we'll likely have to re-work how the user is alerted about success/failures while deleting since they are now inside of a modal.

Happy to discuss this over Zoom some time this week before I go out of town on Wednesday.

agiron123 avatar Aug 15 '22 18:08 agiron123

re: above, I think we should just merge this PR as is and file a task on Linear for design and follow up for confirmation dialog work.

agiron123 avatar Aug 15 '22 18:08 agiron123

I'm a little worried about releasing this without the modal. Why don't we wait to merge this after we cut the release today, and we can then merge this PR and tidy up later this week? @agiron123 @eunice-chan

vsreekanti avatar Aug 15 '22 19:08 vsreekanti

I'm a little worried about releasing this without the modal. Why don't we wait to merge this after we cut the release today, and we can then merge this PR and tidy up later this week? @agiron123 @eunice-chan

@vsreekanti sounds good, i'll be out this week after tomorrow, but I think y'all can take it from here.

agiron123 avatar Aug 15 '22 21:08 agiron123

@eunice-chan, I just wanted to check in on the status of this -- are we planning on adding the modal or merging this first and adding it in later?

vsreekanti avatar Aug 18 '22 15:08 vsreekanti

@vsreekanti I'm planning on adding the modal! This week I have been focusing more on the on-call & documentation tasks, but I plan on getting to this soon.

eunice-chan avatar Aug 18 '22 15:08 eunice-chan

@vsreekanti Added the deletion modal. If this looks good I can merge it in.

Successful Deletion:

https://user-images.githubusercontent.com/30596854/186165569-810472ae-ab26-4ec3-8849-3e52e8ebdb98.mov

Failed Deletion:

https://user-images.githubusercontent.com/30596854/186165582-3c1a39e6-001f-435e-94c7-4b9a943dde73.mov

eunice-chan avatar Aug 23 '22 13:08 eunice-chan

Dialogs look good. When a deletion fails, do we have the option to say "Dismiss" instead of confirm or cancel?

Confirm and cancel options don't really make sense in the error case.

agiron123 avatar Aug 23 '22 17:08 agiron123

Yep, agree with @agiron123! Everything looks good, let's just change the actions when deletion fails.

vsreekanti avatar Aug 23 '22 17:08 vsreekanti

@vsreekanti @agiron123 Thanks for your feedback! Will update deletion failed actions to just be dismiss and merge when that is done.

eunice-chan avatar Aug 23 '22 17:08 eunice-chan

@eunice-chan is this PR good to go?

likawind avatar Aug 23 '22 22:08 likawind

Updated action to "dismiss":

https://user-images.githubusercontent.com/30596854/186427333-0f287ab1-e27c-4125-a32e-9e5c4625cea7.mov

eunice-chan avatar Aug 24 '22 13:08 eunice-chan