Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ResponseOps][Alerting] Provide indication of how many rules are using connector on Connector List view #137181

Merged

Conversation

doakalexi
Copy link
Contributor

@doakalexi doakalexi commented Jul 26, 2022

Resolves #86697

Summary

We wanted to let the users know of the impact deleting the connector. I Added a warning message to the delete confirmation pop-up if a user tries to delete connectors that are being used by a rule.

Single:
Screen Shot 2022-07-25 at 8 53 59 AM

Multiple:
Screen Shot 2022-07-25 at 8 54 10 AM

Checklist

To Verify

  • Delete a connector that is currently used in a rule. Verify that you can see a warning message in the delete confirmation pop-up.
  • Delete a connector that is not used in a rule. Verify that you don't see a warning message.
  • Try to delete multiple connectors where at least one is used in a rule. Verify that you can see a warning message in the delete confirmation pop-up.

@doakalexi doakalexi added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.5.0 labels Jul 26, 2022
@doakalexi doakalexi requested a review from mdefazio July 26, 2022 12:44
@doakalexi doakalexi added the Feature:Actions/ConnectorsManagement Issues related to Connectors Management UX label Jul 26, 2022
@doakalexi doakalexi marked this pull request as ready for review July 26, 2022 14:34
@doakalexi doakalexi requested a review from a team as a code owner July 26, 2022 14:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Verified that this works as expected! Nice job :) Left some minor comments.

@mdefazio
Copy link
Contributor

Some thoughts:

Are we saying 'At least' because we are unable to determine which connector(s) is used in a rule?

Related: Assuming someone wants to clean up their connectors, can I skip deleting those connectors that are still used in rules? Maybe have a checkbox or something that says 'Skip connectors that are used in rules'?

Should we explain further what happens when I delete a connector used in a rule? Does the action also get deleted? Or does that rule just cause errored_actions?

Should we wrap the warning text in a (warning) callout?

@lcawl lcawl added the ui-copy Review of UI copy with docs team is recommended label Jul 26, 2022
@doakalexi
Copy link
Contributor Author

Some thoughts:

Are we saying 'At least' because we are unable to determine which connector(s) is used in a rule?

Related: Assuming someone wants to clean up their connectors, can I skip deleting those connectors that are still used in rules? Maybe have a checkbox or something that says 'Skip connectors that are used in rules'?

Should we explain further what happens when I delete a connector used in a rule? Does the action also get deleted? Or does that rule just cause errored_actions?

Should we wrap the warning text in a (warning) callout?

@mdefazio We know which connectors are used in the rule, we can show that. I was just not sure the best way to display that in the pop-up.

I think that it will just cause an errored_action if you delete a connector used by a rule. I do think it's a good idea to explain that or something like that.

and sure, I can wrap the text in a warning callout too!

@pmuellr
Copy link
Member

pmuellr commented Jul 28, 2022

Are we saying 'At least' because we are unable to determine which connector(s) is used in a rule?

We know which connectors are used in the rule, we can show that. I was just not sure the best way to display that in the pop-up.

I think we'll eventually want to be able to see all the rules using a specific connector, somehow. Maybe a link from the connector to a filtered rule view? Anyway, not for this PR!

But if you assume we'll eventually have that, I'm not sure telling customers "this connector is used in 17 rules" is going to be much help. OTOH, it does feel like the "at least" wording is a bit off. Could we change it to "This connector is used in multiple rules." for now?

Should we wrap the warning text in a (warning) callout?

If that means some additional decoration around the text, +1 for me. The warning icon is good, but maybe not quite enough ...

Should we explain further what happens when I delete a connector used in a rule? Does the action also get deleted? Or does that rule just cause errored_actions?

I think that it will just cause an errored_action if you delete a connector used by a rule. I do think it's a good idea to explain that or something like that.

Correct, when the rule's action group is triggered, the action will start processing and eventually fail since the connector no longer exists. Remediation for that will be yet another issue (maybe delete the action, or ask the user if they want the actions deleted, etc). Adding a bit of additional text indicating errors will be generated if the rule attempts to run those actions would be good. Will be tough to make that understandable and short, I think! :-). @lcawl will have to help us with that, because we don't want to scare users into thinking the rule will fail, just attempting to run that action when the rule is triggered will fail.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, tried it out, worked as expected

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@mdefazio
Copy link
Contributor

I've been trying to help on this, but honestly am torn which way to go. While I'm not sure either of these are correct, here's three angles we can take for this:

  1. Checkbox confirmation to delete actions -- I know this might be out of reach, but has the user acknowledge something may break
  2. Radio buttons to choose to skip those connectors used in rules -- this could be tweaked to use a single checkbox to skip rules'
  3. Or we have a more deliberate confirmation in the third option so they know action errors will occur.

Curious to get others thoughts...
image

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@doakalexi
Copy link
Contributor Author

doakalexi commented Jul 29, 2022

We'll create an issue to help us address some of the follow on concerns of deleting a connector that is used in a rule.

@pmuellr
Copy link
Member

pmuellr commented Jul 29, 2022

Checkbox confirmation to delete actions -- I know this might be out of reach, but has the user acknowledge something may break

Ya, out of reach at the moment - will require some additional analysis to see if this is feasible, etc. We can use the issue that Alexi just opened ^^^ to track.

Radio buttons to choose to skip those connectors used in rules -- this could be tweaked to use a single checkbox to skip rules'

Interesting. I think my only take would be that the customer is probably going to expect to know which ones will and won't be deleted, which makes the UX more confusing. I think in the future we should probably allow filtering for "connectors not used by any rules" (which is complicated / confusing as a connector may be used by a rule that a user can't see because of feature privs), which would provide that sort of UX.

Or we have a more deliberate confirmation in the third option so they know action errors will occur.

Feels a little overbearing, however, I think it may be worth it, considering this is not something we expect to happen often, and we DO want the user to understand that future action executions for these will fail.

I think we'll need to have @shanisagiv1 and @mikecote provide some thoughts about this ...

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@doakalexi
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor

mikecote commented Aug 2, 2022

I think we'll need to have @shanisagiv1 and @mikecote provide some thoughts about this ...

Good points @pmuellr, I think to keep the scope of this PR small, I'm ok with the current state of this PR and the follow up issues being created. Let's get @lcawl's feedback on the labels. My only nit is the referencedByCount behind the scenes also considers cases that reference a connector, so it may be a case or rule that causes this warning message.

Maybe we can keep the wording generic for now until we have better UX. If it helps, @lcawl my thinking is below as a starting point:

Copy when deleting a single connector: This connector is currently in use.
Copy when deleting multiple connectors: Some connectors are currently in use.

…doakalexi/kibana into alerting/warn-before-deleting-connector
@doakalexi
Copy link
Contributor Author

doakalexi commented Aug 3, 2022

I think we'll need to have @shanisagiv1 and @mikecote provide some thoughts about this ...

Good points @pmuellr, I think to keep the scope of this PR small, I'm ok with the current state of this PR and the follow up issues being created. Let's get @lcawl's feedback on the labels. My only nit is the referencedByCount behind the scenes also considers cases that reference a connector, so it may be a case or rule that causes this warning message.

Maybe we can keep the wording generic for now until we have better UX. If it helps, @lcawl my thinking is below as a starting point:

Copy when deleting a single connector: This connector is currently in use. Copy when deleting multiple connectors: Some connectors are currently in use.

Thanks! The copy has been updated in this commit

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 1.0MB 1.0MB +1020.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@doakalexi doakalexi merged commit 05f2072 into elastic:main Aug 3, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 3, 2022
@shanisagiv1
Copy link

Hi Sorry for my late reply here, missed that.
I'm not sure what is the final decision under this PR.

But I would suggest the following and pls let me know wdyt and if there are other issues that refer to it already:

  • When trying to delete a connector there are 2 options as I see it:
  1. Blocking the user from deleting the connectors that are used in order to avoid the failures (A stricter approach that will lead to manual operations and user frustration so I'll go with the second option).
  2. Notify the user that some rules are in use and let him know that by his approval, the Rule's actions will be removed from the rules permanently. Add we can add also the "Skip" checkbox for a better experience but tbh I think the number of connectors is low one digit anyway so it's a "nice to have" and I can't think about a case when a user wants to delete all his 3-4 connectors (and if he's so probably it should be removed from the rules themself as well.. kind of "restart" I guess)
  • A dedicated view to see which connectors are used and where.. (as Defazio mention it's probably for another use case)

Thanks

@doakalexi @pmuellr @mikecote @mdefazio

@mikecote
Copy link
Contributor

mikecote commented Aug 4, 2022

A dedicated view to see which connectors are used and where.. (as Defazio mention it's probably for another use case)

++ we could inspire from the saved-objects relationship flyout to display rules, cases, that reference a connector..


I'm not sure what is the final decision under this PR.

We opted to have a generic message at this time (ex: This connector is currently in use.) and let the user continue as they were before. The follow ups including the points you mentioned should be tracked here => https://github.com/elastic/response-ops-team/issues/68.

Let's make sure you echo your feedback there 🙏 We'll lean on your for priority of this follow up issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Actions/ConnectorsManagement Issues related to Connectors Management UX release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ui-copy Review of UI copy with docs team is recommended v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Provide indication of how many rules are using connector on Connector List view
10 participants