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] Deleting file attachments from a case #154432

Conversation

adcoelho
Copy link
Contributor

@adcoelho adcoelho commented Apr 5, 2023

Summary

In this PR we add the ability to delete files attached to a case.

This can be done in the Case detail view either in the

Files tab Screenshot 2023-04-05 at 10 29 39

or the

Activity tab Screenshot 2023-04-05 at 10 29 53
When a file is deleted the file activity item is replaced by a removed attachment activity item. Screenshot 2023-04-05 at 10 30 05

adcoelho added 2 commits April 4, 2023 14:36
Created FileDeleteButtonIcon.
Created deleteFileAttachments public api.
Created useDeleteFileAttachment hook.
Added FileDeleteButtonIcon tests.
Added useDeleteFileAttachment tests.
@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 Apr 5, 2023
@adcoelho adcoelho requested a review from a team as a code owner April 5, 2023 08:40
@adcoelho adcoelho self-assigned this Apr 5, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@adcoelho adcoelho force-pushed the file-attachment-delete-button branch from 2f7a7b4 to 483ffce Compare April 5, 2023 08:48
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! Tested and is working as expected. In all delete operations in cases we show a delete confirmation modal. I think we should do the same when deleting files.

Screenshot 2023-04-05 at 12 39 34 PM

Screenshot 2023-04-05 at 12 39 42 PM

Screenshot 2023-04-05 at 12 46 25 PM

fileIds: string[];
signal: AbortSignal;
}): Promise<void> => {
await KibanaServices.get().http.fetch<CaseResponse>(
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we should remove the type (CaseResponse) as the endpoint does not return a case response.

{
mutationKey: casesMutationsKeys.deleteFileAttachment,
onSuccess: (_, { successToasterTitle }) => {
showSuccessToast(successToasterTitle);
Copy link
Member

Choose a reason for hiding this comment

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

nit: As this hook is only for file deletions maybe we can hardcoded the success toaster title.

refreshAttachmentsTable();
},
onError: (error: ServerError) => {
showErrorToast(error, { title: i18n.ERROR_TITLE });
Copy link
Member

Choose a reason for hiding this comment

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

The ERROR_TITLE equals to Error fetching data. We should change it to ERROR_DELETING (Error deleting data) or have a custom one only for files, "Error deleting file (s)" for example.

jest.clearAllMocks();
});

it('init', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this test.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test where we delete a file from the files table?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test where we delete a file from the user activity?

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 good 👍

This isn't related to this PR:
Is it just me or does the badge next to Files is slightly not aligned? 😄
image

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.

LGMT! Don't forget to add the tests.

Added delete button tests for files_table.
@adcoelho
Copy link
Contributor Author

adcoelho commented Apr 5, 2023

Screenshot 2023-04-05 at 17 28 00

I'm not sure @js-jankisalvi check out on mine. They look different, right?

Yours looks worse 😅

@js-jankisalvi
Copy link
Contributor

I'm not sure @js-jankisalvi check out on mine. They look different, right?

yeah, yours look better. Not sure why it looked weird on mine 😄
But it's not a big deal. You can leave it. 👍

@kibana-ci
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 611 613 +2

Async chunks

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

id before after diff
cases 388.9KB 376.9KB -11.9KB

Page load bundle

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

id before after diff errors
cases 139.7KB 156.7KB +17.0KB ❌ 15.6KB over limit

History

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

cc @adcoelho

@adcoelho adcoelho merged commit f48ee9f into elastic:cases-detail-view-files-tab Apr 6, 2023
adcoelho added a commit that referenced this pull request Apr 18, 2023
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
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
Development

Successfully merging this pull request may close these issues.

5 participants