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

[Cases] Detail View Attachments Table #152941

Merged
merged 26 commits into from
Mar 27, 2023
Merged

[Cases] Detail View Attachments Table #152941

merged 26 commits into from
Mar 27, 2023

Conversation

adcoelho
Copy link
Contributor

@adcoelho adcoelho commented Mar 8, 2023

Fixes #151723

I am merging this into a feature branch.

Summary

This PR adds a way for users to:

View a list of all files attached to a case(with pagination)table
Attach files to a caseattach
Preview specific image files attached to a casepreview
Search for files attached to a case by file name.Screenshot 2023-03-08 at 18 13 09
Download files attached to a caseScreenshot 2023-03-08 at 18 19 32

TODO

I need to work on the tests, I already added some for the new components but need to write more. Off the top of my head, I still need to create tests for the new hooks and to make sure the changes to the Cases Context are working properly everywhere.

I created new issues for missing functionality that was not described in #151595 yet. This includes the deletion action and the files total next to the tab name on the case detail page.

@adcoelho 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 8, 2023
@adcoelho adcoelho self-assigned this Mar 8, 2023
@adcoelho adcoelho force-pushed the cases-detail-attachments-table branch from 74290d2 to de3165c Compare March 10, 2023 13:44
cnasikas
cnasikas reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Very good job! I left some comments.

x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Show resolved Hide resolved
x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Show resolved Hide resolved
x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/containers/use_get_case_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/containers/use_get_case_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/containers/use_get_case_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/components/case_view/components/case_view_files.tsx Outdated Show resolved Hide resolved
x-pack/plugins/cases/public/plugin.ts Show resolved Hide resolved
@sixstringcode
Copy link

This is looking great. So cool to see the file service getting used outside of the image embeddable already.

@jonathan-buttner jonathan-buttner mentioned this pull request Mar 23, 2023
Open
adcoelho added 6 commits March 23, 2023 15:04
@adcoelho
Include showDanger in useCasesToast.
1c03756 
Cleanup case_view_files tests.
Move caseId out of filteringOptions.
Add type guard for owner in cases context.
Use useCasesToast in add_file component.
@adcoelho
Fix AddFile button in empty FilesTable.
af1f15f 
Add spacer for loading FilesTable.
Use file name as "alt" in FilePreview.
Remove HiddenButtonGroup from utility bar.
Improve query keys for caseFiles.
Remove owner from useGetCaseFiles.
@adcoelho
Use isPreviousData in FilesTable pagination.
3f1d76f 
Pass correct owner to definitions of the file kind.
@adcoelho
Merge remote-tracking branch 'upstream/cases-detail-view-files-tab' i…
df1702c 
…nto cases-detail-attachments-table
@adcoelho
Change files count information in files table.
95926fc 
Change file name and mime type in files table.
@adcoelho
Add FilePreview tests.
81b41d1 
Improve FilesTable tests.
Add FilesUtilityBar tests.
Add UseGetCaseFiles tests.
cnasikas
cnasikas reviewed Mar 27, 2023
View reviewed changes
x-pack/plugins/cases/public/components/files/use_files_table_columns.tsx Outdated
name: i18n.TYPE,
'data-test-subj': 'cases-files-table-filetype',
render: (attachment: FileJSON) => {
return <span>{`${attachment.mimeType?.split('/')[0]}` || ''}</span>;
Copy link
Member

@cnasikas cnasikas Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As the is inside the ticks anything undefined will render to "undefined" (a string) and || will not work except an empty string. What about moving to a function, taking care of edge cases, and returning "Unknown" if there is no mimeType or attachment.mimeType?.split('/') is an empty array or attachment.mimeType?.split('/') contains an empty string, etc. Maybe we can also capitalize the first letter. Not for this PR but about application files, I see that we show the word "application". Do we want that or it is better to say "Zip", "PDF", etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for this PR but about application files, I see that we show the word "application". Do we want that or it is better to say "Zip", "PDF", etc?

I guess that would be not much different than the file type which is what we had before and was changed due to a previous conversation with @jonathan-buttner and @mdefazio 😅

Copy link
Member

Choose a reason for hiding this comment

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

It is not the same 🙂 . For the "image" is fine. It's an image. For applications/* though the file is not an application, is a ZIP, PDF, Text, etc. Also. it does not mean that the second part of the mime type describes correctly the type. We may need our own mapping for that. I think it is fine as it is now. Let's discuss it offline if we should improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think application for me means an executable so I don't think we should show that. I think it'd be better to show Compressed if we want to group zip, gzip, 7z etc into one. I think PDF, Plain Text, and JSON probably make sense for the others.

cnasikas
cnasikas reviewed Mar 27, 2023
View reviewed changes
x-pack/plugins/cases/public/files/index.ts Outdated
@@ -20,10 +20,13 @@ const buildFileKind = (owner: Owner): FileKindBrowser => {
};
};

