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

Jump to bottom

[Security Solution] Prevent rules table auto-refreshing on window focus when auto-refresh disabled #165250

Merged

maximpn merged 9 commits into elastic:main from maximpn:do-not-auto-refresh-on-window-focus

Sep 14, 2023

Contributor

maximpn commented Aug 30, 2023 •

edited by banderror

Loading

Fixes: #165221

Summary

This PR prevents rules management table from being auto-refreshed on window focus or on reconnect if auto-refresh is disabled.

Before:

Auto-refresh is switched off

Screen.Recording.2023-08-30.at.13.29.41.mov

Auto-refresh is switched off AND a rule is selected

Screen.Recording.2023-08-30.at.13.30.27.mov

After:

Auto-refresh is switched off

Screen.Recording.2023-08-30.at.19.41.51.mov

Auto-refresh is switched off AND a rule is selected

Screen.Recording.2023-08-30.at.19.45.28.mov

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

Flaky test runner

rules_table_auto_refresh.cy.ts (150 runs) 🟢

maximpn added bug release_note:fix Team:Detections and Resp Team: SecuritySolution Feature:Rule Management Team:Detection Rule Management v8.11.0 labels

maximpn self-assigned this

maximpn changed the title ~~[Security Solution] Prevent rules management table auto-refreshing on window focus~~ [Security Solution] Prevent rules table auto-refreshing on window focus when auto-refresh disabled

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from 07a2cde to 1229cb4 Compare

August 30, 2023 17:35

maximpn requested a review from xcrzx

August 30, 2023 17:35

maximpn marked this pull request as ready for review

August 30, 2023 17:36

maximpn requested a review from a team as a code owner

August 30, 2023 17:36

Contributor

elasticmachine commented Aug 30, 2023

Pinging @elastic/security-detections-response (Team:Detections and Resp)

Contributor

elasticmachine commented Aug 30, 2023

Pinging @elastic/security-solution (Team: SecuritySolution)

Contributor Author

maximpn commented Aug 30, 2023

@banderror do we want this to be backported to 8.10?

maximpn marked this pull request as draft

August 30, 2023 20:25

maximpn marked this pull request as ready for review

August 30, 2023 20:33

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from 80375df to 96bcfb2 Compare

August 31, 2023 05:04

Contributor

banderror commented Aug 31, 2023

do we want this to be backported to 8.10?

@maximpn I think it would be too late and risky to backport to 8.10. We have only the last BC left to be built.

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch 8 times, most recently from 2ac265e to e47a4eb Compare

September 8, 2023 07:47

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from e47a4eb to ce81ec2 Compare

September 11, 2023 14:14

xcrzx reviewed

View reviewed changes

Contributor

xcrzx left a comment

I haven't tested the PR locally, but the changes look straightforward 👍
I have a few comments regarding the changes, though.

...tection_engine/rule_management_ui/components/rules_table/rules_table/rules_table_context.tsx Outdated

Comment on lines 299 to 300

    
                    refetchOnWindowFocus: isRefreshOn,

                    refetchOnReconnect: isRefreshOn,

Contributor

xcrzx Sep 12, 2023

Should we also turn off those refetches when an action is underway?

