Adding source and submitted_by fields to privacy request #5206

galvana · 2024-08-15T23:35:52Z

❗Contains migration to add new columns to the consentrequest and privacyrequest tables

Description Of Changes

Updating privacy requests to include an optional source field. The options are currently restricted to the following:

Privacy Center - The most common source, for requests sent from the Privacy Center
Request Manager - For request submitted via the Request manager page in the Admin UI (plus only)
Consent Webhook - For requests created as part of the consent webhook flow for bidirectional consent (plus only)

This is a plus feature so I tried to draw the line in a reasonable way that wouldn't add to much scope. For example, I decided against creating overrides for the /consent and /privacy-request endpoints in plus that accept the source value in the request.

Code Changes

Updated requests from the Privacy Center to have a source of Privacy Center
Updated request from the Request manager page to have a source of Request Manager and to store the submitting user ID under the new privacy request column, submitted_by
Updated the Admin UI to show the source column in the privacy request table and privacy request detail page if plus is enabled

Steps to Confirm

Start fidesplus
Submit a review from the Privacy Center and from the Request manager in the Admin UI
Verify the two requests show the source in the Privacy request table and the privacy request details

Pre-Merge Checklist

All CI Pipelines Succeeded
Issue Requirements are Met
Update CHANGELOG.md
If there are any 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!

vercel · 2024-08-15T23:35:55Z

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

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Aug 26, 2024 9:27pm

clients/admin-ui/src/features/privacy-requests/RequestDetails.tsx

clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts

clients/admin-ui/src/types/api/models/CollectionMeta.ts

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

galvana · 2024-08-16T19:49:41Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+            getattr(privacy_request_data, "source")
+            == PrivacyRequestSource.request_manager
+        ):
+            kwargs["submitted_by"] = user_id


This ticket adds a new submitted_by field to privacy requests so we can track the user that submitted a request from the Request manager page within the Admin UI.

do we have an enum set of accepted optional privacy req kwargs somewhere?

No enums here unfortunately but I do have a test for this so we'll know if the kwarg name ever changes

clients/admin-ui/src/features/privacy-requests/RequestTableColumns.tsx

clients/admin-ui/src/features/privacy-requests/SubjectIdentities.tsx

tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py

clients/admin-ui/src/features/privacy-requests/RequestTableColumns.tsx

clients/admin-ui/cypress/fixtures/privacy-requests/list.json

cypress · 2024-08-17T00:09:04Z

fides Run #9659

Run Properties: Passed #9659 • cc6eb2b3a3 ℹ️: Merge fa75aa11264c5a08020860a630cec374d2d33fe7 into b47f171d9b5b4ee1dd1236e824c1...

Project	`fides`
Branch Review	`refs/pull/5206/merge`
Run status	`Passed #9659`
Run duration	`00m 38s`
Commit	`cc6eb2b3a3 ℹ️: Merge fa75aa11264c5a08020860a630cec374d2d33fe7 into b47f171d9b5b4ee1dd1236e824c1...`
Committer	`Adrian Galvan`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
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 ↗︎

eastandwestwind · 2024-08-22T14:16:53Z

Starting review...

eastandwestwind

Looks great @galvana , just a couple clean-up items / suggestions for improvement.

Also kudos for knocking out general improvements to the codebase as you come across them!!

clients/admin-ui/src/features/privacy-requests/RequestTable.tsx

clients/admin-ui/src/features/privacy-requests/RequestTableColumns.tsx

eastandwestwind · 2024-08-22T14:56:06Z

clients/fides-js/src/services/api.ts

@@ -183,7 +183,7 @@ export const patchUserPreference = async (
  debugLog(options.debug, "Calling Fides save preferences API");
  const fetchOptions: RequestInit = {
    ...PATCH_FETCH_OPTIONS,
-    body: JSON.stringify(preferences),
+    body: JSON.stringify({ ...preferences, source: "Fides.js" }),


possible to use a shared CONST here? instead of "Fides.js"?

// See: PrivacyRequestSource enum in Fides const REQUEST_SOURCE = "Fides.js";

eastandwestwind · 2024-08-22T14:58:19Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+    if not include_consent_webhook_requests:
+        query = query.filter(
+            or_(
+                PrivacyRequest.source != PrivacyRequestSource.consent_webhook,
+                PrivacyRequest.source.is_(None),
+            )
+        )


hmm do we want to filter where source is None? Won't this affect the ability to filter for privacy requests created before this work gets merged?

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

eastandwestwind · 2024-08-22T15:20:50Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

@@ -2003,6 +2021,9 @@ def create_privacy_request_func(
            "Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."
        )

+    if privacy_preferences is None:
+        privacy_preferences = []


Can we set this default in the func signature instead?

privacy_preferences: Optional[ List[PrivacyPreferenceHistory] ] = []

I set this as a None value to avoid this gotcha with mutable default arguments https://docs.python-guide.org/writing/gotchas/. We don't mutate privacy_preferences in our function right now but I figured I'd do this now to avoid any pitfalls in the future.

I also changed the None check to this to let mypy know that privacy_preferences will be valued, even if it's an empty array

privacy_preferences = privacy_preferences or []

eastandwestwind · 2024-08-22T15:22:19Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+            getattr(privacy_request_data, "source")
+            == PrivacyRequestSource.request_manager
+        ):
+            kwargs["submitted_by"] = user_id


do we have an enum set of accepted optional privacy req kwargs somewhere?

eastandwestwind · 2024-08-22T15:27:11Z

src/fides/api/models/privacy_request.py

@@ -294,6 +299,7 @@ class PrivacyRequest(
    cancel_reason = Column(String(200))
    canceled_at = Column(DateTime(timezone=True), nullable=True)
    consent_preferences = Column(MutableList.as_mutable(JSONB), nullable=True)
+    source = Column(String, nullable=True)


Can we instead use a Column(EnumColumn(PrivacyRequestSource), nullable=True) here? This way we can still have the benefit of explicitly defining what we should expect in this column without having the enums formalized in the DB?

See how we do something similar with method = Column(EnumColumn(ConsentMethod)) in our PrivacyPreferenceHistory model

Good suggestion 👍

src/fides/api/models/privacy_request.py

eastandwestwind · 2024-08-22T15:28:31Z

src/fides/api/schemas/privacy_request.py

+    """The source where the privacy request originated from"""
+
+    privacy_center = "Privacy Center"
+    request_manager = "Request Manager"


could you add a comment as to what each of these sources mean?

I updated the docstring

The source where the privacy request originated from - Privacy Center: Request created from the Privacy Center - Request Manager: Request submitted from the Admin UI's Request manager page - Consent Webhook: Request created as a side-effect of a consent webhook request (bidirectional consent) - Fides.js: Request created as a side-effect of a privacy preference update from Fides.js

thanks! This is awesome!

galvana

Thanks for the review @eastandwestwind! I made the suggestions you recommended and I left some responses to your questions and concerns

galvana · 2024-08-23T03:44:12Z

clients/fides-js/src/services/api.ts

@@ -183,7 +183,7 @@ export const patchUserPreference = async (
  debugLog(options.debug, "Calling Fides save preferences API");
  const fetchOptions: RequestInit = {
    ...PATCH_FETCH_OPTIONS,
-    body: JSON.stringify(preferences),
+    body: JSON.stringify({ ...preferences, source: "Fides.js" }),


// See: PrivacyRequestSource enum in Fides const REQUEST_SOURCE = "Fides.js";

galvana · 2024-08-23T03:44:56Z

src/fides/api/models/privacy_request.py

@@ -294,6 +299,7 @@ class PrivacyRequest(
    cancel_reason = Column(String(200))
    canceled_at = Column(DateTime(timezone=True), nullable=True)
    consent_preferences = Column(MutableList.as_mutable(JSONB), nullable=True)
+    source = Column(String, nullable=True)


Good suggestion 👍

galvana · 2024-08-23T03:51:48Z

src/fides/api/schemas/privacy_request.py

+    """The source where the privacy request originated from"""
+
+    privacy_center = "Privacy Center"
+    request_manager = "Request Manager"


I updated the docstring

The source where the privacy request originated from - Privacy Center: Request created from the Privacy Center - Request Manager: Request submitted from the Admin UI's Request manager page - Consent Webhook: Request created as a side-effect of a consent webhook request (bidirectional consent) - Fides.js: Request created as a side-effect of a privacy preference update from Fides.js

galvana · 2024-08-23T03:57:14Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+            getattr(privacy_request_data, "source")
+            == PrivacyRequestSource.request_manager
+        ):
+            kwargs["submitted_by"] = user_id


No enums here unfortunately but I do have a test for this so we'll know if the kwarg name ever changes

galvana · 2024-08-23T04:05:57Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+    if not include_consent_webhook_requests:
+        query = query.filter(
+            or_(
+                PrivacyRequest.source != PrivacyRequestSource.consent_webhook,
+                PrivacyRequest.source.is_(None),
+            )
+        )


This threw me off at first too but if I only do

PrivacyRequest.source != PrivacyRequestSource.consent_webhook,

The query will only return privacy requests where source is valued and doesn't equal consent_webook but it won't automatically include any privacy requests with null source values. So the full statement should read:

If we're not including consent webhook requests, then give me all the privacy requests where source (valued) does not equal consent_webhook or all privacy requests where source is null (inclusive or).

galvana · 2024-08-23T05:13:20Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

@@ -2003,6 +2021,9 @@ def create_privacy_request_func(
            "Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."
        )

+    if privacy_preferences is None:
+        privacy_preferences = []


I set this as a None value to avoid this gotcha with mutable default arguments https://docs.python-guide.org/writing/gotchas/. We don't mutate privacy_preferences in our function right now but I figured I'd do this now to avoid any pitfalls in the future.

I also changed the None check to this to let mypy know that privacy_preferences will be valued, even if it's an empty array

privacy_preferences = privacy_preferences or []

galvana

Thanks for the review @eastandwestwind! I made the suggestions you recommended and I left some responses to your questions and concerns

galvana · 2024-08-23T05:21:40Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

@@ -2003,6 +2021,8 @@ def create_privacy_request_func(
            "Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."
        )

+    privacy_preferences = privacy_preferences or []


I changed the way I did this check so my response to your old comment got flagged as "outdated" but I wanted to make sure you read my response.

I set this as a None value to avoid this gotcha with mutable default arguments https://docs.python-guide.org/writing/gotchas/. We don't mutate privacy_preferences in our function right now but I figured I'd do this now to avoid any pitfalls in the future.

I also changed the None check to this to let mypy know that privacy_preferences will be valued, even if it's an empty array

privacy_preferences = privacy_preferences or []

Oh noooo this is the worst gotcha! Wow, thanks for explaining!

codecov · 2024-08-24T19:37:46Z

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.41%. Comparing base (b47f171) to head (fa75aa1).
Report is 1 commits behind head on main.

Files	Patch %	Lines
.../api/api/v1/endpoints/privacy_request_endpoints.py	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5206   +/-   ##
=======================================
  Coverage   86.41%   86.41%           
=======================================
  Files         362      362           
  Lines       22765    22781   +16     
  Branches     3056     3058    +2     
=======================================
+ Hits        19672    19687   +15     
  Misses       2537     2537           
- Partials      556      557    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cypress · 2024-08-26T21:40:28Z

fides Run #9660

Run Properties: Passed #9660 • 619ca8589e: Adding source and submitted_by fields to privacy request (#5206)

Project	`fides`
Branch Review	`main`
Run status	`Passed #9660`
Run duration	`00m 39s`
Commit	`619ca8589e: Adding source and submitted_by fields to privacy request (#5206)`
Committer	`Adrian Galvan`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
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 ↗︎

Adding source and submitted_by fields to privacy request

9521550

Updating type for Admin UI

4eca107

vercel bot deployed to Preview August 15, 2024 23:42 View deployment

galvana added 4 commits August 15, 2024 16:46

Updating source values in Privacy Center to use enums

1b0f595

Storing submitted_by for privacy requests

5753e94

Refactoring logic to filter out consent webhook requests

aa1ad40

Adding tests

146df41

vercel bot deployed to Preview August 16, 2024 23:00 View deployment

Hiding privacy request source data behind plus flag

9066caf

vercel bot deployed to Preview August 16, 2024 23:45 View deployment

Merge branch 'main' into PROD-1788-track-request-origin

ddd554f

galvana marked this pull request as ready for review August 16, 2024 23:53

galvana commented Aug 16, 2024

View reviewed changes

galvana added 2 commits August 16, 2024 18:15

Fixing tests

296fd54

Merge branch 'main' into PROD-1788-track-request-origin

d0adc40

vercel bot deployed to Preview August 20, 2024 18:12 View deployment

galvana requested a review from eastandwestwind August 21, 2024 17:56

Merge branch 'main' into PROD-1788-track-request-origin

fd674ed

vercel bot deployed to Preview August 21, 2024 18:00 View deployment

Adding support for Fides.js as a source

1d69b06

vercel bot deployed to Preview August 22, 2024 00:05 View deployment

eastandwestwind requested changes Aug 22, 2024

View reviewed changes

Merge branch 'main' into PROD-1788-track-request-origin

03a389a

vercel bot deployed to Preview August 23, 2024 04:18 View deployment

galvana added 2 commits August 22, 2024 21:48

Changes based on PR feedback

227ddbd

Refactoring PrivacyRequestSource to avoid cyclic dependency

1301015

galvana commented Aug 23, 2024

View reviewed changes

Prettier fix

837e347

galvana commented Aug 23, 2024

View reviewed changes

vercel bot deployed to Preview August 23, 2024 05:18 View deployment

galvana commented Aug 23, 2024

View reviewed changes

galvana added 2 commits August 24, 2024 10:41

Merge branch 'main' into PROD-1788-track-request-origin

f0b86b3

Fixing tests

13a0893

galvana requested a review from eastandwestwind August 24, 2024 18:28

Enabling custom privacy request field collection for tests

384d8d5

eastandwestwind approved these changes Aug 26, 2024

View reviewed changes

Updating change log

fa75aa1

galvana merged commit 619ca85 into main Aug 26, 2024
20 checks passed

galvana deleted the PROD-1788-track-request-origin branch August 26, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding source and submitted_by fields to privacy request #5206

Adding source and submitted_by fields to privacy request #5206

galvana commented Aug 15, 2024 •

edited

Loading

vercel bot commented Aug 15, 2024 •

edited

Loading

galvana Aug 16, 2024

eastandwestwind Aug 22, 2024

galvana Aug 23, 2024

cypress bot commented Aug 17, 2024 •

edited

Loading

eastandwestwind commented Aug 22, 2024

eastandwestwind left a comment

eastandwestwind Aug 22, 2024

galvana Aug 23, 2024

eastandwestwind Aug 22, 2024

eastandwestwind Aug 22, 2024

galvana Aug 23, 2024

eastandwestwind Aug 22, 2024

eastandwestwind Aug 22, 2024

galvana Aug 23, 2024

eastandwestwind Aug 22, 2024

galvana Aug 23, 2024

eastandwestwind Aug 23, 2024

galvana left a comment

galvana Aug 23, 2024

galvana Aug 23, 2024

galvana Aug 23, 2024

galvana Aug 23, 2024

galvana Aug 23, 2024

galvana Aug 23, 2024

galvana left a comment

galvana Aug 23, 2024

eastandwestwind Aug 23, 2024

codecov bot commented Aug 24, 2024 •

edited

Loading

cypress bot commented Aug 26, 2024

Adding source and submitted_by fields to privacy request #5206

Adding source and submitted_by fields to privacy request #5206

Conversation

galvana commented Aug 15, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Aug 17, 2024 • edited Loading

eastandwestwind commented Aug 22, 2024

eastandwestwind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2024 • edited Loading

Codecov Report

cypress bot commented Aug 26, 2024

galvana commented Aug 15, 2024 •

edited

Loading

vercel bot commented Aug 15, 2024 •

edited

Loading

cypress bot commented Aug 17, 2024 •

edited

Loading

codecov bot commented Aug 24, 2024 •

edited

Loading