export const isRegisteredOwner = (ownerToCheck: string): ownerToCheck is Owner =>
ownerToCheck in CASES_FILE_KINDS;
Copy link
Member

Choose a reason for hiding this comment

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

For security reasons, it is better if we use Object.hasOwn(CASES_FILE_KINDS, ownerToCheck)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option would be to compare against the OWNERS array directly: https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/common/constants/owners.ts#L18

I also need something similar:


const isValidOwner = (ownerToValidate: string): ownerToValidate is Owner => {
  const foundOwner = OWNERS.find((validOwner) => validOwner === ownerToValidate);

  return foundOwner !== undefined;
};

That doesn't guarantee that it is registered though, so if you'd prefer to keep what you have that's fine 👍

adcoelho added 2 commits March 27, 2023 15:46
@adcoelho
Changed implementation of isRegisteredOwner.
68c3047 
Created tests for AddFile component.
Added more tests for FilesTable component.
Created parseMimeType util.
Added a test for showDangerToast.
@adcoelho
Fixed failing jest tests.
e2ad7a5
cnasikas
cnasikas approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Thank you for your patience with all of my comments. I tested and LGTM.

js-jankisalvi
js-jankisalvi reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Verified locally, looks amazing 😻 Awesome work!! 🎉

x-pack/plugins/cases/public/client/ui/get_all_cases_selector_modal.tsx
@@ -23,6 +25,7 @@ const AllCasesSelectorModalLazy: React.FC<AllCasesSelectorModalProps> = lazy(
export const getAllCasesSelectorModalLazy = ({
externalReferenceAttachmentTypeRegistry,
persistableStateAttachmentTypeRegistry,
getFilesClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing the picture here, Why do we need getFilesClient separately in selector modal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if sometimes it isn't used (like in the selector modal that doesn't show the case detail view where the files tab is)
the CasesContext wraps its children in the FilesContextProvider that unfortunately needs the files client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay 👍

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

appMockRender.render(
<>
<div data-test-subj="outsideClickDummy" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure but maybe you can add onClick event and role to the div.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also doesn't work 😞

x-pack/plugins/cases/public/components/files/use_files_table_columns.tsx
color: 'danger',
icon: 'trash',
type: 'icon',
onClick: () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this column be removed, if delete functionality will not be added in this PR?

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 could but I am going to work on this tomorrow so I am not sure it is worth it 😅

Also, it is not being sent to main, it is just the feature branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot this goes to feature branch, doesn't make sense to remove it 😄

@kibana-ci
Copy link
Collaborator

kibana-ci commented Mar 27, 2023 •
edited
Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 565 597 +32

Async chunks

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

id before after diff
cases 363.7KB 385.5KB +21.8KB

Page load bundle

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

id before after diff
cases 133.3KB 134.9KB +1.6KB
Unknown metric groups

async chunk count

id before after diff
cases 16 17 +1

ESLint disabled line counts

id before after diff
cases 56 57 +1
securitySolution 433 436 +3
total +4

Total ESLint disabled count

id before after diff
cases 74 75 +1
securitySolution 513 516 +3
total +4

History

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

cc @adcoelho

@adcoelho adcoelho merged commit 78133a7 into elastic:cases-detail-view-files-tab Mar 27, 2023
adcoelho added a commit that referenced this pull request Mar 28, 2023
@adcoelho
[Cases] Detail View Attachments Table (#152941)
294f1d5 
Fixes #151723
This PR adds a way for users to:
View a list of all files attached to a case(with pagination).
Attach files to a case.
Preview image files attached to a case.
Search for files attached to a case by file name.
Download files attached to a case.
@cnasikas cnasikas mentioned this pull request Apr 3, 2023
Closed
5 tasks
@nastasha-solomon nastasha-solomon mentioned this pull request Apr 4, 2023
Closed
@adcoelho adcoelho mentioned this pull request Apr 11, 2023
Merged
adcoelho added a commit that referenced this pull request Apr 18, 2023
@adcoelho @kibanamachine
[Cases] Attaching files to cases (#154436)
0a38f85 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@js-jankisalvi js-jankisalvi js-jankisalvi left review comments

@jonathan-buttner jonathan-buttner jonathan-buttner left review comments

@mdefazio mdefazio mdefazio left review comments

@cnasikas cnasikas cnasikas approved these changes

Assignees

@adcoelho adcoelho

Labels
enhancement New value added to drive a business result Feature:Cases Cases feature Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@adcoelho @elasticmachine @sixstringcode @kibana-ci @mdefazio @cnasikas @jonathan-buttner @js-jankisalvi @kibanamachine