Suggested change

      
                  refetchOnWindowFocus: isRefreshOn,
          
                  refetchOnReconnect: isRefreshOn,
          
                  refetchOnWindowFocus: isRefreshOn && !isActionInProgress,
          
                  refetchOnReconnect: isRefreshOn && !isActionInProgress,

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts Outdated

    
                  it('Auto refreshes rules', () => {

                    mockGlobalClock();

                    visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);

                  it('gets disabled when any rule selected and enabled after rules unselected', () => {

Contributor

xcrzx Sep 12, 2023

Suggested change

      
                it('gets disabled when any rule selected and enabled after rules unselected', () => {
          
                it('gets deactivated when any rule selected and enabled after rules unselected', () => {

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts Outdated

    
                    beforeEach(() => {

                      mockGlobalClock();

                      visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);

                      // waitForPageToBeLoaded();

Contributor

xcrzx Sep 12, 2023

Please remove any commented code.

Contributor Author

maximpn Sep 12, 2023

Hm, most probably it's an artefact of a merge conflict. Removed.

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts Outdated

Comment on lines 104 to 105

    
                      disableAutoRefresh();

                      cy.tick(ONE_MINUTE_IN_MS * 2); // Make sure enough time has passed to verify auto-refresh doesn't happen

Contributor

xcrzx Sep 12, 2023

We should avoid placing test steps in the beforeEach section. That section is designed for preconditions. Even when certain test steps are common across several test cases, it's almost always better to repeat them.

Contributor Author

maximpn Sep 12, 2023

Could you give an explanation why we should avoid it and give examples of preconditions? What in this particular case doesn't correspond to a precondition?

Contributor Author

maximpn Sep 13, 2023

Discussed pros and cons with @xcrzx and determined it'd be better to have disableAutoRefresh() and cy.tick(ONE_MINUTE_IN_MS * 2) in the test body to improve readability and tests clearness.

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts Outdated

Comment on lines 116 to 126

    
                      // Without a delay here the following expectations pass even it shouldn't happen

                      // 'focus' event gets handled in async way so the status gets updated with some delay

                      // eslint-disable-next-line cypress/no-unnecessary-waiting

                      cy.wait(1000);

Contributor

xcrzx Sep 12, 2023

Is this affecting just a single test case? Do we need to add wait to other cases too?

Contributor Author

maximpn Sep 12, 2023

It's required only for the test checking refresh doesn't happen on window focus. I've updated the comment to make clearer.

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts Outdated

    
                    // auto refresh should be deactivated (which means disabled without an ability to enable it) after rules selected

                    expectAutoRefreshIsDeactivated();

                      cy.get(REFRESH_RULES_STATUS).should('not.contain', 'Updating...');

Contributor

xcrzx Sep 12, 2023

This check is redundant; the next one should suffice.

Contributor Author

maximpn Sep 12, 2023

If we could control when refresh happens it'd be really redundant. It can catch an updating state though it's not really transparent. I've replaced it with a comment and a custom timeout for the next expectation. Without a custom timeout it waits according the global configuration so enough time can pass after a refresh so an updated state shows Updated 2 minutes ago and the test passes while it wouldn't catch a problem.

...ss/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts

    
            @@ -30,10 +30,9 @@ import { createRule } from '../../../../tasks/api_calls/rules';
          
              import { cleanKibana } from '../../../../tasks/common';

              import { getNewRule } from '../../../../objects/rule';

              const DEFAULT_RULE_REFRESH_INTERVAL_VALUE = 60000;

Contributor

xcrzx Sep 12, 2023

Let's keep the previous naming. In this context, DEFAULT_RULE_REFRESH_INTERVAL_VALUE provides more clarity than ONE_MINUTE_IN_MS

Contributor

banderror commented Sep 12, 2023

I haven't tested the PR locally, but the changes look straightforward 👍

Hey @xcrzx, I'm sorry for jumping in, but I think it's important.
We're moving towards the serverless reality fast, where the main branch is clean and can be deployed to production anytime. So we should already treat it as clean now, because the serverless prod is already in use by internal customers. We're discussing the implications of this among leads, but for the time being - could you please test this PR manually? I don't think we can afford not to test PRs anymore, unless changes are trivial.

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from ce81ec2 to b31bd46 Compare

September 12, 2023 12:30

maximpn requested a review from xcrzx

September 12, 2023 12:32

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from b31bd46 to da2d6e6 Compare

September 13, 2023 09:19

xcrzx approved these changes

View reviewed changes

Contributor

xcrzx left a comment

We've gone through the comments with @maximpn and discussed the testing practices we use. We've agreed to move the test steps out of the beforeEach section, so I'm giving this PR a 👍

@banderror: The changes in this PR are quite straightforward. However, I've verified them locally as you requested. Found no issues.

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch 2 times, most recently from 7455a95 to ef1d8e5 Compare

September 13, 2023 12:53

maximpn added 9 commits

September 14, 2023 23:46


          do not refresh rules on window focus if auto-refresh is disabled

8fe0ee2


          allow refetching after invalidation when auto-refresh if off

de919ea


          turn off refetches when an action underway

e63388a


          fix test's name

7202f2b


          improve tests readability

6e03650


          improve rules table auto-refresh test suite

f406a41


          update function's description

59d0cc9


          add a kbn reference

25968c6


          set refresh interval setting only once per test suite

cc9c0a9

maximpn force-pushed the do-not-auto-refresh-on-window-focus branch from 363328f to cc9c0a9 Compare

September 14, 2023 21:46

Collaborator

kibana-ci commented Sep 14, 2023

💛 Build succeeded, but was flaky

Buildkite Build
Commit: cc9c0a9

Failed CI Steps

Test Failures

[job] [logs] Explore - Security Solution Cypress Tests #1 / Cases Creates a new case with timeline and opens the timeline Creates a new case with timeline and opens the timeline
[job] [logs] Investigations - Security Solution Cypress Tests #6 / Timeline search and filters Update kqlMode for timeline should be able to update timeline kqlMode with filter should be able to update timeline kqlMode with filter

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	12.6MB	12.6MB	+156.0B

History

💔 Build #158969 failed 363328fd857a75c1d0e61e62045fa9f579e5df11
💔 Build #158686 failed ef1d8e585958969dd5288c26fe7213846b4a2f05
💔 Build #158592 failed da2d6e672535e0e901d9a6ec228d9e4ece70064c
💔 Build #158307 failed b31bd468210062f7842e8df41cbf97318eb1b565
💔 Build #158004 failed ce81ec2b075c9ce04f996bd598c9ec8b3f1a32b3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

maximpn merged commit ca244ec into elastic:main

kibanamachine added the backport:skip label

maximpn deleted the do-not-auto-refresh-on-window-focus branch

September 14, 2023 23:31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip bug Feature:Rule Management release_note:fix Team:Detection Rule Management Team:Detections and Resp Team: SecuritySolution v8.11.0