[Endpoint] Can delete policies #68567

kevinlog · 2020-06-08T20:26:56Z

Summary

Adds ability to delete Policies from the Endpoint management pages.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

kevinlog · 2020-06-09T15:56:30Z

@elasticmachine merge upstream

…add-delete-action

elasticmachine · 2020-06-09T22:47:53Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-06-09T22:48:00Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

kevinlog · 2020-06-10T11:50:36Z

x-pack/plugins/security_solution/public/common/components/endpoint/page_view.tsx

@@ -24,7 +24,7 @@ import { EuiTabProps } from '@elastic/eui/src/components/tabs/tab';

 const StyledEuiPage = styled(EuiPage)`
  &.endpoint--isListView {
-    padding: 0;
+    padding: 0 70px 0 24px;


updated for 2 reasons, to be more uniform with the rest of the security app and to allow functional tests to execute properly as the overlap with the timeline was blocking click events

kevinlog · 2020-06-10T11:51:20Z

x-pack/test/functional_endpoint/apps/endpoint/policy_list.ts

@@ -79,12 +79,14 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
          expect(relativeDate).to.match(RELATIVE_DATE_FORMAT);
        });
      });
-      it('should show policy name as link', async () => {


removed since I check the link is formed correctly in a unit test now

kevinlog · 2020-06-10T12:10:34Z

x-pack/plugins/security_solution/public/common/components/endpoint/page_view.tsx


 const StyledEuiPage = styled(EuiPage)`
  &.endpoint--isListView {
-    padding: 0;
+    padding: 0 ${gutterTimeline} 0 ${(props) => props.theme.eui.euiSizeL};


updated for 2 reasons, to be more uniform with the rest of the security app and to allow functional tests to execute properly as the overlap with the timeline was blocking click events.

We'll be looking to remove the timeline flyout, so we could remove the functional test and add it back in afterwards, but wouldn't we want the tables to look uniform with the rest of the app regardless?

Thoughts @paul-tavares @bfishel

Here's what it looks like

I think this is ok for now. In the future we will likely move way from <PageView> and use SIEMs page and header components -- and/or -- as you said, remove/hide the timeline

nnamdifrankie · 2020-06-10T13:47:50Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_list.test.tsx

+        expect(policyDeleteButton).not.toBeNull();
+      });
+    });
+    it('should display agent config action as link', async () => {


May be good to put spaces between the ITs

kevinlog · 2020-06-10T15:53:58Z

@elasticmachine merge upstream

kevinlog · 2020-06-10T18:26:30Z

@elasticmachine merge upstream

paul-tavares · 2020-06-10T18:56:19Z

...lugins/security_solution/public/management/pages/policy/store/policy_list/services/ingest.ts

+  body: DeleteDatasourcesRequest,
+  options?: HttpFetchOptions
+) => {
+  return http.post<DeleteDatasourcesResponse>(`${INGEST_API_DELETE_DATASOURCE}`, {


optinal:

Suggested change

return http.post<DeleteDatasourcesResponse>(`${INGEST_API_DELETE_DATASOURCE}`, {

return http.post<DeleteDatasourcesResponse>(INGEST_API_DELETE_DATASOURCE, {

paul-tavares · 2020-06-10T19:03:07Z

x-pack/plugins/security_solution/public/management/pages/policy/store/policy_list/middleware.ts

@@ -47,6 +58,45 @@ export const policyListMiddlewareFactory: ImmutableMiddlewareFactory<PolicyListS
          total: response ? response.total : initialPolicyListState().total,
        },
      });
+    } else if (action.type === 'userClickedPolicyListDeleteButton') {
+      const { policyId } = action.payload;
+      const datasourceIds: string[] = [policyId];


should we use the DeleteDatatsourceRequest['body']['datasourceIds'] here case the interface is ever changed?

paul-tavares · 2020-06-10T19:10:48Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_list.test.tsx

+      expect(policyActionsButton).not.toBeNull();
+
+      reactTestingLibrary.fireEvent.click(policyActionsButton);
+      renderResult.findByTestId('agentConfigLink').then((agentConfigLink) => {


This and similar lines below that are using then() I think are causing the it() test to exit before running the expect. I would one of two things: either use a variable assignment along with await or create a Promise.all() and return that so that the test knows to wait.

paul-tavares · 2020-06-10T19:18:55Z

x-pack/test/functional_endpoint/page_objects/policy_page.ts

+      await actionsButton.click();
+      const deleteAction = await testSubjects.find('policyDeleteButton');
+      await deleteAction.click();
+      return await testSubjects.find('policyListDeleteModal');


FYI - thre are a few modal page objects in common that might do some of this here. Example: below, I use pageObjects.common.clickConfirmOnModal() to click on the modal's confirm button

I actually am using that function in one of the tests itself - https://github.com/elastic/kibana/pull/68567/files#diff-c48c00cc17b949e71b8732594dc70734R98

This is more to go through the motions of actually launching the delete modal through the policy actions

paul-tavares · 2020-06-10T19:27:16Z

This looks great @kevinlog . I checked out the branch and ran through 🌞 day scenario and 👍 .
Left a few comment - only the Jest UT comment seems to me to be the most important

…ete-action

kibanamachine · 2020-06-11T17:21:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ba1b649

History

💔 Build #53314 failed 55f08db
💚 Build #53126 succeeded d8c5e79
💔 Build #53077 failed 7af3ce5
💔 Build #53004 failed a85be9e
💔 Build #53000 failed 5a9079a

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

paul-tavares · 2020-06-12T12:30:08Z

x-pack/plugins/security_solution/public/management/pages/policy/store/policy_list/action.ts

@@ -6,6 +6,7 @@

 import { PolicyData } from '../../../../../../common/endpoint/types';
 import { ServerApiError } from '../../../../../common/types';
+import { GetAgentStatusResponse } from '../../../../../../../ingest_manager/common/types/rest_spec';


is this exposed at top level /common? if so, use that path instead.
You can do this in a subsequent PR 😃

paul-tavares · 2020-06-12T12:38:14Z

x-pack/test/security_solution_endpoint/apps/endpoint/policy_list.ts

+        await (await pageObjects.policy.findFirstActionsButton()).click();
+      });
+
+      it('should delete a policy', async () => {


paul-tavares · 2020-06-12T12:42:20Z

x-pack/test/security_solution_endpoint/page_objects/endpoint_page.ts

@@ -33,6 +33,16 @@ export function EndpointPageProvider({ getService, getPageObjects }: FtrProvider
      });
    },

+    async waitForTableToNotHaveData(dataTestSubj: string) {


This is a nice one and I can see us moving this to a page_utils page object in the future as a common way to wait for a table to empty out 👍

Just a small comment: we should add JSDocs to this method. I think normally, the page objects can be reused by other tests (even outside of Endpoint) to integrate/interact with endpoint functionality

paul-tavares · 2020-06-12T12:45:00Z

x-pack/test/security_solution_endpoint/page_objects/policy_page.ts

+    /**
+     * ensures that the Policy Page is the currently display view
+     */
+    async ensureIsOnPolicyPage() {


rename suggestion: ensureIsOnPolicyListPage()?

paul-tavares

Did not run locally again, but changes look good. Left some minor comments that are optional and can be incorporated in future PRs.

Great job on this 👍
🚀

kevinlog added 6 commits June 8, 2020 16:26

Can delete policies

f18f6eb

agent status summary, styling

8d7ae12

fix type error

6375102

fix tests

83a73d0

add middleware tests

237b2e0

add more unit tests, fix i18n

a1c3afb

elasticmachine and others added 4 commits June 9, 2020 08:56

Merge branch 'master' into task/add-delete-action

0aae16b

fix lint

1cf9d77

Merge branch 'master' of https://github.com/elastic/kibana into task/…

fe800d4

…add-delete-action

add functional tests

524547b

kevinlog marked this pull request as ready for review June 9, 2020 22:46

kevinlog requested review from a team as code owners June 9, 2020 22:46

kevinlog changed the title ~~Can delete policies~~ [Endpoint] Can delete policies Jun 9, 2020

kevinlog added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 Feature:Endpoint Elastic Endpoint feature labels Jun 9, 2020

kevinlog added the Team:Endpoint Management label Jun 9, 2020

update snapshot

4efc733

kevinlog commented Jun 10, 2020

View reviewed changes

kevinlog added 2 commits June 10, 2020 07:54

remove unneeded spy

5a9079a

use variables, adjust styles

a85be9e

kevinlog commented Jun 10, 2020

View reviewed changes

nnamdifrankie reviewed Jun 10, 2020

View reviewed changes

Merge branch 'master' into task/add-delete-action

7af3ce5

Merge branch 'master' into task/add-delete-action

d8c5e79

paul-tavares reviewed Jun 10, 2020

View reviewed changes

kevinlog added 3 commits June 11, 2020 08:52

Merge branch 'master' of github.com:kevinlog/kibana into task/add-del…

d3a1f49

…ete-action

adjust test

55f08db

address comments, adjust tests

ba1b649

paul-tavares reviewed Jun 12, 2020

View reviewed changes

nnamdifrankie approved these changes Jun 12, 2020

View reviewed changes

paul-tavares reviewed Jun 12, 2020

View reviewed changes

paul-tavares approved these changes Jun 12, 2020

View reviewed changes

kevinlog merged commit ccf8def into elastic:master Jun 12, 2020

kevinlog deleted the task/add-delete-action branch June 12, 2020 15:28

kevinlog mentioned this pull request Jun 12, 2020

[7.x] [Endpoint] Can delete policies (#68567) #69083

Merged

kevinlog added a commit to kevinlog/kibana that referenced this pull request Jun 12, 2020

[Endpoint] Can delete policies (elastic#68567)

971c688

kevinlog added a commit that referenced this pull request Jun 15, 2020

[Endpoint] Can delete policies (#68567) (#69083)

6bc0888

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Endpoint] Can delete policies #68567

[Endpoint] Can delete policies #68567

kevinlog commented Jun 8, 2020 •

edited

Loading

kevinlog commented Jun 9, 2020

elasticmachine commented Jun 9, 2020

elasticmachine commented Jun 9, 2020

kevinlog Jun 10, 2020

kevinlog Jun 10, 2020

kevinlog Jun 10, 2020

paul-tavares Jun 10, 2020

nnamdifrankie Jun 10, 2020

kevinlog commented Jun 10, 2020

kevinlog commented Jun 10, 2020

paul-tavares Jun 10, 2020

paul-tavares Jun 10, 2020

paul-tavares Jun 10, 2020

paul-tavares Jun 10, 2020 •

edited

Loading

kevinlog Jun 11, 2020

paul-tavares commented Jun 10, 2020 •

edited

Loading

kibanamachine commented Jun 11, 2020

paul-tavares Jun 12, 2020

paul-tavares Jun 12, 2020

paul-tavares Jun 12, 2020

paul-tavares Jun 12, 2020

paul-tavares Jun 12, 2020

paul-tavares left a comment

	return http.post<DeleteDatasourcesResponse>(`${INGEST_API_DELETE_DATASOURCE}`, {
	return http.post<DeleteDatasourcesResponse>(INGEST_API_DELETE_DATASOURCE, {

[Endpoint] Can delete policies #68567

[Endpoint] Can delete policies #68567

Conversation

kevinlog commented Jun 8, 2020 • edited Loading

Summary

Checklist

For maintainers

kevinlog commented Jun 9, 2020

elasticmachine commented Jun 9, 2020

elasticmachine commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlog commented Jun 10, 2020

kevinlog commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Jun 10, 2020 • edited Loading

kibanamachine commented Jun 11, 2020

💚 Build Succeeded

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

kevinlog commented Jun 8, 2020 •

edited

Loading

paul-tavares Jun 10, 2020 •

edited

Loading

paul-tavares commented Jun 10, 2020 •

edited

Loading