[Security Solution] Reintroduce ML Jobs warning popover on rule upgrade #159868

jpdjere · 2023-06-16T16:50:56Z

Summary

Reintroduces ML Jobs warning popover that was removed during new install/upgrade initial implementation.

ML Jobs warning popover that appears when user has legacy ML jobs installed, and the user attempts to update their prebuilt rules.

Modified behaviour so that the popover now appears in any of the three cases: upgrading all rules, upgrading specific rules and upgrading a single rule.

Testing

In the state returned by UpgradePrebuiltRulesTableContextProvider, replace the value of legacyJobsInstalled for a mock value. See below:

    return {
      state: {
        rules,
        tags,
        isFetched,
        isLoading: isLoading && loadingJobs,
        isRefetching,
        selectedRules,
        loadingRules,
        lastUpdated: dataUpdatedAt,
        legacyJobsInstalled: [
          {
            id: 'rc-rare-process-windows-5',
            description:
              'Looks for rare and anomalous processes on a Windows host. Requires process execution events from Sysmon.',
            groups: ['host'],
            processed_record_count: 8577,
            memory_status: 'ok',
            jobState: 'closed',
            hasDatafeed: true,
            datafeedId: 'datafeed-rc-rare-process-windows-5',
            datafeedIndices: ['winlogbeat-*'],
            datafeedState: 'stopped',
            latestTimestampMs: 1561402325194,
            earliestTimestampMs: 1554327458406,
            isSingleMetricViewerJob: true,
            awaitingNodeAssignment: false,
            jobTags: {},
            bucketSpanSeconds: 900,
          },
          {
            id: 'siem-api-rare_process_linux_ecs',
            description: 'SIEM Auditbeat: Detect unusually rare processes on Linux (beta)',
            groups: ['siem'],
            processed_record_count: 582251,
            memory_status: 'hard_limit',
            jobState: 'closed',
            hasDatafeed: true,
            datafeedId: 'datafeed-siem-api-rare_process_linux_ecs',
            datafeedIndices: ['auditbeat-*'],
            datafeedState: 'stopped',
            latestTimestampMs: 1557434782207,
            earliestTimestampMs: 1557353420495,
            isSingleMetricViewerJob: true,
            awaitingNodeAssignment: false,
            jobTags: {},
            bucketSpanSeconds: 900,
          },
        ],
        isUpgradeModalVisible,
        ruleIdToUpgrade,
        modalConfirmationUpdateMethod,
      },
      actions,
    };

Checklist

Delete any items that are not applicable to this PR.

Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-06-16T16:51:00Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2023-06-16T16:51:00Z

Pinging @elastic/security-solution (Team: SecuritySolution)

jpdjere · 2023-06-16T17:01:31Z

@xcrzx Reintroduced the ML Popover with this approach, but not completely satisfied with what I had to do with the Upgrade context, seems kinda hacky to me.

Can you take a look on Monday and tell me what you think?

xcrzx

I've tested it locally, and the functionality is working as expected. Thanks, @jpdjere! 👍 However, I do have a few suggestions about structuring the business logic within the context. If necessary, we can sync up on this over a Zoom call.

xcrzx · 2023-06-19T13:50:44Z

...onents/rules_table/upgrade_prebuilt_rules_table/use_upgrade_prebuilt_rules_table_columns.tsx

+  const upgradeRule = useCallback(
+    (ruleId: string) => {
+      if (legacyJobsInstalled.length > 0) {
+        showUpgradeModal();
+        setModalConfirmationUpgradeMethod('SINGLE_RULE');
+        setRuleIdToUpgrade(ruleId);
+      } else {
+        upgradeOneRule(ruleId);
+      }
+    },


Let's encapsulate this business logic within the rules upgrade context. This is the very purpose of the context – to accommodate all rule-upgrade-related logic, while minimizing its presence in the UI as much as possible. So, when we talk about a single rule upgrade, we could expose a single method from the context that is aware of the context state, performs all the necessary checks for the upgrade process, and can manage basic state manipulations, like setting loading rules, displaying intermediary modal windows, and so on. That way we'll have atomic operations exposed from the context that holds all related business logic instead of assembling it from many pieces in the UI.

xcrzx · 2023-06-19T13:51:24Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_buttons.tsx

+  const upgradeRulesWrapper = (
+    upgradeMethod: ModalConfirmationUpgradeMethod,
+    upgradeRuleMethod: () => Promise<void>
+  ) => {
+    if (legacyJobsInstalled.length > 0) {
+      showUpgradeModal();
+      setModalConfirmationUpgradeMethod(upgradeMethod);
+    } else {
+      upgradeRuleMethod();
+    }
+  };
+
+  const mlJobUpgradeModalConfirm = useCallback(() => {
+    if (modalConfirmationUpdateMethod === 'ALL_RULES') {
+      upgradeAllRules();
+    } else if (modalConfirmationUpdateMethod === 'SPECIFIC_RULES') {
+      upgradeSelectedRules();
+    } else if (modalConfirmationUpdateMethod === 'SINGLE_RULE') {
+      upgradeOneRule(ruleIdToUpgrade);
+    }
+    hideUpgradeModal();
+  }, [
+    hideUpgradeModal,
+    modalConfirmationUpdateMethod,
+    ruleIdToUpgrade,
+    upgradeAllRules,
+    upgradeOneRule,
+    upgradeSelectedRules,
+  ]);


Same here, we should move this business logic to the context as much as possible.

xcrzx · 2023-06-19T13:53:12Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

+  setModalConfirmationUpgradeMethod: Dispatch<SetStateAction<ModalConfirmationUpgradeMethod>>;
+  setRuleIdToUpgrade: Dispatch<SetStateAction<string>>;
+  showUpgradeModal: () => void;
+  hideUpgradeModal: () => void;


Those are very low-level methods. Let's try to hide them inside the context and implement rule upgrade logic without exposing them.

xcrzx

I'm approving the PR now so we can merge it before the feature freeze. It's perfectly fine to address the comments in a follow-up.

Added popover Delete mock Refactor code to live in context

jpdjere · 2023-06-19T22:20:18Z

@xcrzx Thanks for the review and the approval.

I refactored the code following the feedback, centralizing all code in the context. I think it's much cleaner now. Take a look if you wish, I'll leave the CI running during the night and merge it as soon as it's green tomorrow.

xcrzx · 2023-06-20T10:53:26Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

+      upgradeSingleRuleFromRowCTA,
+      upgradeSelectedRulesCTAClick,
+      upgradeAllRulesCTAClick,


It would be ideal to keep the method names neutral and avoid referencing specific use cases. For example, we should remove terms like Row, Click, or CTA from the method names.

xcrzx · 2023-06-20T11:11:14Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

+  /**
+   * Rule ID of single rule to upgrade when the ML jobs modal is confirmed
+   * */
+  ruleIdToUpgrade: string;


Is this state still used?

xcrzx · 2023-06-20T11:19:09Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

+  /**
+   * Legacy ML Jobs that cause modal to pop up before performing rule ugprades
+   */
+  legacyJobsInstalled: MlSummaryJob[];
+  /**
+   * Upgrade method to execute when the user confirms the modal
+   */
+  modalConfirmationUpdateMethod: ModalConfirmationUpgradeMethod | null;
+  /**
+   * Is true when the modal is visible
+   * */
+  isUpgradeModalVisible: boolean;


I propose we take this a step further and make those state props internal. The rules upgrade context should handle modal confirmation internally without revealing anything to the external environment. I might not have been clear enough about handling that state internally, so let's please sync up on this topic separately.

xcrzx · 2023-06-20T11:28:31Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

+  // Wrapper around upgrade rules methods to display ML Jobs warning modal when necessary
+  const upgradeRulesWrapper = useCallback(
+    (upgradeMethod: ModalConfirmationUpgradeMethod, upgradeRuleMethod: () => Promise<void>) =>
+      () => {
+        if (legacyJobsInstalled.length > 0) {
+          showUpgradeModal();
+          setModalConfirmationUpgradeMethod(upgradeMethod);
+        } else {
+          upgradeRuleMethod();
+        }
+      },
+    [legacyJobsInstalled.length, showUpgradeModal]
+  );


Utilizing the wrapper does add a bit of extra complexity to the upgrade logic. For this specific use case, we have a hook named useAsyncConfirmation. By using it, we can significantly improve the readability of the business logic. Consequently, a single rule upgrade method might look like the following:

const [confirmUpgrade, onConfirm, onCancel] = useAsyncConfirmation({ onInit: showUpgradeModal, onFinish: hideUpgradeModal, }); const upgradeOneRule = useCallback( async (ruleId: RuleSignatureId) => { try { // 1. Set loading state setLoadingRules((prev) => [...prev, ruleId]); // 2. Confirm upgrade if (await confirmUpgrade()) { // 3. Perform upgrade await upgradeSpecificRulesRequest({}); } } finally { // 4. Remove loading state setLoadingRules((prev) => prev.filter((id) => id !== ruleId)); } }, [confirmUpgrade, upgradeSpecificRulesRequest] );

As you can see, all required upgrade steps are located in the method body and are easily readable, no need to split the process into several functions.

jpdjere · 2023-06-20T13:35:26Z

Thanks @xcrzx, this is so much easier and tidier.

useAsyncConfirmation for the win :)

xcrzx

Thank you, @jpdjere, for implementing the suggestions 👍 Verified locally, the modal window works as expected 🎉

kibana-ci · 2023-06-20T14:40:04Z

💚 Build Succeeded

Buildkite Build
Commit: 2b6ecb4

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4197	4199	+2

Async chunks

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

id	before	after	diff
`securitySolution`	10.9MB	10.9MB	+2.6KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+6

History

💛 Build #136356 was flaky ca851e7
💛 Build #135908 was flaky ec1e2b9c86a7f9883ffd931ab40123ca39bab506

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

cc @jpdjere

jpdjere requested a review from a team as a code owner June 16, 2023 16:50

jpdjere self-assigned this Jun 16, 2023

jpdjere requested a review from spong June 16, 2023 16:50

jpdjere requested review from xcrzx and removed request for spong June 16, 2023 16:55

jpdjere force-pushed the ml-jobs-popup branch from 6295cdd to ec1e2b9 Compare June 16, 2023 16:59

maximpn self-requested a review June 19, 2023 09:56

xcrzx reviewed Jun 19, 2023

View reviewed changes

xcrzx approved these changes Jun 19, 2023

View reviewed changes

Add ML Jobs popover with context refactoring

4328381

Added popover Delete mock Refactor code to live in context

jpdjere force-pushed the ml-jobs-popup branch from 9a0015b to 4328381 Compare June 19, 2023 22:17

Refactored context

ca851e7

xcrzx reviewed Jun 20, 2023

View reviewed changes

jpdjere added 2 commits June 20, 2023 15:27

Used asyncConfirm

c387691

Simplify

1b3e83d

jpdjere force-pushed the ml-jobs-popup branch from 9209c6b to 7b85789 Compare June 20, 2023 13:32

Simplify

2b6ecb4

jpdjere force-pushed the ml-jobs-popup branch from 7b85789 to 2b6ecb4 Compare June 20, 2023 13:33

jpdjere requested a review from xcrzx June 20, 2023 13:34

xcrzx approved these changes Jun 20, 2023

View reviewed changes

jpdjere merged commit 21dbbd0 into elastic:main Jun 20, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023

jpdjere mentioned this pull request Jun 21, 2023

[Security Solution] New Rules Installation and Upgrade UI Workflows #158450

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Reintroduce ML Jobs warning popover on rule upgrade #159868

[Security Solution] Reintroduce ML Jobs warning popover on rule upgrade #159868

jpdjere commented Jun 16, 2023 •

edited

Loading

elasticmachine commented Jun 16, 2023

elasticmachine commented Jun 16, 2023

jpdjere commented Jun 16, 2023

xcrzx left a comment

xcrzx Jun 19, 2023

xcrzx Jun 19, 2023

xcrzx Jun 19, 2023

xcrzx left a comment

jpdjere commented Jun 19, 2023

xcrzx Jun 20, 2023

xcrzx Jun 20, 2023

xcrzx Jun 20, 2023

xcrzx Jun 20, 2023

jpdjere commented Jun 20, 2023

xcrzx left a comment

kibana-ci commented Jun 20, 2023

ESLint disabled line counts

Total ESLint disabled count

[Security Solution] Reintroduce ML Jobs warning popover on rule upgrade #159868

[Security Solution] Reintroduce ML Jobs warning popover on rule upgrade #159868

Conversation

jpdjere commented Jun 16, 2023 • edited Loading

Summary

Testing

Checklist

For maintainers

elasticmachine commented Jun 16, 2023

elasticmachine commented Jun 16, 2023

jpdjere commented Jun 16, 2023

xcrzx left a comment

Choose a reason for hiding this comment

xcrzx Jun 19, 2023

Choose a reason for hiding this comment

xcrzx Jun 19, 2023

Choose a reason for hiding this comment

xcrzx Jun 19, 2023

Choose a reason for hiding this comment

xcrzx left a comment

Choose a reason for hiding this comment

jpdjere commented Jun 19, 2023

xcrzx Jun 20, 2023

Choose a reason for hiding this comment

xcrzx Jun 20, 2023

Choose a reason for hiding this comment

xcrzx Jun 20, 2023

Choose a reason for hiding this comment

xcrzx Jun 20, 2023

Choose a reason for hiding this comment

jpdjere commented Jun 20, 2023

xcrzx left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 20, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

jpdjere commented Jun 16, 2023 •

edited

Loading