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

[TableListView] Fix tag selection when passing initialFilter #160871

Merged

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jun 29, 2023

In #160540 we discovered that the tag selection was not applied correctly when passing an initialFilter to the TableListView.

This PR fixes this behaviour.

While debugging I also found another bug in the DashboardListing page, the "clear tag selection" was not working correctly because of an unstable reference to the saved object tagging plugin.

How to test

  • Install one of the sample data set
  • Go to dashboard listing, and edit one of them --> add a tag to the dashboard (e.g. "foo")
  • Go back the listing.
  • Modify the dashboard_listing.tsx file (src/plugins/dashboard/public/dashboard_listing/dashboard_listing.tsx)
--- a/src/plugins/dashboard/public/dashboard_listing/dashboard_listing.tsx
+++ b/src/plugins/dashboard/public/dashboard_listing/dashboard_listing.tsx
@@ -240,7 +240,8 @@ export const DashboardListing = ({
           title={getTableListTitle()}
           headingId="dashboardListingHeading"
           initialPageSize={initialPageSize}
-          initialFilter={initialFilter}
+          // initialFilter={initialFilter}
+          initialFilter={`tag:("foo")`}
           entityName={getEntityName()}
           listingLimit={listingLimit}
           emptyPrompt={emptyPrompt}
  • Refresh the page
  • The search text should be updated and the tag selection should be correct
Screenshot 2023-06-29 at 12 35 50

@@ -618,12 +624,67 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
// ------------
// Callbacks
// ------------
const buildQueryFromText = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This handler has been extracted from updateQueryFromURL() below

@@ -206,6 +206,14 @@ export const DashboardListing = ({

const { getEntityName, getTableListTitle, getEntityNamePlural } = dashboardListingTableStrings;

const savedObjectsTaggingFakePlugin = useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a stable reference of the savedObjectsTagging dep.

Copy link
Contributor

Choose a reason for hiding this comment

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

UseMemo addition makes sense. We'll be introducing a simpler services mechanic at some point, and will remove this.

@sebelga sebelga marked this pull request as ready for review June 29, 2023 13:18
@sebelga sebelga requested review from a team as code owners June 29, 2023 13:18
@sebelga sebelga self-assigned this Jun 29, 2023
@sebelga sebelga added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Feature:Content Management User generated content (saved objects) management Component:TableListView labels Jun 29, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Jun 29, 2023
@sebelga sebelga requested a review from ThomThomson June 29, 2023 14:27
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Dashboard listing changes LGTM

@@ -206,6 +206,14 @@ export const DashboardListing = ({

const { getEntityName, getTableListTitle, getEntityNamePlural } = dashboardListingTableStrings;

const savedObjectsTaggingFakePlugin = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

UseMemo addition makes sense. We'll be introducing a simpler services mechanic at some point, and will remove this.

@nreese
Copy link
Contributor

nreese commented Jun 29, 2023

How about adding a functional test to prevent regression? For dashboard, the tag is passed in the URL like http://localhost:5601/imu/app/dashboards#/list?_g=(filters:!(),refreshInterval:(pause:!f,value:900000),time:(from:now-7d/d,to:now))&s=tag:(foo)

@sebelga
Copy link
Contributor Author

sebelga commented Jun 29, 2023

How about adding a functional test to prevent regression? For dashboard, the tag is passed in the URL

Here we are testing the other direction. An initial filter is passed through prop to the component, which updates the URL (if URL state is enabled).

This functional test that you mention should be added once the security team adds this filter when consuming the dashboard table in security (#160540)

@sebelga sebelga enabled auto-merge (squash) June 30, 2023 10:55
@kibana-ci
Copy link
Collaborator

💚 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
dashboard 360.2KB 360.4KB +163.0B
eventAnnotation 120.6KB 120.7KB +138.0B
filesManagement 88.8KB 89.0KB +132.0B
graph 458.0KB 458.1KB +132.0B
maps 2.7MB 2.7MB +132.0B
visualizations 262.0KB 262.2KB +132.0B
total +829.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 492 496 +4
total +6

History

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

cc @sebelga

@sebelga sebelga merged commit aec3a4b into elastic:main Jun 30, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Jun 30, 2023
@sebelga sebelga deleted the table-list-view/fix-initial-tag-selection branch July 18, 2023 14:16
@sebelga
Copy link
Contributor Author

sebelga commented Jul 18, 2023

💚 All backports created successfully

Status Branch Result
8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

sebelga added a commit to sebelga/kibana that referenced this pull request Jul 18, 2023
sebelga added a commit that referenced this pull request Jul 19, 2023
…160871) (#162169)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[TableListView] Fix tag selection when passing initialFilter
(#160871)](#160871)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sébastien
Loix","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-06-30T12:00:54Z","message":"[TableListView]
Fix tag selection when passing initialFilter
(#160871)","sha":"aec3a4bee3456d3d0ff06802c74caa13545fe2f3","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","Team:SharedUX","Feature:Content
Management","Component:TableListView","v8.10.0"],"number":160871,"url":"https://github.com/elastic/kibana/pull/160871","mergeCommit":{"message":"[TableListView]
Fix tag selection when passing initialFilter
(#160871)","sha":"aec3a4bee3456d3d0ff06802c74caa13545fe2f3"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/160871","number":160871,"mergeCommit":{"message":"[TableListView]
Fix tag selection when passing initialFilter
(#160871)","sha":"aec3a4bee3456d3d0ff06802c74caa13545fe2f3"}}]}]
BACKPORT-->
angorayc added a commit that referenced this pull request Jul 24, 2023
## Summary


1. Align dashboard listing UI with Kibana dashboard.
2. `Security Solution` tags are selected by default and removable by
users.

**Prerequisite:**
This PR is waiting for #160871 to
be merged


**Steps to verify:**
1. Visit Security > Dashboards, and create a dashboard from this page.
2. Back to Security Dashboards page, you should see the dashboard you
just created and Security Solution tag should be selected by default in
the tag filters.
3. Open the tag options, click the Security Solution tag. Observe that
it should be removable, and it should display all the dashboards you
have in the table.

**Known issues:**
#160540 (comment)

**Before:**

<img width="2545" alt="Screenshot 2023-06-27 at 09 24 19"
src="https://github.com/elastic/kibana/assets/6295984/bc0fa0b1-96ad-43b0-afc1-48444dfb5691">


**After:**
<img width="2543" alt="Screenshot 2023-06-27 at 09 22 21"
src="https://github.com/elastic/kibana/assets/6295984/82d0a868-bda2-431f-b0b5-9cbc34d3ae71">


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Pablo Neves Machado <[email protected]>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## Summary


1. Align dashboard listing UI with Kibana dashboard.
2. `Security Solution` tags are selected by default and removable by
users.

**Prerequisite:**
This PR is waiting for elastic#160871 to
be merged


**Steps to verify:**
1. Visit Security > Dashboards, and create a dashboard from this page.
2. Back to Security Dashboards page, you should see the dashboard you
just created and Security Solution tag should be selected by default in
the tag filters.
3. Open the tag options, click the Security Solution tag. Observe that
it should be removable, and it should display all the dashboards you
have in the table.

**Known issues:**
elastic#160540 (comment)

**Before:**

<img width="2545" alt="Screenshot 2023-06-27 at 09 24 19"
src="https://github.com/elastic/kibana/assets/6295984/bc0fa0b1-96ad-43b0-afc1-48444dfb5691">


**After:**
<img width="2543" alt="Screenshot 2023-06-27 at 09 22 21"
src="https://github.com/elastic/kibana/assets/6295984/82d0a868-bda2-431f-b0b5-9cbc34d3ae71">


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Pablo Neves Machado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Component:TableListView Feature:Content Management User generated content (saved objects) management release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.9.0 v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants