[Response Ops] [Rule Form] Add Rule Form Flyout v2 #206685
Conversation
Zacqary
added
Team:ResponseOps
|
@elastic/kibana-design This PR is still in draft but could use some feedback on these two design elements as they weren't in the Figma: 
I'm particularly unsure about the confirmation screen, it's definitely Not A Flyout Opening A Modal™ but it's also a ton of blank space |
Zacqary
force-pushed
the
195211-replace-flyout
branch
2 times, most recently
from
c3b25e9 to
cbf1333
Compare
Hey, @Zacqary. I'm not certain who the designer was on this, so I'll defer to them if they disagree with any of the following opinions. That said, I'd suggest the following: Discard changes screenshotYou made mention of not triggering the confirmation modal from the flyout. Is there a compelling reason not to do so? Personally, I'm wondering if it might be a more common/familiar pattern to open a confirmation modal instead of repurposing the content area of the flyout. It'll also be more forgiving for short messages such as this. No actions configured screenshotSince it's allowed, I'm wondering if we should change the Additionally, since the message is most relevant at the moment the user leaves the "Actions" step and moves to the "Details" step, perhaps it would make sense to move the callout up towards the top of the flyout (just below the header). |
The reason I put the callout on the footer is because it's replacing something the full page Rule Form shows as a confirmation popup. It's a message that we're usually displaying to the user right as they're making the decision to save the rule. I'm happy to move the callout to the top of the screen, but if flyouts are allowed to open confirmation modals I could also just have it pop up the same modal as the rule page instead. |
|
Thanks, @MichaelMarcialis for your comment and thoughts. I forgot to add these screens on Figma and missed the notification for the PR review. @Zacqary is now aware that I'm the one responsible for this area. Discard changes screenshot This behavior is already in use if you create/edit a Rule from Observability: No actions configured screenshot |
This reverts commit 8d58b28.
|
@jughosta I've done some investigating and it looks like this error is expected on CI. Can confirm that editing --- a/x-pack/platform/plugins/shared/alerting/server/plugin.ts
+++ b/x-pack/platform/plugins/shared/alerting/server/plugin.ts
@@ -569,11 +569,11 @@ export class AlertingPlugin {
});
const getRulesClientWithRequest = async (request: KibanaRequest) => {
- if (isESOCanEncrypt !== true) {
- throw new Error(
- `Unable to create alerts client because the Encrypted Saved Objects plugin is missing encryption key. Please set xpack.encryptedSavedObjects.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.`
- );
- }
+ //if (isESOCanEncrypt !== true) {
+ throw new Error(
+ `Unable to create alerts client because the Encrypted Saved Objects plugin is missing encryption key. Please set xpack.encryptedSavedObjects.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.`
+ );
+ //}
return rulesClientFactory!.create(request, core.savedObjects);
};There's something else about the CI environment that's causing Discover to behave differently. I don't want to skip this test but I'm going to need your team's help debugging if we want to get this PR through. I'm hitting a wall here. |
| // until one day somebody makes another change to this package and, for mysterious, unknowable reasons, | ||
| // Webpack decides that today the bundle size shall be engorged once again. | ||
| // To avoid all that maybe just don't delete this dynamic import. | ||
| ecsFlat.current = await import('@elastic/ecs').then((ecs) => ecs.EcsFlat); |
There was a problem hiding this comment.
There is a service to dynamically load ECS data for selected fields: https://github.com/elastic/kibana/blob/main/x-pack/platform/plugins/shared/fields_metadata/README.md
Or when rendering it's possible to simply use <FieldDescription .../> component from https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-field-utils/src/components/field_description/field_description.tsx
| return triggersActionsUi?.getAddRuleFlyout({ | ||
| metadata: discoverMetadata, | ||
| return triggersActionsUi?.getRuleFormFlyout<EsQueryAlertMetaData>({ | ||
| plugins: services, |
There was a problem hiding this comment.
The change in how plugins are accessed could cause the test to fail.
There was a problem hiding this comment.
ruleTypeRegistry and actionTypeRegistry are added by the triggersActionsUI export. That's also how the v1 flyout worked. We haven't yet come up with a solution to allow solutions to directly import the flyout from the @kbn/response-ops-rule-form package so it's still relying on tAUI
There was a problem hiding this comment.
Note the code of triggersActionsUI.getRuleFormFlyout:
getRuleFormFlyout: (props) => {
return getRuleFormFlyoutLazy({
...props,
plugins: {
...validateRuleFormPlugins(props.plugins),
actionTypeRegistry: this.actionTypeRegistry,
ruleTypeRegistry: this.ruleTypeRegistry,
},
});
},validateRuleFormPlugins throws an error if the passed props.plugins doesn't contain anything that's required by getRuleFormFlyout, defined by:
type RequiredRuleFormPlugins = Omit<
RuleFormProps['plugins'],
'actionTypeRegistry' | 'ruleTypeRegistry'
>;And then the two registries are added from triggersActionsUi
| expect(await testSubjects.exists('addRuleFlyoutTitle')).to.be(true); | ||
| // Increase timeout. The rule flyout has to load asynchronously when the user clicks the create rule button, | ||
| // so a higher timeout here reduces flakiness in the test | ||
| expect(await testSubjects.exists('addRuleFlyoutTitle', { timeout: 20000 })).to.be(true); |
There was a problem hiding this comment.
We can try to update test/functional/apps/discover/group1/config.ts with the following:
export default async function ({ readConfigFile }: FtrConfigProviderContext) {
const functionalConfig = await readConfigFile(require.resolve('../../../config.base.js'));
return {
...functionalConfig.getAll(),
kbnTestServer: {
...functionalConfig.get('kbnTestServer'),
serverArgs: [
...functionalConfig.get('kbnTestServer.serverArgs'),
'--xpack.encryptedSavedObjects.encryptionKey="wuGNaIhoMpk5sO4UBxgr3NyW1sFcLgIf"', // it's required for alerts plugin to work
],
},
testFiles: [require.resolve('.')],
};
}
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
|
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
@jughosta Thanks for the help, test is passing now. Confused about why that broke it on CI but not locally, but I'll take the win. PR is out of draft now if you're able to review the Discover codeowner changes |
Summary
Part of #195211
Replaces the create/edit rule flyout with the new rule flyout
Restores the confirmation prompt before canceling or saving a rule without actions defined.
Also fixes most of the design papercuts in the Actions step:
Checklist