-
Notifications
You must be signed in to change notification settings - Fork 86
Feature/asset reporting #7246
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
base: main
Are you sure you want to change the base?
Feature/asset reporting #7246
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile SummaryThis PR adds a new Asset Reporting page to the Admin UI that provides a consolidated view of all discovered assets (cookies, browser requests, images, iframes, JavaScript tags) from the web monitor. The implementation follows established patterns in the codebase and includes comprehensive test coverage. Key changes:
Notable:
Architecture: Confidence Score: 4/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No files reviewed, no comments
gilluminate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfbrown something isn't quite right with the table configuration. Things are getting cut off, likely due to some sizing settings. This screen shot is at 1280 wide browser and scrolled all the way down and right. Be sure to check out https://ethyca.atlassian.net/wiki/spaces/DES/pages/3865214978/Tables for some advice there.
Additionally, the table seems to be wired up for client side sorting and filtering and isn't talking to the server at all when attempting those. You'll need to look at another table doing server-side for examples on how to handle that.
clients/admin-ui/src/features/asset-reporting/hooks/useAssetReportingTable.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/asset-reporting/hooks/useAssetReportingTable.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/assessments/AssessmentChatModal.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/assessments/AssessmentChatModal.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/asset-reporting/asset-reporting.slice.ts
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/asset-reporting/asset-reporting.slice.ts
Outdated
Show resolved
Hide resolved
| sticky: { | ||
| offsetHeader: 40, | ||
| offsetScroll: 0, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be causing some of the cut-off issues. Double check these arbitrary values
1dbd439 to
0b7f107
Compare
@gilluminate This feature also depends on this backend fidesplus branch, which I will include in the description: feature/asset-reporting I think that might resolve some of the issues you were mentioning here? |
@mfbrown I was running that, actually. The endpoints never seem to get fired again when I tested this locally. I'll try again today. |
|
@mfbrown correction: filtering IS working with server-side filtering. It's only the sorting that isn't working. |
51a9b99 to
91b3443
Compare
91b3443 to
e97e247
Compare
@gilluminate Also curious if you were still seeing the cutoff issues or the issue where the domain was rendering in an array. Those should be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks really good, nice work! Tested locally and working well. Just 2 really nit-picky pieces of feedback and it's good to go.
- The changelog is a bit wordy
- I think the nav is a bit confusing having a generic "Reporting" right above "Asset reporting." Can we update "Reporting" to "Data map reporting" or change this to just be "Assets?"
| @@ -0,0 +1,4 @@ | |||
| type: Added | |||
| description: Added asset reporting UI with paginated table, column filtering, sorting, search, and CSV export for cross-system asset aggregation. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit-picky: this is a bit overkill IMO
| description: Added asset reporting UI with paginated table, column filtering, sorting, search, and CSV export for cross-system asset aggregation. | |
| description: Added asset reporting table with CSV export for cross-system asset aggregation. |

Ticket ENG-2418
Description Of Changes
Adds a new Asset Reporting page to the Admin UI that displays a cross-system view of all discovered assets (cookies, browser requests, images, iframes, JavaScript tags). This provides users with a consolidated reporting view of assets discovered by the web monitor, with filtering, sorting, search, and CSV export capabilities.
The "Consent Status" column visibility is gated behind the assetConsentStatusLabels beta flag to align with the action center website monitor.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works