Skip to content

Commit

Permalink
[Security Solution] Re-enable fixed rule snoozing Cypress test (#160037)
Browse files Browse the repository at this point in the history
**Addresses:** #159349

## Summary

This PR fixes and re-enables rule snoozing Cypress test `Rule editing page / actions tab - adds an action to a snoozed rule`. 

## Details

Basically the test wasn't flaky it just failed on the `main` branch and was skipped. The cause lays in interlacing [Rule snooze tests PR](#158195) with [Rule Editing page refactoring PR](#157749). Two PRs were merged independently to the `main` branch (while the tests PR was merged the last) so combining them lead to a test failure while each one separately didn't cause any problems.

### The root cause

The root cause is in a combination of factors.

[useForm](https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts#L33) hook has a flaw it can't update its state when new `defaultValue` comes in. The same issue also in [useField](https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts#L77) hook. Rule Editing page fetched a fresh rule's data every time it's rendered via [useRule](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx#L581). As `useRule` hook is based on `React Query` it returns stale data during the first render while firing an HTTP request for the fresh data. Obviously the problem happens if the stale data is passed to `useForm` hook as `defaultValue` and when fresh data is fetched and passed again to `useForm` hook the latter just ignores it.

Staying longer on the Rule Details page helps to avoid the problem as fetched rule's data is cached and returned as stale data on the Rule Editing page after transition. As stale and fresh data are the same the test would pass. Anyway this behaviour can be reproduced in prod with a slow internet connection.

### What was done to fix the problem?

Functionality has been added to update the cached rule's data (React Query cache) upon mutation successful update rule mutation. The mutation if fired by pressing "Save changes" button on the Rule Editing page. It is possible as update rule endpoint returns an updated rule in its response.

Along the way it turned out update rule endpoint's and read rule endpoint's responses weren't properly typed so it lead to types mismatch. To fix it `updateRule`'s and `UseMutationOptions`'s return types were changed to `Rule` instead of  `RuleResponse`.

### Misc

Along the way it turned out `cy.get(RULE_INDICES).should('be.visible');` isn't required anymore to access rule actions form.
  • Loading branch information
maximpn authored Aug 11, 2023
1 parent d5c4f46 commit 26e5c20
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import { duplicateFirstRule, importRules } from '../../../../../tasks/alerts_det
import { goToActionsStepTab } from '../../../../../tasks/create_new_rule';
import { goToRuleEditSettings } from '../../../../../tasks/rule_details';
import { actionFormSelector } from '../../../../../screens/common/rule_actions';
import { RULE_INDICES } from '../../../../../screens/create_new_rule';
import { addEmailConnectorAndRuleAction } from '../../../../../tasks/common/rule_actions';
import { saveEditedRule } from '../../../../../tasks/edit_rule';
import { DISABLED_SNOOZE_BADGE } from '../../../../../screens/rule_snoozing';
Expand Down Expand Up @@ -169,17 +168,14 @@ describe('rule snoozing', () => {
});
});

