Fix Saved Objects total item count and table filtering #21574

cjcenizal · 2018-08-01T22:23:46Z

Also fixes a bug in which applying filters to the table beyond the first one wouldn't have any effect.

…e header and for the table.

cjcenizal · 2018-08-01T22:25:01Z

..._plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

@@ -196,11 +206,8 @@ export class ObjectsTable extends Component {
  };

  onQueryChange = ({ query }) => {
-    // TODO: investigate why this happens at EUI level
-    if (isSameQuery(query, this.state.activeQuery)) {


I think there's a bug in EUI in which a reference is being reused within the query.ast instance, causing this comparison to incorrectly return true.

elasticmachine · 2018-08-01T23:52:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jen-huang

Tested locally and confirmed that object counts are correct. Just a small question, otherwise LGTM!

jen-huang · 2018-08-02T00:12:17Z

.../public/management/sections/objects/components/objects_table/__tests__/objects_table.test.js

@@ -425,7 +425,6 @@ describe('ObjectsTable', () => {
      expect(mockSavedObjectsClient.delete).toHaveBeenCalledWith(mockSavedObjects[0].type, mockSavedObjects[0].id);
      expect(mockSavedObjectsClient.delete).toHaveBeenCalledWith(mockSavedObjects[1].type, mockSavedObjects[1].id);
      expect(component.state('selectedSavedObjects').length).toBe(0);
-      expect(defaultProps.savedObjectsClient.find.mock.calls.length).toBe(2);


is this meant to be removed? does its equivalence need to be added elsewhere or in another test?

It took me awhile to understand the intention of this test. When I realized it failed on CI because I had added additional calls to fetch the number of saved objects, I decided it was testing an implementation detail. I removed it because I don't think it's providing value.

elasticmachine · 2018-08-02T01:28:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nreese

while you are in this file, how about replacing $http with kfetch? getSavedObjectCounts should just use kfetch.

nreese · 2018-08-02T12:34:03Z

another question - why does the order of the type filter list change when one is selected? Its a short list, why not just always display in alphabetic order? It is annoying when the UI changes underneath you and then a click is registered for the wrong item

nreese · 2018-08-02T12:35:31Z

..._plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

@@ -141,7 +138,8 @@ export class ObjectsTable extends Component {
    if (!activeQuery) {
      return {
        pageOfItems: [],
-        totalItemCount: 0,
+        // Deliberately don't reset totalItemCount because 0 wouldn't be accurate.


Why does this method short circuit when there are not active queries?

I'm not sure and looking through the code I actually don't see how this branch could ever be hit. I'm going to try removing it.

nreese · 2018-08-02T12:38:14Z

..._plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

+          type: INCLUDED_TYPES,
+        });
+
+        totalItemCount = allSavedObjects.total;


Now that the totatItemCount ignores filters, does it still make sense to display right next to the page title? I find this a little misleading. How about moving the totalItemCount to "Export Everything" button since that is where the total number of objects is really only relevant.

Good idea, will do.

nreese · 2018-08-02T13:05:08Z

Filters are applied inconstantly to "Export Everything".

For example, If I only filter by "dashboard" type then export everything still exports everything

However, then if I add another filter that filters by saved object name by typing a string in the input box, then export everything is now filtered by that string but not by type? Why? As a user, how would I know or expect the difference?

Maybe getSavedObjectCounts still needs to be filtered by type and then in the results, if the type is missing, the count is set to zero

cjcenizal · 2018-08-02T13:36:09Z

while you are in this file, how about replacing $http with kfetch?

Great idea, will do!

why does the order of the type filter list change when one is selected?

I find this really annoying too. It's so that all of the selected items are at the top. This is a problem with EUI, so we should file an issue in the EUI repo.

Filters are applied inconstantly to "Export Everything".

This is my mistake. I think the original design was to make this button export everything that's in the table (i.e. after all filters and search have been applied). I think this is a desirable UX -- you can search for some objects by name and bulk export that entire set. What do you think?

nreese · 2018-08-02T14:43:53Z

I think the original design was to make this button export everything that's in the table (i.e. after all filters and search have been applied). I think this is a desirable UX -- you can search for some objects by name and bulk export that entire set. What do you think?

I agree that "export everything" should export the table contents since there is no other way to export the table.

…aved objects in bulk export button. Only show visible types in bulk export modal.

cjcenizal · 2018-08-02T18:48:21Z

@nreese Could you take another look? Considering that this is going into 6.4, I'd prefer to save the $http -> fkfetch migration for another PR.

elasticmachine · 2018-08-02T20:19:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nreese · 2018-08-02T20:26:56Z

Really nice. Do you even need to still display the total saved object count in the page title?

cjcenizal · 2018-08-02T20:28:54Z

@nreese Personally, I think it's useful to know exactly how many saved objects there are when not all of them are visible.

nreese

Thanks for fixing this
lgtm
code review, test changes and verified problem has been addressed.

nreese · 2018-08-02T20:39:54Z

..._plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

    const savedObjectCounts = await getSavedObjectCounts(
      this.props.$http,
-      type,
+      INCLUDED_TYPES,


Why was this changed from filteredTypes to all types?

It's used to populate the filter dropdown in the table. Originally, filtering on a type would reduce the counts of the non-selected types to 0, within the filter dropdown. I'll add a comment.

elasticmachine · 2018-08-02T22:20:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nreese · 2018-08-02T22:54:17Z

Looks like the de-select does not quite work as expected.

Try selecting a saved object
Do a more inclusive filter that includes the selected saved object
Notice that the saved object is still selected but the delete and export buttons are still disabled

cjcenizal · 2018-08-02T23:11:34Z

@nreese I think this is a bug within EUI. There's no way to externally tell EuiBasicTable to reset its selection state. Even EuiInMemoryTable has the same bug you describe, but it manifests itself when you do a search that returns no results. I'll create an EUI issue but I don't think it's a blocker for this PR.

nreese · 2018-08-02T23:14:01Z

Agreed, its not a blocker for this PR. It is easy for the user to recover by de-selecting and then re-selecting the item again.

lgtm

* Remove implementation detail assertion from objects table test. * Reset selection state when table state or query state changes. * Remove total count from title.

cjcenizal added 2 commits August 1, 2018 14:59

Fix Saved Objects total item count by creating separate counts for th…

e08bb2b

…e header and for the table.

Fix broken Saved Objects table filters.

2698e24

cjcenizal added :Management v7.0.0 v6.4.0 v6.5.0 labels Aug 1, 2018

cjcenizal requested review from nreese and jen-huang August 1, 2018 22:23

cjcenizal commented Aug 1, 2018

View reviewed changes

Remove implementation detail assertion from objects table test.

73b1d6b

jen-huang approved these changes Aug 2, 2018

View reviewed changes

nreese reviewed Aug 2, 2018

View reviewed changes

cjcenizal added blocker and removed blocker labels Aug 2, 2018

cjcenizal added 2 commits August 2, 2018 11:42

Show count of all saved objects in title. Show count of all visible s…

0964111

…aved objects in bulk export button. Only show visible types in bulk export modal.

Remove unused logic branch.

432ec84

Reset selection state when table state or query state changes.

fc5d2d1

nreese approved these changes Aug 2, 2018

View reviewed changes

Remove total count from title.

0750f49

Add comment.

099fb09

cjcenizal mentioned this pull request Aug 2, 2018

Consumer needs to be able to reset EuiBasicTable selection state elastic/eui#1083

Closed

cjcenizal merged commit a207d97 into elastic:master Aug 2, 2018

cjcenizal deleted the bug/saved-object-counts branch August 2, 2018 23:18

This was referenced Aug 2, 2018

[6.x] Fix Saved Objects item count and table filtering (#21574) #21620

Merged

[6.4] Fix Saved Objects item count and table filtering (#21574) #21621

Merged

cjcenizal mentioned this pull request Aug 3, 2018

Saved objects UI needs to clear previous selection when search filters are switched. #21615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Saved Objects total item count and table filtering #21574

Fix Saved Objects total item count and table filtering #21574

cjcenizal commented Aug 1, 2018 •

edited

Loading

cjcenizal Aug 1, 2018

elasticmachine commented Aug 1, 2018

jen-huang left a comment

jen-huang Aug 2, 2018

cjcenizal Aug 2, 2018

elasticmachine commented Aug 2, 2018

nreese left a comment

nreese commented Aug 2, 2018

nreese Aug 2, 2018

cjcenizal Aug 2, 2018

nreese Aug 2, 2018

cjcenizal Aug 2, 2018

nreese commented Aug 2, 2018 •

edited

Loading

cjcenizal commented Aug 2, 2018

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

elasticmachine commented Aug 2, 2018

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

nreese left a comment

nreese Aug 2, 2018

cjcenizal Aug 2, 2018 •

edited

Loading

elasticmachine commented Aug 2, 2018

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

nreese commented Aug 2, 2018 •

edited

Loading

Fix Saved Objects total item count and table filtering #21574

Fix Saved Objects total item count and table filtering #21574

Conversation

cjcenizal commented Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 1, 2018

💔 Build Failed

jen-huang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 2, 2018

💚 Build Succeeded

nreese left a comment

Choose a reason for hiding this comment

nreese commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented Aug 2, 2018 • edited Loading

cjcenizal commented Aug 2, 2018

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

elasticmachine commented Aug 2, 2018

💚 Build Succeeded

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 2, 2018

💚 Build Succeeded

nreese commented Aug 2, 2018

cjcenizal commented Aug 2, 2018

nreese commented Aug 2, 2018 • edited Loading

cjcenizal commented Aug 1, 2018 •

edited

Loading

nreese commented Aug 2, 2018 •

edited

Loading

cjcenizal Aug 2, 2018 •

edited

Loading

nreese commented Aug 2, 2018 •

edited

Loading