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

[ResponseOps][Cases] User assignment #140208

Merged
merged 24 commits into from
Sep 12, 2022
Merged

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Sep 7, 2022

Issue: #134160

This PR contains the user assignments feature for cases. We have not addressed the feedback noted here:
#139039

This PR encompasses the following PRs that have individually reviewed:
#139441 (Filtering on the cases list page)
#139392 (User activity feed)
#139754 (Support assignment during creation flow)
#138108 (Assigning a user in the single case view)

We will address the user avatars, participants section, licensing and other design improvements in separate PRs to the main branch after this PR is merged.

Examples

Assign and Filtering
feature.branch.demo.mov

Release Notes

This PR allows users to assign users to cases. Multiple users can be assigned to a case. On the case list page, cases can be filtered using assignees. Cases can be assigned on the single case view page or in the case creation flow.

cnasikas and others added 12 commits August 24, 2022 11:26
* Add reusable user profile selector component

* Refactoring services, auth

* Adding suggest api and tests

* Move to package and add examples

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

* Working integration tests

* Switching suggest api tags

* Adding assignees field

* Adding tests for size and owner

* Add server side example

* CI Fixes

* Adding assignee integration tests

* fix tests

* Addd tests

* Starting user actions changes

* Adding api tag tests

* Addressing feedback

* Using lodash for array comparison logic and tests

* Fixing type error

* Create suggest query

* Adding assignees user action

* [ResponseOps][Cases] Refactoring client args and authentication (#137345)

* Refactoring services, auth

* Fixing type errors

* Adding assignees migration and tests

* Fixing types and added more tests

* Fixing cypress test

* Fixing test

* Add tests

* Add security as dependency and fix types

* Add bulk get profiles query

* Rename folder

* Addressed suggestions from code review

* Fix types

* Adding migration for assignees field and tests

* Adding comments and a few more tests

* Updating comments and spelling

* Revert security solution optional

* PR feedback

* Updated user avatar component

* Reduce size

* Make security required

* Fix tests

* Addressing feedback

* Do not retry suggestions

* Assign users to a case

* Restructure components

* Move assignees to case view

* Show assigned users

* Refactoring bulk get and display name

* Adding tests for user tooltip

* Adding tests

* Hovering and tests

* Fixing errors

* Some pr clean up

* Fixing tests and adding more

* Adding functional tests

* Fixing jest test failure

* Passing in current user profile

* Refactoring assignment with useEffect

* Fixing icon alignment and removal render bug

* Fixing type errors

* Fixing test errors

* Adding bulk get privs and tests

* Fixing popover tests

* Handling unknown users

* Adding tests for unknown users

* Adding wait for popover calls

* Addressing design feedback

* Addressing remaining feedback

* Refactoring css and name prop

* Refactoring popover

* Refactoring search component

* Addressing some feedback

* Adjusting sorting

* Fixing tests

* Fixing type error

* Fixing test error

* Fixing test errors

* Removing display name

Co-authored-by: Thom Heymann <[email protected]>
Co-authored-by: Jonathan Buttner <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Thom Heymann <[email protected]>
Co-authored-by: Jonathan Buttner <[email protected]>
* Adding user actions for assignees

* Fixing merge issues

* Fixing test mock errors

* Fixing display name errors and themselves

* Fixing test for time being

* Addressing feedback

* Addressing comma and uniq feedback

* Using core getUserDisplayName

* Fixing import and removing flickering

* Fixing tests

Co-authored-by: Kibana Machine <[email protected]>
* Init

* Render users

* Assign yourself

* Add tests

* Fix tests

* PR feedback
* Starting the filtering

* Rough draft working for assignees filtering

* Adding integration tests for new route

* Starting to write tests

* Fixing tests

* Cleaning up more tests

* Removing duplicate call for current user

* Fixing type errors and tests

* Adding tests for filtering

* Adding rbac tests

* Fixing translations

* Fixing api integration tests

* Fixing severity tests

* Really fixing arrays equal

* Fixing ml tests and refactoring find assignees

* Fixing cypress tests

* Fixing types

* Fix tests

* Addressing first round of feedback

* Reverting the recent cases changes

* Fixing tests

* Fixing more tests and types

* Allowing multi select

* Fixing attachment framework issue

* Addressing feedback
@jonathan-buttner jonathan-buttner added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes v8.5.0 labels Sep 7, 2022
@jonathan-buttner jonathan-buttner marked this pull request as ready for review September 7, 2022 20:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

security-engineering-productivity changes LGTM

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@jonathan-buttner
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@jamster10 jamster10 left a comment

Choose a reason for hiding this comment

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

Good to go from a Security Solution standpoint ✅

@peteharverson
Copy link
Contributor

Logging in as a user with Read privilege on Cases (all 3 solutions), I see this error from the suggest_user_profiles call:

image

labelAppend={OptionalFieldLabel}
helpText={
<EuiLink data-test-subj="create-case-assign-yourself-link" onClick={onSelfAssign}>
{i18n.ASSIGN_YOURSELF}
Copy link
Contributor

Choose a reason for hiding this comment

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

Screen Shot 2022-09-08 at 3 22 36 PM

Assign Yourself link is available even when creating a Case as a User that does not have a User Profile, such as when using the Anonymous Authentication Provider

This link is correctly hidden on the Edit Case view and should probably be hidden here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks for catching that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Changes for platform security LGTM - great work!

As discussed offline, I think it would be beneficial to hide the user assignment ux if security is disabled in Elasticsearch. That shouldn't block this PR from merging though.

@jonathan-buttner
Copy link
Contributor Author

@elasticmachine merge upstream

@cnasikas
Copy link
Member

@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

I have reviewed all PRs that are merged in this PR. I tested and it is looking good to me. Great job 🚀 !

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 460 484 +24

Async chunks

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

id before after diff
cases 344.4KB 366.6KB +22.2KB

Page load bundle

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

id before after diff
cases 122.9KB 123.0KB +115.0B
Unknown metric groups

async chunk count

id before after diff
cases 16 17 +1

ESLint disabled in files

id before after diff
cases 15 16 +1

ESLint disabled line counts

id before after diff
cases 68 67 -1

History

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

@jonathan-buttner jonathan-buttner merged commit e5cc558 into main Sep 12, 2022
@jonathan-buttner jonathan-buttner deleted the cases-user-assignment branch September 12, 2022 11:37
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 12, 2022
@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Sep 12, 2022

Excellent work here, @jonathan-buttner and @cnasikas! I'm a bit late to the party, but did have one note regarding the assignee count in both the filter and assignees popovers.

When there are no assignees on a case or no assignees being filtered, both assignee and assignee filter popovers currently hide the selected count. This can cause the popover to jump around visually. Can we instead always show the assignee count in these two popovers even when the count is zero (0 assigned and 0 assignees filtered respectively)? I know we went back and forth on this a few times, because we previously thought we might be able to include a total count of all available assignees (which would prevent the row of text from being removed entirely and thus prevent the popover jumping). However, since that didn't work out ultimately, I think added back the count when at zero makes sense again.

image

image

@jonathan-buttner
Copy link
Contributor Author

@MichaelMarcialis sounds good will do!

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 Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.