Dashboard filter by creator by Dosant · Pull Request #180147 · elastic/kibana

Dosant · 2024-04-05T11:54:25Z

Part of https://github.com/elastic/kibana-team/issues/769

Summary

This PR adds a "created by" filter to the dashboard listing page. We've added the created_by field recently here so only newly created by interactive users dashboards will be populated with created_by

Demo

Implementation notes

Suggesting users

We suggest only the users that have at least one dashboard. We do this by aggregating dashboard saved objects by created_by. Because we don't have server-side pagination or sorting yet, in this initial version, I decided to do this client-side to keep it as simple as possible for now.

Initially, I did this server-side inside a dashboard plugin as an internal API route. In the long term, this should probably be moved to content management in some form, but it is not clear how it should look yet. This is the commit where I reverted and moved it to client-side 06316ee

Clint side filtering

The user filter is simply a client-side filter because we anyway currently have an in-memory table with client-side sorting and pagination. There is no much practical value in server-side filter atm
The filter allows to filter-in by multiple users. It also allows to filter items without creators

`created_by` filter isn't part of the query string

I introduced a separate state for this filter, similar to tableSort and pagination. The reason why the created_by filter isn't a part of the searchQuery is because we use profile_id which looks ugly for the user, e.g. u_mGBROF_q5bmFCATLXAcCwKa0k8JvONAwSruelyKA5E_0 so we can't just add it to a user-facing search query like we do with tags. profile_id is the correct identified we should use, username might not be enough in some cases + username in cloud is unreadble account number. The best alternative could be email, but it isn't always available. This is why I decided to keep the created_by filter hidden from the user behind the user picker popover and not expose it to the search bar. The created_by filter is synced into the URL just like the rest of the table state.

No distinguishing the current user

There is no logic in place that somehow highlights or renames the current user in the filter. This can be added, but to limit the scope of the MVP I didn't do it.

User filter component

We use the UsersProfilePopover provided by the security team. We use it without any overrides, so the component decides how to show users in a popover. I noticed a small visual issue with an email truncation in a smaller popover. Might be worth a separate issue.

Testing

Create multiple new users
Login for each of them
Created a dashboard for each of them
Play with the filter

Next Steps

The next step would be to add a column with the user:

Release notes

The dashboard listing page now allows filtering dashboards by creators for dashboards created after version 8.14

Dosant · 2024-04-05T12:35:03Z

/ci

…fix'

packages/content-management/table_list_view_table/src/table_list_view_table.tsx

src/plugins/dashboard/server/plugin.ts

Dosant · 2024-04-05T13:06:10Z

/ci

packages/content-management/table_list_view_table/src/components/table.tsx

…ana into d/2024-04-05-create-by-filter

Dosant · 2024-04-05T13:39:42Z

/ci

…eate-by-filter

Dosant · 2024-04-09T12:58:36Z

/ci

…fix'

…ana into d/2024-04-05-create-by-filter

Dosant · 2024-04-09T13:44:32Z

/ci

…eate-by-filter

…eate-by-filter # Conflicts: # packages/content-management/table_list_view_table/src/components/table.tsx

Dosant · 2024-04-22T13:19:59Z

/ci

davismcphee

Code-only review. kbn-content-management-utils changes LGTM 👍

Heenawter

Code review + tested locally. It works great! Left two nits about the "No creators" filter, but nothing blocking 👍

I noticed a small visual issue with an email truncation in a smaller popover. Might be worth a separate issue.

I think it's worth creating a separate issue for the truncation issues:

This is why I decided to keep the created_by filter hidden from the user behind the user picker popover and not expose it to the search bar. The created_by filter is synced into the URL just like the rest of the table state.

I think this is reasonable! I'm not convinced that a URL-state filter is the right move here in the first place, so I think keeping it all client-side is ideal.

Heenawter · 2024-04-26T17:08:55Z

packages/content-management/table_list_view_table/src/components/user_filter_panel.tsx

+      content={
+        <FormattedMessage
+          id="contentManagement.tableList.listing.userFilter.emptyMessageTooltip"
+          defaultMessage="Creators are assigned when dashboards are created, (for all dashboards created after version 8.14)."


Not sure if the comma is supposed to be there?

Suggested change

defaultMessage="Creators are assigned when dashboards are created, (for all dashboards created after version 8.14)."

defaultMessage="Creators are assigned when dashboards are created (for all dashboards created after version 8.14)."

cc @amyjtechwriter

packages/content-management/table_list_view_table/src/components/user_filter_panel.tsx

sebelga

Great job, tested locally and works as expected 🎉

Regarding the implementation I would change one thing though.
I would not add the bulkGetUserProfiles prop as we can already create that handler internally. Every consumer passes the CoreStart contract to the table context so we can already fetch the user profile (we need to extend the contract here https://github.com/elastic/kibana/blob/main/packages/content-management/table_list_view_table/src/services.tsx#L76).

What we probably will need are some options to disable the createdBy filter.

I see that we have initialFilter as a string.

We could maybe replace it with a filtersConfig prop?

interface FiltersConfig {
  initialValue?: string;
  createdByEnabled?: boolean; // default to false
}

Regarding tests, I think it would be good to add some component integration tests (inside the "__jest__" folder)

packages/content-management/table_list_view_table/src/components/table.tsx

sebelga · 2024-04-29T09:44:04Z

packages/content-management/table_list_view_table/src/components/user_filter_panel.tsx

+      }
+
+      // keep the user if it's selected
+      if (selectedUsers.includes(user.uid)) return true;


Do we want that?

Sorry maybe not clear what I meant. 😊 Don't we want to not show the selected users when searching?

I think it is a better ux. We want to show the selected users while searching even if they don't match, so that the user can unselect without clearing the filter first.

Basically, selected should always stay in the list

azasypkin · 2024-04-29T11:14:59Z

ACK: will review today (on behalf of the Kibana Platform Security)

…eate-by-filter # Conflicts: # src/plugins/dashboard/tsconfig.json

azasypkin

Looks good from the security perspective 👍 If you find yourself needing to add the bulkGetUserProfiles API tag in more features, please let us know.

azasypkin · 2024-04-29T14:24:31Z

packages/kbn-user-profile-components/src/user_profiles_selectable.tsx

      key: userProfile.uid,
      label: getUserDisplayName(userProfile.user),
      data: userProfile,
+      'data-test-subj': `userProfileSelectableOption-${userProfile.user.username}`,


question: any reason we cannot use uid instead of username? I can imagine the case when username might not be unique (same username, different security realms, different uid's).

it is a bit hacky, but this is what was more convenient in tests, since I know the username, but don't know the u_id of the current user. I guess I can make an API request to fetch it. but not sure it is worth it in this case

but not sure it is worth it in this case

It's likely not worth it if it's not trivial. I was just not sure what happens if we have duplicated test attributes in prod - I assume nothing?

nothing! happens relatively often

azasypkin · 2024-04-29T15:23:37Z

x-pack/test/api_integration/apis/content_management/helpers.ts

+export async function loginAsInteractiveUser({
+  getService,
+}: Pick<FtrProviderContext, 'getService'>): Promise<{
+  Cookie: string;
+}> {
+  const config = getService('config');
+  const supertest = getService('supertestWithoutAuth');
+
+  const username = config.get('servers.kibana.username');
+  const password = config.get('servers.kibana.password');
+  const response = await supertest
+    .post('/internal/security/login')
+    .set('kbn-xsrf', 'xxx')
+    .send({
+      providerType: 'basic',
+      providerName: 'basic',
+      currentURL: '/',
+      params: { username, password },
+    })
+    .expect(200);
+  const cookie = parseCookie(response.header['set-cookie'][0])!.cookieString();
+
+  return { Cookie: cookie };
+}


note: not a blocker for this PR, obviously, but it'd be great if we can start embracing testing with non-admin/non-superuser users when possible 🙂 If you're up for it in this or the next PRs, here are the examples: https://github.com/elastic/kibana/blob/main/x-pack/test/security_api_integration/tests/user_profiles/suggest.ts#L17. I'm also happy to guide if you have any questions or issues with that.

Having said that, glad to see more functional and API integration tests with changes like that. Good job!

Thanks, I'll take a look

Is this also prefered for functional tests?

Yes, definitely. Ideally, relying on specially crafted roles should be our default choice everywhere. And if it's not easy for some reason, we (Kibana Platform Security and Kibana QA) should make it simpler. This would help us to see, use, and test Kibana how most of our users see and use it, and sometimes it's not that pretty because we don't care enough about people with limited privileges - security accessibility, so to say.

…eate-by-filter

Dosant · 2024-04-30T11:10:07Z

@elasticmachine merge upstream

…ana into d/2024-04-05-create-by-filter

sebelga

LGTM! 👍 Thanks for addressing the changes

kibana-ci · 2024-04-30T14:26:38Z

💚 Build Succeeded

Buildkite Build
Commit: 294c684

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	446	458	+12
`eventAnnotationListing`	509	520	+11
`filesManagement`	139	150	+11
`graph`	261	272	+11
`maps`	1166	1177	+11
`visualizations`	415	426	+11
total			+67

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/content-management-table-list-view-common`	7	8	+1
`@kbn/content-management-table-list-view-table`	33	34	+1
`@kbn/content-management-utils`	125	126	+1
total			+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cases`	475.9KB	476.1KB	+191.0B
`dashboard`	397.1KB	407.1KB	+10.0KB
`eventAnnotationListing`	198.5KB	208.4KB	+9.8KB
`filesManagement`	90.5KB	100.3KB	+9.8KB
`graph`	387.4KB	397.3KB	+9.8KB
`maps`	2.9MB	3.0MB	+9.9KB
`security`	574.6KB	574.8KB	+191.0B
`securitySolution`	13.7MB	13.7MB	+191.0B
`visualizations`	273.6KB	283.4KB	+9.8KB
total			+59.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dashboard`	37.0KB	37.1KB	+67.0B
`eventAnnotationListing`	10.7KB	10.7KB	+54.0B
`graph`	7.8KB	7.9KB	+54.0B
`maps`	50.5KB	50.5KB	+54.0B
`visualizations`	60.9KB	61.0KB	+54.0B
total			+283.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/content-management-table-list-view-common`	7	8	+1
`@kbn/content-management-table-list-view-table`	50	51	+1
`@kbn/content-management-utils`	191	192	+1
`@kbn/user-profile-components`	80	81	+1
total			+4

History

💔 Build #206883 failed 012c6c3
💔 Build #206826 failed e6b17c7
💚 Build #206674 succeeded 63eefb8
💔 Build #206618 failed 8796ecb
💚 Build #206571 succeeded 5d3c035

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

cc @Dosant

wip create by filter

d841799

Dosant changed the title ~~wip create by filter~~ [wip] Dashboard filter by creator Apr 5, 2024

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

fabaeb7

…fix'

Dosant commented Apr 5, 2024

View reviewed changes

packages/content-management/table_list_view_table/src/components/table.tsx Outdated Show resolved Hide resolved

Dosant added 2 commits April 5, 2024 15:38

fix i18n

7c76a72

Merge branch 'd/2024-04-05-create-by-filter' of github.com:Dosant/kib…

20fccb3

…ana into d/2024-04-05-create-by-filter

Dosant added 4 commits April 8, 2024 11:02

fix test

39bf916

move route to a separate file

8ac3657

add filter state to url

9f045e2

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

029b10d

…eate-by-filter

kibanamachine and others added 3 commits April 9, 2024 13:05

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

08589e0

…fix'

don’t show user filter if not available

3117577

Merge branch 'd/2024-04-05-create-by-filter' of github.com:Dosant/kib…

d236c78

…ana into d/2024-04-05-create-by-filter

Dosant added the ci:cloud-deploy Create or update a Cloud deployment label Apr 9, 2024

Dosant added 9 commits April 17, 2024 11:02

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

8b2469e

…eate-by-filter

“null” option

b992bc0

add bulkGetUserProfiles access check

f31923a

add tests

50792d4

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

c5a67d0

…eate-by-filter

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

77d50f8

…eate-by-filter

add ftr

2fbb20a

simplify to user client side suggest users

06316ee

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

90e24ed

…eate-by-filter # Conflicts: # packages/content-management/table_list_view_table/src/components/table.tsx

davismcphee approved these changes Apr 25, 2024

View reviewed changes

Heenawter self-requested a review April 26, 2024 13:53

Heenawter approved these changes Apr 26, 2024

View reviewed changes

sebelga reviewed Apr 29, 2024

View reviewed changes

change clear label

5d3c035

Dosant added 5 commits April 29, 2024 14:37

hide “no users” option, get userFilter from context

23bdac5

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

dc3720f

…eate-by-filter # Conflicts: # src/plugins/dashboard/tsconfig.json

bulkGetUserProfiles required

8796ecb

add jest integration tests

9f8d058

fixes

63eefb8

Dosant self-assigned this Apr 29, 2024

azasypkin approved these changes Apr 29, 2024

View reviewed changes

Dosant requested a review from sebelga April 29, 2024 15:27

Dosant added 2 commits April 30, 2024 10:35

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

59a1982

…eate-by-filter

use custom user for api test

e6b17c7

Dosant mentioned this pull request Apr 30, 2024

UsersProfilePopover doesn't handle long names well #182084

Closed

Merge branch 'main' of github.com:elastic/kibana into d/2024-04-05-cr…

e5fdac8

…eate-by-filter

kibanamachine and others added 3 commits April 30, 2024 07:10

Merge branch 'main' into d/2024-04-05-create-by-filter

012c6c3

fix types

22f54e9

Merge branch 'd/2024-04-05-create-by-filter' of github.com:Dosant/kib…

294c684

…ana into d/2024-04-05-create-by-filter

sebelga approved these changes Apr 30, 2024

View reviewed changes

Dosant merged commit f5d7eb6 into elastic:main Apr 30, 2024

kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels Apr 30, 2024

Dosant added release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels May 2, 2024

	defaultMessage="Creators are assigned when dashboards are created, (for all dashboards created after version 8.14)."
	defaultMessage="Creators are assigned when dashboards are created (for all dashboards created after version 8.14)."

Conversation

Dosant commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Demo

Implementation notes

Suggesting users

Clint side filtering

created_by filter isn't part of the query string

No distinguishing the current user

User filter component

Testing

Next Steps

Release notes

Uh oh!

Dosant commented Apr 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dosant commented Apr 5, 2024

Uh oh!

Uh oh!

Dosant commented Apr 5, 2024

Uh oh!

Dosant commented Apr 9, 2024

Uh oh!

Dosant commented Apr 9, 2024

Uh oh!

Dosant commented Apr 22, 2024

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Heenawter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebelga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Apr 29, 2024

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant commented Apr 30, 2024

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant commented Apr 5, 2024 •

edited

Loading

`created_by` filter isn't part of the query string

sebelga left a comment •

edited

Loading

azasypkin Apr 29, 2024 •

edited

Loading