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] Disable deletion of cases files in stack mgmt. #155683

Merged

Conversation

adcoelho
Copy link
Contributor

Summary

Since #155179 was merged we can now disable the deletion of Cases files in the Files page in Stack management.

Screen.Recording.2023-04-25.at.08.22.27.mov

@adcoelho adcoelho added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.8.0 labels Apr 25, 2023
@adcoelho adcoelho self-assigned this Apr 25, 2023
@adcoelho adcoelho requested a review from a team as a code owner April 25, 2023 06:26
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

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 👍

@adcoelho adcoelho force-pushed the disable-file-deletion-outside-cases branch from 35affae to 6b16f37 Compare April 25, 2023 11:34
managementUiActions: {
delete: {
enabled: false,
reason: 'File is managed by Cases',
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to translate this message and I wonder if it'd be better to state that the user needs to go to cases to delete the file rather than just saying it's managed by cases. I wonder if we should also include the plugin name here?

@lcawl What do you think? This reason field will show up when a user hovers over a disabled checkbox in the file management page for a cases file. Is this text sufficient for communicating that the user will need to navigate to cases to delete the file? Or maybe we should have something like: Navigate to <Security|Observability|Management> Cases to delete this file

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

I left a suggestion, otherwise UI text LGTM. I think it's better to communicate clearly what the user needs to do to delete the file – even if it slightly increases the word count.

managementUiActions: {
delete: {
enabled: false,
reason: 'File is managed by Cases',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
reason: 'File is managed by Cases',
reason: 'This file is managed by Cases. Navigate to the Cases page under Stack management to delete 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 talked to @szabosteve offline and since the file might exist in a different plugin he suggested something like this:

This file is managed by Cases. Navigate to the Cases page under <Security or Observability or Stack Management> to delete it.

@@ -17,6 +17,19 @@ export const GENERAL_CASES_OWNER = APP_ID;

export const OWNERS = [GENERAL_CASES_OWNER, OBSERVABILITY_OWNER, SECURITY_SOLUTION_OWNER] as const;

export const getOwnerUIName = (owner: Owner) => {
Copy link
Member

Choose a reason for hiding this comment

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

The OWNER_INFO contains an attribute called label. Maybe we can use it. The names are a bit different but we can change it to match what you have here. They are being shown in the case flyout and in the cases 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.

I noticed those but, what is this OWNER_INFO used for? Is it safe to just extend?

@adcoelho adcoelho force-pushed the disable-file-deletion-outside-cases branch from 3a8799b to f2b8a09 Compare April 25, 2023 15:18
@adcoelho adcoelho force-pushed the disable-file-deletion-outside-cases branch from f2b8a09 to f6886c2 Compare April 25, 2023 15:19
const getOwnerUIName = (owner: Owner) => {
switch (owner) {
case SECURITY_SOLUTION_OWNER:
return 'Security Solution';
Copy link
Contributor

@jonathan-buttner jonathan-buttner Apr 25, 2023

Choose a reason for hiding this comment

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

Sorry to nitpick this, Security Solution is an internal name I think. I believe in the external docs we refer to it as Elastic Security https://www.elastic.co/guide/en/security/current/es-ui-overview.html#es-ui-overview

How about we change this to return 'Elastic Security'? or maybe return 'Security'

@jonathan-buttner jonathan-buttner requested a review from a team as a code owner April 25, 2023 18:53
@@ -7,7 +7,7 @@ pageLoadAssetSize:
banners: 17946
bfetch: 22837
canvas: 1066647
cases: 170000
cases: 175000
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm temporarily raising the bundle size here so we can get this merged. We still have this issue: #103748 and we're planning on working on it. We'd just like to get this PR merged in 8.8 and we won't be able to realistically lower the bundle size in this release.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml

@jonathan-buttner jonathan-buttner enabled auto-merge (squash) April 25, 2023 18:56
@jonathan-buttner jonathan-buttner merged commit 933bca2 into elastic:main Apr 25, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 618 619 +1

Page load bundle

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

id before after diff
cases 165.9KB 166.3KB +370.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 397 400 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 477 480 +3
total +5

History

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

cc @adcoelho

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 25, 2023
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:skip Skip the PR/issue when compiling release notes 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.

9 participants