[ML] Fix deletion of models that are not used by pipelines (#114107)

* [ML] Fix deletion of models that are not used by pipelines * [ML] Edits from review * [ML] Fix jest test for index switch in delete job modal * [ML] Fix API test calls to createTestTrainedModels * [ML] Remove unnecessary async from jest test
elastic · Oct 7, 2021 · 0b14195 · 0b14195
1 parent e62b752
commit 0b14195
Show file tree

Hide file tree

Showing 13 changed files with 376 additions and 26 deletions.
diff --git a/...analytics/pages/analytics_management/components/action_delete/delete_action_name.test.tsx b/...analytics/pages/analytics_management/components/action_delete/delete_action_name.test.tsx
@@ -69,7 +69,7 @@ describe('DeleteAction', () => {
   });
 
   describe('When delete model is open', () => {
-    it('should allow to delete target index by default.', () => {
+    it('should not allow to delete target index by default.', () => {
       const mock = jest.spyOn(CheckPrivilige, 'checkPermission');
       mock.mockImplementation((p) => p === 'canDeleteDataFrameAnalytics');
 
@@ -101,10 +101,9 @@ describe('DeleteAction', () => {
       const deleteButton = getByTestId('mlAnalyticsJobDeleteButton');
       fireEvent.click(deleteButton);
       expect(getByTestId('mlAnalyticsJobDeleteModal')).toBeInTheDocument();
-      expect(getByTestId('mlAnalyticsJobDeleteIndexSwitch')).toBeInTheDocument();
-      const mlAnalyticsJobDeleteIndexSwitch = getByTestId('mlAnalyticsJobDeleteIndexSwitch');
-      expect(mlAnalyticsJobDeleteIndexSwitch).toHaveAttribute('aria-checked', 'true');
+      expect(queryByTestId('mlAnalyticsJobDeleteIndexSwitch')).toBeNull();
       expect(queryByTestId('mlAnalyticsJobDeleteIndexPatternSwitch')).toBeNull();
+
       mock.mockRestore();
     });
   });

diff --git a/...frame_analytics/pages/analytics_management/components/action_delete/use_delete_action.tsx b/...frame_analytics/pages/analytics_management/components/action_delete/use_delete_action.tsx
@@ -89,9 +89,9 @@ export const useDeleteAction = (canDeleteDataFrameAnalytics: boolean) => {
       );
     }
   };
-  const checkUserIndexPermission = () => {
+  const checkUserIndexPermission = async () => {
     try {
-      const userCanDelete = canDeleteIndex(indexName, toastNotificationService);
+      const userCanDelete = await canDeleteIndex(indexName, toastNotificationService);
       if (userCanDelete) {
         setUserCanDeleteIndex(true);
       }

diff --git a/...analytics/pages/analytics_management/components/models_management/delete_models_modal.tsx b/...analytics/pages/analytics_management/components/models_management/delete_models_modal.tsx
@@ -30,7 +30,11 @@ export const DeleteModelsModal: FC<DeleteModelsModalProps> = ({ models, onClose
     .map((model) => model.model_id);
 
   return (
-    <EuiModal onClose={onClose.bind(null, false)} initialFocus="[name=cancelModelDeletion]">
+    <EuiModal
+      onClose={onClose.bind(null, false)}
+      initialFocus="[name=cancelModelDeletion]"
+      data-test-subj="mlModelsDeleteModal"
+    >
       <EuiModalHeader>
         <EuiModalHeaderTitle>
           <FormattedMessage
@@ -72,7 +76,12 @@ export const DeleteModelsModal: FC<DeleteModelsModalProps> = ({ models, onClose
           />
         </EuiButtonEmpty>
 
-        <EuiButton onClick={onClose.bind(null, true)} fill color="danger">
+        <EuiButton
+          onClick={onClose.bind(null, true)}
+          fill
+          color="danger"
+          data-test-subj="mlModelsDeleteModalConfirmButton"
+        >
           <FormattedMessage
             id="xpack.ml.trainedModels.modelsList.deleteModal.deleteButtonLabel"
             defaultMessage="Delete"

diff --git a/...a_frame_analytics/pages/analytics_management/components/models_management/models_list.tsx b/...a_frame_analytics/pages/analytics_management/components/models_management/models_list.tsx
@@ -50,6 +50,7 @@ import {
 import { ML_PAGES } from '../../../../../../../common/constants/locator';
 import { DataFrameAnalysisConfigType } from '../../../../../../../common/types/data_frame_analytics';
 import { timeFormatter } from '../../../../../../../common/util/date_utils';
+import { isPopulatedObject } from '../../../../../../../common';
 import { ListingPageUrlState } from '../../../../../../../common/types/common';
 import { usePageUrlState } from '../../../../../util/url_state';
 import { BUILT_IN_MODEL_TAG } from '../../../../../../../common/constants/data_frame_analytics';
@@ -342,6 +343,7 @@ export const ModelsList: FC = () => {
       description: i18n.translate('xpack.ml.trainedModels.modelsList.deleteModelActionLabel', {
         defaultMessage: 'Delete model',
       }),
+      'data-test-subj': 'mlModelsTableRowDeleteAction',
       icon: 'trash',
       type: 'icon',
       color: 'danger',
@@ -353,7 +355,7 @@ export const ModelsList: FC = () => {
       enabled: (item) => {
         // TODO check for permissions to delete ingest pipelines.
         // ATM undefined means pipelines fetch failed server-side.
-        return !item.pipelines;
+        return !isPopulatedObject(item.pipelines);
       },
     },
   ];
@@ -389,6 +391,7 @@ export const ModelsList: FC = () => {
           iconType={itemIdToExpandedRowMap[item.model_id] ? 'arrowUp' : 'arrowDown'}
         />
       ),
+      'data-test-subj': 'mlModelsTableRowDetailsToggle',
     },
     {
       field: ModelsTableToConfigMapping.id,
@@ -397,6 +400,7 @@ export const ModelsList: FC = () => {
       }),
       sortable: true,
       truncateText: true,
+      'data-test-subj': 'mlModelsTableColumnId',
     },
     {
       field: ModelsTableToConfigMapping.description,
@@ -406,6 +410,7 @@ export const ModelsList: FC = () => {
       }),
       sortable: false,
       truncateText: true,
+      'data-test-subj': 'mlModelsTableColumnDescription',
     },
     {
       field: ModelsTableToConfigMapping.type,
@@ -418,11 +423,14 @@ export const ModelsList: FC = () => {
         <EuiFlexGroup gutterSize={'xs'} wrap>
           {types.map((type) => (
             <EuiFlexItem key={type} grow={false}>
-              <EuiBadge color="hollow">{type}</EuiBadge>
+              <EuiBadge color="hollow" data-test-subj="mlModelType">
+                {type}
+              </EuiBadge>
             </EuiFlexItem>
           ))}
         </EuiFlexGroup>
       ),
+      'data-test-subj': 'mlModelsTableColumnType',
     },
     {
       field: ModelsTableToConfigMapping.createdAt,
@@ -432,12 +440,14 @@ export const ModelsList: FC = () => {
       dataType: 'date',
       render: timeFormatter,
       sortable: true,
+      'data-test-subj': 'mlModelsTableColumnCreatedAt',
     },
     {
       name: i18n.translate('xpack.ml.trainedModels.modelsList.actionsHeader', {
         defaultMessage: 'Actions',
       }),
       actions,
+      'data-test-subj': 'mlModelsTableColumnActions',
     },
   ];
 
@@ -492,8 +502,7 @@ export const ModelsList: FC = () => {
               defaultMessage: 'Select a model',
             });
           }
-
-          if (Array.isArray(item.pipelines) && item.pipelines.length > 0) {
+          if (isPopulatedObject(item.pipelines)) {
             return i18n.translate('xpack.ml.trainedModels.modelsList.disableSelectableMessage', {
               defaultMessage: 'Model has associated pipelines',
             });
@@ -507,7 +516,7 @@ export const ModelsList: FC = () => {
 
           return '';
         },
-        selectable: (item) => !item.pipelines && !isBuiltInModel(item),
+        selectable: (item) => !isPopulatedObject(item.pipelines) && !isBuiltInModel(item),
         onSelectionChange: (selectedItems) => {
           setSelectedModels(selectedItems);
         },
@@ -574,6 +583,7 @@ export const ModelsList: FC = () => {
           pagination={pagination}
           onTableChange={onTableChange}
           sorting={sorting}
+          data-test-subj={isLoading ? 'mlModelsTable loading' : 'mlModelsTable loaded'}
         />
       </div>
       {modelsToDelete.length > 0 && (

diff --git a/x-pack/test/api_integration/apis/ml/trained_models/delete_model.ts b/x-pack/test/api_integration/apis/ml/trained_models/delete_model.ts
@@ -17,7 +17,7 @@ export default ({ getService }: FtrProviderContext) => {
   describe('DELETE trained_models', () => {
     before(async () => {
       await ml.testResources.setKibanaTimeZoneToUTC();
-      await ml.api.createdTestTrainedModels('regression', 2);
+      await ml.api.createTestTrainedModels('regression', 2);
     });
 
     after(async () => {

diff --git a/x-pack/test/api_integration/apis/ml/trained_models/get_model_pipelines.ts b/x-pack/test/api_integration/apis/ml/trained_models/get_model_pipelines.ts
@@ -19,7 +19,7 @@ export default ({ getService }: FtrProviderContext) => {
 
     before(async () => {
       await ml.testResources.setKibanaTimeZoneToUTC();
-      testModelIds = await ml.api.createdTestTrainedModels('regression', 2, true);
+      testModelIds = await ml.api.createTestTrainedModels('regression', 2, true);
     });
 
     after(async () => {

diff --git a/x-pack/test/api_integration/apis/ml/trained_models/get_model_stats.ts b/x-pack/test/api_integration/apis/ml/trained_models/get_model_stats.ts
@@ -17,7 +17,7 @@ export default ({ getService }: FtrProviderContext) => {
   describe('GET trained_models/_stats', () => {
     before(async () => {
       await ml.testResources.setKibanaTimeZoneToUTC();
-      await ml.api.createdTestTrainedModels('regression', 2);
+      await ml.api.createTestTrainedModels('regression', 2);
     });
 
     after(async () => {

diff --git a/x-pack/test/api_integration/apis/ml/trained_models/get_models.ts b/x-pack/test/api_integration/apis/ml/trained_models/get_models.ts
@@ -19,7 +19,7 @@ export default ({ getService }: FtrProviderContext) => {
 
     before(async () => {
       await ml.testResources.setKibanaTimeZoneToUTC();
-      testModelIds = await ml.api.createdTestTrainedModels('regression', 5, true);
+      testModelIds = await ml.api.createTestTrainedModels('regression', 5, true);
       await ml.api.createModelAlias('dfa_regression_model_n_0', 'dfa_regression_model_alias');
       await ml.api.createIngestPipeline('dfa_regression_model_alias');
     });

diff --git a/x-pack/test/functional/apps/ml/data_frame_analytics/trained_models.ts b/x-pack/test/functional/apps/ml/data_frame_analytics/trained_models.ts
@@ -12,8 +12,8 @@ export default function ({ getService }: FtrProviderContext) {
 
   describe('trained models', function () {
     before(async () => {
-      await ml.trainedModels.createdTestTrainedModels('classification', 15);
-      await ml.trainedModels.createdTestTrainedModels('regression', 15);
+      await ml.trainedModels.createTestTrainedModels('classification', 15, true);
+      await ml.trainedModels.createTestTrainedModels('regression', 15);
       await ml.securityUI.loginAsMlPowerUser();
       await ml.navigation.navigateToTrainedModels();
     });
@@ -22,10 +22,116 @@ export default function ({ getService }: FtrProviderContext) {
       await ml.api.cleanMlIndices();
     });
 
+    // 'Created at' will be different on each run,
+    // so we will just assert that the value is in the expected timestamp format.
+    const builtInModelData = {
+      modelId: 'lang_ident_model_1',
+      description: 'Model used for identifying language from arbitrary input text.',
+      modelTypes: ['classification', 'built-in'],
+    };
+
+    const modelWithPipelineData = {
+      modelId: 'dfa_classification_model_n_0',
+      description: '',
+      modelTypes: ['classification'],
+    };
+
+    const modelWithoutPipelineData = {
+      modelId: 'dfa_regression_model_n_0',
+      description: '',
+      modelTypes: ['regression'],
+    };
+
     it('renders trained models list', async () => {
-      await ml.trainedModels.assertRowsNumberPerPage(10);
+      await ml.testExecution.logTestStep(
+        'should display the stats bar with the total number of models'
+      );
       // +1 because of the built-in model
       await ml.trainedModels.assertStats(31);
+
+      await ml.testExecution.logTestStep('should display the table');
+      await ml.trainedModels.assertTableExists();
+      await ml.trainedModels.assertRowsNumberPerPage(10);
+    });
+
+    it('displays the built-in model and no actions are enabled', async () => {
+      await ml.testExecution.logTestStep('should display the model in the table');
+      await ml.trainedModelsTable.filterWithSearchString(builtInModelData.modelId, 1);
+
+      await ml.testExecution.logTestStep('displays expected row values for the model in the table');
+      await ml.trainedModelsTable.assertModelsRowFields(builtInModelData.modelId, {
+        id: builtInModelData.modelId,
+        description: builtInModelData.description,
+        modelTypes: builtInModelData.modelTypes,
+      });
+
+      await ml.testExecution.logTestStep(
+        'should not show collapsed actions menu for the model in the table'
+      );
+      await ml.trainedModelsTable.assertModelCollapsedActionsButtonExists(
+        builtInModelData.modelId,
+        false
+      );
+
+      await ml.testExecution.logTestStep(
+        'should not show delete action for the model in the table'
+      );
+      await ml.trainedModelsTable.assertModelDeleteActionButtonExists(
+        builtInModelData.modelId,
+        false
+      );
+    });
+
+    it('displays a model with an ingest pipeline and delete action is disabled', async () => {
+      await ml.testExecution.logTestStep('should display the model in the table');
+      await ml.trainedModelsTable.filterWithSearchString(modelWithPipelineData.modelId, 1);
+
+      await ml.testExecution.logTestStep('displays expected row values for the model in the table');
+      await ml.trainedModelsTable.assertModelsRowFields(modelWithPipelineData.modelId, {
+        id: modelWithPipelineData.modelId,
+        description: modelWithPipelineData.description,
+        modelTypes: modelWithPipelineData.modelTypes,
+      });
+
+      await ml.testExecution.logTestStep(
+        'should show disabled delete action for the model in the table'
+      );
+
+      await ml.trainedModelsTable.assertModelDeleteActionButtonEnabled(
+        modelWithPipelineData.modelId,
+        false
+      );
+    });
+
+    it('displays a model without an ingest pipeline and model can be deleted', async () => {
+      await ml.testExecution.logTestStep('should display the model in the table');
+      await ml.trainedModelsTable.filterWithSearchString(modelWithoutPipelineData.modelId, 1);
+
+      await ml.testExecution.logTestStep('displays expected row values for the model in the table');
+      await ml.trainedModelsTable.assertModelsRowFields(modelWithoutPipelineData.modelId, {
+        id: modelWithoutPipelineData.modelId,
+        description: modelWithoutPipelineData.description,
+        modelTypes: modelWithoutPipelineData.modelTypes,
+      });
+
+      await ml.testExecution.logTestStep(
+        'should show enabled delete action for the model in the table'
+      );
+
+      await ml.trainedModelsTable.assertModelDeleteActionButtonEnabled(
+        modelWithoutPipelineData.modelId,
+        true
+      );
+
+      await ml.testExecution.logTestStep('should show the delete modal');
+      await ml.trainedModelsTable.clickDeleteAction(modelWithoutPipelineData.modelId);
+
+      await ml.testExecution.logTestStep('should delete the model');
+      await ml.trainedModelsTable.confirmDeleteModel();
+      await ml.trainedModelsTable.assertModelDisplayedInTable(
+        modelWithoutPipelineData.modelId,
+        false
+      );
     });
   });
 }
diff --git a/x-pack/test/functional/services/ml/api.ts b/x-pack/test/functional/services/ml/api.ts
@@ -981,11 +981,11 @@ export function MachineLearningAPIProvider({ getService }: FtrProviderContext) {
         .expect(200)
         .then((res: any) => res.body);
 
-      log.debug('> Trained model crated');
+      log.debug('> Trained model created');
       return model;
     },
 
-    async createdTestTrainedModels(
+    async createTestTrainedModels(
       modelType: ModelType,
       count: number = 10,
       withIngestPipelines = false

diff --git a/x-pack/test/functional/services/ml/index.ts b/x-pack/test/functional/services/ml/index.ts
@@ -51,6 +51,7 @@ import { SwimLaneProvider } from './swim_lane';
 import { MachineLearningDashboardJobSelectionTableProvider } from './dashboard_job_selection_table';
 import { MachineLearningDashboardEmbeddablesProvider } from './dashboard_embeddables';
 import { TrainedModelsProvider } from './trained_models';
+import { TrainedModelsTableProvider } from './trained_models_table';
 import { MachineLearningJobAnnotationsProvider } from './job_annotations_table';
 
 export function MachineLearningProvider(context: FtrProviderContext) {
@@ -121,6 +122,7 @@ export function MachineLearningProvider(context: FtrProviderContext) {
   const alerting = MachineLearningAlertingProvider(context, commonUI);
   const swimLane = SwimLaneProvider(context);
   const trainedModels = TrainedModelsProvider(context, api, commonUI);
+  const trainedModelsTable = TrainedModelsTableProvider(context);
 
   return {
     anomaliesTable,
@@ -168,5 +170,6 @@ export function MachineLearningProvider(context: FtrProviderContext) {
     testExecution,
     testResources,
     trainedModels,
+    trainedModelsTable,
   };
 }