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] Added Updated on column in Cases table #149116

Merged
merged 6 commits into from Jan 18, 2023

Conversation

js-jankisalvi
Copy link
Contributor

Summary

Fixes #148871

Adds new column Updated on in Cases table. This column can be sorted and can persist sorting options.

image

Checklist

Release notes

  • Adds new column Updated on in all cases list table. This column can be sorted and can persist sorting options.

@js-jankisalvi js-jankisalvi 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 labels Jan 18, 2023
@js-jankisalvi js-jankisalvi requested a review from a team as a code owner January 18, 2023 12:35
@js-jankisalvi js-jankisalvi self-assigned this Jan 18, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@js-jankisalvi js-jankisalvi requested a review from a team January 18, 2023 12:35
render: (updatedAt: Case['updatedAt']) => {
if (updatedAt != null) {
return (
<span data-test-subj={`case-table-column-updatedAt`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for { } can just be a string.

@@ -519,6 +520,9 @@ export const convertSortField = (sortField: string | undefined): SortFieldCase =
return SortFieldCase.title;
case 'severity':
return SortFieldCase.severity;
case 'updatedAt':
case 'updated_at':
return SortFieldCase.updatedAt;
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw createdAt/_at and closedAt/_at do the same but why do we need both conversions?

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 don't know to be honest, but first I added updated_at only and sorting didn't work.
Then I added updatedAt and it worked 😄 And it makes sense because I added updatedAt sorting filed.

So I assumed updated_at might be used somewhere. Couldn't find it assigned to sortField anywhere though 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I added updatedAt and it worked 😄 And it makes sense because I added updatedAt sorting filed.

This is so weird. Most places I found use SortFieldCase.updatedAt which is updated_at so dunno where updatedAt might be passed 🤔 . Anyway, this is fine, no need to change.

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 I found it. The rows are populated with instances of

export type Case = Omit<SnakeToCamelCase<CaseResponse>, 'comments'> & { comments: Comment[] };

so when adding the columns we do:

columns.push({
  field: 'updatedAt',
  render: (updatedAt: Case['updatedAt']) => {

Clicking a header of this table calls tableOnChangeCallback defined in all_cases_list.tsx row 120.

This is triggered when the sorting changes in the EuiTable(see onChange props).

So that field: 'updatedAt' is passed to this callback and then we need to say sortField: getSortField(sort.field),

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/public/components/all_cases/all_cases_list.tsx#L126

Dunno, it looks unnecessarily complicated.

But don't change it, I am just digging around 😄

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Tested and works fine but

the added column messes up the modal a bit.
Screenshot 2023-01-18 at 15 20 34

@js-jankisalvi
Copy link
Contributor Author

js-jankisalvi commented Jan 18, 2023

Tested and works fine but

the added column messes up the modal a bit.

Screenshot 2023-01-18 at 15 20 34

Good catch!!

Currently I have adjusted width of few columns so that data is visible. Screenshot 2023-01-18 at 15 20 34

However I have another issue in which we will redesign this modal to show only necessary information.

@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 393.2KB 393.5KB +308.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 128.6KB 128.7KB +144.0B

History

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

cc @js-jankisalvi

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

LGTM!

@js-jankisalvi js-jankisalvi merged commit 150b447 into elastic:main Jan 18, 2023
wayneseymour pushed a commit to wayneseymour/kibana that referenced this pull request Jan 19, 2023
## Summary

Fixes elastic#148871

Adds new column `Updated on ` in `Cases` table. This column can be
sorted and can persist sorting options.


![image](https://user-images.githubusercontent.com/117571355/213171804-9fa1bc42-8b8b-4b7b-baf3-03ce649b1dc6.png)


### Checklist
- [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
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))

### Release notes
- Adds new column `Updated on ` in `all cases list ` table. This column
can be sorted and can persist sorting options.
@js-jankisalvi js-jankisalvi deleted the add-updated-on-column branch October 6, 2023 07:37
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.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cases] Add "Updated on" column to the cases table
5 participants