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

Action Center results MVP #5622

Merged
merged 16 commits into from
Jan 8, 2025
Merged

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Dec 18, 2024

Closes HJ-290, HJ-334

Description Of Changes

As a user, I want to view the results of my website monitors in a list view that is by default grouped by monitor run, so that I can triage the results and make updates as needed to my system and data inventory.

Code Changes

Related maintenance:

  • Upgrade version of date-fns
  • Clean up Pagination to properly disable "Next" when there's only 1 page
  • Fix Ant typography's Title to be bold by default by including that in our global CSS

Feature work:

  • New Feature Flag "Website Monitor"
  • New route to new Action Center page
  • Stub out the System monitor page (HJ-314) for links to work
  • New RTK slice file with action center endpoint definitions
  • Cypress tests
  • New website icon utility

Steps to Confirm locally

  1. Checkout and run the backend from Adam's branch in the fidesplus repo (asachs/HJ-295).
  2. Run turbo dev from the clients directory
  3. Visit AdminUI's "About" page and enable the new "Website Monitor" feature flag
  4. Click the "Action center" left nav item
  5. If you receive a message here saying it's disabled, contact Adam to update config.
  6. You should see 2-3 monitors, which are all just dummy data from Adam's BE at the moment.
  7. You should be able to search by monitor name (search for "BQ" for example) and the list should update with results as expected
  8. Verify that each monitor has:
  • A total count in the title. The title should be clickable.
  • A description listing the assets found
  • the monitor's name
  • the last run date as relative time ("...ago"). when hovered, this should show a tooltip with the real date and time in your own local timezone.
  • A link to Review (which goes to the same place as the title)

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Copy link

vercel bot commented Dec 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 6:32pm

@@ -32,7 +32,7 @@ export const debounce = (fn: (props?: any) => void, ms = 0) => {
};

export const formatDate = (value: string | number | Date): string =>
format(new Date(value), "MMMM d, Y, KK:mm:ss z");
format(new Date(value), "MMMM d, y, KK:mm:ss z");
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 update was a breaking change in the new version of date-fns. It does NOT change our existing date formats.

@gilluminate gilluminate force-pushed the gill/HJ-290/action-center-to-show-results branch from 39bfa91 to 7ac5686 Compare December 18, 2024 22:06
Copy link

cypress bot commented Dec 18, 2024

fides    Run #11669

Run Properties:  status check passed Passed #11669  •  git commit dcf5d630e8 ℹ️: Merge 85d47faf2db8c0a34cf439a1c2e95a2f891cf389 into 2d9f1ef27026454fac915d635d9d...
Project fides
Branch Review refs/pull/5622/merge
Run status status check passed Passed #11669
Run duration 01m 03s
Commit git commit dcf5d630e8 ℹ️: Merge 85d47faf2db8c0a34cf439a1c2e95a2f891cf389 into 2d9f1ef27026454fac915d635d9d...
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

@@ -53,7 +53,7 @@ export const useServerSidePagination = () => {
setPageIndex((prev) => prev + 1);
}, [setPageIndex]);
const isNextPageDisabled = useMemo(
() => pageIndex === totalPages,
() => !!totalPages && (pageIndex === totalPages || totalPages < 2),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should disable the "Next" button if there's only 1 page or if there was an error (0 pages).


return (
<List.Item data-testid={`monitor-result-${key}`} {...props}>
<Skeleton avatar title={false} loading={showSkeleton} active>
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 is how Ant's own examples utilize skeletons during loading state. Pretty cool that it just wraps the content!


import { MonitorSummaryPaginatedResponse } from "./types";

const actionCenterApi = baseApi.injectEndpoints({
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 opted to create a dedicated slice for this instead of building on the D&D slice. I may decide to change that depending how big this gets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of splitting them up more in general, I think some of the slice files we have are too broad and get unwieldy.

Copy link
Contributor

@jpople jpople left a comment

Choose a reason for hiding this comment

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

Screenshot 2025-01-08 at 11 50 28

Tested locally, working for me:


const assetCountString = Object.entries(updates)
.map((update) => {
return `${update[1]} ${update[0]}s`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this is formatted based on the API response and not part of this PR, just want to flag that we probably want these to be sentence-cased.

Copy link
Contributor Author

@gilluminate gilluminate Jan 8, 2025

Choose a reason for hiding this comment

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

Good callout. @adamsachs this is for things like "Browser Request" which should be "Browser request", etc.


import { MonitorSummaryPaginatedResponse } from "./types";

const actionCenterApi = baseApi.injectEndpoints({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of splitting them up more in general, I think some of the slice files we have are too broad and get unwieldy.

}, [data, setTotalPages]);

const results = data?.items || [];
const loadingResults = isFetching
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this needed for, out of curiosity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. This is generating a dummy array for the skeleton loader rows and generates as many rows as the page size. Ant's List component has built in skeletons so it needs both a loading state as well as dummy array for the dataSource. See the "load more" example here https://ant.design/components/list#list-demo-loadmore (they use count instead of page size, but similar concept).

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 there is more than one page of data here it will be more apparent how this works.

@gilluminate gilluminate merged commit 483d798 into main Jan 8, 2025
20 checks passed
@gilluminate gilluminate deleted the gill/HJ-290/action-center-to-show-results branch January 8, 2025 18:49
Copy link

cypress bot commented Jan 8, 2025

fides    Run #11671

Run Properties:  status check passed Passed #11671  •  git commit 483d7984e9: Action Center results MVP (#5622)
Project fides
Branch Review main
Run status status check passed Passed #11671
Run duration 00m 50s
Commit git commit 483d7984e9: Action Center results MVP (#5622)
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants