Support filtering by duplicate request groups. Add duplicate request drawer UI.#7965
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7965 +/- ##
==========================================
- Coverage 85.61% 85.60% -0.01%
==========================================
Files 658 658
Lines 42858 42859 +1
Branches 5016 5016
==========================================
Hits 36691 36691
Misses 5063 5063
- Partials 1104 1105 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lucanovera
added
the
db-migration
|
/code-review |
…SR-duplication-detection-activity-log-link
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — PR #7965: Duplicate Request Group Filtering & UI Drawer
Overall this is a well-structured feature. The backend filter is minimal and correct, the migration includes a thoughtful large-table deferral path, and the new DuplicatesDrawer component is clean. A few things worth addressing before merge:
Summary of findings
Frontend
DuplicatesDrawer— silent truncation at 50 results (medium): The query is hardcapped atsize: 50with no pagination shown on the table. For large duplicate groups this silently truncates. Either raise the cap, add overflow messaging, or show a pagination control.columnsnot memoized (minor): Thecolumnsarray closes overcurrentRequestIdbut is recreated on every render. Wrapping inuseMemois consistent with therowstreatment above it.- Empty-state copy (minor):
"No other duplicate requests in this group."is shown when the API returns zero items — but the table includes the current request, so an empty result means even it wasn't found."No duplicate requests found."is more accurate. - Dual
DUPLICATE_DETECTION_DATASET_NAMEconstant (medium): The same string is maintained in two places (Python + TypeScript). The comments document it, but there's no compile-time or test-time guard against the two drifting. Consider a lightweight contract test or alternative approach to surface the coupling.
Backend / Migration
- Migration filename
xx_prefix (minor): Other migration files in the project don't use anxx_prefix — this looks like a WIP marker that should be removed before merge. - Inconsistent DDL style in
downgrade()(minor): Theupgrade()block usessa.text(...).bindparams(...)throughout, butdowngrade()uses a bare f-string for theDROP INDEX. Stylistically inconsistent; consider wrapping insa.text().
Tests
restore_headupgrades toREVISIONnot"head"(medium): If additional migrations land aftere8f9a0b1c2d3, the teardown fixture leaves the database at an older revision rather than the true schema head, potentially breaking downstream tests in the same session. Changing toupgrade_db(alembic_config, "head")is safer.
🔬 Codegraph: connected (47570 nodes)
💡 Write /code-review in a comment to re-run this review.
lucanovera
requested review from
JadeCara,
gilluminate and
vcruces
and removed request for
a team,
gilluminate and
vcruces
JadeCara
left a comment
JadeCara
left a comment
There was a problem hiding this comment.
BE elements look clean! Verified migration round trip and tested duplicates show up as expected in UI!
Very cool!
JadeCara
left a comment
JadeCara
left a comment
There was a problem hiding this comment.
Found an issue - the de duplication group was originally just looking at identities - seeing if I can update to work for policies as well.
…SR-duplication-detection-activity-log-link
@JadeCara After our conversation last week I ended up simplifying everything and just showing related request by the same identity. There were just too many edge cases when trying to figure out all duplicates across time. Now it's just frontend changes that work with the existing identity filters. |
lucanovera
requested review from
a team and
kruulik
and removed request for
a team
…SR-duplication-detection-activity-log-link
lucanovera
requested review from
a team and
gilluminate
and removed request for
a team and
kruulik
| <Typography.Link | ||
| href={`/privacy-requests/${privacyRequest.id}`} | ||
| variant="primary" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| router.push({ | ||
| pathname: PRIVACY_REQUEST_DETAIL_ROUTE, | ||
| query: { id: privacyRequest.id }, | ||
| }); | ||
| }} | ||
| target={link?.target} | ||
| rel={link?.rel} | ||
| onClick={ | ||
| useNativeNavigation | ||
| ? undefined | ||
| : (e) => { | ||
| e.preventDefault(); | ||
| router.push({ | ||
| pathname: PRIVACY_REQUEST_DETAIL_ROUTE, | ||
| query: { id: privacyRequest.id }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Some potential for clean up here. Should just use the new <RouterLink> component which eliminates the need for the href + onclick combo here. PRIVACY_REQUEST_DETAIL_ROUTE should be used in the href as well.
There was a problem hiding this comment.
I tried this. There is a drawback to using RouterLink and it's that it doesn't fully support the query param route replacement. I don't really want to go back to using a .replace("[id] on each use. I think we can improve RouterLink and make it fully support this kind of dynamic path replacements.
…item/ListItem.tsx Co-authored-by: Jason Gill <jason.gill@ethyca.com>
…SR-duplication-detection-activity-log-link
lucanovera
deleted the
ENG-1806-FE-Add-DSR-duplication-detection-activity-log-link
branch
3 participants
Ticket ENG-1806
Description Of Changes
Adds a "View related requests" drawer to the privacy request detail page so admins can see other requests submitted by the same data subject in one click — both from the duplicate-detection entry on the activity timeline and from a new search icon next to each identity in the right-hand details panel.
Why we abandoned the original "View duplicates" approach
The first iteration on this branch added a
duplicate_request_group_idfilter to the privacy-request search endpoint and used it to back the drawer. While reviewing it we found the group id is intentionally rule-version-bound by design:DuplicateGroup.id = md5(rule_version + dedup_key)(duplicate_group.py:18-31)rule_versionis hashed from the duplicate-detection settings (enabled,time_window_days,match_identity_fields)policy_idand atime_window_dayscutoffSo filtering by
duplicate_request_group_idwould not return all requests by the same identity once settings, policy, or time-window changed. That's an "identity-group-under-this-rule-version-and-policy" id, not an identity id — which doesn't match what users want from this UI.Switching to the existing
PrivacyRequestFilter.identitiesfilter (privacy_request_query_utils.py:117-132) gives us a pure identity match (OR semantics across populated fields), independent of duplicate-detection state, policy, or time window — which is the correct semantic for "related requests".New UI
Code Changes
Backend — reverted prior-iteration additions (no backend changes ship in this PR):
duplicate_request_group_idfromPrivacyRequestFilterand its branch infilter_privacy_request_queryset.index=TruefromPrivacyRequest.duplicate_request_group_idand the deferred entry inpost_upgrade_index_creation.py.xx_2026_04_20_1200_e8f9a0b1c2d3_recreate_ix_privacyrequest_duplicate_request_group_id.pyand its upgrade/downgrade test.tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_duplicate_group_filtering.py.TestDeferredDuplicateRequestGroupIdIndexblock intests/api/migrations/test_post_upgrade_index_creation.py.Frontend — new related-requests drawer:
RelatedRequestsDrawer.tsx— callsuseSearchPrivacyRequestsQuery({ identities }), lists results using the existing<ListItem>from the request manager, pins the current request to the top with aCurrenttag, and links open in new tabs.relatedRequestsDrawerUtils.ts— pure helpersextractIdentityFields(builds the{ filter, labels }payload fromPrivacyRequestEntity.identity) andformatLabelList(Oxford-or list builder for the description copy). Unit tested inrelatedRequestsDrawerUtils.test.ts— 15 cases covering 0/1/2/3+ identity fields including email, phone, external id, custom ids.Frontend — entry points to the drawer:
· View related requestsand opens the drawer when the request has any identity value. (ActivityTimeline.tsx, ActivityTimelineEntry.tsx)Frontend —
ListItemextensions (used by both dashboard and drawer):compact?: booleanprop. When true, the days-left/received-on cluster always stacks vertically regardless of viewport (the dashboard's2xl:flex-rowviewport-driven media query is misleading inside a 640px drawer).showActions?: booleanprop (defaulttrue). When false, the per-rowRequestTableActionsblock is omitted.header?: { link?, extraTags? }nested-config prop, mirroring AntD patterns (Table.expandable,Table.pagination):header.link.target/header.link.rel— whentargetis set, the title link bypasses the in-approuter.pushinterceptor so the native anchor handles navigation. Drawer usestarget="_blank".header.extraTags— slot for an extra inline tag rendered alongside the status badge and rule tags. Drawer passes<Tag>Current</Tag>for the current row.Headernow groupsRequestStatusBadge, action-type tags, andextraTagsin a single<Flex>so spacing is consistent.<div className="pr-4">aroundcheckboxnow only renders when a checkbox is provided, so the drawer doesn't carry an unused 1rem left gap.ListItemtests still pass; dashboard call site is behaviorally unchanged.Frontend — type cleanup:
duplicate_request_group_idfromPrivacyRequestEntity(frontendtypes.ts) and from the auto-generatedPrivacyRequestFilter.ts. RenamedhasDuplicateGroup→hasRelatedRequestsonActivityTimelineItem.Changelog: 7965-duplicate-request-drawer.yaml reworded to describe the related-requests drawer;
db-migrationlabel removed.Steps to Confirm
npm run devinclients/admin-ui/) against a local backend.Showing all requests that match this request's email, including any that were marked as duplicates, the current request is pinned to the top with aCurrenttag, and the other request is listed below.· View related requests— same drawer opens.Days left/Received onvertically (no media-query flicker).… email or phone number, including any …and that requests sharing either value appear.RequestTableActionsstill render at the row's right edge (regression check onListItem).Pre-Merge Checklist
CHANGELOG.mdupdated