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

Rename alert status OK to Recovered and fix some UX issues around disabling a rule while being in an error state #98135

Merged
merged 11 commits into from
May 13, 2021
Merged

Rename alert status OK to Recovered and fix some UX issues around disabling a rule while being in an error state #98135

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4cff048
Fix UX when alert is disabled and in an error state
mikecote Apr 23, 2021
7bca356
Reset executionStatus to pending after enabling an alert
mikecote Apr 23, 2021
c7b2485
Renames alert instance status OK to Recovered
mikecote Apr 23, 2021
6b27795
Fix end to end test
mikecote Apr 23, 2021
4c18b9f
Merge
mikecote Apr 27, 2021
0caa15d
Merge branch 'master' of github.com:elastic/kibana into alerting/fix-…
mikecote Apr 29, 2021
8b41644
Update doc screenshot
mikecote Apr 29, 2021
624ae75
Fix confusing test name
mikecote Apr 29, 2021
cfb1c25
Remove flakiness in integration test
mikecote Apr 29, 2021
e57fc5c
Merge with master
mikecote May 11, 2021
07d5dfe
Merge branch 'master' into alerting/fix-disable-ux
kibanamachine May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions x-pack/plugins/alerting/server/alerts_client/alerts_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,11 @@ export class AlertsClient {
),
updatedBy: username,
updatedAt: new Date().toISOString(),
executionStatus: {
status: 'pending',
lastExecutionDate: new Date().toISOString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Does it make sense to reset the lastExecutionDate here? I guess it would quickly be overwritten the next time the rule runs, but technically, this date is not the last time the rule was executed.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we do set that field to the same "current date" for other cases of status: 'pending'. So, it's consistent. I think we picked the name lastExecutionDate before starting to use pending, and so ... the name kinda doesn't make sense any more. Would be better described (I think) as "lastStateUpdateDate" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be better described (I think) as "lastStateUpdateDate" or something.

I think that makes sense. Should I create an issue for 8.0 release? This way we can make it a breaking change.

I think copying how it's done on create is ok for now but definitely strange.

Copy link
Member

Choose a reason for hiding this comment

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

DateDateDate

I'm not against changing the name to better reflect reality, especially as an 8.0 thing. Though I wonder if lastStateUpdate makes sense - the date value is supposed to reflect the last execution, except we knew we didn't have that date in some previous 7.x release, and obviously don't have one for newly created rules. So it does kind of make sense to continue with the current name, with the caveat that you need to check to see if the status is 'pending' which means "it hasn't run yet". Basically, if we call it lastStateUpdate, would folks expect other changes might also change this date? What happens to these fields when disabling a rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read this as "the execution status is pending, and it's been pending since lastExecutionDate". So any name that can reflect that and last time the rule executed I'm +1. Seems like a separate issue to be created?

error: null,
},
});
try {
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
Expand Down
10 changes: 10 additions & 0 deletions x-pack/plugins/alerting/server/alerts_client/tests/enable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,11 @@ describe('enable()', () => {
},
},
],
executionStatus: {
status: 'pending',
lastExecutionDate: '2019-02-12T21:01:22.479Z',
error: null,
},
},
{
version: '123',
Expand Down Expand Up @@ -352,6 +357,11 @@ describe('enable()', () => {
},
},
],
executionStatus: {
status: 'pending',
lastExecutionDate: '2019-02-12T21:01:22.479Z',
error: null,
},
},
{
version: '123',
Expand Down
70 changes: 70 additions & 0 deletions ...rs_actions_ui/public/application/sections/alert_details/components/alert_details.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import * as React from 'react';
import uuid from 'uuid';
import { shallow } from 'enzyme';
import { mountWithIntl, nextTick } from '@kbn/test/jest';
import { act } from '@testing-library/react';
import { AlertDetails } from './alert_details';
import { Alert, ActionType, AlertTypeModel, AlertType } from '../../../../types';
import { EuiTitle, EuiBadge, EuiFlexItem, EuiSwitch, EuiButtonEmpty, EuiText } from '@elastic/eui';
Expand Down Expand Up @@ -463,6 +465,74 @@ describe('disable button', () => {
handler!({} as React.FormEvent);
expect(enableAlert).toHaveBeenCalledTimes(1);
});

it('should bring back error banner after re-enable if previously dismissed', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that I will see the error banner without refreshing the page after reenabling? I see the error banner disappear after disabling but after enabling, I have to refresh the page to see the error banner again

Apr-26-2021 08-23-42

Copy link
Contributor Author

@mikecote mikecote Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After re-enabling the rule, it would have a pending status which I believe explains why the banner doesn't re-appear until you refresh the page (or click the refresh button). Because by then, the rule had a chance to run once and update the execution status. Does this experience seem strange?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. I think that's fine. The test name made me think the error banner should reappear right away but I think it's ok if it doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean. I fixed the test name in this commit: 624ae75.

const alert = mockAlert({
enabled: true,
executionStatus: {
status: 'error',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
error: {
reason: AlertExecutionStatusErrorReasons.Execute,
message: 'Fail',
},
},
});

const alertType: AlertType = {
id: '.noop',
name: 'No Op',
actionGroups: [{ id: 'default', name: 'Default' }],
recoveryActionGroup,
actionVariables: { context: [], state: [], params: [] },
defaultActionGroupId: 'default',
producer: ALERTS_FEATURE_ID,
authorizedConsumers,
minimumLicenseRequired: 'basic',
enabledInLicense: true,
};

const disableAlert = jest.fn();
const enableAlert = jest.fn();
const wrapper = mountWithIntl(
<AlertDetails
alert={alert}
alertType={alertType}
actionTypes={[]}
{...mockAlertApis}
disableAlert={disableAlert}
enableAlert={enableAlert}
/>
);

await act(async () => {
await nextTick();
wrapper.update();
});

// Dismiss the error banner
await act(async () => {
wrapper.find('[data-test-subj="dismiss-execution-error"]').first().simulate('click');
await nextTick();
});

// Disable the alert
await act(async () => {
wrapper.find('[data-test-subj="disableSwitch"] .euiSwitch__button').first().simulate('click');
await nextTick();
});
expect(disableAlert).toHaveBeenCalled();

// Enable the alert
await act(async () => {
wrapper.find('[data-test-subj="disableSwitch"] .euiSwitch__button').first().simulate('click');
await nextTick();
});
expect(enableAlert).toHaveBeenCalled();

// Ensure error banner is back
expect(wrapper.find('[data-test-subj="dismiss-execution-error"]').length).toBeGreaterThan(0);
});
});

describe('mute button', () => {
Expand Down
10 changes: 8 additions & 2 deletions ...riggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ export const AlertDetails: React.FunctionComponent<AlertDetailsProps> = ({
if (isEnabled) {
setIsEnabled(false);
await disableAlert(alert);
// Reset dismiss if previously clicked
setDissmissAlertErrors(false);
} else {
setIsEnabled(true);
await enableAlert(alert);
Expand Down Expand Up @@ -277,7 +279,7 @@ export const AlertDetails: React.FunctionComponent<AlertDetailsProps> = ({
</EuiFlexGroup>
</EuiFlexItem>
</EuiFlexGroup>
{!dissmissAlertErrors && alert.executionStatus.status === 'error' ? (
{alert.enabled && !dissmissAlertErrors && alert.executionStatus.status === 'error' ? (
<EuiFlexGroup>
<EuiFlexItem>
<EuiCallOut
Expand All @@ -293,7 +295,11 @@ export const AlertDetails: React.FunctionComponent<AlertDetailsProps> = ({
<EuiSpacer size="s" />
<EuiFlexGroup gutterSize="s" wrap={true}>
<EuiFlexItem grow={false}>
<EuiButton color="danger" onClick={() => setDissmissAlertErrors(true)}>
<EuiButton
data-test-subj="dismiss-execution-error"
color="danger"
onClick={() => setDissmissAlertErrors(true)}
>
<FormattedMessage
id="xpack.triggersActionsUI.sections.alertDetails.dismissButtonTitle"
defaultMessage="Dismiss"
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
.auth(user.username, user.password)
.expect(200);
expect(typeof updatedAlert.scheduled_task_id).to.eql('string');
expect(updatedAlert.execution_status.status).to.eql('pending');
Copy link
Contributor Author

@mikecote mikecote Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note to self: this is flaky in the scenario the rule finished running before the API call is made.

Error: expected 'ok' to sort of equal 'pending'
    at Assertion.assert (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/alerting_api_integration/spaces_only/tests/alerting/enable.ts:52:55)
    at Object.apply (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: 'ok',
  expected: 'pending',
  showDiff: true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to remove the integration test: cfb1c25. It will always be a race condition between enabling the rule and checking if the execution status is pending. So we'll have to rely on unit tests.

const { _source: taskRecord } = await getScheduledTask(
updatedAlert.scheduled_task_id
);
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/alerting_api_integration/spaces_only/tests/alerting/enable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
.set('kbn-xsrf', 'foo')
.expect(200);
expect(typeof updatedAlert.scheduled_task_id).to.eql('string');
expect(updatedAlert.execution_status.status).to.eql('pending');
Copy link
Contributor Author

@mikecote mikecote Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note to self: this is flaky in the scenario the rule finished running before the API call is made.

Error: expected 'ok' to sort of equal 'pending'
    at Assertion.assert (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/alerting_api_integration/spaces_only/tests/alerting/enable.ts:52:55)
    at Object.apply (/dev/shm/workspace/parallel/16/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: 'ok',
  expected: 'pending',
  showDiff: true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to remove the integration test: cfb1c25. It will always be a race condition between enabling the rule and checking if the execution status is pending. So we'll have to rely on unit tests.

const { _source: taskRecord } = await getScheduledTask(updatedAlert.scheduled_task_id);
expect(taskRecord.type).to.eql('task');
expect(taskRecord.task.taskType).to.eql('alerting:test.noop');
Expand Down