[Cases] Save draft user comment by js-jankisalvi · Pull Request #146327 · elastic/kibana

js-jankisalvi · 2022-11-24T17:26:50Z

Summary

This PR saves user's draft comment in a session storage.

Fixed below scenarios requested by users:

When I’m writing a new comment and someone posts a new comment before me
When I’m writing a new comment and someone updates an old comment
When I’m editing an old comment and someone posts a new comment
When I’m editing an old comment and someone updates an old comment

Draft.comment.mov

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility

Passed Flakey tests suite:

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1647

…-fix'

…alvi/kibana into save-draft-user-cmment

… was updated recently

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2022-11-30T16:12:54Z

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

…st render

…alvi/kibana into save-draft-user-cmment

jonathan-buttner · 2022-12-01T14:44:47Z

x-pack/plugins/cases/public/components/markdown_editor/eui_form.tsx

  bottomRightContent?: React.ReactNode;
  caseTitle?: string;
  caseTags?: string[];
+  draftCommentStorageKey?: string;


If I'm understanding the MarkdownEditorForm correctly, it's used by the description and the comments. I think we could make it a required prop if we applied the same logic you have here for the description and then we wouldn't need to check if it is defined. Would it be complicated to add it to description?

Adding draftCommentStorageKey to description is not complicated. I thought it as an optional feature, hence optional prop. I am okay to make it mandatory prop.

Correction:
we don't need to add it to description as we are creating storage key in UserActionMarkdown. So it handles storing draft for comments and description while editing them. I will make it a required key and remove the check.

Now that you mentioned description, do we need to store draft description while user is creating a case?
Right now I have handled Edit scenarios after the case is created.

do we need to store draft description while user is creating a case?

Na, let's just do the edit scenario 👍

In that case I will keep draftCommentStorageKey?: string; optional as MarkdownEditorForm is also used in description of create cases.

jonathan-buttner · 2022-12-01T16:07:03Z

x-pack/plugins/cases/public/components/markdown_editor/eui_form.tsx

+
+      useDebounce(
+        () => {
+          if (isFirstRender.current) {


Just curious, how come we want to ignore the first render? Is it because it would store the initial value before the user starts typing?

Could we instead just compare the field.value with initialValue and see if they're equal?

yes, every time you visit a case, you get an empty newComment key stored in the session eventhough user didn't touch the markdown.
i.e. xpack.cases.caseView.{caseId}.{newComment}.markdownEditor
So we want to ignore first render so that it doesn't store newComment markdown key with empty value.

Unfortunately initialValue is undefined for newComment markdown until submit(add comment) is triggered. That's why initialValue is not helpful to detect first render.

jonathan-buttner · 2022-12-01T16:09:42Z

x-pack/plugins/cases/public/components/markdown_editor/eui_form.tsx

+
+      useEffect(() => {
+        if (initialValue && initialValue !== field.value) {
+          field.setErrors([{ message: i18n.COMMENT_VERSION_CONFLICT_ERROR }]);


Thinking about this more, do we really need the user to refresh? We just want them to be aware that something changed under them. Maybe an alternative would be to some warning text that it change and saving will overwrite 🤷‍♂️

If we set the errors we still let the user save the change but nothing happens, I wonder if we should disable the save button if want to continue with this type of solution.

I like the first approach, showing warning text instead of error as user is already in writing a comment and suddenly disabling the save button is a preventive action. So just a text warning suggesting that comment has changed, and letting user decide makes sense.

So just a text warning suggesting that comment has changed, and letting user decide makes sense.

Sounds good to me!

…-ref HEAD~1..HEAD --fix'

… in Markdown after added

cnasikas · 2022-12-13T13:55:05Z

x-pack/plugins/cases/public/components/add_comment/index.tsx

+
+          removeItemFromSessionStorate(draftStorageKey);
+
+          /* had to add to this check that session key is removed to avaoid weird issue that on reset 


nit: avaoid -> avoid

cnasikas · 2022-12-13T14:01:03Z

x-pack/plugins/cases/public/components/markdown_editor/utils.ts

+  caseId: string,
+  commentId: string
+): string => {
+  const appIdKey = appId !== '' ? appId : 'kibana';


Let's default it to cases instead of kibana.

We are returning key as return cases.${appIdKey}.${caseIdKey}.${commentIdKey}.markdownEditor so if we put cases when no appId, it will be cases.cases.${caseId}.${commentId}.markdownEditor

shall I put const appIdKey = appId !== '' ? appId : 'default'; ?

I think is better to put const appIdKey = appId !== '' ? appId : 'cases';

…alvi/kibana into save-draft-user-cmment

peteharverson

Functionally this LGTM. Suggest any other edits to the UX, such as making the messaging clearer when the comment being edited has been updated, be done in a follow-up.

cnasikas · 2022-12-14T08:47:44Z

@elasticmachine merge upstream

x-pack/plugins/cases/public/components/create/form_context.test.tsx

cnasikas · 2022-12-14T10:25:01Z

x-pack/plugins/cases/public/components/markdown_editor/use_markdown_session_storage.tsx

+      if (isFirstRender.current) {
+        if (isEmpty(sessionValue) && !isEmpty(field.value)) {
+          /* this condition is used to for lens draft comment, 
+            when user selects and visualization and comes back to Markdown editor,
+            it is a first render for Markdown editor, however field has value of visualization which is not stored in session 
+            hence saving this item in session storage
+          */
+
+          setSessionValue(field.value);
+        }
+        isFirstRender.current = false;
+        return;
+      }
+      setSessionValue(field.value);


I am not 100% sure and maybe I miss something but I think this can be simplified as:

Suggested change

if (isFirstRender.current) {

if (isEmpty(sessionValue) && !isEmpty(field.value)) {

/* this condition is used to for lens draft comment,

when user selects and visualization and comes back to Markdown editor,

it is a first render for Markdown editor, however field has value of visualization which is not stored in session

hence saving this item in session storage

*/

setSessionValue(field.value);

}

isFirstRender.current = false;

return;

}

setSessionValue(field.value);

if (isFirstRender.current) {

isFirstRender.current = false;

}

setSessionValue(field.value);

I just saw this test should not update the session value when it is first render so probably I am wrong 🙂.

hmm.. may be. let me verify this in local

spoke too early. It fails the test with jest.advanceTimersByTime. better to not update it.

Weird, with my code the test should not pass as it will update the value in the first render. Could you please remind me why we do not want to update the session value on the first render? I do not remember.

cnasikas · 2022-12-14T10:37:03Z

x-pack/plugins/cases/public/components/user_actions/markdown_form.test.tsx

+          </MockHookWrapperComponent>
+        );
+
+        expect(result.container.querySelector('textarea')!.value).toEqual('value set in storage');


nit: this can be done expect(result.getByText('value set in storage')).toBeInTheDocument()

…nce block, unit tests updated

kibana-ci · 2022-12-14T13:18:48Z

💚 Build Succeeded

Buildkite Build
Commit: 4de1551

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	515	518	+3

Async chunks

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

id	before	after	diff
`cases`	348.1KB	349.8KB	+1.7KB

Page load bundle

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

id	before	after	diff
`cases`	127.1KB	127.4KB	+240.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+21

History

💚 Build #94610 succeeded 520e3c5
💔 Build #94450 failed 426ab55
💔 Build #94426 failed ee3d6f9
💔 Build #94357 failed 99c71cd
💔 Build #94355 failed 4879bc1
💚 Build #94317 succeeded 8781fcf

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

cc @js-jankisalvi

cnasikas

Code LGTM! Tested without any issues. Great job 🚀!

js-jankisalvi · 2022-12-14T14:31:42Z

Code LGTM! Tested without any issues. Great job 🚀!

Thank you for the help :)

* main: (21 commits) [Profiling] Remove link to 'Other' bucket (elastic#147523) [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265) [DOCS] Updates what's new pages (elastic#147483) [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361) [Guided onboarding] Update guide IDs (elastic#147348) [Synthetics] Add synthetics settings alerting default (elastic#147339) [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212) [Cases] Save draft user comment (elastic#146327) [API Docs] Fix `--plugin` filter (elastic#147500) [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439) Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503) [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324) [Discover] Fix Discover navigation from Lens embeddable (elastic#147000) Allow users to Update API Keys (elastic#146237) Update dependency xstate to ^4.35.0 (main) (elastic#147463) [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429) [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432) [APM] Switching service groups from grid to flex layout (elastic#147448) [Fleet] Add missing endpoints to openApi specs (elastic#147452) [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253) ...

js-jankisalvi added 4 commits November 21, 2022 15:12

feat: store draft description and comment

d121368

feat: save draft comments to session storage using storage kibana utils

58925e1

Merge remote-tracking branch 'upstream/main' into save-draft-user-cmment

5701728

feat: added unit test and e2e UI tests

cffe533

js-jankisalvi changed the title ~~Save draft user cmment~~ [Cases] Save draft user comment Nov 24, 2022

js-jankisalvi self-assigned this Nov 24, 2022

js-jankisalvi added v8.7.0 Feature:Cases Cases feature labels Nov 24, 2022

kibanamachine and others added 10 commits November 24, 2022 18:02

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

21f7e8b

…-fix'

feat: added debounce, unit tests for draft comments

9c88803

Merge branch 'save-draft-user-cmment' of https://github.com/js-jankis…

080decf

…alvi/kibana into save-draft-user-cmment

fixed conflict issue

06c2da9

fix: show version conflict error when user is editing a comment which…

ffbab09

… was updated recently

fix: update error message

5374a93

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

66e3837

…-ref HEAD~1..HEAD --fix'

test description updated

8759595

add constant for debounce time

9e7d3e6

Merge branch 'main' into save-draft-user-cmment

8553dc5

js-jankisalvi requested a review from a team November 30, 2022 16:12

js-jankisalvi marked this pull request as ready for review November 30, 2022 16:12

js-jankisalvi requested a review from a team as a code owner November 30, 2022 16:12

js-jankisalvi added the release_note:skip Skip the PR/issue when compiling release notes label Nov 30, 2022

js-jankisalvi added 2 commits December 1, 2022 11:50

fix: remove empty keys from session storage, prevent empty key on fir…

d20b100

…st render

Merge branch 'save-draft-user-cmment' of https://github.com/js-jankis…

20dfeae

…alvi/kibana into save-draft-user-cmment

jonathan-buttner reviewed Dec 1, 2022

View reviewed changes

js-jankisalvi added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 2, 2022

js-jankisalvi added 2 commits December 2, 2022 18:32

fix: show version conflict warning instead of error

04942ae

Merge branch 'main' into save-draft-user-cmment

7e29808

js-jankisalvi and others added 5 commits December 13, 2022 10:37

fix: type check fixed for form_context

9775b94

remove session storage key after each test

1a1d54f

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

6a39c82

…-ref HEAD~1..HEAD --fix'

fix: add existing storage key test case, weird issue of comment stays…

acbcdfe

… in Markdown after added

Merge branch 'main' into save-draft-user-cmment

8781fcf

cnasikas reviewed Dec 13, 2022

View reviewed changes

js-jankisalvi added 4 commits December 13, 2022 15:50

typos fixed

cbfb2c6

Merge branch 'save-draft-user-cmment' of https://github.com/js-jankis…

4879bc1

…alvi/kibana into save-draft-user-cmment

Merge branch 'main' into save-draft-user-cmment

99c71cd

Merge branch 'main' into save-draft-user-cmment

ee3d6f9

peteharverson approved these changes Dec 13, 2022

View reviewed changes

fix

426ab55

Merge branch 'main' into save-draft-user-cmment

520e3c5

cnasikas reviewed Dec 14, 2022

View reviewed changes

x-pack/plugins/cases/public/components/create/form_context.test.tsx Show resolved Hide resolved

cnasikas reviewed Dec 14, 2022

View reviewed changes

fix: description text updated, isFristrender check removed from debou…

4de1551

…nce block, unit tests updated

cnasikas approved these changes Dec 14, 2022

View reviewed changes

js-jankisalvi merged commit 412fa42 into elastic:main Dec 14, 2022

kibanamachine added the backport:skip This PR does not require backporting label Dec 14, 2022

cnasikas added the Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// label Jan 17, 2023

nastasha-solomon mentioned this pull request Feb 9, 2023

Changes to the case feature in 8.7 elastic/security-docs#2799

Closed

7 tasks

cnasikas mentioned this pull request May 16, 2023

Move case features to the correct section in kibana release notes #157805

Merged

js-jankisalvi deleted the save-draft-user-cmment branch October 6, 2023 07:36

adcoelho mentioned this pull request Jul 1, 2024

[ResponseOps][Cases] Allow users to create case using templates #187138

Merged

6 tasks


		removeItemFromSessionStorate(draftStorageKey);

		/* had to add to this check that session key is removed to avaoid weird issue that on reset

Conversation

js-jankisalvi commented Nov 24, 2022 • edited by cnasikas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Nov 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js-jankisalvi Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner Dec 2, 2022 • edited by cnasikas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js-jankisalvi Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Dec 14, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js-jankisalvi Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Dec 14, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

cnasikas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

js-jankisalvi commented Nov 24, 2022 •

edited by cnasikas

Loading

js-jankisalvi Dec 2, 2022 •

edited

Loading

jonathan-buttner Dec 2, 2022 •

edited by cnasikas

Loading

js-jankisalvi Dec 2, 2022 •

edited

Loading

js-jankisalvi Dec 14, 2022 •

edited

Loading

cnasikas left a comment •

edited

Loading