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

[Actions] Fixes issue which causes PagerDuty Params to rerender continuously. #85050

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Dec 4, 2020

Summary

Fixes issue which causes PagerDuty Params to rerender continuously.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@gmmorris gmmorris requested a review from a team as a code owner December 4, 2020 18:08
@gmmorris gmmorris added Feature:Actions release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Dec 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@@ -115,7 +115,7 @@ export const ActionTypeForm = ({
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [actionItem.group, defaultParams]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the important part of the fix.
It ensures that we only reapply the defaultParams when the group has been changed.
Ideally we'd only do so when the group has changed and the params aren't the same as what's already there, but this doesn't work because it causes it to overwrite non-default values selected by the user.

In the future we should apply the default to the actual action object in memory a level above, instead of the ActionForm doing it. But that's a bigger fix that would cause Main to remain broken for longer.

Comment on lines +314 to +323
const recoveryActionGroup = selectedAlertType?.recoveryActionGroup?.id;
const getDefaultActionParams = useCallback(
(actionTypeId: string, actionGroupId: string): Record<string, AlertActionParam> | undefined =>
getDefaultsForActionParams(
actionTypeId,
actionGroupId,
actionGroupId === recoveryActionGroup
),
[recoveryActionGroup]
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures we can keep the concept of a recovery group out of the actions code.
Not ideal, but good enough for now IMHO

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Verified that adding a PagerDuty to an alert does not freeze the browser

@mikecote mikecote self-requested a review December 4, 2020 18:44
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Ran it locally and can confirm the params form renders normally.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
triggersActionsUi 1.5MB 1.5MB +371.0B

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

@ymao1 ymao1 merged commit c85f254 into elastic:master Dec 4, 2020
ymao1 pushed a commit to ymao1/kibana that referenced this pull request Dec 4, 2020
…nuously. (elastic#85050)

* prevent aciton form from rerendering constantly

* fixed typing
ymao1 added a commit that referenced this pull request Dec 4, 2020
…nuously. (#85050) (#85063)

* prevent aciton form from rerendering constantly

* fixed typing

Co-authored-by: Gidi Meir Morris <github@gidi.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants