[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job #67922

qn895 · 2020-06-02T05:29:27Z

This PR adds the option to delete destination index & index pattern when deleting transform job(s) in two scenarios: single delete vs bulk delete.

When we are deleting transforms in bulk:

Delete message

Delete toast messages when all tasks succeed (in this case one transform job didn't have any index pattern)

Delete toast messages when some tasks failed

When we are deleting just one transform job individually:

UI checks if user has permission to delete that individual index, shows the option if user has permission, and enables the option by default.
UI checks if there's an index pattern that exists with the same name as the destination index, shows the option if user has permission and index pattern does exists, and enables the option by default.

Toast messages

If all operations succeed

If one of the tasks failed

## Original discussion
When we are deleting in bulk:
<img width="493" alt="Screen Shot 2020-06-01 at 10 53 39 PM" src="https://user-images.githubusercontent.com/43350163/83482735-a5975800-a466-11ea-85c7-f3af5ca0f04a.png">

1) Should we assume user has permission to delete all the indices selected?

2) Should option to delete index pattern be visible and enabled? If one job might have index pattern but 2 other jobs might not.

3) Should we show success/error message for each individual job and operation or should we combine them to prevent toast message overload?

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…eting job

peteharverson · 2020-06-02T11:31:53Z

My thoughts on the three questions you asked @qn895 :

Should we assume user has permission to delete all the indices selected?

As long as the user has permission to delete one of the indexes, I would show the 'Delete destination index` option in the modal. I would keep the permission checks on the client-side to a minimum, and then do the full permission checks in the server-side endpoint for each destination index, returning an error to display in a toast for each index the user doesn't have permission to delete.

Should option to delete index pattern be visible and enabled? If one job might have index pattern but 2 other jobs might not.

I think we should show the Delete destination index patterns option as long as any one of the destination indexes has an index pattern.

Should we show success/error message for each individual job and operation or should we combine them to prevent toast message overload?

For the bulk delete, I think it would be best to just show one success message for each operation i.e. one toast for the transform, one for the destination index, and one for the index pattern delete. So if you delete 5 transforms, you only get 3 toasts. But for error messages, I would show a toast for each individual transform / operation that has failed.

For bulk delete, the text I would use for the options in the modal are Delete destination indexes and Delete destination index patterns.

x-pack/test/functional/services/transform/security_common.ts

elasticmachine · 2020-06-04T04:39:52Z

Pinging @elastic/ml-ui (:ml)

x-pack/test/functional/services/transform/api.ts

x-pack/test/api_integration/apis/transform/delete_transforms.ts

x-pack/test/functional/services/transform/security_common.ts

x-pack/plugins/transform/public/app/hooks/use_delete_transform.tsx

x-pack/test/api_integration/apis/transform/delete_transforms.ts

walterra

Great job overall, and great to have some integration tests! Added a few comments/questions.

x-pack/plugins/transform/public/app/hooks/use_delete_transform.tsx

...ansform/public/app/sections/transform_management/components/transform_list/action_delete.tsx

x-pack/plugins/transform/common/index.ts

x-pack/plugins/transform/server/routes/api/transforms.ts

peteharverson

Left some minor comments about labelling. Tested and functionally all looks good.

...ansform/public/app/sections/transform_management/components/transform_list/action_delete.tsx

darnautov · 2020-06-05T08:30:41Z

...ansform/public/app/sections/transform_management/components/transform_list/action_delete.tsx

+  const deleteModalContent = (
+    <>
+      <p>
+        <FormattedMessage
+          id="xpack.transform.transformList.deleteModalBody"
+          defaultMessage="Are you sure you want to delete this transform?"
+        />
+      </p>
+      <EuiFlexGroup direction="column" gutterSize="none">
+        <EuiFlexItem>
+          {userCanDeleteIndex && (
+            <EuiSwitch
+              data-test-subj="transformDeleteIndexSwitch"
+              style={{ paddingBottom: 10 }}
+              label={i18n.translate(
+                'xpack.transform.actionDeleteTransform.deleteDestinationIndexTitle',
+                {
+                  defaultMessage: 'Delete destination index {destinationIndex}',
+                  values: { destinationIndex: items[0] && items[0].config.dest.index },
+                }
+              )}
+              checked={deleteDestIndex}
+              onChange={toggleDeleteIndex}
+            />
+          )}
+        </EuiFlexItem>
+        <EuiFlexItem>
+          {userCanDeleteIndex && indexPatternExists && (
+            <EuiSwitch
+              data-test-subj="transformDeleteIndexPatternSwitch"
+              label={i18n.translate(
+                'xpack.transform.actionDeleteTransform.deleteDestIndexPatternTitle',
+                {
+                  defaultMessage: 'Delete index pattern {destinationIndex}',
+                  values: { destinationIndex: items[0] && items[0].config.dest.index },
+                }
+              )}
+              checked={deleteIndexPattern}
+              onChange={toggleDeleteIndexPattern}
+            />
+          )}
+        </EuiFlexItem>
+      </EuiFlexGroup>
+    </>


this block looks the same expect i18n strings are different. I'd prefer to have only one and just have conditional rendering of the labels.

x-pack/plugins/transform/server/routes/api/transforms.ts

darnautov · 2020-06-05T08:35:29Z

x-pack/plugins/transform/server/routes/api/transforms.ts

+) {
+  const response = await savedObjectsClient.find<IIndexPattern>({
+    type: 'index-pattern',
+    perPage: 10,


does it require 10 results per page?

Fixed in def2ccf

x-pack/plugins/transform/server/routes/api/transforms.ts

walterra

Latest changes LGTM.

…index-when-del-job # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

pheyos

Functional tests LGTM

kibanamachine · 2020-06-09T23:53:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6f0fc96

History

💚 Build #52710 succeeded d340b35
💚 Build #51859 succeeded 47ec2cc
💚 Build #51844 succeeded 1269b1d
💚 Build #51805 succeeded ec3acd3
💛 Build #51640 was flaky 90a2d5e

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

darnautov

LGTM

…n deleting transform job (elastic#67922)

…tern when deleting transform job (#67922)" This reverts commit 9bc0936.

tylersmalley · 2020-06-10T17:05:15Z

A reference to the removed file was added in https://github.com/elastic/kibana/pull/68462/files#diff-0b479419da389da8e3bf4ee485c9fe25R24 which was merged after this PR went green. This resulted in CI master failing. This PR was reverted to unblock downstream teams and can be re-opened.

…n deleting transform job (elastic#67922)

[Transform] Add ability to delete dest index & index pattern when del…

9a72929

…eting job

qn895 added 5 commits June 2, 2020 15:11

[Transform] Combine success messages when deleting job transforms

a9620c6

[Transform] Change preview text length to 50

f2d672b

[Transform] Add API integration tests for delete_transforms

1e6bd0c

[Transform] Update tests and fix i18n duplicates

b32b884

[Transform] Update bulk delete messages

90a2d5e

qn895 requested review from pheyos, alvarezmelissa87 and peteharverson June 4, 2020 03:13

qn895 self-assigned this Jun 4, 2020

qn895 added :ml Feature:Transforms ML transforms v7.9.0 v8.0.0 release_note:enhancement labels Jun 4, 2020

qn895 requested a review from walterra June 4, 2020 04:37

qn895 changed the title ~~WIP: Add ability to delete dest index & index pattern when deleting transform job~~ [Transform] Add ability to delete dest index & index pattern when deleting transform job Jun 4, 2020

qn895 commented Jun 4, 2020

View reviewed changes

x-pack/test/functional/services/transform/security_common.ts Outdated Show resolved Hide resolved

qn895 marked this pull request as ready for review June 4, 2020 04:39

qn895 requested a review from a team as a code owner June 4, 2020 04:39

pheyos requested changes Jun 4, 2020

View reviewed changes

walterra changed the title ~~[Transform] Add ability to delete dest index & index pattern when deleting transform job~~ [ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job Jun 4, 2020

peteharverson reviewed Jun 4, 2020

View reviewed changes

walterra reviewed Jun 4, 2020

View reviewed changes

peteharverson reviewed Jun 4, 2020

View reviewed changes

x-pack/plugins/transform/server/routes/api/transforms.ts Outdated Show resolved Hide resolved

peteharverson reviewed Jun 4, 2020

View reviewed changes

qn895 added 2 commits June 4, 2020 08:22

[Transform] Update tests

d87248d

[Transform] Rename transform job -> transform, fix ts

d3c37b5

darnautov reviewed Jun 5, 2020

View reviewed changes

...ansform/public/app/sections/transform_management/components/transform_list/action_delete.tsx Outdated Show resolved Hide resolved

darnautov reviewed Jun 5, 2020

View reviewed changes

...ansform/public/app/sections/transform_management/components/transform_list/action_delete.tsx Outdated Show resolved Hide resolved

darnautov reviewed Jun 5, 2020

View reviewed changes

x-pack/plugins/transform/server/routes/api/transforms.ts Outdated Show resolved Hide resolved

darnautov reviewed Jun 5, 2020

View reviewed changes

x-pack/plugins/transform/server/routes/api/transforms.ts Show resolved Hide resolved

qn895 added 4 commits June 5, 2020 11:28

[Transform] Update tests to include waitForIndicesToExist

d8f901c

[Transform] Refactor delete transform schema

cd293f2

[Transform] Add spacer, rename indices,

8e7b21a

[Transform] Refactor new IndexService

def2ccf

walterra approved these changes Jun 8, 2020

View reviewed changes

qn895 added 2 commits June 9, 2020 08:40

[Transform] Extend MLResponseError in extractErrorMessage

f2b70fb

pheyos approved these changes Jun 9, 2020

View reviewed changes

qn895 added 2 commits June 9, 2020 16:16

[Transform] Add unit tests extractErrorMessage and modify type defs

a5fda7f

[Transform] Refactor extractErrorMessage to account for boom responses

6f0fc96

darnautov approved these changes Jun 10, 2020

View reviewed changes

qn895 merged commit 9bc0936 into elastic:master Jun 10, 2020

qn895 deleted the transform-delete-index-when-del-job branch June 10, 2020 15:11

qn895 mentioned this pull request Jun 10, 2020

[7.x] [ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job (#67922) #68777

Closed

qn895 added a commit to qn895/kibana that referenced this pull request Jun 10, 2020

[ML] Transforms: Add ability to delete dest index & index pattern whe…

6ceef9c

…n deleting transform job (elastic#67922)

qn895 added a commit that referenced this pull request Jun 10, 2020

Revert "[ML] Transforms: Add ability to delete dest index & index pat…

3328ac2

…tern when deleting transform job (#67922)" This reverts commit 9bc0936.

qn895 mentioned this pull request Jun 10, 2020

Revert "[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job" #68793

Merged

tylersmalley added the reverted label Jun 10, 2020

qn895 mentioned this pull request Jun 10, 2020

[ML] Transforms - Updated: Add ability to delete dest index & index pattern when deleting transform job #68800

Closed

8 tasks

qn895 added a commit to qn895/kibana that referenced this pull request Jun 11, 2020

[ML] Transforms: Add ability to delete dest index & index pattern whe…

5090fc0

…n deleting transform job (elastic#67922)

qn895 mentioned this pull request Jun 11, 2020

[ML] Transforms - Updated: Add ability to delete dest index & index pattern when deleting transform job #68896

Merged

8 tasks

peteharverson mentioned this pull request Jul 31, 2020

[ML] Increase API integration test coverage #63700

Closed

90 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job #67922

[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job #67922

qn895 commented Jun 2, 2020 •

edited

Loading

peteharverson commented Jun 2, 2020

elasticmachine commented Jun 4, 2020

walterra left a comment

peteharverson left a comment

darnautov Jun 5, 2020

darnautov Jun 5, 2020

qn895 Jun 10, 2020

walterra left a comment

pheyos left a comment

kibanamachine commented Jun 9, 2020

darnautov left a comment

tylersmalley commented Jun 10, 2020

[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job #67922

[ML] Transforms: Add ability to delete dest index & index pattern when deleting transform job #67922

Conversation

qn895 commented Jun 2, 2020 • edited Loading

When we are deleting transforms in bulk:

When we are deleting just one transform job individually:

Checklist

For maintainers

peteharverson commented Jun 2, 2020

elasticmachine commented Jun 4, 2020

walterra left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

darnautov Jun 5, 2020

Choose a reason for hiding this comment

darnautov Jun 5, 2020

Choose a reason for hiding this comment

qn895 Jun 10, 2020

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

pheyos left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 9, 2020

💚 Build Succeeded

History

darnautov left a comment

Choose a reason for hiding this comment

tylersmalley commented Jun 10, 2020

qn895 commented Jun 2, 2020 •

edited

Loading