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.

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

[Security Solution][Endpoint] Port trusted apps to ArtifactlsListPage component #129208

Conversation

ashokaditya
Copy link
Member

@ashokaditya ashokaditya commented Apr 1, 2022

Summary

  • Ported Trusted Apps UI to use ArtifactsListPage component.
  • Updated tests
  • Removed unused components/files
  • Add tests for trusted app form

Checklist

@ashokaditya ashokaditya self-assigned this Apr 1, 2022
@ashokaditya ashokaditya added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.3.0 labels Apr 1, 2022
@ashokaditya ashokaditya force-pushed the task/olm-3092-port-trusted-apps-to-ArtifactListPage-component branch 14 times, most recently from 46d0792 to abc21e7 Compare April 7, 2022 11:22
@ashokaditya ashokaditya force-pushed the task/olm-3092-port-trusted-apps-to-ArtifactListPage-component branch from abc21e7 to 8e14218 Compare April 7, 2022 11:39
@ashokaditya ashokaditya marked this pull request as ready for review April 7, 2022 11:40
@ashokaditya ashokaditya requested review from a team as code owners April 7, 2022 11:40
@ashokaditya ashokaditya force-pushed the task/olm-3092-port-trusted-apps-to-ArtifactListPage-component branch 4 times, most recently from 03ac500 to 880485e Compare April 20, 2022 14:49
without any trusted app entries, it should see the trusted empty page
@ashokaditya ashokaditya force-pushed the task/olm-3092-port-trusted-apps-to-ArtifactListPage-component branch from 880485e to 534efeb Compare April 20, 2022 14:53
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Left a question about a change you made in the newer url_routing utilities, but looks good to me for merge.

Love the amount of modules/code we removed

export const isDefaultOrMissing = <T>(value: T | undefined, defaultValue: T) => {
return value === undefined || value === defaultValue;
export const isDefaultOrMissing = <T>(value: T | undefined | 0, defaultValue: T) => {
return value === undefined || value === defaultValue || value === 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add 0 here? 😕

Note: I'm hopping that these new set of utilities here will handle only the "new" type of our standardized URL params - like page is 1 based, while pages that still use page index should be using the older routing functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, so one of the routing tests was expecting the page to be 0 instead of 1 when the URL params were manually set to the default page (which is 0) or page=0. See here. I'm assuming, in this case, it should be reset to 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should not be using page=0 for new pages, as we agreed that going forward they all should be "1-based". We also should not be using MANAGEMENT_DEFAULT_PAGE because that holds a page index (0-based) and not a page number (1-based). Actually - we probably don't even need a "DEFULT_PAGE" const (I don't remember why we even added it, but it was when we first created the plugin in Kibana)

@ashokaditya ashokaditya enabled auto-merge (squash) April 21, 2022 12:06
@ashokaditya ashokaditya changed the title [Security Solution][Endpoint] Port trusted apps to ArtifactlLstPage component [Security Solution][Endpoint] Port trusted apps to ArtifactlsListPage component Apr 21, 2022
@MadameSheema
Copy link
Member

@elasticmachine merge upstream

@ashokaditya ashokaditya merged commit ef3bd04 into elastic:main Apr 21, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / Alerts timeline Privileges: can crud should allow a user with crud privileges to attach alerts to cases

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2775 2756 -19

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB -22.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 248.1KB 248.0KB -23.0B
Unknown metric groups

async chunk count

id before after diff
securitySolution 24 23 -1

ESLint disabled line counts

id before after diff
securitySolution 449 445 -4

Total ESLint disabled count

id before after diff
securitySolution 518 514 -4

History

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

cc @ashokaditya

ashokaditya added a commit to ashokaditya/kibana that referenced this pull request Apr 26, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
…e` component (elastic#129208)

* move validations to artifacts

fixes elastic/security-team/issues/3092

* use ArtifactsListPage

fixes elastic/security-team/issues/3092

* Remove redundant files

fixes elastic/security-team/issues/3092

* Update trusted app list ftr test

fixes elastic/security-team/issues/3092

* fix test mock

fixes elastic/security-team/issues/3092

* add trusted app form tests

fixes elastic/security-team/issues/3092

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* default page index to 1 when set to 0

refs elastic/pull/129099/commits/b688d505b460964117ba1a303b417760b13a2888

* add tests for new routing methods

refs elastic/pull/129099/

* review changes

fixes elastic/security-team/issues/3092

* update fleet integration test

without any trusted app entries, it should see the trusted empty page

* update translation again after merge

refs 6a792fc

* add a test for search field KQL

review suggestions

* remove redundant flyout size

defaults to `m`

Co-authored-by: kibanamachine <[email protected]>
@ashokaditya ashokaditya deleted the task/olm-3092-port-trusted-apps-to-ArtifactListPage-component branch November 29, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants