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

[Cases] Total external references and persistable state attachments per case #162071

Merged
merged 12 commits into from
Jul 25, 2023

Conversation

adcoelho
Copy link
Contributor

Connected to #146945

Summary

Description Limit Done? Documented?
Total number of attachments (external references and persistable state) per case 100 No

Checklist

Delete any items that are not applicable to this PR.

Release Notes

A case can now only have 100 external references and persistable state(excluding files) attachments combined.

@adcoelho adcoelho added release_note:breaking Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.10.0 labels Jul 17, 2023
@adcoelho adcoelho self-assigned this Jul 17, 2023
@adcoelho adcoelho marked this pull request as ready for review July 20, 2023 16:10
@adcoelho adcoelho requested a review from a team as a code owner July 20, 2023 16:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

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

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Great work! Could you please add an integration test to test the validation? You can bulk create the attachments with one call. It should be fast.

x-pack/plugins/cases/server/client/cases/mock.ts Outdated Show resolved Hide resolved
x-pack/plugins/cases/server/client/cases/mock.ts Outdated Show resolved Hide resolved
x-pack/plugins/cases/server/services/attachments/index.ts Outdated Show resolved Hide resolved
createAlertRequests,
createExternalReferenceRequests,
createPersistableStateRequests,
createUserRequests,
Copy link
Member

Choose a reason for hiding this comment

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

nit: There are the same mocks in x-pack/plugins/cases/server/mocks.ts or x-pack/test/cases_api_integration/common/lib/mock.ts. Otherwise, we can extract these mock functions to x-pack/plugins/cases/server/mocks.ts.

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Changes looks good to me 👍 🎉
May be you can add an api integration test if possible.

@adcoelho
Copy link
Contributor Author

Added integration tests for the internal bulk_create and addComment.

service.countPersistableStateAndExternalReferenceAttachments({ caseId: 'test-id' })
).resolves.not.toThrow();

await expect(unsecuredSavedObjectsClient.find).toHaveBeenCalledWith(
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need the await here.

).resolves.not.toThrow();

await expect(unsecuredSavedObjectsClient.find).toHaveBeenCalledWith(
expect.objectContaining({
Copy link
Member

Choose a reason for hiding this comment

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

I think we should validate the filter passed to unsecuredSavedObjectsClient.find. You can do

expect(unsecuredSavedObjectsClient.find.mock.calls[0][0]).toMatchInlineSnapshot(...)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

🚀

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @adcoelho

@adcoelho adcoelho merged commit 7429c82 into elastic:main Jul 25, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2023
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
…er case (elastic#162071)

Connected to elastic#146945

## Summary

| Description  | Limit | Done? | Documented?
| ------------- | ---- | :---: | ---- |
| Total number of attachments (external references and persistable
state) per case | 100 | ✅ | No |

### Checklist

Delete any items that are not applicable to this PR.

- [x]
[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

### Release Notes

A case can now only have 100 external references and persistable
state(excluding files) attachments combined.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Cases Cases feature release_note:breaking Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants