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.

Sign up for GitHub

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

Jump to bottom

Fleet UI: Query report (table, buttons, api calls, etc) #14325

Merged
merged 10 commits into from
Oct 9, 2023
Merged

Fleet UI: Query report (table, buttons, api calls, etc) #14325

merged 10 commits into from
Oct 9, 2023

Conversation

RachelElysia
Copy link
Member

@RachelElysia RachelElysia commented Oct 5, 2023 •
edited
Loading

Issue

Cerra #13472

Description

  • Surface query report on the /queries/{id} route
  • Include table buttons to show query and export query
  • Include results count
  • Clientside sorting and filtering for columns
  • Add mock data to frontend integration mocks and to API mocks for concurrent development
    • 331 + 351 + 2 = 684 lines of code is just mocking data and not actual changes
  • If modifying sorting/filter, modify the exported results sorting/filter as well
  • Last fetched column is sentence cased, sortable by chronological order and not alpha order of the readable string (e.g., "a year ago" should be sorted after "over 1 month ago" if sorted most recent to oldest even though a comes before o in the alphabet)

Screen recordings (Uses mock data)

Screen.Recording.2023-10-09.at.12.21.17.PM.mov
Screen.Recording.2023-10-09.at.12.20.11.PM.mov

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • TODO for QA: Added/updated E2E tests (consider testing some of the features mentioned in the description)
  • Manual QA for all new/changed functionality

@RachelElysia RachelElysia force-pushed the 7766-frontend-query-report branch from 2ede82a to 8f1bb1c Compare October 6, 2023 17:30
@RachelElysia RachelElysia marked this pull request as ready for review October 6, 2023 20:05
@RachelElysia RachelElysia requested a review from a team as a code owner October 6, 2023 20:05
@RachelElysia RachelElysia changed the title [DRAFT] Fleet UI: Query report (table, buttons, api calls, etc) Fleet UI: Query report (table, buttons, api calls, etc) Oct 6, 2023
@jacobshandling
Copy link
Contributor

👀

@codecov
Copy link

codecov bot commented Oct 9, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (7766-frontend@11fc7ed). Click here to learn what that means.

Additional details and impacted files 
@@               Coverage Diff                @@
##             7766-frontend   #14325   +/-   ##
================================================
  Coverage                 ?   58.79%           
================================================
  Files                    ?      909           
  Lines                    ?    74568           
  Branches                 ?     2142           
================================================
  Hits                     ?    43840           
  Misses                   ?    27230           
  Partials                 ?     3498
Flag Coverage Δ
frontend 53.12% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jacobshandling
jacobshandling requested changes Oct 9, 2023
View reviewed changes
frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx Outdated
}
);

const isLoading = isStoredQueryLoading || isQueryReportLoading; // TODO: Add || isCachedResultsLoading for new API response
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these TODOs be done?

frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/components/QueryReport/QueryReport.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/components/QueryReport/QueryReport.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/components/QueryReport/QueryReport.tsx Outdated Show resolved Hide resolved
frontend/pages/queries/details/components/QueryReport/QueryReportTableConfig.tsx
To create headers, use JS set to create an array of all unique column names */
const uniqueColumnNames = Array.from(
results.reduce(
(s, o) => Object.keys(o).reduce((t, k) => t.add(k), s),
Copy link
Contributor

@jacobshandling jacobshandling Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This could be clearer. The single var names are tricky to reason about when combined with the nested reduces. Maybe:

s –> uniqueColumns
o –> resultColumns

and then add each key of resultColumns to uniqueColumns before returning uniqueColumns. You might do a return new Set.from([...uniqueColumns, ...Object.keys(resultColumns))].

Copy link
Contributor

Choose a reason for hiding this comment

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

Just the variable renames would be nice. Not necessary for now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this can be prioritized later for a refactor if need be. I'm going to leave this as is since it was my solution for a bug in QueryResultsTableConfig.tsx (where we were only displaying columns from the first row of results and not all columns returned) and would require out of scope refactoring. Currently it's just being used in these two spots.

frontend/services/entities/query_report.ts
import { buildQueryStringFromParams } from "utilities/url";

// Mock API requests to be used in developing FE for #7766 in parallel with BE development
import { sendRequest } from "services/mock_service/service/service";
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO - use the real service

jacobshandling
jacobshandling requested changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@jacobshandling jacobshandling left a comment

Choose a reason for hiding this comment

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

  • We want "on" to be bolded on QueryDetailsPage > 218
  • QueryDetailsPage > 94 – why do we need to use as? This should definitely be a number right?
  • IQueryReportResultRow.columns can be typed better

@jacobshandling jacobshandling merged commit a85f399 into fleetdm:7766-frontend Oct 9, 2023
7 of 8 checks passed
@RachelElysia RachelElysia deleted the 7766-frontend-query-report branch October 9, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobshandling jacobshandling jacobshandling approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RachelElysia @jacobshandling