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

[SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs #60410

Merged
merged 11 commits into from
Mar 19, 2020

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Mar 17, 2020

Summary

This PR resolves the following All Cases issues from #59041:

  • implement bulk open/close
    bulk

This PR resolves the following Case View issues from #58526:

  • make the user avatar the currentUser for update mode on description/comments and also on new comments
    av

  • change the breadcrumbs to display Case Title instead of Case Id

bc

@stephmilovic stephmilovic added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@stephmilovic stephmilovic changed the title [SIEM] [Case] [skip-ci] Bulk status update, add comment avatar, id => title in breadcrumbs [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs Mar 17, 2020
@stephmilovic stephmilovic marked this pull request as ready for review March 17, 2020 19:44
key={i18n.BULK_ACTION_CLOSE_SELECTED}
icon="magnet"
disabled={true} // TO DO
onClick={async () => {
onClick={() => {
Copy link
Member

Choose a reason for hiding this comment

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

Better to use useCallback to avoid unnecessary re-renders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not in a react component here. FWIW the actions passed here are in their own useCallbacks

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure given that EuiContextMenuItem is a component. @patrykkopycinski ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @stephmilovic is right, but to be sure you can always use why-did-you-render or Profiler :)

key={i18n.BULK_ACTION_DELETE_SELECTED}
icon="trash"
onClick={async () => {
onClick={() => {
Copy link
Member

Choose a reason for hiding this comment

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

Better to use useCallback to avoid unnecessary re-renders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not in a react component here. FWIW the actions passed here are in their own useCallbacks

@cnasikas
Copy link
Member

cnasikas commented Mar 18, 2020

I noticed that when you open a case the title is the ID of the case. After that, it's change to the case's name. I understand why this is happening. Is that ok to leave it like that or is it better to have it empty until the case is loaded?

id_first

selectedCaseIds,
updateCaseStatus,
}: GetBulkItems) => {
return [
caseStatus === 'open' ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice if we have a type of all the caseStatus defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i agree. i can do this in a follow up PR, i have another one up that deals a lot with status right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephmilovic
Copy link
Contributor Author

I noticed that when you open a case the title is the ID of the case. After that, it's change to the case's name. I understand why this is happening. Is that ok to leave it like that or is it better to have it empty until the case is loaded?

id_first

better empty, i made this change. thanks 👍

}
export interface StartPlugins {
data: DataPublicPluginStart;
embeddable: EmbeddableStart;
inspector: InspectorStart;
newsfeed?: NewsfeedStart;
uiActions: UiActionsStart;
security: SecurityPluginSetup;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the Start contract, SecurityPluginStart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SecurityPluginStart does not have the authc we need. It is only in setup. thats why we needed to add it explicitly in the object https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/public/plugin.tsx#L137

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh, I get it now. We're doing some similar stuff on the server, but the basic idea is: store dependencies in setup() to use later in start(). This should be fine for now, but I'll follow up with platform and change it in the NP migration if necessary. 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@stephmilovic stephmilovic merged commit 0163a71 into elastic:master Mar 19, 2020
@stephmilovic stephmilovic deleted the moar-case-ui branch March 19, 2020 23:08
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Mar 19, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master: (52 commits)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410)
  [Alerting] add functional tests for index threshold alertType (elastic#60597)
  [Ingest]EMT-248: add post action request handler and resources (elastic#60581)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
…o alerting/tls-warning

* 'alerting/tls-warning' of github.com:gmmorris/kibana: (32 commits)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410)
  [Alerting] add functional tests for index threshold alertType (elastic#60597)
  [Ingest]EMT-248: add post action request handler and resources (elastic#60581)
  Return incident's url (elastic#60617)
  [Endpoint] TEST: GET alert details - boundary test for first alert retrieval (elastic#60320)
  [ML] Transforms: Fix pivot preview table mapping. (elastic#60609)
  [Endpoint] Log random seed for sample data CLI to console (elastic#60646)
  Use common event model for determining if event is v0 or v1 (elastic#60667)
  Disables PR Project Assigner workflow
  [Reporting] Allow reports to be deleted in Management > Kibana > Reporting (elastic#60077)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants