Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Transforms: Adds a link to discover from the transform list to the actions menu. #97805

Merged
merged 13 commits into from
Apr 27, 2021

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Apr 21, 2021

Summary

Adds a link to discover from the transform list to the actions menu.

image

Conditions for the link to be enabled:

  • Kibana index pattern must be available
  • Transform must have been started once and done some progress so there's the destination index available

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@walterra walterra force-pushed the ml-transform-link-to-discover branch from 97e7a2e to 7882f2c Compare April 22, 2021 10:40
@walterra walterra marked this pull request as ready for review April 22, 2021 10:44
@walterra walterra requested review from a team as code owners April 22, 2021 10:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the changes in src/core/public/saved_objects/saved_objects_service.mock.ts are not needed.

@@ -16,7 +16,7 @@ const createStartContractMock = () => {
bulkUpdate: jest.fn(),
delete: jest.fn(),
bulkGet: jest.fn(),
find: jest.fn(),
find: jest.fn().mockResolvedValue({ savedObjects: [] }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind setting the mock if needed on your own tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the code to apply that mock in the transform plugin code (171a65c)

Comment on lines 38 to 40
export function getDiscoverUrl(indexPatternId: string, baseUrl: string): string {
return `${baseUrl}/app/discover#?${getDiscoverUrlState(indexPatternId)}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discover plugin provides a URL generator

export class DiscoverUrlGenerator

Copy link
Contributor Author

@walterra walterra Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, we hadn't used those generators yet in the transform plugin. I refactored to make use of it, the result is that these utils are not necessary anymore 🥳 - related update in bb30f3f.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see functional test coming as part of this PR 🎉
Just left a few suggestions.

x-pack/test/functional/services/transform/discover.ts Outdated Show resolved Hide resolved
x-pack/test/functional/services/transform/discover.ts Outdated Show resolved Hide resolved
const action: TransformListAction = useMemo(
() => ({
name: (item: TransformListRow) => <DiscoverActionName items={[item]} />,
enabled: (item: TransformListRow) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the action is disabled, the text seems to have a disabled style but not the icon. Do we have any control over the icon state? The Start icon in contrast does get a disabled style

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EUI's app icons don't support a disabled state yet. As discussed I changed it to use the visTable icon instead (189cbb2).

Created an issue in EUI here: elastic/eui#4755

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest changes and LGTM

@afharo afharo requested review from afharo and removed request for afharo April 27, 2021 11:19
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No-longer touching anything on the Core side. LGTM :)

Thanks for doing the changes @walterra

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

[ML] Fix assertion.

Co-authored-by: Robert Oskamp <traeluki@gmail.com>
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional tests LGTM

@walterra walterra enabled auto-merge (squash) April 27, 2021 14:26
@walterra walterra added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 27, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
transform 221 224 +3

Async chunks

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

id before after diff
transform 903.8KB 908.6KB +4.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
transform 19.5KB 19.5KB -69.0B

History

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

cc @walterra

@walterra walterra merged commit 18d9d43 into elastic:master Apr 27, 2021
@walterra walterra deleted the ml-transform-link-to-discover branch April 27, 2021 14:50
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 27, 2021
…he actions menu. (elastic#97805)

Adds a link to discover from the transform list to the actions menu. Conditions for the link to be enabled:
- Kibana index pattern must be available
- Transform must have been started once and done some progress so there's the destination index available
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 27, 2021
…he actions menu. (#97805) (#98491)

Adds a link to discover from the transform list to the actions menu. Conditions for the link to be enabled:
- Kibana index pattern must be available
- Transform must have been started once and done some progress so there's the destination index available

Co-authored-by: Walter Rafelsberger <walter@rafelsberger.at>
madirey pushed a commit to madirey/kibana that referenced this pull request May 11, 2021
…he actions menu. (elastic#97805)

Adds a link to discover from the transform list to the actions menu. Conditions for the link to be enabled:
- Kibana index pattern must be available
- Transform must have been started once and done some progress so there's the destination index available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Transforms ML transforms :ml release_note:enhancement v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants