-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Actionable Observability] update alerts table rule details link to point to o11y rule detail page #132479
Conversation
27935b9
to
400c613
Compare
@@ -223,7 +223,9 @@ export default ({ getService }: FtrProviderContext) => { | |||
const actionsButton = await observability.alerts.common.getActionsButtonByIndex(0); | |||
await actionsButton.click(); | |||
await observability.alerts.common.viewRuleDetailsButtonClick(); | |||
expect(await find.existsByCssSelector('[title="Rules and Connectors"]')).to.eql(true); | |||
expect(await find.existsByCssSelector('[data-test-subj="ruleDetailPageTitle"]')).to.eql( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fkanout Currently there's a problem with current test which keeps failing because as you can see in the screenshot rule is undefined (you can even see it in the breadcrumb) and rule details page can not be loaded.
Here's where rule is undefined https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/rule_details/index.tsx#L201. Stack Management Rule details page is getting the rule as param https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/sections/rule_details/components/rule_details.tsx#L76 and I guess that's why the old rule detail page loads in the tests without issues. This refactoring is out of scope of this issue. How do you suggest we proceed with this failing test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively I could assert in my test if the page title contains Rules - Alerts - Observability
, or even check the rendered breadcrumb if it contains Observability. What do you think? All we want to check with this test, is that it goes to the new Obsservability rule detail page and not the old Rules and Connectors
page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a fix that checks if Observability is in the breadcrumb 1f94a1f#diff-5a75bdfd91813e385e6c322eed601a2fd55d34f4a56b7669b0bb193014df1648R226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgiota, I think this ok for the purpose of this test and in the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested, and it worked as expected. Thank you for spotting the need for these changes!
One suggestion about using the URL var instead of the hardcoded one
@@ -8,6 +8,7 @@ | |||
export const paths = { | |||
observability: { | |||
alerts: '/app/observability/alerts', | |||
ruleDetails: (ruleId: string) => `/app/observability/alerts/rules/${encodeURI(ruleId)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of using this var for the URL https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/rule_details/config.ts#L22?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep great suggestion! I'll change it
const linkToRule = ruleId && prepend ? prepend(paths.management.ruleDetails(ruleId)) : null; | ||
|
||
const linkToRule = | ||
ruleId && prepend ? prepend(`/app/observability/alerts/rules/${ruleId}`) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fkanout I'll move your variables in the paths file, which is a common place in observability plugin and refactor accordingly
6952f3c
to
b4e82ef
Compare
I moved the variables in the common paths file and I refactored following places to make use of it in following places:
|
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @mgiota |
Fixes #132389