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] File user action in Case detail view #153957

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/common/api/cases/comment/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const FileAttachmentMetadataRt = rt.type({
name: rt.string,
extension: rt.string,
mimeType: rt.string,
createdAt: rt.string,
created: rt.string,
})
),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface AttachmentViewObject<Props = {}> {
actions?: EuiCommentProps['actions'];
event?: EuiCommentProps['event'];
children?: React.LazyExoticComponent<React.FC<Props>>;
hideDefaultActions?: boolean;
}

export interface CommonAttachmentViewProps {
Expand All @@ -38,7 +39,9 @@ export interface AttachmentType<Props> {
id: string;
icon: IconType;
displayName: string;
getAttachmentViewObject: () => AttachmentViewObject<Props>;
getAttachmentViewObject: (
props: ExternalReferenceAttachmentViewProps
adcoelho marked this conversation as resolved.
Show resolved Hide resolved
) => AttachmentViewObject<Props>;
}

export type ExternalReferenceAttachmentType = AttachmentType<ExternalReferenceAttachmentViewProps>;
Expand Down
47 changes: 23 additions & 24 deletions x-pack/plugins/cases/public/components/files/add_file.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,31 +136,30 @@ describe('AddFile', () => {
userEvent.click(await screen.findByTestId('testOnDone'));

await waitFor(() =>
expect(createAttachmentsMock).toBeCalledWith(
expect.objectContaining({
caseId: 'foobar',
caseOwner: mockedTestProvidersOwner[0],
data: [
{
externalReferenceAttachmentTypeId: '.files',
externalReferenceId: mockedExternalReferenceId,
externalReferenceMetadata: {
files: [
{
createdAt: '2020-02-19T23:06:33.798Z',
extension: 'png',
mimeType: 'image/png',
name: 'my-super-cool-screenshot',
},
],
},
externalReferenceStorage: { soType: 'file', type: 'savedObject' },
type: 'externalReference',
expect(createAttachmentsMock).toBeCalledWith({
caseId: 'foobar',
caseOwner: mockedTestProvidersOwner[0],
data: [
{
externalReferenceAttachmentTypeId: '.files',
externalReferenceId: mockedExternalReferenceId,
externalReferenceMetadata: {
files: [
{
created: '2020-02-19T23:06:33.798Z',
extension: 'png',
mimeType: 'image/png',
name: 'my-super-cool-screenshot',
},
],
},
],
throwOnError: true,
})
)
externalReferenceStorage: { soType: 'file', type: 'savedObject' },
type: 'externalReference',
},
],
throwOnError: true,
updateCase: expect.any(Function),
})
);

await waitFor(() =>
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/public/components/files/add_file.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const AddFileComponent: React.FC<AddFileProps> = ({ caseId }) => {
name: file.fileJSON.name,
extension: file.fileJSON.extension ?? '',
mimeType: file.fileJSON.mimeType ?? '',
createdAt: file.fileJSON.created,
created: file.fileJSON.created,
},
],
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { screen } from '@testing-library/react';

import type { AppMockRenderer } from '../../common/mock';
import {
createAppMockRenderer,
mockedFilesClient,
mockedTestProvidersOwner,
} from '../../common/mock';
import { FileDownloadButtonIcon } from './file_download_button_icon';
import { basicFileMock } from '../../containers/mock';
import { constructFileKindIdByOwner } from '../../../common/constants';

describe('FileDownloadButtonIcon', () => {
let appMockRender: AppMockRenderer;

beforeEach(() => {
jest.clearAllMocks();
appMockRender = createAppMockRenderer();
});

it('renders download button with correct href', async () => {
appMockRender.render(<FileDownloadButtonIcon fileId={basicFileMock.id} />);

expect(await screen.findByTestId('cases-files-download-button')).toBeInTheDocument();

expect(mockedFilesClient.getDownloadHref).toBeCalledTimes(1);
expect(mockedFilesClient.getDownloadHref).toHaveBeenCalledWith({
fileKind: constructFileKindIdByOwner(mockedTestProvidersOwner[0]),
id: basicFileMock.id,
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import { EuiButtonIcon } from '@elastic/eui';
import { useFilesContext } from '@kbn/shared-ux-file-context';

import type { Owner } from '../../../common/constants/types';

import { constructFileKindIdByOwner } from '../../../common/constants';
import { useCasesContext } from '../cases_context/use_cases_context';
import * as i18n from './translations';

interface FileDownloadButtonIconProps {
fileId: string;
}

const FileDownloadButtonIconComponent: React.FC<FileDownloadButtonIconProps> = ({ fileId }) => {
const { owner } = useCasesContext();
const { client: filesClient } = useFilesContext();

return (
<EuiButtonIcon
iconType={'download'}
aria-label={i18n.DOWNLOAD}
href={filesClient.getDownloadHref({
Copy link
Member

@cnasikas cnasikas Mar 31, 2023

Choose a reason for hiding this comment

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

I noticed through the reviews of the files PRs that we use this pattern a lot. Should we extract the logic of the construction of the href to a util function? For example:

const getFileDownloadHref = ({ filesClient, fileId, owner }) => filesClient.getDownloadHref({
        fileKind: constructFileKindIdByOwner(owner[0] as Owner),
        id: fileId,
      })

and use it like:

href={getFileDownloadHref( { filesClient, fileId, owner: owner[0] } )}

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 can but I don't see much difference between

filesClient.getDownloadHref(A, B)

and

newFunction(filesClient.getDownloadHref, A, B) 😄

Copy link
Member

Choose a reason for hiding this comment

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

The only difference is this Cases logic: fileKind: constructFileKindIdByOwner(owner[0] as Owner) is replicated over and over again. If we need to change it in the future we have to go to all places to do it. I am fine with leaving it as it is. Not a big deal. I get your point.

fileKind: constructFileKindIdByOwner(owner[0] as Owner),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah bummer that we need the cast, maybe constructFileKindIdByOwner should just take a string 🤔

id: fileId,
})}
data-test-subj={'cases-files-download-button'}
/>
);
};
FileDownloadButtonIconComponent.displayName = 'FileDownloadButtonIcon';

export const FileDownloadButtonIcon = React.memo(FileDownloadButtonIconComponent);
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { screen, waitFor } from '@testing-library/react';

import type { AppMockRenderer } from '../../common/mock';
import { createAppMockRenderer } from '../../common/mock';
import userEvent from '@testing-library/user-event';
import { FileNameLink } from './file_name_link';
import { basicFileMock } from '../../containers/mock';

describe('FileNameLink', () => {
let appMockRender: AppMockRenderer;

const defaultProps = {
file: basicFileMock,
showPreview: jest.fn(),
};

beforeEach(() => {
jest.clearAllMocks();
appMockRender = createAppMockRenderer();
});

it('renders clickable name if file is image', async () => {
appMockRender.render(<FileNameLink {...defaultProps} />);

const nameLink = await screen.findByTestId('cases-files-name-link');

expect(nameLink).toBeInTheDocument();

userEvent.click(nameLink);

await waitFor(() => expect(defaultProps.showPreview).toHaveBeenCalled());
});

it('renders simple text name if file is not image', async () => {
appMockRender.render(
<FileNameLink
showPreview={defaultProps.showPreview}
file={{ ...basicFileMock, mimeType: 'text/csv' }}
/>
);

const nameLink = await screen.findByTestId('cases-files-name-text');

expect(nameLink).toBeInTheDocument();

userEvent.click(nameLink);

await waitFor(() => expect(defaultProps.showPreview).not.toHaveBeenCalled());
});
});
40 changes: 40 additions & 0 deletions x-pack/plugins/cases/public/components/files/file_name_link.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import { EuiLink } from '@elastic/eui';
import type { FileJSON } from '@kbn/shared-ux-file-types';

import * as i18n from './translations';
import { isImage } from './utils';

interface FileNameLinkProps {
file: FileJSON;
showPreview: () => void;
}

const FileNameLinkComponent: React.FC<FileNameLinkProps> = ({ file, showPreview }) => {
const fileName = `${file.name}.${file.extension}`;

if (isImage(file)) {
return (
<EuiLink onClick={showPreview} data-test-subj="cases-files-name-link">
{fileName}
</EuiLink>
);
} else {
return (
<span title={i18n.NO_PREVIEW} data-test-subj="cases-files-name-text">
{fileName}
</span>
);
}
};
FileNameLinkComponent.displayName = 'FileNameLink';

export const FileNameLink = React.memo(FileNameLinkComponent);
18 changes: 7 additions & 11 deletions x-pack/plugins/cases/public/components/files/file_preview.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import { screen, waitFor } from '@testing-library/react';
import type { AppMockRenderer } from '../../common/mock';

import { constructFileKindIdByOwner } from '../../../common/constants';
import { createAppMockRenderer, mockedTestProvidersOwner } from '../../common/mock';
import {
createAppMockRenderer,
mockedTestProvidersOwner,
mockedFilesClient,
} from '../../common/mock';
import { basicFileMock } from '../../containers/mock';
import { FilePreview } from './file_preview';

Expand All @@ -24,18 +28,10 @@ describe('FilePreview', () => {
});

it('FilePreview rendered correctly', async () => {
const mockGetDownloadRef = jest.fn();

appMockRender.render(
<FilePreview
closePreview={jest.fn()}
selectedFile={basicFileMock}
getDownloadHref={mockGetDownloadRef}
/>
);
appMockRender.render(<FilePreview closePreview={jest.fn()} selectedFile={basicFileMock} />);

await waitFor(() =>
expect(mockGetDownloadRef).toBeCalledWith({
expect(mockedFilesClient.getDownloadHref).toHaveBeenCalledWith({
id: basicFileMock.id,
fileKind: constructFileKindIdByOwner(mockedTestProvidersOwner[0]),
})
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugins/cases/public/components/files/file_preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import styled from 'styled-components';
import type { FileJSON } from '@kbn/shared-ux-file-types';

import { EuiOverlayMask, EuiFocusTrap, EuiImage } from '@elastic/eui';
import { useFilesContext } from '@kbn/shared-ux-file-context';

import type { Owner } from '../../../common/constants/types';

Expand All @@ -18,7 +19,6 @@ import { useCasesContext } from '../cases_context/use_cases_context';

interface FilePreviewProps {
closePreview: () => void;
getDownloadHref: (args: Pick<FileJSON<unknown>, 'id' | 'fileKind'>) => string;
selectedFile: FileJSON;
}

Expand All @@ -32,7 +32,8 @@ const StyledOverlayMask = styled(EuiOverlayMask)`
}
`;

export const FilePreview = ({ closePreview, selectedFile, getDownloadHref }: FilePreviewProps) => {
export const FilePreview = ({ closePreview, selectedFile }: FilePreviewProps) => {
const { client: filesClient } = useFilesContext();
const { owner } = useCasesContext();

return (
Expand All @@ -41,7 +42,7 @@ export const FilePreview = ({ closePreview, selectedFile, getDownloadHref }: Fil
<EuiImage
alt={selectedFile.name}
size="original"
src={getDownloadHref({
src={filesClient.getDownloadHref({
id: selectedFile.id || '',
fileKind: constructFileKindIdByOwner(owner[0] as Owner),
})}
Expand Down
Loading