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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

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 Dec 19, 2024 9:29pm

@@ -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 #11608

Run Properties:  status check passed Passed #11608  •  git commit fe812008ed ℹ️: Merge 3243d5407014f2f0f4b3c363cd2fba240d182654 into 35602e771b8afb01834255fbe6ce...
Project fides
Branch Review refs/pull/5622/merge
Run status status check passed Passed #11608
Run duration 00m 48s
Commit git commit fe812008ed ℹ️: Merge 3243d5407014f2f0f4b3c363cd2fba240d182654 into 35602e771b8afb01834255fbe6ce...
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
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
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.

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.

1 participant