[ResponceOps][MaintenanceWindow] MX Pagination #202539
Merged
+1,149
−184
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
…e/kibana into MX-193076-MX-pagination-backend
…e/kibana into MX-193076-MX-pagination-backend
guskovaue
added
release_note:skip
cnasikas
reviewed
Dec 16, 2024
...gins/alerting/server/application/maintenance_window/methods/find/find_maintenance_windows.ts
Outdated
Show resolved
Hide resolved
...outes/maintenance_window/apis/find/transforms/transform_find_maintenance_window_params/v1.ts
Outdated
Show resolved
Hide resolved
.../maintenance_window/apis/find/transforms/transform_find_maintenance_window_params/v1.test.ts
Show resolved
Hide resolved
...gins/alerting/server/application/maintenance_window/methods/find/find_maintenance_windows.ts
Outdated
Show resolved
Hide resolved
Comment on lines
32
to
37
| const fullQuery = statuses | ||
| ?.slice(1) | ||
| .reduce( | ||
| (acc, currentStatusFilter) => acc + ` or ${statusToQueryMapping[currentStatusFilter] || ''}`, | ||
| statusToQueryMapping[statuses[0]] || '' | ||
| ); |
There was a problem hiding this comment.
Suggested change
| const fullQuery = statuses | |
| ?.slice(1) | |
| .reduce( | |
| (acc, currentStatusFilter) => acc + ` or ${statusToQueryMapping[currentStatusFilter] || ''}`, | |
| statusToQueryMapping[statuses[0]] || '' | |
| ); | |
| const fullQuery = statuses.map((status) => statusToQueryMapping[status]).filter(Boolean).join('or') |
|
|
||
| const [inputText, setInputText] = useState<string>(''); | ||
| const [selectedStatuses, setSelectedStatuses] = useState<MaintenanceWindowStatus[]>([]); | ||
| const [searchText, setSearchText] = useState<string>(''); |
There was a problem hiding this comment.
I am a bit confused 🙂. Why do we need the inputText and the searchText. Could we do it only with searchText?
There was a problem hiding this comment.
We need to states here. We do not have incremental search here. We search when Enter key will be pressed. So we need one state for search line and one state for current input.
x-pack/plugins/alerting/public/pages/maintenance_windows/index.tsx
Outdated
Show resolved
Hide resolved
...tional_with_es_ssl/apps/triggers_actions_ui/maintenance_windows/maintenance_windows_table.ts
Outdated
Show resolved
Hide resolved
| }); | ||
| await browser.refresh(); | ||
|
|
||
| await pageObjects.maintenanceWindows.searchMaintenanceWindows('pagination'); |
There was a problem hiding this comment.
We are filtering here because we do not have delete endpoint and all previous MWs are in place.
It was written like this before by Alexi, so I decided to continue like this for now and change, when we introduce delete MW.
There was a problem hiding this comment.
Ok, then I would suggest moving the state for the input in x-pack/plugins/alerting/public/pages/maintenance_windows/components/maintenance_windows_list.tsx and keeping only the state for the search in this component. Example
// x-pack/plugins/alerting/public/pages/maintenance_windows/index.tsx
const [search, setSearch] = useState<string>('');
<MaintenanceWindowsList
onSearchChange={setSearch}
/>
// x-pack/plugins/alerting/public/pages/maintenance_windows/components/maintenance_windows_list.tsx
const [search, setSearch] = useState<string>('');
<EuiFieldSearch
incremental={false}
placeholder={i18n.SEARCH_PLACEHOLDER}
value={inputText}
onSearch={onSearchChange} // from props
onChange={(e) => setSearch(e.target.value)}
/>
x-pack/plugins/alerting/public/services/maintenance_windows_api/find.ts
Outdated
Show resolved
Hide resolved
|
I forgot to say that I noticed that when we loading (filtering by status for example) we show a spinner and hide the whole page. What about showing a loading state deeper in the code, for example a progress bar in the table or similar? Screen.Recording.2024-12-16.at.3.05.56.PM.mov |
…ue/kibana into MX-193076-MX-pagination-frontend
cnasikas
reviewed
Dec 19, 2024
Comment on lines
12
to
19
| // loadTestFile(require.resolve('./home_page')); | ||
| // loadTestFile(require.resolve('./rules_list')); | ||
| // loadTestFile(require.resolve('./alert_create_flyout')); | ||
| // loadTestFile(require.resolve('./details')); | ||
| // loadTestFile(require.resolve('./connectors')); | ||
| // loadTestFile(require.resolve('./logs_list')); | ||
| // loadTestFile(require.resolve('./rules_settings')); | ||
| // loadTestFile(require.resolve('./stack_alerts_page')); |
Comment on lines
13
to
14
| // loadTestFile(require.resolve('./maintenance_window_create_form')); | ||
| // loadTestFile(require.resolve('./maintenance_window_update_form')); |
There was a problem hiding this comment.
Already done. I forgot to do it first
| @@ -53,7 +66,7 @@ export const useFindMaintenanceWindows = (props?: UseFindMaintenanceWindowsProps | |||
| }); | |||
|
|
|||
| return { | |||
| maintenanceWindows: data, | |||
| data: data || { maintenanceWindows: [], total: 0 }, | |||
There was a problem hiding this comment.
Yes, this is happening because on an error data will be undefined. What if you keep the placeholderData and also do
const { data, ..// other stuff } = useFindMaintenanceWindows({ ...// args })
const { maintenanceWindows, total } = data ?? { maintenanceWindows: [], total: 0 };
Comment on lines
32
to
37
| const fullQuery = statuses | ||
| ?.slice(1) | ||
| .reduce( | ||
| (acc, currentStatusFilter) => acc + ` or ${statusToQueryMapping[currentStatusFilter] || ''}`, | ||
| statusToQueryMapping[statuses[0]] || '' | ||
| ); |
| [MaintenanceWindowStatus.Upcoming]: | ||
| '(not (maintenance-window.attributes.events: "now") and maintenance-window.attributes.events >= "now")', | ||
| [MaintenanceWindowStatus.Finished]: | ||
| '(not (maintenance-window.attributes.events >= "now" or maintenance-window.attributes.expirationDate < "now"))', |
There was a problem hiding this comment.
Is it possible for an archived MW to have events where maintenance-window.attributes.events < "now")?
| }); | ||
| await browser.refresh(); | ||
|
|
||
| await pageObjects.maintenanceWindows.searchMaintenanceWindows('pagination'); |
There was a problem hiding this comment.
Ok, then I would suggest moving the state for the input in x-pack/plugins/alerting/public/pages/maintenance_windows/components/maintenance_windows_list.tsx and keeping only the state for the search in this component. Example
// x-pack/plugins/alerting/public/pages/maintenance_windows/index.tsx
const [search, setSearch] = useState<string>('');
<MaintenanceWindowsList
onSearchChange={setSearch}
/>
// x-pack/plugins/alerting/public/pages/maintenance_windows/components/maintenance_windows_list.tsx
const [search, setSearch] = useState<string>('');
<EuiFieldSearch
incremental={false}
placeholder={i18n.SEARCH_PLACEHOLDER}
value={inputText}
onSearch={onSearchChange} // from props
onChange={(e) => setSearch(e.target.value)}
/>
| queryKey: ['findMaintenanceWindows'], | ||
| const queryKey = ['findMaintenanceWindows', page, perPage, search, selectedStatus]; | ||
|
|
||
| const { isLoading, isFetching, isInitialLoading, data, refetch } = useQuery({ |
There was a problem hiding this comment.
Let's also add keepPreviousData: true to avoid having the table clear every time we change status.
| perPage: number; | ||
| total: number; | ||
| onPageChange: ({ page: { index, size } }: { page: { index: number; size: number } }) => void; | ||
| inputText: string; |
| [MaintenanceWindowStatus.Upcoming]: | ||
| '(not (maintenance-window.attributes.events: "now") and maintenance-window.attributes.events >= "now")', | ||
| [MaintenanceWindowStatus.Finished]: | ||
| '(not (maintenance-window.attributes.events >= "now" or maintenance-window.attributes.expirationDate < "now"))', |
There was a problem hiding this comment.
Yes sorry what I meant was not maintenance-window.attributes.events: > "now" and maintenance-window.attributes.expirationDate > "now"
…ue/kibana into MX-193076-MX-pagination-frontend
…ue/kibana into MX-193076-MX-pagination-frontend
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
cc @guskovaue |
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 21, 2024
Fixes: elastic#198252 In this PR I introduced pagination in MW frontend part and also pass filters(status and search) to the backend. Pagination arguments were passed to backend in another PR: https://github.com/elastic/kibana/pull/197172/files#diff-f375a192a08a6db3fbb6b6e927cecaab89ff401efc4034f00761e8fc4478734c How to test: Go to Maintenance Window, create more than 10 MW with different statuses. Try pagination, search on text and filter by status. Check the PR satisfies following conditions: - [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) - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 43abe23)
💚 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
Dec 21, 2024
# Backport This will backport the following commits from `main` to `8.x`: - [[ResponceOps][MaintenanceWindow] MX Pagination (#202539)](#202539) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Julia","email":"iuliia.guskova@elastic.co"},"sourceCommit":{"committedDate":"2024-12-21T20:01:11Z","message":"[ResponceOps][MaintenanceWindow] MX Pagination (#202539)\n\nFixes: https://github.com/elastic/kibana/issues/198252\r\n\r\nIn this PR I introduced pagination in MW frontend part and also pass\r\nfilters(status and search) to the backend. Pagination arguments were\r\npassed to backend in another PR:\r\nhttps://github.com//pull/197172/files#diff-f375a192a08a6db3fbb6b6e927cecaab89ff401efc4034f00761e8fc4478734c\r\n\r\nHow to test:\r\nGo to Maintenance Window, create more than 10 MW with different\r\nstatuses. Try pagination, search on text and filter by status.\r\n\r\nCheck the PR satisfies following conditions:\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"43abe233d814cb9a5519a63a2f5942f8802879b2","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","Feature:Alerting/RulesManagement","backport:prev-minor","v8.18.0"],"title":"[ResponceOps][MaintenanceWindow] MX Pagination","number":202539,"url":"https://github.com/elastic/kibana/pull/202539","mergeCommit":{"message":"[ResponceOps][MaintenanceWindow] MX Pagination (#202539)\n\nFixes: https://github.com/elastic/kibana/issues/198252\r\n\r\nIn this PR I introduced pagination in MW frontend part and also pass\r\nfilters(status and search) to the backend. Pagination arguments were\r\npassed to backend in another PR:\r\nhttps://github.com//pull/197172/files#diff-f375a192a08a6db3fbb6b6e927cecaab89ff401efc4034f00761e8fc4478734c\r\n\r\nHow to test:\r\nGo to Maintenance Window, create more than 10 MW with different\r\nstatuses. Try pagination, search on text and filter by status.\r\n\r\nCheck the PR satisfies following conditions:\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"43abe233d814cb9a5519a63a2f5942f8802879b2"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202539","number":202539,"mergeCommit":{"message":"[ResponceOps][MaintenanceWindow] MX Pagination (#202539)\n\nFixes: https://github.com/elastic/kibana/issues/198252\r\n\r\nIn this PR I introduced pagination in MW frontend part and also pass\r\nfilters(status and search) to the backend. Pagination arguments were\r\npassed to backend in another PR:\r\nhttps://github.com//pull/197172/files#diff-f375a192a08a6db3fbb6b6e927cecaab89ff401efc4034f00761e8fc4478734c\r\n\r\nHow to test:\r\nGo to Maintenance Window, create more than 10 MW with different\r\nstatuses. Try pagination, search on text and filter by status.\r\n\r\nCheck the PR satisfies following conditions:\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"43abe233d814cb9a5519a63a2f5942f8802879b2"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Julia <iuliia.guskova@elastic.co>
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: #198252
In this PR I introduced pagination in MW frontend part and also pass filters(status and search) to the backend. Pagination arguments were passed to backend in another PR: https://github.com/elastic/kibana/pull/197172/files#diff-f375a192a08a6db3fbb6b6e927cecaab89ff401efc4034f00761e8fc4478734c
How to test:
Go to Maintenance Window, create more than 10 MW with different statuses. Try pagination, search on text and filter by status.
Check the PR satisfies following conditions:
release_note:*label is applied per the guidelines