Refines hasAnyData check for alerts by claudiopro · Pull Request #117499 · elastic/kibana

claudiopro · 2021-11-04T12:13:48Z

Summary

This PR fixes #116759 by refining the condition that determines hasAnyData to accommodate for the case when app === 'alerts' whose hasData field is always set to an Array, eventually empty:

Once applied this change, the Alerts page renders fine for a user in a role with the matrix of permissions described in #116759.

Before

After

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

afgomez · 2021-11-04T15:14:55Z

I feel the change that needs to happen is to not use an array for alerts, but I'm not sure why is there. @cauemarcondes The logic seems to have been introdued in https://github.com/elastic/kibana/pull/80644/files#diff-68524697b54a56da17a8561e97878fc82f52d1e5064230c81a9e60568989415dR21. Do you remember the reason to treat alerts differently and store the presence of data as an array?

x-pack/plugins/observability/public/context/has_data_context.tsx

claudiopro · 2021-11-04T15:20:49Z

@afgomez that sounds the cleanest approach, however I wasn't sure it was the correct one. I would be worried if the has data context uses the list of alerts in another call site.

cauemarcondes · 2021-11-04T15:27:37Z

I feel the change that needs to happen is to not use an array for alerts, but I'm not sure why is there. @cauemarcondes The logic seems to have been introdued in https://github.com/elastic/kibana/pull/80644/files#diff-68524697b54a56da17a8561e97878fc82f52d1e5064230c81a9e60568989415dR21. Do you remember the reason to treat alerts differently and store the presence of data as an array?

@afgomez FWIR alert is handled differently because it is the only plugin that we don't use the standard registration process to get the data.

afgomez · 2021-11-04T15:53:19Z

@cauemarcondes but what's the reason to keep the hasData as an array? Do we use any of the array elements anywhere?

cauemarcondes · 2021-11-04T15:55:11Z

@cauemarcondes but what's the reason to keep the hasData as an array? Do we use any of the array elements anywhere?

I believe that was a mistake on my part, hasData must be a boolean, and we should figure out another way to store the alerts data to be used here https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/overview/index.tsx#L75:L76

claudiopro · 2021-11-04T16:21:27Z

@cauemarcondes nice, sounds like a good follow-up task 😜

afgomez

I do think it's worth removing the array from hasData, but it looks like a slightly more convoluted task and it can be done in a separate PR (maybe by @elastic/unified-observability).

We can move forward with this as-is to unblock you @claudiopro

kibanamachine · 2021-11-04T18:26:18Z

💚 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
`observability`	375.7KB	375.7KB	+55.0B

History

💚 Build #4330 succeeded 3602685
💔 Build #4318 failed 72b01e2

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

* Refines hasAnyData check for alerts * Fixes type refinements * Applies review feedback * Naming is hard

kibanamachine · 2021-11-04T18:30:42Z

💚 Backport successful

Status	Branch	Result
✅	8.0

This backport PR will be merged automatically after passing CI.

…ink-to-kibana-app * 'main' of github.com:elastic/kibana: (290 commits) [Connectors][ServiceNow] Remove SN flags (elastic#117511) [ML] Functional tests - stabilize and re-enable feature importance tests (elastic#117503) [RAC] Disable the actions button if the user has inadequate privileges (elastic#117488) [Visualize] [xyChart] filter labels by default (elastic#117288) Fix warning when setting description to undefined (elastic#117338) [build] Set monitoring.ui.container.elasticsearch.enabled for all containers (elastic#115087) fix types [Alerting] UX fixes for execution duration chart (elastic#117193) [CI] Delete node_modules in between bootstrap attempts (elastic#117588) Flaky test fixes (elastic#117028) [Security Solution] [Sourcerer] [Feature Branch] Update to use Kibana Data Views (elastic#114806) [ML] Hide anomaly entity filter button tooltips when clicked (elastic#117493) adjust the synthetics journey type (elastic#117316) Refines hasAnyData check for alerts (elastic#117499) [Fleet] Default to APM tutorial (elastic#117421) [Maps] update docs for index pattern -> data view rename (elastic#117400) [Logs UI][Metrics UI] Remove deprecated config fields from APIs and SavedObjects (elastic#116821) [Fleet] Fix agent logs not reading query from URL (elastic#117286) Fixing Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alert_create_flyout·ts - Actions and Triggers app create alert should successfully test valid es_query alert (elastic#114917) [Metrics UI] Add docs link to redundant groupBy detection (elastic#116822) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap # x-pack/plugins/reporting/public/shared_imports.ts # x-pack/plugins/reporting/server/routes/management/jobs.ts

afgomez · 2021-11-05T11:10:53Z

it looks like a slightly more convoluted task and it can be done in a separate PR.

I'm tracking this in #117638

kibanamachine · 2021-11-08T18:29:52Z

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.

claudiopro · 2021-11-08T20:48:22Z

@afgomez I'm confused about the message by @kibanamachine, does it require an action on my end?

kibanamachine · 2021-11-09T18:39:51Z

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.

afgomez · 2021-11-10T10:00:32Z

@claudiopro yes. We need to take care of this PR #117578

The backport mechanism replicates PRs on top of different branches, in this case on top of the 8.0 branch. Normally they get merged without any issues but sometimes there are hiccups.

As a first step I'd run elasticmachine merge upstream in that PR and wait for it to build again. The errors I'm seeing don't seem related to your changes, so a merge with the latest commit might be enough to make the CI happy. If it keeps failing we can take a look together.

claudiopro · 2021-11-10T10:36:08Z

Super, thanks! I'll merge and watch the build status.

* Refines hasAnyData check for alerts * Fixes type refinements * Applies review feedback * Naming is hard Co-authored-by: Claudio Procida <claudio.procida@gmail.com>

Refines hasAnyData check for alerts

72b01e2

claudiopro added v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed Theme: rac label obsolete v8.1.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Nov 4, 2021

claudiopro requested review from afgomez and weltenwort November 4, 2021 12:13

claudiopro added the release_note:skip Skip the PR/issue when compiling release notes label Nov 4, 2021

Fixes type refinements

3602685

Zacqary reviewed Nov 4, 2021

View reviewed changes

x-pack/plugins/observability/public/context/has_data_context.tsx Outdated Show resolved Hide resolved

Applies review feedback

5c8928b

Naming is hard

9b3983e

afgomez approved these changes Nov 4, 2021

View reviewed changes

claudiopro merged commit debdfd2 into elastic:main Nov 4, 2021

claudiopro deleted the issue_116759 branch November 4, 2021 18:27

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 4, 2021

Refines hasAnyData check for alerts (elastic#117499)

a6db4b7

* Refines hasAnyData check for alerts * Fixes type refinements * Applies review feedback * Naming is hard

kibanamachine mentioned this pull request Nov 4, 2021

[8.0] Refines hasAnyData check for alerts (#117499) #117578

Merged

afgomez mentioned this pull request Nov 5, 2021

[Unified Observability] Refine the data presence checks for the overview page #117638

Closed

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 8, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 10, 2021

Conversation

claudiopro commented Nov 4, 2021

Summary

Before

After

Checklist

Uh oh!

afgomez commented Nov 4, 2021

Uh oh!

Uh oh!

claudiopro commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cauemarcondes commented Nov 4, 2021

Uh oh!

afgomez commented Nov 4, 2021

Uh oh!

cauemarcondes commented Nov 4, 2021

Uh oh!

claudiopro commented Nov 4, 2021

Uh oh!

afgomez left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 4, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Nov 4, 2021

💚 Backport successful

Uh oh!

afgomez commented Nov 5, 2021

Uh oh!

kibanamachine commented Nov 8, 2021

Uh oh!

claudiopro commented Nov 8, 2021

Uh oh!

kibanamachine commented Nov 9, 2021

Uh oh!

afgomez commented Nov 10, 2021

Uh oh!

claudiopro commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

claudiopro commented Nov 4, 2021 •

edited

Loading