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

Fix explore tables don't display data when a global filter is applied #130024

Merged
merged 1 commit into from Apr 13, 2022

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented Apr 12, 2022

issue: #129986

Summary

Tables inside security explore had the unexpected behaviour of persisting the selected page when the filter or time range changed. So when users changed the filter or time range and fewer items were returned, the table would still display the previously selected page even though that page had no data.

Impact: Every table inside Security Solutions / Explore

How to reproduce it

  • Open kibana security
  • Open Host page
  • Select the second page on all host's table
  • Hover a host.name on the table and click on filter in

Current behaviour

The table shows 0 items

Expected behaviour

The table should reset the page selection when the query, filter, or time range changes.

Before fix

Apr-12-2022 11-03-20

After fix

Apr-12-2022 17-15-22

@machadoum machadoum force-pushed the siem-explore-issue-129986 branch 2 times, most recently from 30e9dd5 to 6d20b3b Compare April 12, 2022 15:08
@machadoum machadoum self-assigned this Apr 12, 2022
@machadoum machadoum added v8.1.0 v8.2.0 v8.1.3 Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore bug Fixes for quality problems that affect the customer experience and removed v8.1.0 labels Apr 12, 2022
@machadoum machadoum marked this pull request as ready for review April 12, 2022 15:17
@machadoum machadoum requested a review from a team as a code owner April 12, 2022 15:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

It has been discussed and we agreed to go ahead with this approach since it is the fastest way to fix the bug.

However, It is not desirable to have these dependencies between the SiemSearchBar shared component and all the tables that are using it.
It would be great to move those action dispatches out of the SearchBar in the future.

LGTM

useEffect(() => {
if (fromStr != null && toStr != null) {
timefilter.setTime({ from: fromStr, to: toStr });
} else if (start != null && end != null) {
setTablesActivePageToZero();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need setTablesActivePageToZero in Line 104 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, we should. But I can't find a use case where it is necessary. Do you know a use case where the user can change fromStr or toStr beside the search bar? For example, line 106 is called when we update the time filter by interacting with a histogram.

@angorayc Do you think I should add setTablesActivePageToZero to 104 for consistency? It would also add the side-effect that active pages are cleaned when the user navigates between pages because line 104 is called don when search_bar initializes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, I guess I was confused between start and fromStr, end and toStr. If it is only called on navigating between pages, then no need to add it. Thanks Pablo.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

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
securitySolution 4.8MB 4.8MB +276.0B

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

cc @machadoum

@machadoum machadoum merged commit 3877763 into elastic:main Apr 13, 2022
machadoum added a commit to machadoum/kibana that referenced this pull request Apr 13, 2022
machadoum added a commit to machadoum/kibana that referenced this pull request Apr 13, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
…rint-media-attempt-2

* 'main' of github.com:elastic/kibana: (75 commits)
  [Lens] Hide disabled toolbar entries (elastic#129994)
  Fix explore tables don't display data when a global filter is applied (elastic#130024)
  [Console] Add option to disable keyboard shortcuts (elastic#128887)
  [Discover] Update refreshOnClick flaky test (elastic#130001)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/formats/pdf/index.ts
machadoum added a commit that referenced this pull request Apr 13, 2022
machadoum added a commit to machadoum/kibana that referenced this pull request Apr 13, 2022
machadoum added a commit that referenced this pull request Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.1.3 v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants