[Cases][AttachmentV2] Migrate Lens to unified registry#261407
[Cases][AttachmentV2] Migrate Lens to unified registry#261407christineweng merged 7 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/kibana-cases (Team:Cases) |
ef1545b to
acca786
Compare
| data: { | ||
| state: { attributes, timeRange, metadata }, | ||
| }, | ||
| } as unknown as UnifiedValueAttachmentWithoutOwner); |
There was a problem hiding this comment.
Do you think there is a way we can fix this type cast?
There was a problem hiding this comment.
The lens data contains LensSavedObjectAttributes (from @kbn/lens-plugin/public) which is a deeply nested type with specific Lens-domain properties -- these are structurally incompatible with JsonValue even though they serialize to JSON just fine at runtime.
Since { state: { attributes: LensSavedObjectAttributes, ... } } is not assignable to Record<string, JsonValue>, TypeScript requires the unknown intermediary. There is no way to avoid this without changing either the Lens types or the UnifiedValueAttachmentPayload type definition.
|
I wonder if we need to update kibana/x-pack/platform/plugins/shared/cases/server/client/cases/utils.ts Lines 93 to 114 in b9f9ec0 |
| const isRecord = (value: unknown): value is Record<string, unknown> => | ||
| typeof value === 'object' && value !== null; |
There was a problem hiding this comment.
You can use isPlainObject from lodash instead of this helper.
There was a problem hiding this comment.
isPlainObject alone is not enough because it's not a type guard, but I've updated the helper to use isPlainObject instead
| * Internal unified types are registered in | ||
| * x-pack/platform/plugins/shared/cases/server/internal_attachments/index.ts | ||
| */ | ||
| describe('Unified attachment types', () => { |
There was a problem hiding this comment.
What do you think about adding a full roundtrip for the new attachment types in an API test as well? E.g. with flag on, create a new attachment and read it back, same with flag off.
There was a problem hiding this comment.
wdyt if i do it in a separate PR and covers this, comment and event?
persistable state / lens was filtered out before this function is called kibana/x-pack/platform/plugins/shared/cases/server/client/cases/utils.ts Lines 270 to 281 in 87ac14f |
acca786 to
35a1211
Compare
Could you add a comment to |
35a1211 to
842d0f6
Compare
PhilippeOberti
left a comment
There was a problem hiding this comment.
LGTM for the @elastic/security-threat-hunting-investigations team
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
## Summary This PR is part 1 of migrating persistable state attachment type to V2 architecture. Lens is migrated, and other persistable state attachments (such as single metric viewer, more in [this ticket](elastic/security-team#15569 (comment))) should continue to behave with the feature flag off. Feature flag: `xpack.cases.attachments.enabled: true` (restart kibana after turning the flag on/off) How to test: - Lens attachment can be added via dashboards. In Security, go to Dashboard, Overview, click more action to add the lens chart to a case <img width="1011" height="325" alt="image" src="https://github.com/user-attachments/assets/fe566665-bab8-42b6-97e3-6340a90addbe" /> - Lens added before the change should continue to show - Feature flag off: Lens attachment can be added and saved to `cases-comments` SO - Feature flag on: Lens attachment can be added and saved to `cases-attachments` SO Note: turning off the feature flag will exclude attachments from the attachments SO, attachments added with flag on and disappearing when the flag is off is expected ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
… charts (#262597) ## Summary Dependecy: #261407 to be merged first This PR migrates the remaining persistable state attachments to the unified attachment registry. Case attachment type constants are moved to cases plugin, and a naming conversion is added to keep type names follow the same convention. Feature flag: xpack.cases.attachments.enabled: true (restart kibana after turning the flag on/off) ### Key changes per plugin: Cases: - Removed persistable state registry in user actions, updated related tests - Added schema validator on the client side Machine learning: - Updated persistable state registry to unified registry, updated the shape to match the new schema - Removed locally defined constants related to cases AiOps: - Updated persistable state registry to unified registry, updated the shape to match the new schema - Removed locally defined constants related to cases ### How to test: Check and confirm the charts can be added and viewed in a case with the feature flag off/on. Note that turning the flag on then off will hide any attachments added when the flag is on, this is expected. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
Summary
This PR is part 1 of migrating persistable state attachment type to V2 architecture. Lens is migrated, and other persistable state attachments (such as single metric viewer, more in this ticket) should continue to behave with the feature flag off.
Feature flag:
xpack.cases.attachments.enabled: true(restart kibana after turning the flag on/off)How to test:
cases-commentsSOcases-attachmentsSONote: turning off the feature flag will exclude attachments from the attachments SO, attachments added with flag on and disappearing when the flag is off is expected
Checklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.