-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add assignee user actions #139392
[ResponseOps][Cases] Add assignee user actions #139392
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
Jenkins, test this |
@elasticmachine merge upstream |
x-pack/plugins/cases/public/components/user_actions/assignees.tsx
Outdated
Show resolved
Hide resolved
grow={false} | ||
key={assignee.uid} | ||
> | ||
<EuiText size="s" className="eui-textBreakWord"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't reproduce this one a second time! Maybe some stale state?
}); | ||
|
||
return ( | ||
<EuiFlexGroup alignItems="baseline" gutterSize="xs" component="span" responsive={false}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the mocks are showing the display_name
, we don't have that within elasticsearch yet so the fallback order is full name -> email -> username
Once we have support for display name the user can customize the name that is displayed.
Not directly related to the changes here, but the current behavior of the 'Edit assignees' is somewhat confusing in my opinion, in that it doesn't show any users if there is no search text entered. The first time I saw this, I thought it was broken and couldn't find the users I have set up: As a minimum, could we show the currently logged in user as a suggestion? |
x-pack/plugins/cases/public/components/user_actions/assignees.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this another test and looks good - the only extra change I suggested is to wrap the list of assignees #139392 (comment)
I agree, this was also confusing to me and I think pre-loading some users would be helpful. It also seems that if you start typing, it briefly shows a "No matching users with required access" message before loading the suggestions. Can we add a loading state? Otherwise users with slow network connections might just think there are no assignable users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested assigning and unassigning different combos of users and the activity showed up in the case. Left a comment about the assign dropdown but I think that's unrelated to this PR.
Also I see that each the assign/unassign action has a reference link at the end that just links back to the case. Is that link supposed to go somewhere else? If it is just the link to the case, does it make sense to show it when you're on the case already? Probably not related to this PR either tho :)
@elasticmachine merge upstream |
full_name: rt.union([rt.undefined, rt.null, rt.string]), | ||
username: rt.union([rt.undefined, rt.null, rt.string]), | ||
}), | ||
rt.partial({ profile_uid: rt.string }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference betweenrt.partial({ profile_uid: rt.string })
and profile_uid: rt.union([rt.undefined, rt.null, rt.string])
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using a partial
we don't have to set the profile_uid
wherever a user object is defined. That way we can avoid updating all the locations in the code where we create a user object. I believe we can also avoid writing a migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. Thanks!
}); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use a useEffect
to derive the uids. You can do:
const uidsToRetrieve = uniq([...userActionsData?.profileUids ?? [], ...assignees]);
const { data: userProfiles, isLoading: isLoadingUserProfiles } = useBulkGetUserProfiles({
uids: uidsToRetrieve,
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, just to make sure I understand, let's say useGetCaseUserActions
initially returns undefined
because it's loading, we'll call useBulkGetUserProfiles
with only the assignees
. Once useGetCaseUserActions
is finished loading, the component will rerender and it'll automatically call useBulkGetUserProfiles
with the updated uidsToRetrieve
if we don't include a useEffect
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! Usually, you do need a useEffect
to derive the state from props or functions. These articles are great to read about the topic: https://tkdodo.eu/blog/dont-over-use-state and https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
}); | ||
|
||
export const AND_SPACE = i18n.translate('xpack.cases.caseView.assignee.and', { | ||
defaultMessage: 'and ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe is better to remove the space and put it in the code manually.
import * as i18n from './translations'; | ||
|
||
export const getName = (user?: UserProfileUserInfo): string => { | ||
if (!user) { | ||
return i18n.UNKNOWN; | ||
} | ||
|
||
return user.full_name ?? user.email ?? user.username; | ||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the exposed getUserDisplayName
function from the security plugin for consistency? (#139091)
import * as i18n from './translations'; | ||
import { getUsernameDataTestSubj } from '../user_profiles/data_test_subject'; | ||
|
||
const FormatListItem: React.FC<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
const uids = userActions.reduce<string[]>((acc, userAction) => { | ||
if (userAction.type === ActionTypes.assignees) { | ||
const uidsFromPayload = userAction.payload.assignees.map((assignee) => assignee.uid); | ||
acc.push(...uidsFromPayload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we convert it to a set to avoid duplicates?
it('renders assigned users', () => { | ||
const userAction = getUserAction('assignees', Actions.add, { | ||
createdBy: { | ||
// damaged_raccoon uid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
[ | ||
USER_PROFILES_CACHE_KEY, | ||
USER_PROFILES_SUGGEST_CACHE_KEY, | ||
{ name: debouncedName, owners, size }, | ||
], | ||
() => { | ||
if (isEmpty(name)) { | ||
return []; | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I try to return undefined
I was getting an error that onSuccess
can't receive undefined 🤷♂️
@@ -173,7 +173,7 @@ const SuggestUsersPopoverComponent: React.FC<SuggestUsersPopoverProps> = ({ | |||
searchPlaceholder: i18n.SEARCH_USERS, | |||
clearButtonLabel: i18n.REMOVE_ASSIGNEES, | |||
emptyMessage: <EmptyMessage />, | |||
noMatchesMessage: <NoMatches />, | |||
noMatchesMessage: searchResultProfiles ? <NoMatches /> : <EmptyMessage />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cnasikas I don't understand why, but as soon as I type something if the results passed to options
are undefined
or null
it defaults to showing the noMatchesMessage
. Maybe that's how it's supposed to work where it only shows emptyMessage
if you haven't typed something in the search box? This is how I was able to get the no results flickering to stop.
Even with this and the isFetching
change it doesn't show the loading state for very long. I also tried just setting isLoading
to true
and adding a spinner for the loadingMessage
. I don't see the loadingMessage
being displayed and the only thing that changes is that there's a spinner to the right of the searchbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Let's merge this PR and resolve the issue on another PR.
@elasticmachine merge upstream |
There was a problem hiding this 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
💔 Build FailedFailed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
* [ResponseOps][Cases] Assign users on cases sidebar section (#138108) * 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]> * [ResponseOps][Cases] Add assignee user actions (#139392) * 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]> * Detect when user is typing * Use select to tranform data * [Cases] Assign users when creating a case (#139754) * Init * Render users * Assign yourself * Add tests * Fix tests * PR feedback * [ResponseOps][Cases] Filter by assignees (#139441) * 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 * Fixing type error * Fixing tests * Sort users and improve loading * Fix ml security dependecies * Fix permissions when fetching suggestions * Fixing read permissions and suggest api * Hiding assignee delete icon * Hiding the assign yourself text when undefined * Create page fixes Co-authored-by: Christos Nasikas <[email protected]> Co-authored-by: Thom Heymann <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Thom Heymann <[email protected]>
* [ResponseOps][Cases] Assign users on cases sidebar section (#138108) * 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]> * [ResponseOps][Cases] Add assignee user actions (#139392) * 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]> * Detect when user is typing * Use select to tranform data * [Cases] Assign users when creating a case (#139754) * Init * Render users * Assign yourself * Add tests * Fix tests * PR feedback * [ResponseOps][Cases] Filter by assignees (#139441) * 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 * Fixing type error * Fixing tests * Refactoring components * Refactoring the users lists * Switching all user action avatars * Fixing merge issues * Fixing types * Addressing feedback * Fixing type errors Co-authored-by: Christos Nasikas <[email protected]> Co-authored-by: Thom Heymann <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Thom Heymann <[email protected]>
* [ResponseOps][Cases] Assign users on cases sidebar section (#138108) * 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]> * [ResponseOps][Cases] Add assignee user actions (#139392) * 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]> * Detect when user is typing * Use select to tranform data * [Cases] Assign users when creating a case (#139754) * Init * Render users * Assign yourself * Add tests * Fix tests * PR feedback * [ResponseOps][Cases] Filter by assignees (#139441) * 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 * Fixing type error * Fixing tests * Sort users and improve loading * Fix ml security dependecies * Fix permissions when fetching suggestions * Fixing read permissions and suggest api * Hiding assignee delete icon * Hiding the assign yourself text when undefined * Show licensing callout to the all cases page * Hide assignees column * Hide assignees & connectors column * Renames * Check licensing when assigning users to a case * Hide assinee from create case page * Hide assignee filtering * Check license when filtering by assignees * Add UI tests & fixes * Add integration tests * Fix tests * Fix i18n * Revert core changes * PR feedback and fix tests * PR feedback 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]>
This PR adds assigning and unassigning of users to the user actions activity view within Cases.
NOTE: This PR does not address the that the user making the action is shown differently than the assignee/unassigned activity. This will be done in a separate PR since it requires backend work and should be addressed throughout the entire cases plugin.
User activity example
User activity tooltip