[Roles] Fix bug with roles grid not sorting on clicking table header #194196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SiddharthMantri
added
bug
|
Pinging @elastic/kibana-security (Team:Security) |
|
@elasticmachine merge upstream |
SiddharthMantri
added
backport:prev-major
jeramysoucy
requested changes
Sep 27, 2024
There was a problem hiding this comment.
LGTM! Just some suggestions on the unit tests.
I tested in stateful and serverless with custom roles enabled. I found it interesting that the default sort puts lowercase after uppercase. Not shocking, but not what I would expect. Nothing we're doing, but maybe worth pinging EUI to see if this is the intended sort order.
| base: [], | ||
| spaces: [], | ||
| feature: {}, | ||
| expect(wrapper.find(EuiBasicTable).props().items).toEqual( |
There was a problem hiding this comment.
I noticed that you switched to expect.arrayContaining rather than matching an explicit array. Shouldn't these be looking for an EuiInMemoryTable now? If the EUI type is updated, it seems you can switch back to matching explicit values.
Suggested change
| expect(wrapper.find(EuiBasicTable).props().items).toEqual( | |
| expect(wrapper.find(EuiInMemoryTable).props().items).toEqual( |
There was a problem hiding this comment.
The test originally used EuiBasicTable even before I switched in this PR. I think we can switch to EuiInMemoryTable and then bring back basic table whenever the server side API is ready.
There was a problem hiding this comment.
Done in: 2775e0c
There was a problem hiding this comment.
Nit: should we consider adding a test that confirms the rows can be sorted by each of the columns?
There was a problem hiding this comment.
Added in 2775e0c
|
@elasticmachine merge upstream |
jeramysoucy
approved these changes
Sep 27, 2024
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
|
Starting backport for target branches: 8.15, 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 27, 2024
…lastic#194196) Fixes elastic#193786 ## Summary Reverts a few changes made when the Roles grid page was moved to a functional component. Fixes regression in table sorting. ### Notes When preparing for the Query Roles API, we had moved the roles grid page to be a functional component. In doing so, we also migrated away from the In Memory table in favor of the basic table. EUIBasicTable does not support sorting out of the box and is meant to be used for server-side sorting, etc (unless we implement custom sorting logic). I've made a few changes: - Bring back the InMemoryTable but keep the Search Bar. - Remove few (now) unused functions which are to be brought back whenever the Query Roles API is ready. - Update tests ### Screen recording https://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100 ### Release notes Fixes UI regression in Roles listing page where users could not sort table by using the headers. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit f4ca9e4)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 27, 2024
…lastic#194196) Fixes elastic#193786 ## Summary Reverts a few changes made when the Roles grid page was moved to a functional component. Fixes regression in table sorting. ### Notes When preparing for the Query Roles API, we had moved the roles grid page to be a functional component. In doing so, we also migrated away from the In Memory table in favor of the basic table. EUIBasicTable does not support sorting out of the box and is meant to be used for server-side sorting, etc (unless we implement custom sorting logic). I've made a few changes: - Bring back the InMemoryTable but keep the Search Bar. - Remove few (now) unused functions which are to be brought back whenever the Query Roles API is ready. - Update tests ### Screen recording https://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100 ### Release notes Fixes UI regression in Roles listing page where users could not sort table by using the headers. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit f4ca9e4)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Sep 27, 2024
…header (#194196) (#194282) # Backport This will backport the following commits from `main` to `8.15`: - [[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)](#194196) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Sid","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T10:59:28Z","message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","regression","release_note:fix","Team:Security","Feature:Users/Roles/API Keys","v9.0.0","backport:prev-major"],"title":"[Roles] Fix bug with roles grid not sorting on clicking table header","number":194196,"url":"https://github.com/elastic/kibana/pull/194196","mergeCommit":{"message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194196","number":194196,"mergeCommit":{"message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5"}}]}] BACKPORT--> Co-authored-by: Sid <[email protected]>
kibanamachine
added a commit
that referenced
this pull request
Sep 27, 2024
…eader (#194196) (#194283) # Backport This will backport the following commits from `main` to `8.x`: - [[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)](#194196) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Sid","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T10:59:28Z","message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","regression","release_note:fix","Team:Security","Feature:Users/Roles/API Keys","v9.0.0","backport:prev-major"],"title":"[Roles] Fix bug with roles grid not sorting on clicking table header","number":194196,"url":"https://github.com/elastic/kibana/pull/194196","mergeCommit":{"message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194196","number":194196,"mergeCommit":{"message":"[Roles] Fix bug with roles grid not sorting on clicking table header (#194196)\n\nFixes https://github.com/elastic/kibana/issues/193786\r\n\r\n## Summary\r\nReverts a few changes made when the Roles grid page was moved to a\r\nfunctional component. Fixes regression in table sorting.\r\n\r\n### Notes\r\n\r\nWhen preparing for the Query Roles API, we had moved the roles grid page\r\nto be a functional component. In doing so, we also migrated away from\r\nthe In Memory table in favor of the basic table. EUIBasicTable does not\r\nsupport sorting out of the box and is meant to be used for server-side\r\nsorting, etc (unless we implement custom sorting logic). I've made a few\r\nchanges:\r\n- Bring back the InMemoryTable but keep the Search Bar.\r\n- Remove few (now) unused functions which are to be brought back\r\nwhenever the Query Roles API is ready.\r\n- Update tests\r\n\r\n### Screen recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4ac4f771-e7d1-4e17-807e-d6262767d100\r\n\r\n\r\n\r\n### Release notes\r\nFixes UI regression in Roles listing page where users could not sort\r\ntable by using the headers.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f4ca9e4730ee6b13d19808be9fcc633dce9087d5"}}]}] BACKPORT--> Co-authored-by: Sid <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #193786
Summary
Reverts a few changes made when the Roles grid page was moved to a functional component. Fixes regression in table sorting.
Notes
When preparing for the Query Roles API, we had moved the roles grid page to be a functional component. In doing so, we also migrated away from the In Memory table in favor of the basic table. EUIBasicTable does not support sorting out of the box and is meant to be used for server-side sorting, etc (unless we implement custom sorting logic). I've made a few changes:
Screen recording
Screen.Recording.2024-09-27.at.00.00.17.mov
Release notes
Fixes UI regression in Roles listing page where users could not sort table by using the headers.