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] Add the ability to filter for cases without assignees #143390

Merged
merged 8 commits into from
Oct 18, 2022

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Oct 14, 2022

Summary

This PR adds the ability to filter for cases without assignees. The backend accepts an array of strings to filter assignees. Each string is expected to be a valid user profile ID. We introduce the none keyword to filter cases without assignees. The api/cases/_find?assignees=none API call brings all cases without assignees. The api/cases/_find?assignees=none&assignees=123 will bring all cases without assignees OR all cases where the user with profile ID 123 is assigned to. We considered null as an option to filter unassigned cases but JS converts null query parameters to a string "null" which makes it unintuitive. The frontend uses the null value to indicate the "No assignees" filter option but before making the call to the backend it converts it to the "none" value.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release notes

Filter cases without assignees

@cnasikas cnasikas added release_note:enhancement backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.6.0 labels Oct 14, 2022
@cnasikas cnasikas self-assigned this Oct 14, 2022
@cnasikas cnasikas marked this pull request as ready for review October 17, 2022 10:22
@cnasikas cnasikas requested a review from a team as a code owner October 17, 2022 10:22
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Looking good, tested it too. Couple questions and changes.

@@ -352,3 +353,10 @@ export type AllReportersFindRequest = AllTagsFindRequest;

export type GetCaseIdsByAlertIdAggs = rt.TypeOf<typeof GetCaseIdsByAlertIdAggsRt>;
export type CasesByAlertId = rt.TypeOf<typeof CasesByAlertIdRt>;

export type CasesFindQueryParams = Partial<
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of an internal type to the backend do we need to expose it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to use it in the UI but in the end, I did not and I forgot to remove it. Thanks!

`);
});

it('hides non assignee filtering when searching', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

non -> no

(users: AssigneesFilteringSelection[]) => {
const usersWithNoAssigneeSelection = removeNoAssigneesSelection(users);
const sortedUsers =
bringCurrentUserToFrontAndSort(currentUserProfile, usersWithNoAssigneeSelection) ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make the code easier if we modified bringCurrentUserToFrontAndSort and related functions to handle null? Maybe they could put the nulls at the front and we could just rename it to orderAssignees or something like that.

Copy link
Member Author

@cnasikas cnasikas Oct 18, 2022

Choose a reason for hiding this comment

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

The bringCurrentUserToFrontAndSort is used in other places where the concept of "no assignees" is not used. I created a helper function called orderAssigneesIncludingNone and put the logic of the onChange hook in it. Let me know if you want me to do something different.

if (!isEqual(newAssignees, selectedAssignees)) {
setSelectedAssignees(newAssignees);
onFilterChanged({ assignees: newAssignees.map((assignee) => assignee.uid) });
onFilterChanged({
assignees: newAssignees.map((assignee) => (assignee != null ? assignee.uid : null)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work?

onFilterChanged({ assignees: newAssignees.map((assignee) => assignee?.uid ?? null) })

});
});

test('should not send the assignees field if it an empty array', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, the fetchMock will be called with an empty assignees array right? If so then maybe we change the test text to say should send an empty assignees field when it is an empty array for the filters options or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fetchMock should not be called with an empty assignees array. If you pass an empty array the URL will contain unnecessary &. For example api/cases/_find?&&sortOrder='desc'. Do you want me to rephrase the text to make it clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I misread the expect. Sounds good!

) ?? NO_ASSIGNEES_FILTERING_KEYWORD,
}
: {}),
...(filterOptions.reporters.length > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the entire logic for reporters inside of the constructReportersFilter so it returns an object that can be spread.

(assignee) => assignee !== NO_ASSIGNEES_FILTERING_KEYWORD
);
const hasNoneAssignee =
assigneesAsArray.find((assignee) => assignee === NO_ASSIGNEES_FILTERING_KEYWORD) !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be written with assigneesAsArray.some() to avoid the ? true : false?

return nodeBuilder.or(assigneesFilter);
}

const filterCasesWithoutAssigneesKueryNode = stringToKueryNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use fromKueryExpression directly and avoid the if block below.

auth: { user: superUser, space: 'space1' },
});

const caseWithNoAssignees = await createCase(supertest, postCaseReq);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be created using a Promise.all or do we need them in a certain order?

expect(await secondCaseTitle.getVisibleText()).be('matchme');
});

it('filters cases without assignees', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test name is the same as above. Is it testing that one of the cases is returned because it matches an assignee? Maybe update it to filters cases with and without assignees.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
cases 372.3KB 372.9KB +652.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 125.8KB 126.1KB +297.0B

History

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

cc @cnasikas

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Nice work!

@cnasikas cnasikas merged commit 6324008 into elastic:main Oct 18, 2022
@cnasikas cnasikas deleted the no_assignees_filtering branch October 18, 2022 13:59
kc13greiner pushed a commit to kc13greiner/kibana that referenced this pull request Oct 18, 2022
…c#143390)

* Return unsigneed cases

* Fix filter

* Change to none

* Add and fix testing

* PR feedback
@EricDavisX
Copy link
Contributor

tested from local cluster - with 2001 cases and confirmed the functional use case works as well as performance concern; there was no performance concern, approximately 2000 cases returned from the 'no assignees' selection near instantaneously.

cnasikas added a commit that referenced this pull request Nov 14, 2022
## Summary

With the introduction of the "No assignees" filtering in
#143390 we no longer have
assignees for filtering. Having the text say "1 assignee selected" when
selecting the "No assignees" filtering is misleading. This PR fixes this
issue with the label.

<img width="536" alt="Screenshot 2022-11-14 at 3 52 28 PM"
src="https://user-images.githubusercontent.com/7871006/201680341-e007931f-e9d2-4e5f-96d9-0909f4478bc8.png">
<img width="536" alt="Screenshot 2022-11-14 at 3 52 20 PM"
src="https://user-images.githubusercontent.com/7871006/201680344-76b8bf04-8d0e-48ee-97ab-0d860f076f3f.png">
<img width="551" alt="Screenshot 2022-11-14 at 3 51 48 PM"
src="https://user-images.githubusercontent.com/7871006/201680346-a3525a41-cef2-43a9-aa89-7d9238bb3944.png">


### Checklist

Delete any items that are not applicable to this PR.

- [x] 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/packages/kbn-i18n/README.md)
- [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

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
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:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants