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 pagination bugs in CCR and Remote Clusters #65931

Merged
merged 6 commits into from
May 14, 2020

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented May 8, 2020

A user reported that the pagination on these apps wasn't working. This is due to elastic/eui#3445. My solution was to calculate the filtered items and cache them on state, instead of deriving them in the render method.

However, this caused a new bug: when you delete an item the table doesn't update, because the original list of items no longer acts as an input to the render method. This required the introduction of a getDerivedStateFromProps method to update the filtered items cache when the list of items changes.

All in all, a fairly complex solution due to the way we implemented EuiInMemoryTable -- and git blame reveals that I was the one who implemented the responsible lines of code! 😅

ccr_pagination

How to test

You can edit the code to populate the tables to test the pagination behavior, though this doesn't let you delete items. To test that deleting items updates the table, just configure a remote cluster pointed at 127.0.0.1:9300 and you can create follower indices and auto-follow patterns that use it.

// remote_cluster_list.container.js

const clusters = [];
for( var i = 0; i < 30; i++) {
  clusters.push({ name: `${i}`, seeds: [] });
}

const mapStateToProps = state => {
  return {
    clusters,//: getClustersList(state),
    isDetailPanelOpen: isDetailPanelOpen(state),
    isLoading: isLoading(state),
    isCopyingCluster: isEditingCluster(state),
    isRemovingCluster: isRemovingCluster(state),
    clusterLoadError: clusterLoadError(state),
  };
};

// follower_indices_list.container.js

const followerIndices = [];
for( var i = 0; i < 30; i++) {
  followerIndices.push({ name: `${i}`, remoteCluster: 'a', leaderIndex: 'a' });
}

const mapStateToProps = state => ({
  followerIndices,//: getListFollowerIndices(state),
  followerIndexId: getSelectedFollowerIndexId('detail')(state),
  apiStatus: getApiStatus(scope)(state),
  apiError: getApiError(scope)(state),
  isAuthorized: isApiAuthorized(scope)(state),
});

// auto_follow_patterns_list.container.js

const autoFollowPatterns = [];
for( var i = 0; i < 30; i++) {
  autoFollowPatterns.push({ name: `${i}`, remoteCluster: 'a', leaderIndexPatterns: [], followIndexPatternPrefix: 'a', followIndexPatternSuffix: 'a' });
}

const mapStateToProps = state => ({
  autoFollowPatterns,//: getListAutoFollowPatterns(state),
  autoFollowPatternId: getSelectedAutoFollowPatternId('detail')(state),
  apiStatus: getApiStatus(scope)(state),
  apiError: getApiError(scope)(state),
  isAuthorized: isApiAuthorized(scope)(state),
});

Release note

The table pagination buttons in Cross-Cluster Replication and Remote Clusters weren't working, and now they do.

form.setInputValue(component.find('input[type="search"]'), 'unique');
const { tableCellsValues } = table.getMetaData('autoFollowPatternListTable');
expect(tableCellsValues.length).toBe(1);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga There are two other search tests like this one in the other tests files, and they are all failing based on the tableCellsValues.length remaining unchanged at 20. It looks like the input is being found and the change event is simulated, but it's not bubbling back up to the list component (which is responsible for handling it and updating the array of filtered items). Any ideas on what could be causing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have your tried adding component.update() after form.setInputValue()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you decided to use component.find('input[type="search"]') instead of using a data-test-subj?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have your tried adding component.update() after form.setInputValue()?

Yup, no change.

Is there a reason why you decided to use component.find('input[type="search"]') instead of using a data-test-subj?

Fixed! I didn't realize this was possible until you suggested it and I looked at the EUI search bar config.

@cjcenizal cjcenizal added Feature:CCR and Remote Clusters release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.8.0 v7.9.0 v8.0.0 labels May 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal cjcenizal marked this pull request as ready for review May 14, 2020 00:19
@cjcenizal cjcenizal requested a review from a team as a code owner May 14, 2020 00:19
@cjcenizal
Copy link
Contributor Author

@sebelga Would you mind reviewing this? I skipped the failing search bar tests. I don't think we need to block this fix on getting those working.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Nice job tracking down this bug @cjcenizal! LGTM. Tested pagination behavior with mocked responses for CCR and Remote clusters. I also tested the delete behavior with real data for remote clusters only.

@cjcenizal cjcenizal merged commit 8f99f60 into elastic:master May 14, 2020
@cjcenizal cjcenizal deleted the bug/ccr-pagination branch May 14, 2020 21:05
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 14, 2020
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 14, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 15, 2020
…ent/add-support-in-url-for-hidden-toggle

* 'master' of github.com:elastic/kibana: (34 commits)
  [SIEM][CASE] Fix bug when connector is deleted. (elastic#65876)
  [SIEM][CASE] Improve layout (elastic#66232)
  [Index Management] Support Hidden Indices (elastic#66422)
  Add Login Selector functional tests. (elastic#65705)
  Lens drilldowns (elastic#65675)
  [ML] Custom template for apiDoc markdown (elastic#66567)
  Don't bootstrap core type emits (elastic#66377)
  [Dashboard] Improve loading error handling (elastic#66372)
  [APM] Minor style fixes for the node strokes (elastic#66574)
  [Ingest Manager] Fix create data source from integration (elastic#66626)
  [Metrics UI] Fix default metric alert interval for new conditions (elastic#66610)
  [Metrics UI] Fix alignment and allow clearing metric value (elastic#66589)
  Don't return package name for non-package data streams (elastic#66606)
  [Ingest Manager] Consolidate routing and add breadcrumbs to all pages (elastic#66475)
  [Docs/Reporting] Have the docs about granular timeout match Cloud docs (elastic#66267)
  Don't automatically add license header to code inside plugins dir. (elastic#66601)
  [APM] Don't trigger map layout if no elements (elastic#66625)
  [Logs UI] Validate ML job setup time ranges (elastic#66426)
  Fix pagination bugs in CCR and Remote Clusters (elastic#65931)
  Add cloud icon for supported settings and embed single-sourced getting started (elastic#65610)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/sections/home/index_list/index_table/index_table.js
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
// Mount the component
({ find, component, table, actions, form } = setup());

await nextTick(); // Make sure that the http request is fulfilled
Copy link
Contributor

Choose a reason for hiding this comment

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

From my notes and finding, we should not add nextTick() in our tests anymore but use jest.useFakeTimers()


// Skipped until we can figure out how to get this test to work.
test.skip('search works', () => {
form.setInputValue(find('followerIndexSearch'), 'unique');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably do this

act(() => {
  form.setInputValue(find('followerIndexSearch'), 'unique');
});
component.update();
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:CCR and Remote Clusters release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants