[Tech Debt] Reorder Rules page #152897

CoenWarmer · 2023-03-08T11:25:27Z

Summary

This PR updates the Rules Page to follow the newly agreed upon structure for the Observability app.

This PR is part of a larger effort to have all pages in the Observability app follow the same structure so they are more consistent with each other. More details in the epic: #152783

It also cleans up dead and unused components where found.

All design and functionality should be exactly the same as before.

…order-rules-page

x-pack/plugins/observability/public/pages/rules/rules.tsx

kibana-ci · 2023-03-19T20:07:31Z

💚 Build Succeeded

Buildkite Build
Commit: 0ddbb65

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	517	513	-4

Async chunks

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

id	before	after	diff
`observability`	1.1MB	1.1MB	-1.9KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💔 Build #113941 failed 2672729
💔 Build #113187 failed 92a6840
💚 Build #112702 succeeded e47ab7c
💔 Build #112684 failed bc4a8866cc935e3bb22b34aede4db4505c10bcf2
💔 Build #112672 failed 3f20bd52a8b0d3b05755692057a93acc35016deb

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

CoenWarmer · 2023-03-19T20:33:16Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -394,7 +394,7 @@ export const RulesList = ({
    if (lastRunOutcomeFilter) {
      updateFilters({ filter: 'ruleLastRunOutcomes', value: lastRunOutcomeFilter });
    }
-  }, [lastResponseFilter]);
+  }, [lastRunOutcomeFilter]);


Shouldn't ESLint have caught this? It seems the exhaustive-deps rule is not kicking in here.

Good catch, looks like we have a legacy file wide /* eslint-disable react-hooks/exhaustive-deps */ at the top of the file.

I will create an issue to remove it

CoenWarmer · 2023-03-19T21:37:07Z

x-pack/plugins/observability/public/pages/rules/rules.tsx

+              'ruleExecutionStatus',
+              'ruleExecutionState',
+            ]}
+            onLastRunOutcomeFilterChange={handleLastRunOutcomeFilterChange}


Changing the lastResponse filter in the Rules List UI didn't update the URL in the previous implementation. Now updated so it does

CoenWarmer · 2023-03-19T21:37:58Z

x-pack/plugins/observability/public/pages/rules/rules.tsx

-
-function WrappedRulesPage() {
-  return (
-    <Provider value={rulesPageStateContainer}>


Replaced state container with a simpler pattern that offers the same functionality

What pattern did you use instead and where is the implementation?

I've tagged you in 2 comments.

CoenWarmer · 2023-03-19T21:39:13Z

x-pack/plugins/translations/translations/fr-FR.json

@@ -24691,18 +24691,7 @@
    "xpack.observability.rules.addRuleButtonLabel": "Créer une règle",
    "xpack.observability.rules.deleteSelectedIdsConfirmModal.cancelButtonLabel": "Annuler",
    "xpack.observability.rules.docsLinkText": "Documentation",
-    "xpack.observability.rules.loadError": "Impossible de charger les règles",


All these translations were exported from translations.ts but never imported anywhere

CoenWarmer · 2023-03-19T21:43:27Z

x-pack/plugins/observability/public/pages/rules/state_container/state_container.tsx

-}
-
-const transitions: RulesPageStateTransitions = {
-  setLastResponse: (state) => (lastResponse) => {


All this code seems to be referencing a previous implementation of this table that is not used in the current page.

I suspect the RuleList component used to live inside Observability, but has been moved to TriggersActionUi. In that process this implementation was not properly cleaned up.

JiaweiWu

triggers actions UI changes LGTM!

JiaweiWu · 2023-03-20T04:21:56Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -394,7 +394,7 @@ export const RulesList = ({
    if (lastRunOutcomeFilter) {
      updateFilters({ filter: 'ruleLastRunOutcomes', value: lastRunOutcomeFilter });
    }
-  }, [lastResponseFilter]);
+  }, [lastRunOutcomeFilter]);


Good catch, looks like we have a legacy file wide /* eslint-disable react-hooks/exhaustive-deps */ at the top of the file.

I will create an issue to remove it

CoenWarmer · 2023-03-20T11:18:24Z

x-pack/plugins/observability/public/pages/rules/rules.tsx

+  const urlStateStorage = createKbnUrlStateStorage({
+    history,
+    useHash: false,
+    useHashQuery: false,
+  });
+
+  const { status, lastResponse } = urlStateStorage.get<{
+    status: RuleStatus[];
+    lastResponse: string[];
+  }>('_a') || { status: [], lastResponse: [] };
+
+  const [stateStatus, setStatus] = useState<RuleStatus[]>(status);
+  const [stateLastResponse, setLastResponse] = useState<string[]>(lastResponse);
+  const [stateRefresh, setRefresh] = useState(new Date());


@maryam-saeidi new code as replacement of state container (part 1) (line 45-58)

CoenWarmer · 2023-03-20T11:19:07Z

x-pack/plugins/observability/public/pages/rules/rules.tsx

+  const handleStatusFilterChange = (newStatus: RuleStatus[]) => {
+    setStatus(newStatus);
+    urlStateStorage.set('_a', { status: newStatus, lastResponse });
+    return { lastResponse: stateLastResponse || [], status: newStatus };
+  };
+
+  const handleLastRunOutcomeFilterChange = (newLastResponse: string[]) => {
+    setRefresh(new Date());
+    setLastResponse(newLastResponse);
+    urlStateStorage.set('_a', { status, lastResponse: newLastResponse });
+    return { lastResponse: newLastResponse, status: stateStatus || [] };
+  };


@maryam-saeidi and part 2 (line 76-87)

kdelemme

LGTM and works as expected locally 👍🏻

…loy-my-kibana-oblt * upstream/main: (727 commits) Upgrade caniuse-lite db (elastic#153318) [Security Solution] expanded flyout - right section - json tab implementation (elastic#152935) chore(slo): Make APM indicator's index required (elastic#153311) skip failing test suite (elastic#136688) [Security Solution] Fix security-solution storybook package codeowners (elastic#153307) [EUI] Add `scrollLock` workaround CSS to Kibana's `body` (elastic#153227) [Cloud Security] Show coming soon deployments of vulnerability management (elastic#153249) [Cloud Security] fixed onboarding link directs to cspm integration (elastic#153268) [Response Ops][Alerting] Reusable functions for FAAD resource installation (elastic#152849) remove geohash_grid aggregation support (elastic#152952) [Tech Debt] Reorder Rules page (elastic#152897) [Saved Object Finder] Add help text & left button (elastic#152742) [Transform] Replace SavedObjectsFinder component (elastic#153128) Make pipeline creation endpoint accept a full pipeline definition (elastic#153133) [Fleet] Displaying policy changes in Agent activity (elastic#153237) skip flaky suite (elastic#152852) [Security Solution][Endpoint] Add tests to cover RBAC entries in the Role Kibana Privileges flyout (elastic#153068) [Security Solution][Endpoint] Additional tests for Response Console History Log page (covers TestRail manual tests) (elastic#153042) [Monitoring] Display node roles in Nodes table (elastic#152127) Rename getEditAlertFlyout to getEditRuleFlyout (elastic#153243) ...

CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Mar 8, 2023

CoenWarmer force-pushed the chore/152790-reorder-rules-page branch 2 times, most recently from 78a1847 to bc4a886 Compare March 8, 2023 11:44

CoenWarmer changed the title ~~Reorder Rules page~~ [Tech Debt] Reorder Rules page Mar 8, 2023

Reorder Rules page

e47ab7c

CoenWarmer force-pushed the chore/152790-reorder-rules-page branch from bc4a886 to e47ab7c Compare March 8, 2023 12:51

CoenWarmer marked this pull request as ready for review March 8, 2023 14:46

CoenWarmer requested a review from a team as a code owner March 8, 2023 14:46

CoenWarmer added 2 commits March 9, 2023 23:08

Remove state container

92a6840

Merge branch 'main' of github.com:elastic/kibana into chore/152790-re…

205e140

…order-rules-page

CoenWarmer marked this pull request as draft March 10, 2023 11:53

CoenWarmer added 2 commits March 19, 2023 18:17

Refactor Rules page

fad6721

Merge branch 'main' of github.com:elastic/kibana into chore/152790-re…

2672729

…order-rules-page

CoenWarmer commented Mar 19, 2023

View reviewed changes

x-pack/plugins/observability/public/pages/rules/rules.tsx Outdated Show resolved Hide resolved

Update x-pack/plugins/observability/public/pages/rules/rules.tsx

0ddbb65

CoenWarmer marked this pull request as ready for review March 19, 2023 20:13

CoenWarmer requested a review from a team as a code owner March 19, 2023 20:13

CoenWarmer commented Mar 19, 2023

View reviewed changes

CoenWarmer enabled auto-merge (squash) March 19, 2023 21:50

JiaweiWu approved these changes Mar 20, 2023

View reviewed changes

CoenWarmer commented Mar 20, 2023

View reviewed changes

kdelemme approved these changes Mar 20, 2023

View reviewed changes

CoenWarmer merged commit 6a78ca4 into elastic:main Mar 20, 2023

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tech Debt] Reorder Rules page #152897

[Tech Debt] Reorder Rules page #152897

CoenWarmer commented Mar 8, 2023

kibana-ci commented Mar 19, 2023

ESLint disabled line counts

Total ESLint disabled count

CoenWarmer Mar 19, 2023

JiaweiWu Mar 20, 2023

CoenWarmer Mar 19, 2023 •

edited

Loading

CoenWarmer Mar 19, 2023

maryam-saeidi Mar 20, 2023

CoenWarmer Mar 20, 2023

CoenWarmer Mar 19, 2023

CoenWarmer Mar 19, 2023

JiaweiWu left a comment

JiaweiWu Mar 20, 2023

CoenWarmer Mar 20, 2023 •

edited

Loading

CoenWarmer Mar 20, 2023 •

edited

Loading

kdelemme left a comment

[Tech Debt] Reorder Rules page #152897

[Tech Debt] Reorder Rules page #152897

Conversation

CoenWarmer commented Mar 8, 2023

Summary

kibana-ci commented Mar 19, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Mar 19, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaweiWu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

CoenWarmer Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

kdelemme left a comment

Choose a reason for hiding this comment

CoenWarmer Mar 19, 2023 •

edited

Loading

CoenWarmer Mar 20, 2023 •

edited

Loading

CoenWarmer Mar 20, 2023 •

edited

Loading