adds createdAt and updatedAt fields to alerting by gmmorris · Pull Request #53793 · elastic/kibana

gmmorris · 2019-12-24T13:15:57Z

Summary

When users are writing UI's they need to see when an alert was created and when it was last updated, to this end we've added these two fields on the alert type.

closes #52738

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-24T13:17:10Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

kibanamachine · 2019-12-24T14:38:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: dcd574e

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

kibanamachine · 2019-12-24T18:42:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1721ec2

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

* master: Possibility to filter when testing scripted fields (elastic#35379) (elastic#44220) Update maps telemetry mappings to account for recent updates (elastic#53803) [Maps] Only show legend when layer is visible (elastic#53781) remove use of experimental fs.promises api (elastic#53346) [APM] Add log statements for flaky test (elastic#53775)

kibanamachine · 2019-12-27T12:54:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 430fe9b

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

gmmorris · 2019-12-30T10:38:34Z

@elasticmachine merge upstream

mikecote

Mostly side notes and a comment but LGTM!

x-pack/legacy/plugins/alerting/server/alerts_client.ts

mikecote · 2019-12-31T13:38:25Z

x-pack/legacy/plugins/alerting/server/alerts_client.ts

+    return this.getAlertFromRaw(
+      createdAlert.id,
+      createdAlert.attributes,
+      createdAlert.updated_at,


One thing worth noting in regards to comment #52738 (comment): This won't allow users to sort on the last time the alert was updated.

I'm not sure if this is a requirement but if ever it becomes, a separate PR to add updated_at to the following file is all we'd have to do:

kibana/src/core/server/saved_objects/service/lib/search_dsl/sorting_params.ts

Line 23 in e233e41

const TOP_LEVEL_FIELDS = ['_id', '_score'];

Ah, that's an interesting point.
Thank's for pointing it out.

x-pack/legacy/plugins/alerting/server/alerts_client.ts

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts

…ris/kibana into alerting/created_at-and-updated_at * 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana: Allow chromeless applications to render via non-/app routes (elastic#51527) Add server rendering service to enable standalone route rendering (elastic#52161)

* master: Bump year in NOTICE.txt Add kibanamachine support to Github PR comments (elastic#53852) Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333) Skip failing test suite [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799) Do not remount applications between page navigations (elastic#53851) [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129) [Canvas] Migrate usage collector to NP plugin (elastic#53303)

pmuellr

I'm curious about the cases where updatedAt can be null, because it doesn't seem like that would happen in real life. Also, I think the update functional tests could test a bit more (eg that update date > create date).

Other than that LGTM.

pmuellr · 2020-01-02T20:10:00Z

x-pack/legacy/plugins/alerting/server/alerts_client.test.ts

          "interval": "10s",
        },
        "scheduledTaskId": "task-123",
+        "updatedAt": null,


strange seeing all the "updatedAt": null lines, where createdAt is not null, but I thinks that's because the updated_at value in the saved objects mock is null. Could happen IRL, so not a bad test! But weird :-)

pmuellr · 2020-01-02T20:13:19Z

x-pack/legacy/plugins/alerting/server/routes/get.test.ts

    bar: true,
  },
+  createdAt: new Date(),
+  updatedAt: null,


I would think the normal case would be that updatedAt would never be null, so seems like it should be set to a non-null value here.

pmuellr · 2020-01-02T20:17:28Z

x-pack/legacy/plugins/alerting/server/types.ts

  createdBy: string | null;
  updatedBy: string | null;
+  createdAt: Date;
+  updatedAt: Date | null;


hmmm ... now I'm really wondering about when updatedAt can be null, and createdAt can't be null. That doesn't seem right. Upon creation, I'd think createdAt and updatedAt would be the same value, and then updatedAt will get updated from then on. So, updatedAt can't really ever be null, seems like.

pmuellr · 2020-01-02T20:24:12Z

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts

+                createdAt: response.body.createdAt,
+                updatedAt: response.body.updatedAt,
              });
+              expect(Date.parse(response.body.createdAt)).to.be.greaterThan(0);


I think for the update tests, we should check that the create/update dates:

aren't the same

update > create

Can probably just make this one test, something like:

expect(Date.parse(response.body.updatedAt)).to.be.greaterThan(Date.parse(response.body.createdAt));

gmmorris

I'm curious about the cases where updatedAt can be null, because it doesn't seem like that would happen in real life. Also, I think the update functional tests could test a bit more (eg that update date > create date).

Other than that LGTM.

@pmuellr
The reason updatedAd can be null is that we do not set it's value, it's provided by SavedObjects client, and there it's Date | undefined.
We talked in the past about enforcing null in our concrete types, hence the undefined => null, but as we do not set the value, it didn't feel right to suddenly set it.
We could use the value of createdAt on updatedAt if SavedObjects fails to provide it for some reason.
Thoughts?

The above is also why I didn't test that createdAt < updatedAt, as we don't set the value of updatedAt, but I can add that assertion in the functional test, which I guess could catch some underlying breakage I guess, except it'll fail if we then use the value of createdAt in updatedAt, in which case it could become a <=. 🤷‍♂

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts

gmmorris · 2020-01-03T11:47:56Z

@elasticmachine merge upstream

…ris/kibana into alerting/created_at-and-updated_at * 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use handlebars@4.5.3 (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)

* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)

pmuellr

LGTM with the new tests comparing created/updated times in the functional tests

kibanamachine · 2020-01-03T16:27:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6dff233

History

💔 Build #17758 failed cf24b03
💔 Build #17741 failed db0d578
💔 Build #17726 failed 954a71a
💚 Build #17522 succeeded fe1902e
💚 Build #17393 succeeded b6c9385

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

When users are writing UI's they need to see when an alert was created and when it was last updated, to this end we've added these two fields on the alert type.

* master: [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

When users are writing UI's they need to see when an alert was created and when it was last updated, to this end we've added these two fields on the alert type.

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893)

…nsole-dependencies * 'master' of github.com:elastic/kibana: (33 commits) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768) increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) ... # Conflicts: # yarn.lock

added createdAt and updatedAt fields in alerting

dcd574e

gmmorris added Feature:Alerting Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.6.0 v8.0.0 labels Dec 24, 2019

gmmorris added the release_note:skip Skip the PR/issue when compiling release notes label Dec 24, 2019

use the SavedObjets updated_at instead of a new field in alerting

1721ec2

gmmorris marked this pull request as ready for review December 27, 2019 09:13

gmmorris changed the title ~~[DRAFT] adds createdAt and updatedAt fields to alerting~~ adds createdAt and updatedAt fields to alerting Dec 27, 2019

Merge branch 'master' into alerting/created_at-and-updated_at

b6c9385

mikecote approved these changes Dec 31, 2019

View reviewed changes

gmmorris added 3 commits January 2, 2020 08:57

added missing assetion

e8dbc15

pmuellr reviewed Jan 2, 2020

View reviewed changes

gmmorris commented Jan 3, 2020

View reviewed changes

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts Show resolved Hide resolved

gmmorris added 4 commits January 3, 2020 09:56

used createdAt in place of empty updatedAt

1fb5735

fixed misuse of null on updatedAt

2f50541

fixed tests

5a4057d

fixed tests

954a71a

elasticmachine and others added 3 commits January 3, 2020 03:48

Merge branch 'master' into alerting/created_at-and-updated_at

db0d578

updatedAt should equal createdAt on creation

924393f

Merge branch 'master' into alerting/created_at-and-updated_at

cf24b03

* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)

pmuellr approved these changes Jan 3, 2020

View reviewed changes

removed assertions for updatedAt in non-update operations

6dff233

gmmorris merged commit d4b1966 into elastic:master Jan 3, 2020

gmmorris mentioned this pull request Jan 3, 2020

[7.x] adds createdAt and updatedAt fields to alerting (#53793) #53952

Merged

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 3, 2020

Merge branch 'master' into alerting/client-types

ae0314d

* master: [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

gmmorris added the backported label Jan 6, 2020

pmuellr mentioned this pull request Nov 6, 2020

[Alerting] the alert updated_at field is no representative of the last update by a user #82701

Closed

Conversation

gmmorris commented Dec 24, 2019

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 24, 2019

Uh oh!

kibanamachine commented Dec 24, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 24, 2019

💚 Build Succeeded

Uh oh!

kibanamachine commented Dec 27, 2019

💚 Build Succeeded

Uh oh!

gmmorris commented Dec 30, 2019

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikecote Dec 31, 2019

Choose a reason for hiding this comment

Uh oh!

gmmorris Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmmorris commented Jan 3, 2020

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 3, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants