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

[Alerting] Allow user to select existing connector of same type when fixing broken connector #89062

Merged
merged 16 commits into from Feb 2, 2021

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Jan 22, 2021

Resolves #87533

Summary

Fixes the issue when you have an alert associated with a connector and that connector gets deleted, you are forced to create a new connector of the same type, even if there already exists another connector of the same type. This PR adds a dropdown selector in order to quickly switch the connector.

View when there exists one or more connectors of the same type as the deleted connector

Screen Shot 2021-01-26 at 4 18 42 PM

View when there is no other connector of that type (only option is to add new connector)

Screen Shot 2021-01-27 at 10 57 03 AM

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

@mdefazio
Copy link
Contributor

mdefazio commented Jan 25, 2021

Should this be a warning message? Can the alert be saved without making this edit? I wonder if we just use the invalid state of the dropdown to indicate the error and not have the callout in the connector accordion. But perhaps that depends on whether this can be saved with this unfixed.

Some thoughts on copy if we go the invalid state route:
image

I think we will then need a callout at the top of the alert itself, as well as an icon next to the name of the connector.

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 25, 2021

Should this be a warning message? Can the alert be saved without making this edit? I wonder if we just use the invalid state of the dropdown to indicate the error and not have the callout in the connector accordion. But perhaps that depends on whether this can be saved with this unfixed.

I think we will then need a callout at the top of the alert itself, as well as an icon next to the name of the connector.

@mdefazio The alert cannot be saved without either deleting or fixing the broken connector, so I think an error icon makes sense next to the name of the connector.

What about the case where there is no connector of the same type to select (second screenshot in the PR). Right now we don't show any dropdown, just the callout and a button to Create connector. Would you suggest any changes to that view?

@mdefazio
Copy link
Contributor

Assuming it still makes sense to have an error icon next to the connector title, then I think we could keep the button (but remove the callout like we do in the first screenshot)

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 25, 2021

@mdefazio What do you think?

When no other connectors are available:
Screen Shot 2021-01-25 at 3 49 13 PM

When other connectors are available:
Screen Shot 2021-01-25 at 3 47 56 PM

I think the second screenshot might be a little aggressive, what do you think? In order to show the error message underneath the dropdown, I had to set isInvalid to true, which then turns the label above the dropdown red, so they are both red

@mdefazio
Copy link
Contributor

I think it's ok they are both red. This is our invalid pattern so I think it's clear that it needs to be fixed.

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 26, 2021

I think it's ok they are both red. This is our invalid pattern so I think it's clear that it needs to be fixed.

@mdefazio Ok thanks! One last question:

When no other connectors are available, I've removed the callout and added the error icon with tooltip. Do you think that is enough differentiation between how it looks when you add a new action but need to add a connector for that action? In the screenshot below I have two broken connectors and 1 new action:

Screen Shot 2021-01-26 at 2 49 04 PM

@ymao1 ymao1 self-assigned this Jan 26, 2021
@ymao1 ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 v8.0.0 labels Jan 26, 2021
@ymao1 ymao1 marked this pull request as ready for review January 26, 2021 21:32
@ymao1 ymao1 requested a review from a team as a code owner January 26, 2021 21:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 27, 2021

@elasticmachine merge upstream

@mdefazio
Copy link
Contributor

If we're concerned the state where I've just added a connector is not different enough from the connector with nothing selected, we could show the 'Select a connector' dropdown (in default state), and when they open it can simply say "No connectors found". The user can then create a connector from this view as well. I see your concern though. We can also show the error text in the same location as 'No slack connectors' right?

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 29, 2021

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Feb 1, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 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.4MB 1.4MB +3.4KB

History

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

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Tested locally and it all looks great

@ymao1 ymao1 merged commit f3d7d37 into elastic:master Feb 2, 2021
ymao1 added a commit to ymao1/kibana that referenced this pull request Feb 2, 2021
…fixing broken connector (elastic#89062)

* Adding dropdown for selecting different connector of same type

* Updating design

* Cleanup and i18n

* Adding functiional test

* Fixing unit test

* Fixing functional test

* Updating design

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@mikecote mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 2, 2021
ymao1 added a commit that referenced this pull request Feb 2, 2021
…fixing broken connector (#89062) (#90001)

* Adding dropdown for selecting different connector of same type

* Updating design

* Cleanup and i18n

* Adding functiional test

* Fixing unit test

* Fixing functional test

* Updating design

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
phillipb added a commit to phillipb/kibana that referenced this pull request Feb 2, 2021
…-ml-jobs

* 'master' of github.com:elastic/kibana: (254 commits)
  [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927)
  Skip test for cloud (elastic#89450)
  [Fleet] Fix duplicate data streams being shown in UI (elastic#89812)
  Bump package dependencies (elastic#90034)
  [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811)
  TypeScript project references for Observability plugin (elastic#89320)
  [SearchSource] Combine sort and parent fields when serializing (elastic#89808)
  Made imports static (elastic#89935)
  [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640)
  [Discover] Adapt default column behavior (elastic#89826)
  Round start and end values (elastic#89030)
  Rename getProxyAgents to getCustomAgents (elastic#89813)
  [Form lib] UseField `onError` listener (elastic#89895)
  [APM] use latency sum instead of avg for impact (elastic#89990)
  migrate more core-owned plugins to tsproject ref (elastic#89975)
  [Logs UI] Load <LogStream> entries via async searches (elastic#86899)
  [APM] Abort browser requests when appropriate (elastic#89557)
  [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062)
  [Data Table] Use shared CSV export mechanism (elastic#89702)
  chore(NA): improve logic check when installing Bazel tools (elastic#89634)
  ...
@ymao1 ymao1 deleted the alerting/broken-connector branch March 25, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Alert with broken connector should allow user to select existing connector of same type
7 participants