Fixed UI/UX issues: alerts delete confirmation, combobox behaviors by YulNaumenko · Pull Request #60703 · elastic/kibana

YulNaumenko · 2020-03-19T21:53:39Z

elasticmachine · 2020-03-19T21:53:42Z

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

…sues # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

gmmorris

Looks good!

Happy to approve, but I suggest the description needs to be amended before merging as this will close both #58364 and #60204 but it only addresses the delete confirmation and those issues include a bunch of other things that still need to be address in subsequent PRs.

...i/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx

gmmorris · 2020-03-20T13:15:55Z

..._actions_ui/public/application/sections/common/components/with_bulk_alert_api_operations.tsx

+  deleteAlert: (
+    alert: Alert
+  ) => Promise<{
+    successes: string[];
+    errors: string[];
+  }>;


I like that we're using only one api for deleting (using deleteAlerts for both multiple and single deletion requests), but I have a little nit picky thought. :)
It feels weird returning { successes: string[]; errors: string[]; } when the user calls deleteAlert, as it's only one id.

Could we perhaps change the signature to { successes?: string; errors?: string;}?

If we're going to inline the type as is done here, agree w/Gidi that we could change the structure to match the semantic value. On the other hand, I can see this pattern of successes/errors being more widely used, and probably worth a separate type itself, and then having two different types - a "single" and "multiple" version - could get unwieldy.

pmuellr

LGTM, but added a few comments. UI works as expected.

x-pack/plugins/triggers_actions_ui/public/application/components/delete_modal_confirmation.tsx

...i/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx

pmuellr · 2020-03-20T15:25:08Z

x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api.ts

+}): Promise<{ successes: string[]; errors: string[] }> {
+  const successes: string[] = [];
+  const errors: string[] = [];
+  await Promise.all(ids.map(id => http.delete(`${BASE_ALERT_API_PATH}/${id}`))).then(


I think if one delete fails, they will all report as failing. Seems like this needs to be restructured so that the delete's are wrapped in a function that never fails, but internally, it updates successes and errors, so that it can report a combination of successes and failures.

Seems survivable for 7.7, and should probably just be a new tech debt issue.

pmuellr · 2020-03-20T15:30:38Z

..._actions_ui/public/application/sections/common/components/with_bulk_alert_api_operations.tsx

+  deleteAlert: (
+    alert: Alert
+  ) => Promise<{
+    successes: string[];
+    errors: string[];
+  }>;


If we're going to inline the type as is done here, agree w/Gidi that we could change the structure to match the semantic value. On the other hand, I can see this pattern of successes/errors being more widely used, and probably worth a separate type itself, and then having two different types - a "single" and "multiple" version - could get unwieldy.

…sues # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

…sues # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

kibanamachine · 2020-03-21T04:04:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7ab39b0

History

💔 Build #35127 failed 886d75c
💔 Build #35096 failed 699dd87
💔 Build #35049 failed d22aea5
💔 Build #35016 failed 0612f4e
💚 Build #34775 succeeded 2813433

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

…lastic#60703) * Fixed UI/UX issues: alerts delete confirmation * Fixed 4. Popover disappears when clearing the field selector * Fixed tests * Fixed due to comments * fixed tests * Fixed test # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

kibanamachine · 2020-03-22T04:38:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…lastic#60703) * Fixed UI/UX issues: alerts delete confirmation * Fixed 4. Popover disappears when clearing the field selector * Fixed tests * Fixed due to comments * fixed tests * Fixed test # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

…60703) (#60871) * Fixed UI/UX issues: alerts delete confirmation * Fixed 4. Popover disappears when clearing the field selector * Fixed tests * Fixed due to comments * fixed tests * Fixed test # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

* master: (39 commits) [APM]Create custom link from Trace summary (elastic#59648) [ML] Fixing app clean up (elastic#60853) [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734) [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016) Skip failing test [Uptime]Update fetch effect failed action handling (elastic#60742) [npm] upgrade elastic/maki (elastic#60829) [Uptime] Add Settings Page (elastic#53550) [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836) [Index management] Re-enable index template tests (elastic#60780) Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703) [SIEM] Fix patching of ML Rules (elastic#60830) [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477) [Alerting] fix flaky test for index threshold grouping (elastic#60792) [SIEM][Detection Engine] Adds test scripts for machine learning feature Flatten child api response for resolver (elastic#60810) Change "url" to "urls" in APM agent instructions (elastic#60790) [DOCS] Updates API requests and examples (elastic#60695) [SIEM] [Cases] Create case from timeline (elastic#60711) [Lens] Resetting a layer generates new suggestions (elastic#60674) ...

Fixed UI/UX issues: alerts delete confirmation

0f38d5d

YulNaumenko added bug Fixes for quality problems that affect the customer experience Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// labels Mar 19, 2020

YulNaumenko requested a review from a team as a code owner March 19, 2020 21:53

YulNaumenko self-assigned this Mar 19, 2020

YulNaumenko linked an issue Mar 20, 2020 that may be closed by this pull request

A number of alerting UX observations of lower prio #60204

Closed

YulNaumenko added 3 commits March 19, 2020 18:11

Fixed 4. Popover disappears when clearing the field selector

88da512

Fixed tests

33f0469

gmmorris approved these changes Mar 20, 2020

View reviewed changes

pmuellr approved these changes Mar 20, 2020

View reviewed changes

YulNaumenko added 5 commits March 20, 2020 10:55

Merge remote-tracking branch 'upstream/master' into alerting-ui-ux-is…

d22aea5

…sues # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts

Fixed due to comments

699dd87

fixed tests

886d75c

Fixed test

7ab39b0

YulNaumenko merged commit 0390251 into elastic:master Mar 21, 2020

YulNaumenko mentioned this pull request Mar 21, 2020

[7.x] Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (#60703) #60848

Closed

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 22, 2020

YulNaumenko mentioned this pull request Mar 22, 2020

[7.x] Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (#60703) #60871

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed UI/UX issues: alerts delete confirmation, combobox behaviors#60703

Fixed UI/UX issues: alerts delete confirmation, combobox behaviors#60703
YulNaumenko merged 9 commits intoelastic:masterfrom
YulNaumenko:alerting-ui-ux-issues

YulNaumenko commented Mar 19, 2020

Uh oh!

elasticmachine commented Mar 19, 2020

Uh oh!

gmmorris left a comment

Uh oh!

Uh oh!

gmmorris Mar 20, 2020

Uh oh!

pmuellr Mar 20, 2020

Uh oh!

pmuellr left a comment

Uh oh!

Uh oh!

Uh oh!

pmuellr Mar 20, 2020

Uh oh!

pmuellr Mar 20, 2020

Uh oh!

kibanamachine commented Mar 21, 2020

Uh oh!

kibanamachine commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

YulNaumenko commented Mar 19, 2020

Uh oh!

elasticmachine commented Mar 19, 2020

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmmorris Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pmuellr Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 21, 2020

💚 Build Succeeded

History

Uh oh!

kibanamachine commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants