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

[Security Solution] Updates Timeline tooltips to use action words / fewer words #132756

Merged
merged 2 commits into from
May 24, 2022
Merged

[Security Solution] Updates Timeline tooltips to use action words / fewer words #132756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee5e524
## [Security Solution] Updates Timeline tooltips to use action words …
andrew-goldstein May 23, 2022
ce0c17d
- pr feedback
andrew-goldstein May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion ...lugins/security_solution/public/timelines/components/timeline/body/actions/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { mount } from 'enzyme';
import React from 'react';

import { TestProviders, mockTimelineModel, mockTimelineData } from '../../../../../common/mock';
import { Actions } from '.';
import { Actions, isAlert } from '.';
import { mockTimelines } from '../../../../../common/mock/mock_timelines_plugin';
import { useIsExperimentalFeatureEnabled } from '../../../../../common/hooks/use_experimental_features';
import { mockCasesContract } from '@kbn/cases-plugin/public/mocks';
Expand Down Expand Up @@ -221,4 +221,14 @@ describe('Actions', () => {
expect(wrapper.find('[data-test-subj="view-in-analyzer"]').exists()).toBe(false);
});
});

describe('isAlert', () => {
test('it returns true when the eventType is an alert', () => {
expect(isAlert('signal')).toBe(true);
});

test('it returns false when the eventType is NOT an alert', () => {
expect(isAlert('raw')).toBe(false);
});
});
});
5 changes: 5 additions & 0 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,15 @@ import {
ActionProps,
OnPinEvent,
TimelineTabs,
TimelineEventsType,
} from '../../../../../../common/types/timeline';
import { timelineActions, timelineSelectors } from '../../../../store/timeline';
import { timelineDefaults } from '../../../../store/timeline/defaults';
import { isInvestigateInResolverActionEnabled } from '../../../../../detections/components/alerts_table/timeline_actions/investigate_in_resolver';

export const isAlert = (eventType: TimelineEventsType | Omit<TimelineEventsType, 'all'>): boolean =>
eventType === 'signal';

const ActionsContainer = styled.div`
align-items: center;
display: flex;
Expand Down Expand Up @@ -221,6 +225,7 @@ const ActionsComponent: React.FC<ActionProps> = ({
/>
<PinEventAction
ariaLabel={i18n.PIN_EVENT_FOR_ROW({ ariaRowindex, columnValues, isEventPinned })}
isAlert={isAlert(eventType)}
key="pin-event"
onPinClicked={handlePinClicked}
noteIds={eventIdToNoteIds ? eventIdToNoteIds[eventId] || emptyNotes : emptyNotes}
Expand Down
6 changes: 5 additions & 1 deletion .../security_solution/public/timelines/components/timeline/body/actions/pin_event_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { TimelineType } from '../../../../../../common/types/timeline';

interface PinEventActionProps {
ariaLabel?: string;
isAlert: boolean;
noteIds: string[];
onPinClicked: () => void;
eventIsPinned: boolean;
Expand All @@ -24,6 +25,7 @@ interface PinEventActionProps {

const PinEventActionComponent: React.FC<PinEventActionProps> = ({
ariaLabel,
isAlert,
noteIds,
onPinClicked,
eventIsPinned,
Expand All @@ -33,10 +35,11 @@ const PinEventActionComponent: React.FC<PinEventActionProps> = ({
() =>
getPinTooltip({
isPinned: eventIsPinned,
isAlert,
eventHasNotes: eventHasNotes(noteIds),
timelineType,
}),
[eventIsPinned, noteIds, timelineType]
[eventIsPinned, isAlert, noteIds, timelineType]
);

return (
Expand All @@ -47,6 +50,7 @@ const PinEventActionComponent: React.FC<PinEventActionProps> = ({
ariaLabel={ariaLabel}
allowUnpinning={!eventHasNotes(noteIds)}
data-test-subj="pin-event"
isAlert={isAlert}
onClick={onPinClicked}
pinned={eventIsPinned}
timelineType={timelineType}
Expand Down
4 changes: 2 additions & 2 deletions ...lution/public/timelines/components/timeline/body/column_headers/events_select/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export const getEventsSelectOptions = (): EventsSelectOption[] => [
dropdownDisplay: (
<PinActionItem>
<PinIconContainer>
<Pin allowUnpinning={true} pinned={true} />
<Pin allowUnpinning={true} isAlert={false} pinned={true} />
</PinIconContainer>
<DropdownDisplay text={i18n.PIN_SELECTED} />
</PinActionItem>
Expand All @@ -99,7 +99,7 @@ export const getEventsSelectOptions = (): EventsSelectOption[] => [
dropdownDisplay: (
<PinActionItem>
<PinIconContainer>
<Pin allowUnpinning={true} pinned={false} />
<Pin allowUnpinning={true} isAlert={false} pinned={false} />
</PinIconContainer>
<DropdownDisplay text={i18n.UNPIN_SELECTED} />
</PinActionItem>
Expand Down
90 changes: 83 additions & 7 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,37 +196,113 @@ describe('helpers', () => {
describe('getPinTooltip', () => {
test('it indicates the event may NOT be unpinned when `isPinned` is `true` and the event has notes', () => {
expect(
getPinTooltip({ isPinned: true, eventHasNotes: true, timelineType: TimelineType.default })
getPinTooltip({
isAlert: false,
isPinned: true,
eventHasNotes: true,
timelineType: TimelineType.default,
})
).toEqual('This event cannot be unpinned because it has notes');
});

test('it indicates the alert may NOT be unpinned when `isPinned` is `true` and the alert has notes', () => {
expect(
getPinTooltip({
isAlert: true,
isPinned: true,
eventHasNotes: true,
timelineType: TimelineType.default,
})
).toEqual('This alert cannot be unpinned because it has notes');
});

test('it indicates the event is pinned when `isPinned` is `true` and the event does NOT have notes', () => {
expect(
getPinTooltip({ isPinned: true, eventHasNotes: false, timelineType: TimelineType.default })
).toEqual('Pinned event');
getPinTooltip({
isAlert: false,
isPinned: true,
eventHasNotes: false,
timelineType: TimelineType.default,
})
).toEqual('Unpin event');
});

test('it indicates the alert is pinned when `isPinned` is `true` and the alert does NOT have notes', () => {
expect(
getPinTooltip({
isAlert: true,
isPinned: true,
eventHasNotes: false,
timelineType: TimelineType.default,
})
).toEqual('Unpin alert');
});

test('it indicates the event is NOT pinned when `isPinned` is `false` and the event has notes', () => {
expect(
getPinTooltip({ isPinned: false, eventHasNotes: true, timelineType: TimelineType.default })
).toEqual('Unpinned event');
getPinTooltip({
isAlert: false,
isPinned: false,
eventHasNotes: true,
timelineType: TimelineType.default,
})
).toEqual('Pin event');
});

test('it indicates the alert is NOT pinned when `isPinned` is `false` and the alert has notes', () => {
expect(
getPinTooltip({
isAlert: true,
isPinned: false,
eventHasNotes: true,
timelineType: TimelineType.default,
})
).toEqual('Pin alert');
});

test('it indicates the event is NOT pinned when `isPinned` is `false` and the event does NOT have notes', () => {
expect(
getPinTooltip({ isPinned: false, eventHasNotes: false, timelineType: TimelineType.default })
).toEqual('Unpinned event');
getPinTooltip({
isAlert: false,
isPinned: false,
eventHasNotes: false,
timelineType: TimelineType.default,
})
).toEqual('Pin event');
});

test('it indicates the alert is NOT pinned when `isPinned` is `false` and the alert does NOT have notes', () => {
expect(
getPinTooltip({
isAlert: true,
isPinned: false,
eventHasNotes: false,
timelineType: TimelineType.default,
})
).toEqual('Pin alert');
});

test('it indicates the event is disabled if timelineType is template', () => {
expect(
getPinTooltip({
isAlert: false,
isPinned: false,
eventHasNotes: false,
timelineType: TimelineType.template,
})
).toEqual('This event may not be pinned while editing a template timeline');
});

test('it indicates the alert is disabled if timelineType is template', () => {
expect(
getPinTooltip({
isAlert: true,
isPinned: false,
eventHasNotes: false,
timelineType: TimelineType.template,
})
).toEqual('This alert may not be pinned while editing a template timeline');
});
});

describe('eventIsPinned', () => {
Expand Down
21 changes: 13 additions & 8 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,27 @@ export const stringifyEvent = (ecs: Ecs): string => JSON.stringify(ecs, omitType
export const eventHasNotes = (noteIds: string[]): boolean => !isEmpty(noteIds);

export const getPinTooltip = ({
isAlert,
isPinned,
// eslint-disable-next-line @typescript-eslint/no-shadow
eventHasNotes,
timelineType,
}: {
isAlert: boolean;
isPinned: boolean;
eventHasNotes: boolean;
timelineType: TimelineTypeLiteral;
}) =>
timelineType === TimelineType.template
? i18n.DISABLE_PIN
: isPinned && eventHasNotes
? i18n.PINNED_WITH_NOTES
: isPinned
? i18n.PINNED
: i18n.UNPINNED;
}) => {
if (timelineType === TimelineType.template) {
return i18n.DISABLE_PIN(isAlert);
} else if (isPinned && eventHasNotes) {
return i18n.PINNED_WITH_NOTES(isAlert);
} else if (isPinned) {
return i18n.PINNED(isAlert);
} else {
return i18n.UNPINNED(isAlert);
}
Comment on lines +42 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up, it's much more readable now 👍

};

export interface IsPinnedParams {
eventId: string;
Expand Down
47 changes: 24 additions & 23 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/translations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
import { i18n } from '@kbn/i18n';

export const NOTES_TOOLTIP = i18n.translate(
'xpack.securitySolution.timeline.body.notes.addOrViewNotesForThisEventTooltip',
'xpack.securitySolution.timeline.body.notes.addNoteTooltip',
{
defaultMessage: 'Add notes for this event',
defaultMessage: 'Add note',
}
);

Expand Down Expand Up @@ -42,23 +42,24 @@ export const INVESTIGATE = i18n.translate(
}
);

export const UNPINNED = i18n.translate(
'xpack.securitySolution.timeline.body.pinning.unpinnedTooltip',
{
defaultMessage: 'Unpinned event',
}
);
export const UNPINNED = (isAlert: boolean) =>
i18n.translate('xpack.securitySolution.timeline.body.pinning.pinTooltip', {
values: { isAlert },
defaultMessage: 'Pin {isAlert, select, true{alert} other{event}}',
});

export const PINNED = i18n.translate('xpack.securitySolution.timeline.body.pinning.pinnedTooltip', {
defaultMessage: 'Pinned event',
});
export const PINNED = (isAlert: boolean) =>
i18n.translate('xpack.securitySolution.timeline.body.pinning.unpinTooltip', {
values: { isAlert },
defaultMessage: 'Unpin {isAlert, select, true{alert} other{event}}',
});

export const PINNED_WITH_NOTES = i18n.translate(
'xpack.securitySolution.timeline.body.pinning.pinnnedWithNotesTooltip',
{
defaultMessage: 'This event cannot be unpinned because it has notes',
}
);
export const PINNED_WITH_NOTES = (isAlert: boolean) =>
i18n.translate('xpack.securitySolution.timeline.body.pinning.pinnnedWithNotesTooltip', {
values: { isAlert },
defaultMessage:
'This {isAlert, select, true{alert} other{event}} cannot be unpinned because it has notes',
});

export const SORTED_ASCENDING = i18n.translate(
'xpack.securitySolution.timeline.body.sort.sortedAscendingTooltip',
Expand All @@ -74,12 +75,12 @@ export const SORTED_DESCENDING = i18n.translate(
}
);

export const DISABLE_PIN = i18n.translate(
'xpack.securitySolution.timeline.body.pinning.disablePinnnedTooltip',
{
defaultMessage: 'This event may not be pinned while editing a template timeline',
}
);
export const DISABLE_PIN = (isAlert: boolean) =>
i18n.translate('xpack.securitySolution.timeline.body.pinning.disablePinnnedTooltip', {
values: { isAlert },
defaultMessage:
'This {isAlert, select, true{alert} other{event}} may not be pinned while editing a template timeline',
});

export const VIEW_DETAILS = i18n.translate(
'xpack.securitySolution.timeline.body.actions.viewDetailsAriaLabel',
Expand Down