[Cases] File user action in Case detail view #153957

adcoelho · 2023-03-29T15:39:12Z

Fixes #152088

Summary

Users are now able to see file activity in the case detail view.

Image files have a different icon and a clickable file name to preview
Other files have a standard "document" icon and the name is not clickable
The file can be downloaded by clicking the download icon

If the file has invalid metadata (not likely) we display the activity item below:

Registered FileType in attachment registry. Created FileDownloadButtonIcon. Created FileNameLink. Added filesContext to FilePreview. Updated file external reference metadata. Added missing translations. Added hideDefaultActions in attachment props. Created isValidFileExternalReferenceMetadata util. Added tests.

elasticmachine · 2023-03-29T15:39:15Z

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

elasticmachine · 2023-03-29T15:39:16Z

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

jonathan-buttner

Looking good, I left a few questions

jonathan-buttner · 2023-03-29T17:25:01Z

x-pack/plugins/cases/public/components/files/file_download_button_icon.tsx

+      iconType={'download'}
+      aria-label={i18n.DOWNLOAD}
+      href={filesClient.getDownloadHref({
+        fileKind: constructFileKindIdByOwner(owner[0] as Owner),


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

x-pack/plugins/cases/public/components/files/utils.tsx

jonathan-buttner · 2023-03-29T17:39:44Z

x-pack/plugins/cases/public/components/files/file_type.tsx

+  const fileId = props.externalReferenceId;
+
+  // @ts-ignore
+  const partialFileJSON = props.externalReferenceMetadata?.files[0] as Partial<FileJSON>;


How about we create a subset type so we don't need to cast here?

Something like this:

type DownloadableFile = Pick<FileJSON, 'extension' | 'fileKind' | 'id' | 'mimeType' | 'name'>;

And use that throughout where we need it. That way we don't have to cast and we don't have to fake it being a FileJSON.

But won't we have to cast it instead to DownloadableFile? I am not sure I understand your suggestion 😅

The problem here is that if I don't cast we get

Element implicitly has an 'any' type because expression of type '0' can't be used to index type 'string | number | boolean | JsonObject | JsonArray'. Property '0' does not exist on type 'string | number | boolean | JsonObject | JsonArray'

Weird that 0 does not exist on JsonArray.

Still, this comes from CommentRequestExternalReferenceType['externalReferenceMetadata']; where we can't really say what the metadata is going to be since it depends on the attachment type ™️.

Moreover, I don't actually need a cast here the @ts-ignore is rather for the indexing error mentioned above 😫

To remove the @ts-ignore you can do

import { JsonArray } from '@kbn/utility-types'; const files = props.externalReferenceMetadata?.files as JsonArray; const partialFileJSON = files[0] as Partial<FileJSON>;

I don't think we can avoid the casting as the framework has a bug with the types and cannot detect the correct types based on the registered type.

jonathan-buttner · 2023-03-29T17:49:28Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.tsx

+            {!attachmentViewObject.hideDefaultActions && (
+              <RegisteredAttachmentsPropertyActions
+                isLoading={isLoading}
+                onDelete={() => handleDeleteComment(comment.id)}


Should we allow deleting a file via the attachment?

Next PR I will create the DeleteFileButton component and I will use it here instead of the RegisteredAttachmentsPropertyActions and in the files table.

Also, the hideDefaultActions attribute hides the delete action from files.

x-pack/plugins/cases/public/client/attachment_framework/types.ts

Fixed typechek. Addressed PR comments.

cnasikas · 2023-03-31T09:42:02Z

x-pack/plugins/cases/public/components/files/file_download_button_icon.tsx

+    <EuiButtonIcon
+      iconType={'download'}
+      aria-label={i18n.DOWNLOAD}
+      href={filesClient.getDownloadHref({


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] } )}

I can but I don't see much difference between

filesClient.getDownloadHref(A, B)

and

newFunction(filesClient.getDownloadHref, A, B) 😄

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.

cnasikas · 2023-03-31T09:47:40Z

x-pack/plugins/cases/public/components/files/file_type.tsx

+  const [isPreviewVisible, setIsPreviewVisible] = useState(false);
+
+  const closePreview = () => setIsPreviewVisible(false);
+  const showPreview = () => setIsPreviewVisible(true);


I think we use the same logic elsewhere (x-pack/plugins/cases/public/components/files/files_table.tsx). What about creating a hook (inside the files folder) called useFilePreview that encapsulates the previewing logic? For example:

const useFilePreview = () => { const [isPreviewVisible, setIsPreviewVisible] = useState(false); const closePreview = () => setIsPreviewVisible(false); const showPreview = () => setIsPreviewVisible(true); return { showPreview, closePreview } } // consume it as const { showPreview, closePreview } = useFilePreview();

cnasikas · 2023-03-31T09:56:24Z

x-pack/plugins/cases/public/components/files/file_type.tsx

+
+export const getFileType = (): ExternalReferenceAttachmentType => ({
+  id: FILE_ATTACHMENT_TYPE,
+  icon: 'image',


nit: This is reserved for future use. It means to be an icon to represent the registered attachment (all files). I think we should change it to document as it is more generic.

cnasikas · 2023-03-31T10:59:26Z

x-pack/plugins/cases/public/components/files/utils.test.tsx

+    expect(isValidFileExternalReferenceMetadata({ files: 'bar' })).toBeFalsy();
+  });
+
+  it('should return false if file.length !== 1', () => {


nit: I don't think that the function returns false because of the file.length !== 1. I think it returns false because it is not an array of valid metadata. The check in the isValidFileExternalReferenceMetadata is externalReferenceMetadata?.files?.length >= 1. Maybe we can delete the test or we can change it to check that it validates an array of two valid metadata.

You're right. This is a leftover from the previous implementation of isValidFileExternalReferenceMetadata. I'll just remove it altogether.

cnasikas · 2023-03-31T11:02:39Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.tsx

  let AttachmentElement: React.ReactElement;

  const renderCallback = (props: object) => {
+    const attachmentViewObject = attachmentType.getAttachmentViewObject(
+      props as ExternalReferenceAttachmentViewProps


In combination with the comment here https://github.com/elastic/kibana/pull/153957/files#r1154257681 we should remove the casting.

nit: Also to avoid the extra call to getAttachmentViewObject maybe we can pass the attachmentViewObject as an argument to renderCallback.

cnasikas · 2023-03-31T11:03:23Z

x-pack/plugins/cases/public/components/user_actions/comment/registered_attachments.tsx

    const props = {
      ...getAttachmentViewProps(),
      caseData: { id: caseData.id, title: caseData.title },
    };
+    const attachmentViewObject = attachmentType.getAttachmentViewObject(
+      props as ExternalReferenceAttachmentViewProps


…nto cases-detail-file-user-activity

Removed test from files utils. Created useFilePreview hook. And tests. Removed unnecessary type casts.

kibana-ci · 2023-03-31T20:53:01Z

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [5dfc418]

History

💔 Build #117076 failed cb24818
💔 Build #116732 failed 16d8b80
💔 Build #116552 failed 4579f5d24b4f46a17735c732f0a8dc6f218531a8
💔 Build #116472 failed 9223262

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

cc @adcoelho

Fixes #151595 ## Summary In this PR we will be merging a feature branch into `main`. This feature branch is a collection of several different PRs with file functionality for cases. - #152941 - #153957 - #154432 - #153853 Most of the code was already reviewed so this will mainly be used for testing. - Files tab in the case detail view. - Attach files to a case. - View a list of all files attached to a case (with pagination). - Preview image files attached to a case. - Search for files attached to a case by file name. - Download files attached to a case. - Users are now able to see file activity in the case detail view. - Image files have a different icon and a clickable file name to preview. - Other files have a standard "document" icon and the name is not clickable. - The file can be downloaded by clicking the download icon. ## Release notes Support file attachments in Cases. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

adcoelho added enhancement New value added to drive a business result Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.8.0 labels Mar 29, 2023

adcoelho self-assigned this Mar 29, 2023

adcoelho requested a review from a team as a code owner March 29, 2023 15:39

jonathan-buttner reviewed Mar 29, 2023

View reviewed changes

Fixed jest tests.

16d8b80

Fixed typechek. Addressed PR comments.

adcoelho force-pushed the cases-detail-file-user-activity branch from 4579f5d to 16d8b80 Compare March 30, 2023 12:42

cnasikas reviewed Mar 31, 2023

View reviewed changes

adcoelho added 4 commits March 31, 2023 14:19

Merge remote-tracking branch 'upstream/cases-detail-view-files-tab' i…

9c2c261

…nto cases-detail-file-user-activity

Changed parameters of getAttachmentRenderer.

33379c9

Removed test from files utils. Created useFilePreview hook. And tests. Removed unnecessary type casts.

Remove wrong usage of CASES_FILE_KINDS.

cb24818

Fix jest test.

741b91a

adcoelho merged commit 0b060bd into elastic:cases-detail-view-files-tab Apr 1, 2023

cnasikas mentioned this pull request Apr 3, 2023

[Cases][Meta]: File attachments #151595

Closed

5 tasks

adcoelho mentioned this pull request Apr 11, 2023

[Cases] Attaching files to cases #154436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] File user action in Case detail view #153957

[Cases] File user action in Case detail view #153957

adcoelho commented Mar 29, 2023

elasticmachine commented Mar 29, 2023

elasticmachine commented Mar 29, 2023

jonathan-buttner left a comment

jonathan-buttner Mar 29, 2023

jonathan-buttner Mar 29, 2023

adcoelho Mar 29, 2023

adcoelho Mar 29, 2023

cnasikas Mar 31, 2023 •

edited

Loading

jonathan-buttner Mar 29, 2023

adcoelho Mar 29, 2023

cnasikas Mar 31, 2023

cnasikas Mar 31, 2023 •

edited

Loading

adcoelho Mar 31, 2023

cnasikas Mar 31, 2023

cnasikas Mar 31, 2023 •

edited

Loading

cnasikas Mar 31, 2023

cnasikas Mar 31, 2023

adcoelho Mar 31, 2023 •

edited

Loading

cnasikas Mar 31, 2023

cnasikas Mar 31, 2023

cnasikas Mar 31, 2023 •

edited

Loading

kibana-ci commented Mar 31, 2023 •

edited

Loading

[Cases] File user action in Case detail view #153957

[Cases] File user action in Case detail view #153957

Conversation

adcoelho commented Mar 29, 2023

Summary

elasticmachine commented Mar 29, 2023

elasticmachine commented Mar 29, 2023

jonathan-buttner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adcoelho Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Mar 31, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

cnasikas Mar 31, 2023 •

edited

Loading

cnasikas Mar 31, 2023 •

edited

Loading

cnasikas Mar 31, 2023 •

edited

Loading

adcoelho Mar 31, 2023 •

edited

Loading

cnasikas Mar 31, 2023 •

edited

Loading

kibana-ci commented Mar 31, 2023 •

edited

Loading