[Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy #140399

logeekal · 2022-09-09T13:26:17Z

Summary

Fixes #119593
Fixes #140614

‼️ Important

Please go through #140614 to get the context of this PR.

tl,dr;

Timeline bottom bar has been enabled on below pages.

Security / Dashboards
Security / Explore
Security / Cases / ....

When user navigates to any other pages within security solution where timeline bottom bar is not visible, use will be reminded that they have an unsaved timeline.

Justification for enabling timeline bottom bar on above pages is to prevent users from getting too many timeline save reminders if they navigate within Security Solution plugin.

Detailed Workflow

Currently, when user navigates away from Security Solution plugin, system asks user to either Save/Discard the timeline.

When User discards the timeline
1. System will again ask user to save an unsaved timeline when user tried to navigate away after timeline has been discarded.
2. This is because before adding Title and description of the timeline, Timeline is in draft state. In draft state, it can either contain some changes by the user OR none at all.
3. Generally, system identifies by the update date. If it has a value, means user has done some updates, if it is a new timeline, updateDate is undefined.
4. This PR changes updateDate to undefined if user decides to discard the timeline.

There 2 workflow issues that are highlighted in the bug :

[ Fixed ] Issue 1

Steps to Reproduce*

Login into an instance with data
Go to the Overview page
Navigate to a page outside Security Solution application

This has been resolved as stated above.

[ Fixed ] Issue 2

Preconditions

Kibana should be running
Untitled timeline with events should be exist

Steps to Reproduce

Navigate to endpoints tab of security
Click on actions icons
Click on "View agent policy"
Unsaved changes modal will be open.
Click on cancel button
Now click any other tab
Observe after click on cancel button of unsaved timeline modal Timeline save modal is only displayed if we click on some other tab

Please go through #140614 for the detailed discussion on this workflow

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…meline prompt

michaelolo24 · 2022-09-27T13:51:15Z

x-pack/plugins/security_solution/cypress/e2e/timelines/unsaved_timeline.cy.ts

+import { createNewTimeline, populateTimeline, waitForTimelineChanges } from '../../tasks/timeline';
+import { HOSTS_URL } from '../../urls/navigation';
+
+describe('Save Timeline Prompts', () => {


michaelolo24 · 2022-09-27T13:52:26Z

x-pack/plugins/security_solution/public/timelines/components/flyout/index.tsx


 import type { AppLeaveHandler } from '@kbn/core/public';
-import { TimelineId, TimelineStatus, TimelineTabs } from '../../../../common/types/timeline';
+import type { TimelineId } from '../../../../common/types/timeline';
+import {} from '../../../../common/types/timeline';


shadow import 😄

✔ Corrected now.

logeekal · 2022-09-27T14:31:18Z

Files by Code Owner

elastic/security-solution

x-pack/plugins/security_solution/cypress/e2e/timelines/unsaved_timeline.cy.ts
x-pack/plugins/security_solution/cypress/screens/kibana_navigation.ts
x-pack/plugins/security_solution/cypress/tasks/alerts_detection_rules.ts
x-pack/plugins/security_solution/cypress/tasks/timeline.ts
x-pack/plugins/security_solution/public/cases/links.ts
x-pack/plugins/security_solution/public/landing_pages/links.ts
x-pack/plugins/security_solution/public/timelines/components/flyout/index.tsx
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/use_create_timeline.test.tsx
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/use_create_timeline.tsx
x-pack/plugins/security_solution/public/timelines/components/use_timeline_save_prompt/index.ts
x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts

elastic/security-threat-hunting-explore

x-pack/plugins/security_solution/public/cases/links.ts

elastic/security-threat-hunting-investigations

x-pack/plugins/security_solution/public/timelines/components/flyout/index.tsx
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/use_create_timeline.test.tsx
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/use_create_timeline.tsx
x-pack/plugins/security_solution/public/timelines/components/use_timeline_save_prompt/index.ts
x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts

michaelolo24 · 2022-09-27T14:38:18Z

@logeekal this issue was probably there prior to these changes, but the timeline isn't actually deleted once a user navigates away, most likely due to it's preservation in localstorage. It might be worth removing it from localstorage if the user confirms they want it to be removed. Alternatively we could change the message around?

Screen.Recording.2022-09-27.at.10.34.12.AM.mov

logeekal · 2022-09-27T14:43:19Z

@logeekal this issue was probably there prior to these changes, but the timeline isn't actually deleted once a user navigates away, most likely due to it's preservation in localstorage. It might be worth removing it from localstorage if the user confirms they want it to be removed. Alternatively we could change the message around?

Thanks for pointing this error out. Will correct this.

But I thought about removing data from local storage but I assumed that when user clicks on Confirm (i.e. I want to navigate away ), they do NOT necessary want to discard the timeline. They are just taking the risk that they might lose it if they forgot about it.

So at the time, I did not see the need to reset/discard timeline contents when user navigates away.

What do you think?

michaelolo24 · 2022-09-27T14:47:43Z

So at the time, I did not see the need to reset/discard timeline contents when user navigates away.

What do you think?

100% agree. I think if there's anything it might be instead of showing the save modal on return to a timeline enabled page, we either show nothing or show some notification that their unsaved timeline was restored. 🤷🏾‍♂️ Not sure which the best route is

logeekal · 2022-09-27T15:05:05Z

100% agree. I think if there's anything it might be instead of showing the save modal on return to a timeline enabled page, we either show nothing or show some notification that their unsaved timeline was restored. 🤷🏾‍♂️ Not sure which the best route is

Hmm that is a great idea. I have added the fix where I do not display anything i.e Timeline stays as it was. I am leaning towards displaying a toast/notification but wondering how easy it will be to determine that we have restored some changes. Let me get back to you on this.

x-pack/plugins/security_solution/public/timelines/components/use_timeline_save_prompt/index.ts

semd

@logeekal that looks great! Thanks for doing it. I've written some comments.
Also, about the location of the public/timelines/components/use_timeline_save_prompt/index.ts, it is the only hook in this /components directory. Probably it would make more sense to move it to another place, alternatives:

public/common/utils/timeline/use_timeline_save_prompt.ts
public/common/hooks/timeline/use_timeline_save_prompt.ts
public/timelines/hooks/use_timeline_save_prompt.ts (new /hooks dir)

x-pack/plugins/security_solution/public/timelines/components/use_timeline_save_prompt/index.ts

semd

Thank you @logeekal for doing the changes and for taking the time to answer the questions.
LGTM! 🚀

michaelolo24

Thanks for making the updates. Code looks good and works really well!

kibana-ci · 2022-10-05T15:45:34Z

💚 Build Succeeded

Buildkite Build
Commit: f7c9763

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3148	3150	+2

Async chunks

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

id	before	after	diff
`securitySolution`	6.6MB	6.6MB	+314.0B

Page load bundle

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

id	before	after	diff
`securitySolution`	264.5KB	264.8KB	+300.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`securitySolution`	71	73	+2

History

💚 Build #76827 succeeded 6fb0097
💔 Build #76501 failed 01d1eff
💔 Build #76431 failed 2e04ab9
💔 Build #76098 failed 312c808
💛 Build #75548 was flaky d2456f3
💚 Build #71248 succeeded 2ee7f30c5a7974a91624a73f42513b0d1c6d52b8

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

kibanamachine · 2022-10-07T16:11:45Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 140399 locally

kibanamachine · 2022-10-10T16:21:42Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 140399 locally

…ave prompt Strategy (elastic#140399) Please go through elastic#140614 to get the context of this PR. _tl,dr;_ Timeline bottom bar has been enabled on below pages. - Security / Dashboards - Security / Explore - Security / Cases / .... When user navigates to any other pages within security solution where `timeline bottom bar` is not visible, use will be reminded that they have an unsaved timeline. Justification for enabling `timeline bottom bar` on above pages is to prevent users from getting too many timeline save reminders if they navigate within `Security Solution` plugin.

logeekal · 2022-10-11T11:11:57Z

💚 All backports created successfully

Status	Branch	Result
✅	8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ave prompt Strategy (elastic#140399) Please go through elastic#140614 to get the context of this PR. _tl,dr;_ Timeline bottom bar has been enabled on below pages. - Security / Dashboards - Security / Explore - Security / Cases / .... When user navigates to any other pages within security solution where `timeline bottom bar` is not visible, use will be reminded that they have an unsaved timeline. Justification for enabling `timeline bottom bar` on above pages is to prevent users from getting too many timeline save reminders if they navigate within `Security Solution` plugin. (cherry picked from commit 803189f)

…ave prompt Strategy (#140399) (#143050) Please go through #140614 to get the context of this PR. _tl,dr;_ Timeline bottom bar has been enabled on below pages. - Security / Dashboards - Security / Explore - Security / Cases / .... When user navigates to any other pages within security solution where `timeline bottom bar` is not visible, use will be reminded that they have an unsaved timeline. Justification for enabling `timeline bottom bar` on above pages is to prevent users from getting too many timeline save reminders if they navigate within `Security Solution` plugin. (cherry picked from commit 803189f)

…ave prompt Strategy (elastic#140399) Please go through elastic#140614 to get the context of this PR. _tl,dr;_ Timeline bottom bar has been enabled on below pages. - Security / Dashboards - Security / Explore - Security / Cases / .... When user navigates to any other pages within security solution where `timeline bottom bar` is not visible, use will be reminded that they have an unsaved timeline. Justification for enabling `timeline bottom bar` on above pages is to prevent users from getting too many timeline save reminders if they navigate within `Security Solution` plugin.

logeekal force-pushed the fix/unsaved_timeline_modal branch from cdbc409 to ddbc017 Compare September 9, 2022 14:00

logeekal changed the title ~~fix: set updateDate to null creating new timeline~~ [Security Solution][Fix]-Reset discarded timeline Sep 12, 2022

logeekal marked this pull request as ready for review September 12, 2022 11:02

logeekal requested a review from a team as a code owner September 12, 2022 11:02

logeekal added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.4.2 labels Sep 12, 2022

logeekal added 2 commits September 26, 2022 11:50

fix: set updateDate to null creating new timeline

b0421a9

Updated tests

d2456f3

logeekal force-pushed the fix/unsaved_timeline_modal branch from 2ee7f30 to d2456f3 Compare September 26, 2022 09:50

refactor(timelineSavePrompt): updated strategy for showing unsaved ti…

8f82932

…meline prompt

logeekal requested review from a team as code owners September 27, 2022 13:07

logeekal changed the title ~~[Security Solution][Fix]-Reset discarded timeline~~ [Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy Sep 27, 2022

michaelolo24 reviewed Sep 27, 2022

View reviewed changes

test: more + improved tests

6be5617

tests: test for a new edge case

312c808

semd reviewed Sep 27, 2022

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/use_timeline_save_prompt/index.ts Outdated Show resolved Hide resolved

semd reviewed Sep 27, 2022

View reviewed changes

refactor: update use show timeline logic

85b6cb6

logeekal force-pushed the fix/unsaved_timeline_modal branch from 01d1eff to 85b6cb6 Compare September 29, 2022 11:23

Merge branch 'main' into fix/unsaved_timeline_modal

6fb0097

semd approved these changes Sep 29, 2022

View reviewed changes

yctercero approved these changes Sep 30, 2022

View reviewed changes

michaelolo24 approved these changes Oct 5, 2022

View reviewed changes

Merge branch 'main' into fix/unsaved_timeline_modal

f7c9763

logeekal enabled auto-merge (squash) October 5, 2022 13:57

logeekal merged commit 803189f into elastic:main Oct 5, 2022

logeekal self-assigned this Oct 6, 2022

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 7, 2022

logeekal mentioned this pull request Oct 11, 2022

[8.5] [Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy (#140399) #143050

Merged

kibanamachine added v8.5.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy #140399

[Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy #140399

logeekal commented Sep 9, 2022 •

edited

Loading

michaelolo24 Sep 27, 2022

michaelolo24 Sep 27, 2022

logeekal Sep 27, 2022

logeekal commented Sep 27, 2022

michaelolo24 commented Sep 27, 2022

logeekal commented Sep 27, 2022

michaelolo24 commented Sep 27, 2022 •

edited

Loading

logeekal commented Sep 27, 2022 •

edited

Loading

semd left a comment

semd left a comment

michaelolo24 left a comment

kibana-ci commented Oct 5, 2022

References to deprecated APIs

kibanamachine commented Oct 7, 2022

kibanamachine commented Oct 10, 2022

logeekal commented Oct 11, 2022

[Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy #140399

[Security Solution][Fix]-Reset discarded timeline & Change timeline Save prompt Strategy #140399

Conversation

logeekal commented Sep 9, 2022 • edited Loading

Summary

‼️ Important

Detailed Workflow

[ Fixed ] Issue 1

[ Fixed ] Issue 2

Checklist

For maintainers

michaelolo24 Sep 27, 2022

Choose a reason for hiding this comment

michaelolo24 Sep 27, 2022

Choose a reason for hiding this comment

logeekal Sep 27, 2022

Choose a reason for hiding this comment

logeekal commented Sep 27, 2022

Files by Code Owner

elastic/security-solution

elastic/security-threat-hunting-explore

elastic/security-threat-hunting-investigations

michaelolo24 commented Sep 27, 2022

logeekal commented Sep 27, 2022

michaelolo24 commented Sep 27, 2022 • edited Loading

logeekal commented Sep 27, 2022 • edited Loading

semd left a comment

Choose a reason for hiding this comment

semd left a comment

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 5, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

References to deprecated APIs

History

kibanamachine commented Oct 7, 2022

kibanamachine commented Oct 10, 2022

logeekal commented Oct 11, 2022

💚 All backports created successfully

Questions ?

logeekal commented Sep 9, 2022 •

edited

Loading

michaelolo24 commented Sep 27, 2022 •

edited

Loading

logeekal commented Sep 27, 2022 •

edited

Loading