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.

Sign up for GitHub

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

Jump to bottom

[ML] Transforms: Fix tab ids for expanded row. #80666

Merged
merged 4 commits into from
Oct 17, 2020
Merged

[ML] Transforms: Fix tab ids for expanded row. #80666

merged 4 commits into from
Oct 17, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Oct 15, 2020 •
edited
Loading

Summary

Part of #80650.

We based the IDs to identify expanded row tabs on the transform ID. This could break the page because the transform ID could include characters (e.g. dots) not supported by EUI's tabbed content component.

This PR fixes the issue by using the stringHash() utility to create the IDs.

Checklist

@walterra walterra added bug Fixes for quality problems that affect the customer experience release_note:fix :ml v8.0.0 Feature:Transforms ML transforms v7.10.0 v7.11.0 labels Oct 15, 2020
@walterra walterra requested a review from a team as a code owner October 15, 2020 13:51
@walterra walterra self-assigned this Oct 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 self-requested a review October 15, 2020 15:14
alvarezmelissa87
alvarezmelissa87 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@alvarezmelissa87 alvarezmelissa87 self-requested a review October 15, 2020 17:06
@alvarezmelissa87
Copy link
Contributor

Tested with latest changes and running into an issue where I get a full blank page when I try to expand more than one transform row from the table at a time.

transform-bug

With this error:

image

alvarezmelissa87
alvarezmelissa87 requested changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Running into an issue where I get a full blank page when I try to expand more than one transform row from the table at a time. Added screenshot in separate comment.

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@walterra
Copy link
Contributor Author

Fixed opening multiple rows in ad1f94e.

The problem was that EUI's ID generator is random and not deterministic so across re-renders this could change and break things. To solve this, I copied over stringHash() from the ML plugin. Copied it directly to the expanded-row file because I didn't want to touch code sharing code across plugins at this stage for 7.10. I will do a follow-up for 7.11 with some code clean-up.

alvarezmelissa87
alvarezmelissa87 approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Tested and latest changes LGTM ⚡

peteharverson
peteharverson approved these changes Oct 16, 2020
View reviewed changes
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.

Code LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
transform 1.2MB 1.2MB +208.0B

History

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

@walterra walterra merged commit 5858dd8 into elastic:master Oct 17, 2020
@walterra walterra deleted the ml-transforms-fix-tabs-id branch October 17, 2020 10:25
This was referenced Oct 17, 2020
Merged
Merged
walterra added a commit to walterra/kibana that referenced this pull request Oct 17, 2020
@walterra
[ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
f9f8201 
We based the IDs to identify expanded row tabs on the transform ID. This could break the page because the transform ID could include characters (e.g. dots) not supported by EUI's tabbed content component. This fixes the issue by using the stringHash() utility to create the IDs.
walterra added a commit to walterra/kibana that referenced this pull request Oct 17, 2020
@walterra
[ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
84d3497 
We based the IDs to identify expanded row tabs on the transform ID. This could break the page because the transform ID could include characters (e.g. dots) not supported by EUI's tabbed content component. This fixes the issue by using the stringHash() utility to create the IDs.
@walterra walterra mentioned this pull request Oct 17, 2020
Closed
2 tasks
walterra added a commit that referenced this pull request Oct 17, 2020
@walterra
[ML] Transforms: Fix tab ids for expanded row. (#80666) (#80934)
59348cc 
We based the IDs to identify expanded row tabs on the transform ID. This could break the page because the transform ID could include characters (e.g. dots) not supported by EUI's tabbed content component. This fixes the issue by using the stringHash() utility to create the IDs.
walterra added a commit that referenced this pull request Oct 17, 2020
@walterra
[ML] Transforms: Fix tab ids for expanded row. (#80666) (#80935)
1a7fe4d 
We based the IDs to identify expanded row tabs on the transform ID. This could break the page because the transform ID could include characters (e.g. dots) not supported by EUI's tabbed content component. This fixes the issue by using the stringHash() utility to create the IDs.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
@gmmorris
Merge branch 'master' into task-manager/refactor-oversized-class
c30d71b 
* master: (43 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
@gmmorris
Merge branch 'master' into task-manager/health
12e52e6 
* master: (23 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@jgowdyelastic jgowdyelastic Awaiting requested review from jgowdyelastic

Assignees

@walterra walterra

Labels
bug Fixes for quality problems that affect the customer experience Feature:Transforms ML transforms :ml release_note:fix v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@walterra @elasticmachine @alvarezmelissa87 @kibanamachine @peteharverson