Skip to content

ENG-942 #6504

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
Merged
merged 8 commits into from
Aug 29, 2025
Merged

ENG-942 #6504

merged 8 commits into from
Aug 29, 2025

Conversation

tvandort
Copy link
Contributor

@tvandort tvandort commented Aug 28, 2025 •
edited
Loading

Closes ENG-942

Description Of Changes

Solves some problems that exist with the consent reporting page.

image

Code Changes

  • Support open ended pagination.
  • Local start and end to day.

Steps to Confirm

  1. Check the network for when the timestamps on the consent report page are dispatched.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • Followup issues:
    • Followup issues created
    • 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

@vercel Vercel
Copy link

vercel bot commented Aug 28, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ready Ready Preview Comment Aug 29, 2025 5:06pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
fides-privacy-center Ignored Ignored Aug 29, 2025 5:06pm

tvandort
tvandort commented Aug 28, 2025
View reviewed changes
@Kelsey-Ethyca
Copy link
Contributor

@greptile

greptile-apps[bot]
greptile-apps bot reviewed Aug 29, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR improves the consent reporting functionality in the Fides admin UI by addressing performance and user experience issues. The primary changes implement open-ended pagination to solve performance problems with large consent datasets and fix timezone handling for accurate date filtering.

Key Technical Changes:

  • Pagination Refactor: Replaces server-side pagination with a custom usePagination hook that syncs pagination state to URL parameters for better user experience
  • Performance Optimization: Removes expensive total count calculations by setting includeTotal: false in API calls, using item count comparison to determine pagination state instead
  • Timezone Fix: Modifies date handling in startOfDayIso and endOfDayIso functions to process dates in local timezone before UTC conversion, ensuring accurate date range filtering
  • UI Enhancement: Implements custom pagination controls with previous/next buttons that disable appropriately based on data availability

The changes integrate well with the existing Fides codebase architecture, utilizing the established Redux Toolkit Query patterns for API calls and maintaining consistency with other admin UI components. The pagination approach is particularly clever - by checking if the returned item count is less than the page size, it can determine when to disable the "Next" button without requiring expensive database count operations.

Important Files Changed

File Change Summary
Filename Score Overview
CHANGELOG.md 5/5 Documents three consent reporting improvements with proper categorization and PR references
clients/admin-ui/src/features/consent-reporting/consent-reporting.slice.ts 4/5 Fixes timezone handling and adds optional includeTotal parameter for performance optimization
clients/admin-ui/src/pages/consent/reporting/index.tsx 4/5 Major refactor implementing open-ended pagination with custom UI controls and URL state sync
clients/admin-ui/src/features/common/hooks/usePagination.ts 4/5 Adds nextPage and previousPage functions to support the new pagination pattern

Confidence score: 4/5

  • This PR addresses legitimate performance and UX issues with well-architected solutions that integrate cleanly with existing code patterns
  • Score reflects solid implementation choices and thorough changes, though the timezone fix could benefit from additional testing verification
  • Pay close attention to the timezone handling changes in the consent reporting slice to ensure date ranges work correctly across different user timezones

Sequence Diagram

sequenceDiagram
    participant User
    participant ConsentReportingPage
    participant usePagination
    participant consentReportingApi
    participant Backend

    User->>ConsentReportingPage: "Load consent reporting page"
    ConsentReportingPage->>usePagination: "Initialize pagination"
    usePagination->>usePagination: "Set pageIndex=1, pageSize=25 (default)"
    
    ConsentReportingPage->>consentReportingApi: "useGetAllHistoricalPrivacyPreferencesQuery({page, size, includeTotal: false})"
    consentReportingApi->>Backend: "GET /historical-privacy-preferences"
    Backend-->>consentReportingApi: "Return paginated results"
    consentReportingApi-->>ConsentReportingPage: "Display privacy preferences data"
    
    User->>ConsentReportingPage: "Select date range"
    ConsentReportingPage->>ConsentReportingPage: "Convert dates to local start/end of day UTC ISO strings"
    ConsentReportingPage->>consentReportingApi: "Re-query with date filters"
    consentReportingApi->>Backend: "GET /historical-privacy-preferences?request_timestamp_gt=startOfDay&request_timestamp_lt=endOfDay"
    Backend-->>ConsentReportingPage: "Return filtered results"
    
    User->>ConsentReportingPage: "Click next page"
    ConsentReportingPage->>usePagination: "nextPage()"
    usePagination->>usePagination: "Update URL with page=pageIndex+1"
    ConsentReportingPage->>consentReportingApi: "Query with new page index"
    consentReportingApi->>Backend: "GET /historical-privacy-preferences?page=2"
    Backend-->>ConsentReportingPage: "Return next page of results"
    
    User->>ConsentReportingPage: "Change page size"
    ConsentReportingPage->>usePagination: "updatePageSize(newSize)"
    usePagination->>usePagination: "Reset to page 1, update URL with new size"
    ConsentReportingPage->>consentReportingApi: "Query with new page size"
    consentReportingApi->>Backend: "GET /historical-privacy-preferences?page=1&size=newSize"
    Backend-->>ConsentReportingPage: "Return results with new page size"
Loading

4 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

@tvandort tvandort merged commit 02f4bee into main Aug 29, 2025
20 checks passed
@tvandort tvandort deleted the ENG-942-updated branch August 29, 2025 17:11
tvandort added a commit that referenced this pull request Aug 29, 2025
@tvandort @speaker-ender
ENG-942 (#6504)
7e888dd 
Co-authored-by: 3nder <speaker_ender@protonmail.com>
@cypress Cypress
Copy link

cypress bot commented Aug 29, 2025

fides    Run #13301

Run Properties:  status check failed Failed #13301  •  git commit 02f4bee8c3: ENG-942 (#6504)
Project fides
Branch Review main
Run status status check failed Failed #13301
Run duration 00m 59s
Commit git commit 02f4bee8c3: ENG-942 (#6504)
Committer Tom Van Dort
View all properties for this run ↗︎
Test results
Tests that failed  Failures 1
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 ↗︎

Tests for review

Failed  cypress/e2e/smoke_test.cy.ts • 1 failed test

View Output Video

Test Artifacts
Smoke test > can submit an access request from the Privacy Center Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kelsey-Ethyca Kelsey-Ethyca Kelsey-Ethyca approved these changes

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tvandort @Kelsey-Ethyca @speaker-ender