Skip to content

Comments

[Cloud Security][Bug Fix] Fix for sorting issue on Posture score column on Group by resources findings table#156938

Merged
animehart merged 3 commits intoelastic:mainfrom
animehart:posture-score-sort
May 8, 2023
Merged

[Cloud Security][Bug Fix] Fix for sorting issue on Posture score column on Group by resources findings table#156938
animehart merged 3 commits intoelastic:mainfrom
animehart:posture-score-sort

Conversation

@animehart
Copy link
Contributor

@animehart animehart commented May 6, 2023 •
edited by kibanamachine
Loading

Summary

This PR is for fixing the issue we have with sorting on posture score column on Group by resources findings table where clicking on the column doesn't do anything

Before

Screen.Recording.2023-05-07.at.12.33.05.PM.mov

After

Screen.Recording.2023-05-07.at.12.34.08.PM.mov

@animehart animehart linked an issue May 6, 2023 that may be closed by this pull request
[Findings][Cloud Security][Bug]Posture Score column only sorts 1 way #156241
Closed
@animehart animehart marked this pull request as ready for review May 6, 2023 04:06
@animehart animehart requested a review from a team as a code owner May 6, 2023 04:06
@animehart animehart added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 v8.9.0 Team:Cloud Security Cloud Security team related labels May 6, 2023
@elasticmachine
Copy link
Contributor

elasticmachine commented May 6, 2023

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

JordanSh
JordanSh requested changes May 7, 2023
View reviewed changes
Copy link
Contributor

@JordanSh JordanSh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please attach a video after the changes. the changes here only replace the property name as far as I can see, so I'm not completely sure why was the problem happening and how this has solved it. a video and explanation would be very helpful

...e/public/pages/configurations/latest_findings_by_resource/findings_by_resource_container.tsx
setTableOptions={setTableOptions}
sorting={{
sort: { field: 'compliance_score', direction: urlQuery.sortDirection },
sort: { field: 'compliance_score', direction: urlQuery.sort.direction },
Copy link
Contributor

@JordanSh JordanSh May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @animehart, I'm not sure how this fix would change the sorting behavior. as far as I can tell, the only change is the properties names that we pass the value from. but the values should be the same, we just get it from sort.direction instead of sortDirection aren't we?

I might be missing something here so let me know.

Copy link
Contributor Author

@animehart animehart May 7, 2023

Choose a reason for hiding this comment

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

@JordanSh
https://user-images.githubusercontent.com/8703149/236698689-48a331a4-6749-4199-8bfc-627dbd2dd533.mov

so it seems like the table sorts from the direction field on sort field on urlQuery
Screenshot 2023-05-07 at 12 08 28 PM
the sortDirection on urlQuery doesn't seem to change nor do anything to the table
I tried removing it from urlQuery and it doesnt seem to be affecting the table ( I tried sorting it without sortDirection on urlQuery)

...e/public/pages/configurations/latest_findings_by_resource/findings_by_resource_container.tsx Outdated
filters,
pageIndex: 0,
sortDirection: 'asc',
sortDirection: 'desc',
Copy link
Contributor

@JordanSh JordanSh May 7, 2023

Choose a reason for hiding this comment

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

  1. Does this change effect the UX of the product? lower compliance scores should be displayed first by default unless the user re-sorts. can you please attach a video here and also for future PRs? it would be very helpful
  2. Do we still use sortDirection after your changes? it looks like the new sort field should be sufficient isn't it? maybe we can remove sortDirection completely?

Copy link
Contributor

@kfirpeled kfirpeled May 7, 2023

Choose a reason for hiding this comment

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

@animehart page should start with ascending order - with the idea that the lower score is more relevant to see first when looking at mis configurations

JordanSh
JordanSh requested changes May 7, 2023
View reviewed changes
Copy link
Contributor

@JordanSh JordanSh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please attach a video after the changes. the changes here only replace the property name as far as I can see, so I'm not completely sure why was the problem happening and how this has solved it. a video and explanation would be very helpful

kfirpeled
kfirpeled requested changes May 7, 2023
View reviewed changes
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

checked locally and it works now

  • the default direction should be reverted back to ascending

...e/public/pages/configurations/latest_findings_by_resource/findings_by_resource_container.tsx Outdated
filters,
pageIndex: 0,
sortDirection: 'asc',
sortDirection: 'desc',
Copy link
Contributor

@kfirpeled kfirpeled May 7, 2023

Choose a reason for hiding this comment

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

@animehart page should start with ascending order - with the idea that the lower score is more relevant to see first when looking at mis configurations

@animehart animehart requested review from JordanSh and kfirpeled May 7, 2023 19:59
@kibana-ci
Copy link

kibana-ci commented May 7, 2023

💚 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
cloudSecurityPosture 194.6KB 194.7KB +30.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 398 401 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 478 481 +3
total +5

History

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

kfirpeled
kfirpeled approved these changes May 7, 2023
View reviewed changes
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

tested again, LGTM

JordanSh
JordanSh approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@JordanSh JordanSh left a comment

Choose a reason for hiding this comment

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

🚀

@animehart animehart merged commit a92782c into elastic:main May 8, 2023
@kibanamachine kibanamachine mentioned this pull request May 8, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 8, 2023
@animehart
[Cloud Security][Bug Fix] Fix for sorting issue on Posture score colu…
013da20 
…mn on Group by resources findings table (elastic#156938)

## Summary

This PR is for fixing the issue we have with sorting on posture score
column on Group by resources findings table where clicking on the column
doesn't do anything

## Before

https://user-images.githubusercontent.com/8703149/236698942-04799955-1d2d-4bb8-931c-0ef1589b92f3.mov

## After

https://user-images.githubusercontent.com/8703149/236698949-381d4264-54f9-4639-b541-2fb6fec2dbfa.mov
(cherry picked from commit a92782c)
@kibanamachine
Copy link
Contributor

kibanamachine commented May 8, 2023

💚 All backports created successfully

Status Branch Result
✅ 8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 8, 2023
@kibanamachine @animehart
[8.8] [Cloud Security][Bug Fix] Fix for sorting issue on Posture scor…
d99df38 
…e column on Group by resources findings table (#156938) (#157001)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Cloud Security][Bug Fix] Fix for sorting issue on Posture score
column on Group by resources findings table
(#156938)](#156938)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Rickyanto
Ang","email":"rickyangwyn@gmail.com"},"sourceCommit":{"committedDate":"2023-05-08T14:15:44Z","message":"[Cloud
Security][Bug Fix] Fix for sorting issue on Posture score column on
Group by resources findings table (#156938)\n\n## Summary\r\n\r\nThis PR
is for fixing the issue we have with sorting on posture score\r\ncolumn
on Group by resources findings table where clicking on the
column\r\ndoesn't do anything\r\n\r\n##
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698942-04799955-1d2d-4bb8-931c-0ef1589b92f3.mov\r\n\r\n##
After\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698949-381d4264-54f9-4639-b541-2fb6fec2dbfa.mov","sha":"a92782cb18a33822616d0f95a32bd1b2c5bf7bbb","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Cloud
Security","v8.8.0","v8.9.0"],"number":156938,"url":"https://github.com/elastic/kibana/pull/156938","mergeCommit":{"message":"[Cloud
Security][Bug Fix] Fix for sorting issue on Posture score column on
Group by resources findings table (#156938)\n\n## Summary\r\n\r\nThis PR
is for fixing the issue we have with sorting on posture score\r\ncolumn
on Group by resources findings table where clicking on the
column\r\ndoesn't do anything\r\n\r\n##
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698942-04799955-1d2d-4bb8-931c-0ef1589b92f3.mov\r\n\r\n##
After\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698949-381d4264-54f9-4639-b541-2fb6fec2dbfa.mov","sha":"a92782cb18a33822616d0f95a32bd1b2c5bf7bbb"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156938","number":156938,"mergeCommit":{"message":"[Cloud
Security][Bug Fix] Fix for sorting issue on Posture score column on
Group by resources findings table (#156938)\n\n## Summary\r\n\r\nThis PR
is for fixing the issue we have with sorting on posture score\r\ncolumn
on Group by resources findings table where clicking on the
column\r\ndoesn't do anything\r\n\r\n##
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698942-04799955-1d2d-4bb8-931c-0ef1589b92f3.mov\r\n\r\n##
After\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8703149/236698949-381d4264-54f9-4639-b541-2fb6fec2dbfa.mov","sha":"a92782cb18a33822616d0f95a32bd1b2c5bf7bbb"}}]}]
BACKPORT-->

Co-authored-by: Rickyanto Ang <rickyangwyn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JordanSh JordanSh JordanSh approved these changes

@kfirpeled kfirpeled kfirpeled approved these changes

Assignees

No one assigned

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.8.0 v8.9.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Findings][Cloud Security][Bug]Posture Score column only sorts 1 way

6 participants

@animehart @elasticmachine @kibana-ci @kibanamachine @JordanSh @kfirpeled