From 04a71b13a3ae9d454c67a5031e79cbdfebd761ca Mon Sep 17 00:00:00 2001
From: James Gowdy <jgowdy@elastic.co>
Date: Thu, 21 Jan 2021 10:54:28 +0000
Subject: [PATCH] [ML] Improving model snapshot revert UI experience (#88588)

* [ML] Improving model snapshot revert UI experience

* removing button disabling

* updating component is mounted check

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
---
 .../model_snapshots/model_snapshots_table.tsx | 29 +++++++----
 .../revert_model_snapshot_flyout.tsx          | 52 ++++++++++---------
 2 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx b/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx
index 5b175eb06a4a3..edf038160e0a8 100644
--- a/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx
+++ b/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { FC, useEffect, useCallback, useState } from 'react';
+import React, { FC, useEffect, useCallback, useState, useRef } from 'react';
 import { i18n } from '@kbn/i18n';
 
 import {
@@ -50,15 +50,24 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
   const [closeJobModalVisible, setCloseJobModalVisible] = useState<ModelSnapshot | null>(null);
   const [combinedJobState, setCombinedJobState] = useState<COMBINED_JOB_STATE | null>(null);
 
+  const isMounted = useRef(true);
   useEffect(() => {
     loadModelSnapshots();
+    return () => {
+      isMounted.current = false;
+    };
   }, []);
 
-  const loadModelSnapshots = useCallback(async () => {
+  async function loadModelSnapshots() {
+    if (isMounted.current === false) {
+      // table refresh can be triggered a while after a snapshot revert has been triggered.
+      // ensure the table is still visible before attempted to refresh it.
+      return;
+    }
     const { model_snapshots: ms } = await ml.getModelSnapshots(job.job_id);
     setSnapshots(ms);
     setSnapshotsLoaded(true);
-  }, [job]);
+  }
 
   const checkJobIsClosed = useCallback(
     async (snapshot: ModelSnapshot) => {
@@ -107,13 +116,14 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
     }
   }, []);
 
-  const closeRevertFlyout = useCallback((reload: boolean) => {
+  const closeRevertFlyout = useCallback(() => {
     setRevertSnapshot(null);
-    if (reload) {
-      loadModelSnapshots();
-      // wait half a second before refreshing the jobs list
-      setTimeout(refreshJobList, 500);
-    }
+  }, []);
+
+  const refresh = useCallback(() => {
+    loadModelSnapshots();
+    // wait half a second before refreshing the jobs list
+    setTimeout(refreshJobList, 500);
   }, []);
 
   const columns: Array<EuiBasicTableColumn<any>> = [
@@ -231,6 +241,7 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
           snapshots={snapshots}
           job={job}
           closeFlyout={closeRevertFlyout}
+          refresh={refresh}
         />
       )}
 
diff --git a/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx b/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx
index 62f5623f67964..05e624c194e6e 100644
--- a/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx
+++ b/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx
@@ -53,10 +53,17 @@ interface Props {
   snapshot: ModelSnapshot;
   snapshots: ModelSnapshot[];
   job: CombinedJobWithStats;
-  closeFlyout(reload: boolean): void;
+  closeFlyout(): void;
+  refresh(): void;
 }
 
-export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, closeFlyout }) => {
+export const RevertModelSnapshotFlyout: FC<Props> = ({
+  snapshot,
+  snapshots,
+  job,
+  closeFlyout,
+  refresh,
+}) => {
   const { toasts } = useNotifications();
   const { loadAnomalyDataForJob, loadEventRateForJob } = useMemo(
     () => chartLoaderProvider(mlResultsService),
@@ -73,7 +80,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
   const [eventRateData, setEventRateData] = useState<LineChartPoint[]>([]);
   const [anomalies, setAnomalies] = useState<Anomaly[]>([]);
   const [chartReady, setChartReady] = useState(false);
-  const [applying, setApplying] = useState(false);
 
   useEffect(() => {
     createChartData();
@@ -110,13 +116,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
     setChartReady(true);
   }, [job]);
 
-  function closeWithReload() {
-    closeFlyout(true);
-  }
-  function closeWithoutReload() {
-    closeFlyout(false);
-  }
-
   function showRevertModal() {
     setRevertModalVisible(true);
   }
@@ -125,7 +124,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
   }
 
   async function applyRevert() {
-    setApplying(true);
     const end =
       replay && runInRealTime === false ? job.data_counts.latest_record_timestamp : undefined;
     try {
@@ -138,17 +136,19 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
             }))
           : undefined;
 
-      await ml.jobs.revertModelSnapshot(
-        job.job_id,
-        currentSnapshot.snapshot_id,
-        replay,
-        end,
-        events
-      );
+      ml.jobs
+        .revertModelSnapshot(job.job_id, currentSnapshot.snapshot_id, replay, end, events)
+        .then(() => {
+          toasts.addSuccess(
+            i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertSuccessTitle', {
+              defaultMessage: 'Model snapshot revert successful',
+            })
+          );
+          refresh();
+        });
       hideRevertModal();
-      closeWithReload();
+      closeFlyout();
     } catch (error) {
-      setApplying(false);
       toasts.addError(new Error(error.body.message), {
         title: i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertErrorTitle', {
           defaultMessage: 'Model snapshot revert failed',
@@ -166,7 +166,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
 
   return (
     <>
-      <EuiFlyout onClose={closeWithoutReload} hideCloseButton size="m">
+      <EuiFlyout onClose={closeFlyout} hideCloseButton size="m">
         <EuiFlyoutHeader hasBorder>
           <EuiTitle size="s">
             <h5>
@@ -347,7 +347,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
         <EuiFlyoutFooter>
           <EuiFlexGroup justifyContent="spaceBetween">
             <EuiFlexItem grow={false}>
-              <EuiButtonEmpty iconType="cross" onClick={closeWithoutReload} flush="left">
+              <EuiButtonEmpty iconType="cross" onClick={closeFlyout} flush="left">
                 <FormattedMessage
                   id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.closeButton"
                   defaultMessage="Close"
@@ -392,10 +392,14 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
                 defaultMessage: 'Apply',
               }
             )}
-            confirmButtonDisabled={applying}
             buttonColor="danger"
             defaultFocusedButton="confirm"
-          />
+          >
+            <FormattedMessage
+              id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.modalBody"
+              defaultMessage="The snapshot revert will be carried out in the background and may take some time."
+            />
+          </EuiConfirmModal>
         </EuiOverlayMask>
       )}
     </>