From e897daafd8e86b2023bb54d6d16f6da3aa8a9ed5 Mon Sep 17 00:00:00 2001
From: James Gowdy <jgowdy@elastic.co>
Date: Mon, 18 Jan 2021 11:49:31 +0000
Subject: [PATCH] [ML] Improving model snapshot revert UI experience

---
 .../model_snapshots/model_snapshots_table.tsx | 27 +++++++----
 .../revert_model_snapshot_flyout.tsx          | 48 +++++++++++--------
 2 files changed, 47 insertions(+), 28 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..c57c8bd719d99 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
@@ -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);
 
+  let unmounted = false;
   useEffect(() => {
     loadModelSnapshots();
+    return () => {
+      unmounted = true;
+    };
   }, []);
 
-  const loadModelSnapshots = useCallback(async () => {
+  async function loadModelSnapshots() {
+    if (unmounted === true) {
+      // 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..d999d18c73d7b 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),
@@ -110,13 +117,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
     setChartReady(true);
   }, [job]);
 
-  function closeWithReload() {
-    closeFlyout(true);
-  }
-  function closeWithoutReload() {
-    closeFlyout(false);
-  }
-
   function showRevertModal() {
     setRevertModalVisible(true);
   }
@@ -138,15 +138,18 @@ 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), {
@@ -166,7 +169,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 +350,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"
@@ -395,7 +398,12 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
             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>
       )}
     </>