aqueduct
aqueduct copied to clipboard
Eng 1519 add delete integration functionality to UI
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:")
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, 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.
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 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.
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.
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.
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
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.
@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 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.
@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
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.
Yep, agree with @agiron123! Everything looks good, let's just change the actions when deletion fails.
@vsreekanti @agiron123 Thanks for your feedback! Will update deletion failed actions to just be dismiss
and merge when that is done.
@eunice-chan is this PR good to go?
Updated action to "dismiss":
https://user-images.githubusercontent.com/30596854/186427333-0f287ab1-e27c-4125-a32e-9e5c4625cea7.mov