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

Jump to bottom

[Cases] Get connector's information from actionsTypeRegistry #101279

Merged
merged 4 commits into from
Jun 8, 2021
Merged

[Cases] Get connector's information from actionsTypeRegistry #101279

merged 4 commits into from
Jun 8, 2021

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Jun 3, 2021 •
edited
Loading

Summary

Current PR resolves the second part of the scope defined for the #95871. Specifically:

  • Removes direct exposure of connector's definition used by Cases.
  • Connector's definition is being accessed through actionsTypeRegistry which is exposed by the triggersActionsUi plugin setup contract.

Adaptation: #100442

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas self-assigned this Jun 3, 2021
@cnasikas cnasikas added 7.14 candidate release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v8.0.0 labels Jun 3, 2021
@cnasikas cnasikas force-pushed the cases_alert_ui_fix branch 2 times, most recently from b339ec9 to 43be09a Compare June 4, 2021 14:06
@cnasikas cnasikas marked this pull request as ready for review June 4, 2021 14:06
@cnasikas cnasikas requested review from a team as code owners June 4, 2021 14:06
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

jonathan-buttner
jonathan-buttner approved these changes Jun 4, 2021
View reviewed changes
x-pack/plugins/cases/public/components/case_view/index.test.tsx
@@ -160,6 +163,10 @@ describe('CaseView ', () => {
pushCaseToExternalService,
}));
useConnectorsMock.mockImplementation(() => ({ connectors: connectorsMock, loading: false }));
useKibanaMock().services.triggersActionsUi.actionTypeRegistry.get = jest.fn().mockReturnValue({
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the tests throughout this PR fail if get returns undefined? I'm just wondering if it's sufficient to jest.mock('../../common/lib/kibana')

Seems like we do this in some places: https:/elastic/kibana/pull/101279/files#diff-94a9922e9734d8791131f05c1124fd6197bdceb756e6bc2770191639b905e169R85

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean! It is not sufficient because of the way the mock is being created (triggersActionsUiMock.createStart() in x-pack/plugins/cases/public/common/lib/kibana/kibana_react.mock.ts). The actionTypeRegistry is not mocked so when the flyout access the actionTypeRegistry to get the connector type you will get an error that the type is not registered.

x-pack/plugins/cases/public/components/configure_cases/mapping.tsx Outdated Show resolved Hide resolved
@YulNaumenko
Copy link
Contributor

@elasticmachine merge upstream

YulNaumenko
YulNaumenko approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Alerting changes LGTM!
Woohoo!
Screen Shot 2021-06-04 at 12 44 33 PM

@YulNaumenko YulNaumenko mentioned this pull request Jun 5, 2021
Open
@cnasikas
Copy link
Member Author

cnasikas commented Jun 7, 2021

@elasticmachine merge upstream

@jonathan-buttner jonathan-buttner deleted the branch elastic:master June 7, 2021 13:37
@cnasikas cnasikas changed the base branch from cases-rbac-poc to master June 7, 2021 14:07
@cnasikas cnasikas requested review from a team as code owners June 7, 2021 14:07
YulNaumenko and others added 3 commits June 7, 2021 17:10
@YulNaumenko @cnasikas
[Alerting UI] Replace direct components links in Cases plugin with th…
4846fae 
…e actionsTypeRegistry exposed by the triggersActionsUi plugin setup contract
@cnasikas
Fix tests
68a4569
@cnasikas
PR Feedback
f62728a
@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jun 7, 2021 •
edited
Loading

@cnasikas I'm not sure why the core team was automatically added as reviewers. From what I can tell, our team isn't a CODEOWNER to any files changed.

@cnasikas cnasikas removed the request for review from a team June 8, 2021 06:38
@cnasikas
Copy link
Member Author

cnasikas commented Jun 8, 2021

@cnasikas I'm not sure why the core team was automatically added as reviewers. From what I can tell, our team isn't a CODEOWNER to any files changed.

You are right. This PR was based on another PR (the RBAC PR) that touched some of your files. The PR got merged and I switched the base to master. It seems that your team stayed as a reviewer. Sorry for the trouble!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 255 254 -1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
triggersActionsUi 232 228 -4

Async chunks

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

id before after diff
cases 502.1KB 503.5KB +1.4KB
triggersActionsUi 1.7MB 1.7MB -9.0B
total +1.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
triggersActionsUi 29 19 -10

Page load bundle

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

id before after diff
cases 133.5KB 132.1KB -1.4KB
triggersActionsUi 73.8KB 71.3KB -2.4KB
total -3.8KB
Unknown metric groups

API count

id before after diff
triggersActionsUi 241 237 -4

History

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

cc @cnasikas

@cnasikas cnasikas merged commit 451f64b into elastic:master Jun 8, 2021
@cnasikas cnasikas deleted the cases_alert_ui_fix branch June 8, 2021 10:35
@cnasikas cnasikas mentioned this pull request Jun 8, 2021
Merged
cnasikas added a commit that referenced this pull request Jun 8, 2021
@cnasikas @YulNaumenko
[Cases] Get connector's information from actionsTypeRegistry (#101279…
919eb94 
…) (#101570)

Co-authored-by: Yuliia Naumenko <[email protected]>

Co-authored-by: Yuliia Naumenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legrego legrego legrego approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@jonathan-buttner jonathan-buttner jonathan-buttner approved these changes

Assignees

@cnasikas cnasikas

Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cnasikas @elasticmachine @YulNaumenko @TinaHeiligers @kibanamachine @legrego @jonathan-buttner