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

[Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case #76967

Merged
merged 11 commits into from
Sep 15, 2020

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Sep 8, 2020

Summary

The PR fixes a bug where an error occurred when trying to attach a timeline to an existing case or to a new case.

Attach timeline to a new case

bug1

bug2

Attach timeline to an existing case

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 8, 2020
@cnasikas cnasikas requested review from a team as code owners September 8, 2020 18:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@cnasikas cnasikas added the bug Fixes for quality problems that affect the customer experience label Sep 8, 2020
@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

code and manual testing LGTM. looks like you may need help from @MadameSheema on getting the cypress passing consistently.

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

// insertTimeline selector is defined to attached a timeline to a case outside of the case page.
// FYI, if you are in the case page we only use handleOnTimelineChange to attach a timeline to a case.
useEffect(() => {
const currentValue = form.getFormData()[fieldName];
Copy link
Contributor

Choose a reason for hiding this comment

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

@sebelga could you take a look at that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use the useFormData hook and directly provide the field value to this hook. For example, the AddComment would be

const { form } = useForm<CommentRequest>({
  defaultValue: initialCommentValue,
  options: { stripEmptyFields: false },
  schema,
});
const { setFieldValue, reset, submit } = form;

// Get the updated field value
const [{ comment }] = useFormData({ form, watch: ['comment'] });

// Pass it to the hook
const { handleCursorChange, handleOnTimelineChange } = useInsertTimeline<CommentRequest>(comment);

// I am not sure I understand what this does...
// If you need to format the field value on each keystroke, use a "formatter" on the field
// If you need to format the field when initiated, use a "deserializer" on the field.
const addQuote = useCallback(
  (quote) => {
    setFieldValue('comment', `${comment}${comment.length > 0 ? '\n\n' : ''}${quote}`);
  },
  [comment, setFieldValue]
);

Copy link
Member Author

Choose a reason for hiding this comment

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

@sebelga Thank you for your feedback! The addQuote function takes the content of a comment and make it a quote. This will only happen when the user clicks the add quote button and not on each keystroke or at the initiation of the field.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the explanation @cnasikas

Copy link
Contributor

@patrykkopycinski patrykkopycinski 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 using useFormData hook and adding Cypress tests, LGTM ❤️ 💪

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
securitySolution 10.0MB +4.0KB 10.0MB

distributable file count

id value diff baseline
default 45553 +1 45552

History

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

@cnasikas cnasikas merged commit 7524891 into elastic:master Sep 15, 2020
@cnasikas cnasikas deleted the fix_case_timeline_bug branch September 15, 2020 20:48
cnasikas added a commit to cnasikas/kibana that referenced this pull request Sep 15, 2020
… to a case (elastic#76967)

Co-authored-by: Gloria Hornero <snootchie.boochies@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
cnasikas added a commit that referenced this pull request Sep 16, 2020
…meline to a case (#76967) (#77547)

Co-authored-by: Gloria Hornero <snootchie.boochies@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Gloria Hornero <snootchie.boochies@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (95 commits)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  [@kbn/utils] Adds missing dependency (elastic#77536)
  Add the Enterprise Search logo to the Overview search result (elastic#77514)
  [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482)
  [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402)
  Adds @kbn/utils package (elastic#76518)
  Map layout changes (elastic#77132)
  [Security Solution] [Detections] EQL Rule Creation (elastic#76831)
  Adding test user to maps tests under documents source folder  (elastic#77245)
  Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 16, 2020
* master: (55 commits)
  [Grok] Fix missing error message in error toasts (elastic#77499)
  [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563)
  [ML] fix type in apply_influencer_filters_action (elastic#77495)
  [Lens] create reusable component for filters and range aggregation (elastic#77453)
  fix eslint violations
  Collapse alert chart previews by default (elastic#77479)
  [ML] Fixing field caps wrapper endpoint (elastic#77546)
  showing service maps when filte by environment not defined (elastic#77483)
  [Lens] Settings panel redesign and separate settings per y axis (elastic#76373)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 16, 2020
* master: (54 commits)
  [Grok] Fix missing error message in error toasts (elastic#77499)
  [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563)
  [ML] fix type in apply_influencer_filters_action (elastic#77495)
  [Lens] create reusable component for filters and range aggregation (elastic#77453)
  fix eslint violations
  Collapse alert chart previews by default (elastic#77479)
  [ML] Fixing field caps wrapper endpoint (elastic#77546)
  showing service maps when filte by environment not defined (elastic#77483)
  [Lens] Settings panel redesign and separate settings per y axis (elastic#76373)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 16, 2020
* master: (76 commits)
  Fixing service maps API test (elastic#77586)
  [Grok] Fix missing error message in error toasts (elastic#77499)
  [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563)
  [ML] fix type in apply_influencer_filters_action (elastic#77495)
  [Lens] create reusable component for filters and range aggregation (elastic#77453)
  fix eslint violations
  Collapse alert chart previews by default (elastic#77479)
  [ML] Fixing field caps wrapper endpoint (elastic#77546)
  showing service maps when filte by environment not defined (elastic#77483)
  [Lens] Settings panel redesign and separate settings per y axis (elastic#76373)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants