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 number of user actions on a case. #161848

Merged
merged 17 commits into from
Jul 27, 2023

Conversation

adcoelho
Copy link
Contributor

Connected to #146945

Summary

Description Limit Done? Documented?
Total number of user actions and comments combined on a case 10000 No

Checklist

Delete any items that are not applicable to this PR.

Release Notes

Updating a case will now fail if the operation makes it reach more than 10000 user actions.

@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 13, 2023
@adcoelho adcoelho self-assigned this Jul 13, 2023
@adcoelho adcoelho force-pushed the total-user-actions-comments branch from e85a59a to eba58af Compare July 14, 2023 13:16
}: BulkCreateBulkUpdateCaseUserActions): Promise<void> {
const builtUserActions = updatedCases.reduce<UserActionEvent[]>((acc, updatedCase) => {
const originalCase = originalCases.find(({ id }) => id === updatedCase.id);
public buildUserActions({ updatedCases, user }: BuildUserActionsDictParams): UserActionsDict {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An UserActionsDict is a dictionary where the key is the caseId and the value is an array of all user actions to be created.

This will help us during the validation where we can check for each case independently if they will break the limit.

userActionsDict: UserActionsDict;
userActionService: CaseUserActionService;
}) {
const result = await userActionService.getMultipleCasesUserActionsTotal({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do the aggregation for each case being updated.

caseIds: Object.keys(userActionsDict),
});

result.aggregations?.references.caseUserActions.buckets.forEach(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For each case check if the result of the aggregation + the user actions to be created is bigger than the max allowed value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean once reaching maximum limit user can't delete or update the existing comment?

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 job with splitting the logic of the creation of user actions. I did a review only of the code. I will do another pass when the PR is ready for review.

@adcoelho adcoelho marked this pull request as ready for review July 19, 2023 14:59
@adcoelho adcoelho requested a review from a team as a code owner July 19, 2023 14:59
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

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.

Complex validation 😅 , great work 👍

@@ -500,13 +504,73 @@ describe('CaseUserActionService', () => {
});
});

describe('buildUserActions', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

great work with tests and mocks 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, it was such a headache 😅

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 job 🚀!

Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a test where you check updating multiple cases with multiple attributes to update? Some cases should fail and some of them not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure I understood

Some cases should fail and some of them not.

but the call is expected to fail right? because some cases in the payload will hit the limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed some changes, is this what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is what I have in mind, thanks!

@@ -141,4 +164,59 @@ describe('UserActionPersister', () => {
});
});
});

describe('buildUserActions', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we test more user action types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which ones?

All tests have different payloads and:

  • patchCasesRequest changes status, title, connector, and category.
  • patchAssigneesCasesRequest, patchRemoveAssigneesCasesRequest and patchAddRemoveAssigneesCasesRequest handle the assignees
  • and patchTagsCasesRequest the tags user actions

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed that the patchCasesRequest contains all these changes. It is difficult to verify the tests without having inline snapshots. Thanks!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 664 665 +1

Async chunks

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

id before after diff
cases 421.5KB 421.6KB +27.0B

Page load bundle

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

id before after diff
cases 150.5KB 150.5KB +43.0B

History

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

cc @adcoelho

@adcoelho adcoelho merged commit 33195fb into elastic:main Jul 27, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 27, 2023
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
Connected to elastic#146945

## Summary

| Description  | Limit | Done? | Documented?
| ------------- | ---- | :---: | ---- |
| Total number of user actions and comments combined on a case | 10000 |
✅ | 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

Updating a case will now fail if the operation makes it reach more than
10000 user actions.
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