// SKIPPED: https://github.com/elastic/kibana/issues/159349
describe.skip('Rule editing page / actions tab', () => {
describe('Rule editing page / actions tab', () => {
beforeEach(() => {
deleteConnectors();
});

it('adds an action to a snoozed rule', () => {
createSnoozedRule(getNewRule({ name: 'Snoozed rule' })).then(({ body: rule }) => {
visitWithoutDateRange(ruleEditUrl(rule.id));
// Wait for rule data being loaded
cy.get(RULE_INDICES).should('be.visible');
goToActionsStepTab();

addEmailConnectorAndRuleAction('[email protected]', 'Test action');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,15 @@ export const createRule = async ({ rule, signal }: CreateRulesProps): Promise<Ru
* @param rule RuleUpdateProps to be updated
* @param signal to cancel request
*
* @returns Promise<Rule> An updated rule
*
* In fact this function should return Promise<RuleResponse> but it'd require massive refactoring.
* It should be addressed as a part of OpenAPI schema adoption.
*
* @throws An error if response is not OK
*/
export const updateRule = async ({ rule, signal }: UpdateRulesProps): Promise<RuleResponse> =>
KibanaServices.get().http.fetch<RuleResponse>(DETECTION_ENGINE_RULES_URL, {
export const updateRule = async ({ rule, signal }: UpdateRulesProps): Promise<Rule> =>
KibanaServices.get().http.fetch<Rule>(DETECTION_ENGINE_RULES_URL, {
method: 'PUT',
body: JSON.stringify(rule),
signal,
Expand Down Expand Up @@ -198,6 +203,11 @@ export const fetchRules = async ({
* @param id Rule ID's (not rule_id)
* @param signal to cancel request
*
* @returns Promise<Rule>
*
* In fact this function should return Promise<RuleResponse> but it'd require massive refactoring.
* It should be addressed as a part of OpenAPI schema adoption.
*
* @throws An error if response is not OK
*/
export const fetchRuleById = async ({ id, signal }: FetchRuleProps): Promise<Rule> =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,27 @@ export const useInvalidateFetchRuleByIdQuery = () => {
});
}, [queryClient]);
};

/**
* We should use this hook to update the rules cache when modifying a rule.
* Use it with the new rule data after operations like rule edit.
*
* @returns A rules cache update callback
*/
export const useUpdateRuleByIdCache = () => {
const queryClient = useQueryClient();
/**
* Use this method to update rules data cached by react-query.
* It is useful when we receive new rules back from a mutation query (bulk edit, etc.);
* we can merge those rules with the existing cache to avoid an extra roundtrip to re-fetch updated rules.
*/
return useCallback(
(updatedRuleResponse: Rule) => {
queryClient.setQueryData<ReturnType<typeof useFetchRuleByIdQuery>['data']>(
[...FIND_ONE_RULE_QUERY_KEY, updatedRuleResponse.id],
transformInput(updatedRuleResponse)
);
},
[queryClient]
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,43 @@
*/
import type { UseMutationOptions } from '@tanstack/react-query';
import { useMutation } from '@tanstack/react-query';
import type {
RuleResponse,
RuleUpdateProps,
} from '../../../../../common/api/detection_engine/model/rule_schema';
import type { RuleUpdateProps } from '../../../../../common/api/detection_engine/model/rule_schema';
import { transformOutput } from '../../../../detections/containers/detection_engine/rules/transforms';
import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants';
import { updateRule } from '../api';
import { useInvalidateFindRulesQuery } from './use_find_rules_query';
import { useInvalidateFetchRuleByIdQuery } from './use_fetch_rule_by_id_query';
import { useUpdateRuleByIdCache } from './use_fetch_rule_by_id_query';
import { useInvalidateFetchRuleManagementFiltersQuery } from './use_fetch_rule_management_filters_query';
import { useInvalidateFetchCoverageOverviewQuery } from './use_fetch_coverage_overview';
import type { Rule } from '../../logic/types';

export const UPDATE_RULE_MUTATION_KEY = ['PUT', DETECTION_ENGINE_RULES_URL];

export const useUpdateRuleMutation = (
options?: UseMutationOptions<RuleResponse, Error, RuleUpdateProps>
options?: UseMutationOptions<Rule, Error, RuleUpdateProps>
) => {
const invalidateFindRulesQuery = useInvalidateFindRulesQuery();
const invalidateFetchRuleManagementFilters = useInvalidateFetchRuleManagementFiltersQuery();
const invalidateFetchRuleByIdQuery = useInvalidateFetchRuleByIdQuery();
const invalidateFetchCoverageOverviewQuery = useInvalidateFetchCoverageOverviewQuery();
const updateRuleCache = useUpdateRuleByIdCache();

return useMutation<RuleResponse, Error, RuleUpdateProps>(
return useMutation<Rule, Error, RuleUpdateProps>(
(rule: RuleUpdateProps) => updateRule({ rule: transformOutput(rule) }),
{
...options,
mutationKey: UPDATE_RULE_MUTATION_KEY,
onSettled: (...args) => {
invalidateFindRulesQuery();
invalidateFetchRuleByIdQuery();
invalidateFetchRuleManagementFilters();
invalidateFetchCoverageOverviewQuery();

if (options?.onSettled) {
options.onSettled(...args);
const [response] = args;

if (response) {
updateRuleCache(response);
}

options?.onSettled?.(...args);
},
}
);
Expand Down

0 comments on commit 26e5c20

Please sign in to comment.