Skip to content

Commit

Permalink
[RO] Incorrect Deletion of Webhook Actions in Kibana Rules (#159204)
Browse files Browse the repository at this point in the history
## Summary

Fixes #158167

The issue occurred because we were using a number called "index" as the
React key prop. When we removed the first element, the second one took
its place, but React still showed the removed element.

To fix this problem, we found a solution by using the uuid field that
each action item has. We now generate it for each new action we create
in the action form and use that as key

I was told to add @pmuellr as you might know if we are missing something
🙇

### Test: why xpath selector?

I had to use the xpath selector to fix a problem we had. The problem was
between two actions we set up. They look the same, but the body content
is the only different (I've attached a screenshot for more details).

We use a third party component for these actions. This component doesn't
have any "value" attribute and doesn't add anything besides the HTML
text. I tried to find other useful details but couldn't find any.

The problem comes up when we try to delete one of the actions. To fix
it, we needed to check if that component was missing. We already have
tools that can look for missing components, but they don't work with the
xpath selector. So, I added a new function that can do this. Now, we can
use the xpath selector to look for missing components and fix the
problem
 
<details>
<summary>See Screenshot</summary>
<img
src="https://github.com/elastic/kibana/assets/17549662/5447795d-0281-4847-aa85-76d0e5fdec3d"/>
</details>

```[tasklist]
- [x] Make sure that it's ok to generate the uuid
- [x] Test
- [x] Do we need to backport? Versions?
```
  • Loading branch information
jcger committed Jun 12, 2023
1 parent b08c322 commit 55bc8cf
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 1 deletion.
10 changes: 10 additions & 0 deletions test/functional/services/common/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,16 @@ export class FindService extends FtrService {
}, timeout);
}

public async existsByXpath(
selector: string,
timeout: number = this.WAIT_FOR_EXISTS_TIME
): Promise<boolean> {
this.log.debug(`Find.existsByXpath('${selector}') with timeout=${timeout}`);
return await this.exists(async (drive) => {
return this.wrapAll(await drive.findElements(By.xpath(selector)));
}, timeout);
}

public async clickByCssSelectorWhenNotDisabled(selector: string, opts?: TimeoutOpt) {
const timeout = opts?.timeout ?? this.defaultFindTimeout;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
RuleActionFrequency,
RuleActionParam,
} from '@kbn/alerting-plugin/common';
import { v4 as uuidv4 } from 'uuid';
import { betaBadgeProps } from './beta_badge_props';
import { loadActionTypes, loadAllActions as loadConnectors } from '../../lib/action_connector_api';
import {
Expand Down Expand Up @@ -242,6 +243,7 @@ export const ActionForm = ({
group: defaultActionGroupId,
params: {},
frequency: defaultRuleFrequency,
uuid: uuidv4(),
});
setActionIdByIndex(actionTypeConnectors[0].id, actions.length - 1);
} else {
Expand All @@ -255,6 +257,7 @@ export const ActionForm = ({
group: defaultActionGroupId,
params: {},
frequency: DEFAULT_FREQUENCY,
uuid: uuidv4(),
});
setActionIdByIndex(actionTypeConnectors[0].id, actions.length - 1);
}
Expand Down Expand Up @@ -427,7 +430,7 @@ export const ActionForm = ({
actionItem={actionItem}
actionConnector={actionConnector}
index={index}
key={`action-form-action-at-${index}`}
key={`action-form-action-at-${actionItem.uuid}`}
setActionParamsProperty={setActionParamsProperty}
setActionFrequencyProperty={setActionFrequencyProperty}
setActionAlertsFilterProperty={setActionAlertsFilterProperty}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,41 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
await testSubjects.click('rulesTab');
});

it('should delete the right action when the same action has been added twice', async () => {
// create a new rule
const ruleName = generateUniqueKey();
await rules.common.defineIndexThresholdAlert(ruleName);

// create webhook connector
await testSubjects.click('.webhook-alerting-ActionTypeSelectOption');
await testSubjects.click('createActionConnectorButton-0');
await testSubjects.setValue('nameInput', 'webhook-test');
await testSubjects.setValue('webhookUrlText', 'https://test.test');
await testSubjects.setValue('webhookUserInput', 'fakeuser');
await testSubjects.setValue('webhookPasswordInput', 'fakepassword');

// save rule
await find.clickByCssSelector('[data-test-subj="saveActionButtonModal"]:not(disabled)');
await find.setValueByClass('kibanaCodeEditor', 'myUniqueKey');
await testSubjects.click('saveRuleButton');

// add new action and remove first one
await testSubjects.click('ruleSidebarEditAction');
await testSubjects.click('.webhook-alerting-ActionTypeSelectOption');
await find.clickByCssSelector(
'[data-test-subj="alertActionAccordion-0"] [aria-label="Delete"]'
);

// check that the removed action is the right one
const doesExist = await find.existsByXpath(".//*[text()='myUniqueKey']");
expect(doesExist).to.eql(false);

// clean up created alert
const alertsToDelete = await getAlertsByName(ruleName);
await deleteAlerts(alertsToDelete.map((rule: { id: string }) => rule.id));
expect(true).to.eql(true);
});

it('should create an alert', async () => {
const alertName = generateUniqueKey();
await rules.common.defineIndexThresholdAlert(alertName);
Expand Down

0 comments on commit 55bc8cf

Please sign in to comment.