-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ResponseOps] [Cases] Update mapping for case title #147341
[ResponseOps] [Cases] Update mapping for case title #147341
Conversation
Pinging @elastic/response-ops-cases (Feature:Cases) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! I tested and working as expected. I left some comments.
@@ -506,6 +507,8 @@ export const sortToSnake = (sortField: string | undefined): SortFieldCase => { | |||
case 'closedAt': | |||
case 'closed_at': | |||
return SortFieldCase.closedAt; | |||
case 'title': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if you rename the function form sortToSnake
to getSortField
or similar? The function does not convert from camel case to snake case any more.
fields: { | ||
keyword: { | ||
type: 'keyword', | ||
ignore_above: 160, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove it (my mistake, I know 🙂 ) in case it will cause a migration if we change it. The backend validation is enough. @rudolf if we change the ignore_above
in the future will it trigger a migration (reindex)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean having the default value for this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean remove the ignore_above: 160
entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would always reindex if any key was removed from the mappings (like removing the ignore_above
) or if updating the mappings of the existing index fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rudolf! What I meant is if we changing the ignore_above
value from 160
to 200
will trigger a migration? I am just curious 🙂. We will not add it (never had it before this PR) so we do not have this dilemma in the future.
… the PR comments.
💚 Build Succeeded
Metrics [docs]Saved Objects .kibana field count
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Connected to #132041
Summary
This is the first in a series of PRs with migrations of the Cases' Saved objects to enable sorting by additional fields in the all-cases view.
In this PR the case title becomes a multi-field with an additional keyword field for sorting.
Added a small integration test to confirm sorting by title works.