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

[Cloud Posture] Support pagination in benchmarks page #128486

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

ari-aviran
Copy link
Contributor

Summary

Adds sorting to the benchmarks table in the Cloud Security Posture application.

Checklist

Delete any items that are not applicable to this PR.

@ari-aviran ari-aviran requested a review from a team as a code owner March 24, 2022 12:53
@ari-aviran ari-aviran added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 Team:Cloud Security Cloud Security team related labels Mar 24, 2022
Copy link
Contributor

@orouz orouz left a comment

Choose a reason for hiding this comment

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

looks good!

minor comments

@@ -129,7 +138,7 @@ export const Benchmarks = () => {
return (
<CspPageTemplate pageHeader={PAGE_HEADER}>
<BenchmarkSearchField
isLoading={queryResult.isLoading}
isLoading={queryResult.isFetching}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this preferred over isLoading ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When sorting the data, react-query dispatches an additional query, but isLoading remains false (it only applies to initial loading of data), and we do want the table and search field to present the loading state

Comment on lines 158 to 159
// @ts-ignore - EUI types currently do not support sorting by nested fields
sort: { field: query.sortField, direction: query.sortOrder },
Copy link
Contributor

@orouz orouz Mar 24, 2022

Choose a reason for hiding this comment

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

1 - can you clarify what error do we get here? i don't recall having to do this in other tables.
2 - prefer @ts-expect-error (optionally add TS[number]) so it's clear what error the author wants to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only applies when sorting based on a nested field, as the type for sort.field is keyof T (where T is the data object for the table), and keyof only applies to top-level fields.

? // Asserting since type inference does not support sorting of nested fields
(sort.field as UseCspBenchmarkIntegrationsProps['sortField'])
: current.sortField,
sortOrder: sort?.direction ? sort.direction : current.sortOrder,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sortOrder: sort?.direction ? sort.direction : current.sortOrder,
sortOrder: sort?.direction || current.sortOrder,

Comment on lines 33 to 42
const query: BenchmarksQuerySchema = {
benchmark_name: name,
per_page: perPage,
page,
sort_field: sortField,
sort_order: sortOrder,
};

return useQuery(
[QUERY_KEY, { name, perPage, page, sortField, sortOrder }],
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 it makes sense to make the queryKey take the full query. as in:

[QUERY_KEY, { query }]

* - package_policy.package.title
* - package_policy.package.version
*/
sort_field: schema.oneOf(
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 support every field in PackagePolicy (as we don't own it anyway)

and maybe we should just take what's currently applicable by the UI. as in - just the column headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we technically support every field in that object type, but better be explicit in the docs and schema. Regarding using only what's in the UI - IMO its better to leave the API more flexible for the future

@ari-aviran
Copy link
Contributor Author

@elasticmachine merge upstream

@ari-aviran ari-aviran enabled auto-merge (squash) March 24, 2022 15:26
@ari-aviran ari-aviran merged commit 51e0845 into elastic:main Mar 24, 2022
@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
cloudSecurityPosture 364.8KB 365.1KB +332.0B

History

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

@ari-aviran ari-aviran deleted the benchmarks_sorting branch March 25, 2022 07:05
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 release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants