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

[Cases] Backlink to cases in external services #143174

Merged
merged 9 commits into from Oct 14, 2022

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Oct 12, 2022

Summary

This PR adds a backlink to cases in the description of the case when pushing to an external service. It also removes unnecessary code.

Fixes: #141059, #141061

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release notes

Adds a backlink to cases when pushing in external services.

@cnasikas cnasikas added backport:skip This commit does not require backporting 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.6.0 labels Oct 12, 2022
@cnasikas cnasikas self-assigned this Oct 12, 2022
@cnasikas cnasikas requested a review from a team as a code owner October 12, 2022 10:43
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@@ -19,14 +26,6 @@ export type CaseViewPathParams = {
commentId?: string;
} & CaseViewPathSearchParams;

export const CASES_CREATE_PATH = '/create' as const;
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to common folder

@@ -96,9 +97,6 @@ function getToastContent({
return undefined;
}

const isValidOwner = (owner: string): owner is keyof typeof OWNER_INFO =>
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to common folder

@@ -39,8 +39,3 @@ export interface OnUpdateFields {
onSuccess?: () => void;
onError?: () => void;
}

export enum CASE_VIEW_PAGE_TABS {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to common folder

const alerts = await getAlerts(alertsInfo, clientArgs);

const getMappingsResponse = await casesClientInternal.configuration.getMappings({
Copy link
Member Author

Choose a reason for hiding this comment

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

Mapping is hard coded for the moment. I removed unnecessary calls and code. I believe there is no need to maintain and support code that is not used by users.

connector: theCase.connector,
});

const mappings =
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here.


const externalServiceFields =
casesConnectors.get(connector.actionTypeId)?.format(theCase, alerts) ?? {};

let incident: Partial<PushToServiceApiParams['incident']> = { ...externalServiceFields };
const connectorMappings = casesConnectors.get(connector.actionTypeId)?.getMapping() ?? [];
Copy link
Member Author

Choose a reason for hiding this comment

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

As the mappings are hard-coded, we get them from the connector's map.


if (externalId) {
Copy link
Member Author

@cnasikas cnasikas Oct 12, 2022

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 do a call to an external service. This was needed in case we need to append the case description to the description of the external service. We never supported this feature so I removed the unnecessary call.

Comment on lines 209 to 220
const fields = prepareFieldsForTransformation({
defaultPipes,
mappings,
params,
});
const mappedIncident = mapCaseFieldsToExternalSystemFields(
{ title: theCase.title, description: descriptionWithKibanaInformation },
connectorMappings
);

const transformedFields = transformFields<BasicParams, ExternalServiceParams, Incident>({
params,
fields,
currentIncident,
userProfiles,
});
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 removed the unnecessary transformation of fields. We always overwrite the content with our values. If we want to support different actions in the feature we can code it better (I coded this hahahaha. I don't like it 😄 ).


const caseUrl = getCaseViewPath({ publicBaseUrl, caseId: theCase.id, owner: theCase.owner });

return `${descriptionWithKibanaInformation}\nFor more details view this case in Kibana.\nCase URL: ${caseUrl}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would read better with a , after details

For more details, view this case in Kibana.

or consider making the link part of the message

For more details, <<view this case in Kibana>>.

And can that message be message be internationalized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also testing on IBM Security QRadar SOAR, the text isn't rendered as a link (whereas it is in e.g. Jira)

Copy link
Member Author

Choose a reason for hiding this comment

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

Each external service renders the content differently. All external services escape the content before showing it for security reasons. Some of them support the markdown format and some of them have their own syntax (Jira for example). For this reason, we decided with @shanisagiv1 to send only the URL as text and leave the interpretation of the link to the external services. Btw, I added the comma to the text and I translated the message.

Copy link
Member Author

Choose a reason for hiding this comment

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

@peteharverson @lcawl I added the following text in the total alerts comments. What do you think?

Screenshot 2022-10-13 at 12 02 57 PM

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 the clarification @cnasikas - I see we are limited on formatting the link here as it will be the external service that displays the content. So the change to the text, and internationalization, LGTM.

@cnasikas cnasikas requested a review from a team as a code owner October 13, 2022 08:35
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 functionally LGTM

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

I tested with Jira, looks good. Left a few nit comments.

@@ -1011,32 +877,26 @@ describe('utils', () => {
it('returns an empty string with neither updatedBy or createdBy are defined', () => {
expect(
getEntity({
createdAt: '',
// @ts-expect-error createdBy should be defined but for testing purposes we want to make sure the function handles null
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the test names to say returns Unknown...? There are a few tests below this one that would need to be changed too.

userProfiles?: Map<string, UserProfile>
): string => {
return (
getDisplayName(entity.updatedBy, userProfiles) ??
getDisplayName(entity.createdBy, userProfiles) ??
''
'Unknown'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we translate the string Unknown?

commentId?: string;
tabId?: CASE_VIEW_PAGE_TABS;
}): string => {
const normalizePath = (path: string): string => path.replaceAll('//', '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

If we used the URL class would we be able to avoid removing trailing slashing and the normalizing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, the URL class keeps the double slashes. For example:

node > new URL('https://e.com//api//cases')
URL {
  href: 'https://e.com//api//cases',
  origin: 'https://e.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'e.com',
  hostname: 'e.com',
  port: '',
  pathname: '//api//cases',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

@cnasikas cnasikas enabled auto-merge (squash) October 14, 2022 07:32
@cnasikas cnasikas merged commit c5f6670 into elastic:main Oct 14, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 500 501 +1

Async chunks

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

id before after diff
cases 372.3KB 372.3KB -14.0B

Page load bundle

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

id before after diff
cases 125.5KB 125.8KB +338.0B

History

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

cc @cnasikas

@cnasikas cnasikas deleted the backlink_to_cases branch October 14, 2022 08:24
@cnasikas cnasikas added release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels Nov 17, 2022
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:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
6 participants