ENG-2338 - Configure when Banners resurface#7292
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile SummaryThis PR implements configurable banner resurfacing for privacy experiences, allowing consent banners to automatically re-display after a user rejects or dismisses them. It adds a The implementation is well-structured and consistent with existing patterns. Two issues were found:
Confidence Score: 3/5
Important Files Changed
|
...migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.py
Outdated
Show resolved
Hide resolved
...migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.py
Outdated
Show resolved
Hide resolved
...migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.py
Outdated
Show resolved
Hide resolved
...migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.py
Outdated
Show resolved
Hide resolved
|
@greptileai review |
tvandort
left a comment
There was a problem hiding this comment.
There's one aspect of this feature that I hadn't considered prior but do think we should reach out to product to ask about now.
If you change an experience from no resurfacing to resurface on reject, should rejections that were made prior to the behavior change resurface?
Here are the steps to reproduce my question:
- As the Privacy Admin configure an experience with both resurface methods off.
- As the End User (on cookie house or another test site) reject all.
- As the Privacy Admin change the same experience to resurface on reject.
- As the End User refresh the page
You will now be served the consent banner again. I think this could make sense but it could also make sense to not force users to re-consent at this point.
@mfbrown Probably has opinions here?
clients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx
Outdated
Show resolved
Hide resolved
| sa.Column( | ||
| "resurface_behavior", | ||
| sa.ARRAY(sa.String()), | ||
| nullable=True, |
There was a problem hiding this comment.
Would it be worthwhile to migrate these columns as non-nullable with the default selections in the column? Might allow us to remove defaulting in the code if it exists.
There was a problem hiding this comment.
We can do that, assuming we don't need to differentiate between "not configured" and "configured as empty"
There was a problem hiding this comment.
I don't think we need that.
| sa.Column( | ||
| "resurface_behavior", | ||
| sa.ARRAY(sa.String()), | ||
| nullable=True, |
There was a problem hiding this comment.
| sa.Column( | ||
| "resurface_behavior", | ||
| sa.ARRAY(sa.String()), | ||
| nullable=True, |
There was a problem hiding this comment.
This one probably should stay null since it's historical.
Co-authored-by: Tom Van Dort <tom@ethyca.com>
@mfbrown and I agreed that yes it should resurface. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new resurface_behavior option (REJECT, DISMISS) across DB schema, backend model, API types, admin UI, fides-js client logic, docs, tests, and changelog to control when consent banners/modals are re-shown. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI (Form)
participant API as Backend API
participant DB as Database
participant Client as Site (fides-js)
participant Storage as Cookie/LocalStorage
Admin->>API: Submit ExperienceConfig with resurface_behavior
API->>DB: Persist resurface_behavior (ARRAY enum)
DB-->>API: Acknowledge
API-->>Admin: 200 OK
Client->>Storage: Read stored consentMethod (reject/dismiss)
Client->>API: Fetch ExperienceConfig
API->>DB: Read resurface_behavior
DB-->>API: Return config
API-->>Client: Config with resurface_behavior
Client->>Client: shouldResurfaceBanner checks consentMethod ∈ resurface_behavior → show/hide banner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
clients/fides-js/docs/interfaces/FidesExperienceConfig.md (1)
135-144: Clarify the distinction between empty array and null.The description mentions "or be empty/null" but the examples only demonstrate
null. If an empty array[]andnullhave the same meaning (default behavior), consider either:
- Adding an example showing
[], or- Updating line 135 to say "or null" (removing "empty/")
If they have different meanings, add an example with
[]and explain the difference.📝 Proposed clarification
If
[]andnullare equivalent:-Can include "reject", "dismiss", both, or be empty/null for default behavior (only resurface on cookie expiration or vendor changes). +Can include "reject", "dismiss", both, or be null for default behavior (only resurface on cookie expiration or vendor changes).Or if they differ, add an example:
["reject", "dismiss"] // Resurface on both +[] // Explicit empty array - [explain behavior] null // Default behavior (no resurfacing)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/fides-js/docs/interfaces/FidesExperienceConfig.md` around lines 135 - 144, Update the FidesExperienceConfig docs to clarify whether an empty array [] differs from null: either add an explicit example and sentence showing [] (e.g., ["reject"], ["dismiss"], ["reject","dismiss"], [], null) and state that [] means the same as null (default behavior), or remove the word "empty/" from the description so it reads "or null" if only null is supported; update the Examples block in the FidesExperienceConfig.md to reflect the chosen behavior and ensure both [] and null cases are represented if they differ.tests/api/models/test_privacy_experience.py (1)
811-857: Use the versionedupdate()path in the resurface update test.This test currently mutates the ORM object directly and calls
save(), so it never exercisesPrivacyExperienceConfig.update(), which is the code path that createsPrivacyExperienceConfigHistorysnapshots for config changes. That leaves the audit/history behavior ofresurface_behavioreffectively untested.🧪 Suggested test shape
- # Update to include both - config.resurface_behavior = [ - ResurfaceBehavior.REJECT, - ResurfaceBehavior.DISMISS, - ] - config.save(db) - - assert config.resurface_behavior == [ - ResurfaceBehavior.REJECT, - ResurfaceBehavior.DISMISS, - ] + config.update( + db=db, + data={ + "resurface_behavior": [ + ResurfaceBehavior.REJECT, + ResurfaceBehavior.DISMISS, + ], + "translations": [ + { + "language": "en", + "description": "Privacy description", + "privacy_preferences_link_label": "Manage", + "modal_link_label": "Manage", + "privacy_policy_link_label": "Policy", + "privacy_policy_url": "http://example.com", + "reject_button_label": "Reject", + "save_button_label": "Save", + "title": "Privacy", + "accept_button_label": "Accept", + "acknowledge_button_label": "OK", + "banner_description": "Description", + "banner_title": "Title", + "is_default": True, + } + ], + }, + ) + db.refresh(config) + assert config.resurface_behavior == [ + ResurfaceBehavior.REJECT, + ResurfaceBehavior.DISMISS, + ] + assert config.translations[0].histories.count() == 2 + assert config.translations[0].histories[1].resurface_behavior == [ + ResurfaceBehavior.REJECT, + ResurfaceBehavior.DISMISS, + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/models/test_privacy_experience.py` around lines 811 - 857, The test mutates the ORM object and calls save(), so it never exercises the versioned update path that creates PrivacyExperienceConfigHistory entries; change the test to call PrivacyExperienceConfig.update(...) (the class/method under test) instead of assigning to config.resurface_behavior and config.save(), invoking the update API for each change (add both values, then set to None) and assert both the resurface_behavior on the returned/loaded config and that PrivacyExperienceConfigHistory entries are created as expected; reference PrivacyExperienceConfig.update, PrivacyExperienceConfig.create, and PrivacyExperienceConfigHistory when locating where to change the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx`:
- Around line 469-514: When dismissable is turned off in PrivacyExperienceForm,
existing values.resurface_behavior may still include ResurfaceBehavior.DISMISS;
add normalization to remove that value (or set resurface_behavior to null if no
values remain). Implement this by filtering values.resurface_behavior to exclude
ResurfaceBehavior.DISMISS whenever values.dismissable becomes false (e.g., in a
useEffect watching values.dismissable and values.resurface_behavior) and call
setFieldValue("resurface_behavior", filteredValuesOrNull). Also ensure any UI
handlers that toggle dismissable perform the same cleanup to prevent submitting
an invalid DISMISS selection.
In `@clients/fides-js/src/docs/fides-experience-config.ts`:
- Around line 90-102: The resurface_behavior property is documented to accept
null but its type is currently Array<string>; update the type of
resurface_behavior in fides-experience-config (the resurface_behavior? property)
to be nullable (e.g., Array<string> | null or string[] | null) so the TypeScript
type matches the documented/API shape, and adjust the JSDoc if needed to
explicitly state it can be null.
In `@clients/fides-js/src/lib/consent-utils.ts`:
- Around line 314-321: The TCF overlay early-return prevents honoring
experience.experience_config?.resurface_behavior for TCF overlays; update the
resurface logic in the consent-resurfacing function (the block that checks
ComponentType.TCF_OVERLAY and the block that checks
cookie?.fides_meta.consentMethod /
experience.experience_config?.resurface_behavior) so that resurface_behavior is
checked for TCF overlays before returning early, or incorporate the same
resurface_behavior check into the TCF overlay branch (use the same
cookie.fides_meta.consentMethod and
experience.experience_config?.resurface_behavior check and return true when
matched) ensuring ComponentType.TCF_OVERLAY experiences can resurface on
reject/dismiss.
---
Nitpick comments:
In `@clients/fides-js/docs/interfaces/FidesExperienceConfig.md`:
- Around line 135-144: Update the FidesExperienceConfig docs to clarify whether
an empty array [] differs from null: either add an explicit example and sentence
showing [] (e.g., ["reject"], ["dismiss"], ["reject","dismiss"], [], null) and
state that [] means the same as null (default behavior), or remove the word
"empty/" from the description so it reads "or null" if only null is supported;
update the Examples block in the FidesExperienceConfig.md to reflect the chosen
behavior and ensure both [] and null cases are represented if they differ.
In `@tests/api/models/test_privacy_experience.py`:
- Around line 811-857: The test mutates the ORM object and calls save(), so it
never exercises the versioned update path that creates
PrivacyExperienceConfigHistory entries; change the test to call
PrivacyExperienceConfig.update(...) (the class/method under test) instead of
assigning to config.resurface_behavior and config.save(), invoking the update
API for each change (add both values, then set to None) and assert both the
resurface_behavior on the returned/loaded config and that
PrivacyExperienceConfigHistory entries are created as expected; reference
PrivacyExperienceConfig.update, PrivacyExperienceConfig.create, and
PrivacyExperienceConfigHistory when locating where to change the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4c9f6e4-eeb2-4d38-9ec6-d91365c1763a
📒 Files selected for processing (14)
.fides/db_dataset.ymlchangelog/7292.yamlclients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsxclients/admin-ui/src/types/api/index.tsclients/admin-ui/src/types/api/models/ExperienceConfigCreate.tsclients/admin-ui/src/types/api/models/ExperienceConfigUpdate.tsclients/admin-ui/src/types/api/models/ResurfaceBehavior.tsclients/fides-js/__tests__/lib/consent-utils.test.tsclients/fides-js/docs/interfaces/FidesExperienceConfig.mdclients/fides-js/src/docs/fides-experience-config.tsclients/fides-js/src/lib/consent-utils.tssrc/fides/api/alembic/migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.pysrc/fides/api/models/privacy_experience.pytests/api/models/test_privacy_experience.py
| {(values.component === ComponentType.BANNER_AND_MODAL || | ||
| values.component === ComponentType.TCF_OVERLAY) && ( | ||
| <Box> | ||
| <FormLabel fontSize="sm" fontWeight="semibold" mb={2}> | ||
| Resurface banner | ||
| </FormLabel> | ||
| <Text fontSize="sm" color="gray.600" mb={3}> | ||
| Choose when to show the banner again after the user has interacted | ||
| with it. Leave unchecked for default behavior (only resurface on | ||
| cookie expiration, vendor changes, and other mandatory updates.) | ||
| </Text> | ||
| <CheckboxGroup | ||
| value={values.resurface_behavior ?? []} | ||
| onChange={(selectedValues) => { | ||
| setFieldValue( | ||
| "resurface_behavior", | ||
| selectedValues.length > 0 ? selectedValues : null, | ||
| ); | ||
| }} | ||
| > | ||
| <Stack spacing={2}> | ||
| {resurfaceBehaviorOptions.map((option) => { | ||
| const isDisabled = | ||
| option.value === ResurfaceBehavior.DISMISS && | ||
| !values.dismissable; | ||
| return ( | ||
| <Checkbox | ||
| key={option.value} | ||
| value={option.value} | ||
| isDisabled={isDisabled} | ||
| > | ||
| <Box> | ||
| <Text fontSize="sm" fontWeight="medium"> | ||
| {option.label} | ||
| </Text> | ||
| <Text fontSize="xs" color="gray.600"> | ||
| {option.description} | ||
| </Text> | ||
| </Box> | ||
| </Checkbox> | ||
| ); | ||
| })} | ||
| </Stack> | ||
| </CheckboxGroup> | ||
| </Box> | ||
| )} |
There was a problem hiding this comment.
Normalize resurface_behavior when dismissal is disabled.
isDisabled only blocks editing here; it does not clear an existing ResurfaceBehavior.DISMISS selection from Formik state. If someone edits a config that already has DISMISS selected and then turns dismissable off, this form can still save that now-invalid combination.
💡 Suggested fix
{values.component !== ComponentType.PRIVACY_CENTER &&
values.component !== ComponentType.HEADLESS && (
<Box p="1px">
<CustomSwitch
name="dismissable"
id="dismissable"
label="Allow user to dismiss"
variant="stacked"
+ onChange={(checked) => {
+ setFieldValue("dismissable", checked);
+ if (!checked) {
+ const nextValues =
+ values.resurface_behavior?.filter(
+ (value) => value !== ResurfaceBehavior.DISMISS,
+ ) ?? [];
+ setFieldValue(
+ "resurface_behavior",
+ nextValues.length ? nextValues : null,
+ );
+ }
+ }}
/>
</Box>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx`
around lines 469 - 514, When dismissable is turned off in PrivacyExperienceForm,
existing values.resurface_behavior may still include ResurfaceBehavior.DISMISS;
add normalization to remove that value (or set resurface_behavior to null if no
values remain). Implement this by filtering values.resurface_behavior to exclude
ResurfaceBehavior.DISMISS whenever values.dismissable becomes false (e.g., in a
useEffect watching values.dismissable and values.resurface_behavior) and call
setFieldValue("resurface_behavior", filteredValuesOrNull). Also ensure any UI
handlers that toggle dismissable perform the same cleanup to prevent submitting
an invalid DISMISS selection.
| /** | ||
| * This corresponds with the "Resurface banner" configuration option. | ||
| * Controls when to show the consent banner again after the user has interacted with it. | ||
| * Can include "reject", "dismiss", both, or be empty/null for default behavior (only resurface on cookie expiration or vendor changes). | ||
| * @example | ||
| * ```ts | ||
| * ["reject"] // Resurface only on reject | ||
| * ["dismiss"] // Resurface only on dismiss | ||
| * ["reject", "dismiss"] // Resurface on both | ||
| * null // Default behavior (no resurfacing) | ||
| * ``` | ||
| */ | ||
| resurface_behavior?: Array<string>; |
There was a problem hiding this comment.
Allow null here to match the documented/API shape.
The example block says null disables resurfacing, but the declared type only allows Array<string>. The generated API types already model this as nullable, so this doc contract is currently narrower than what callers can actually receive.
Suggested fix
- resurface_behavior?: Array<string>;
+ resurface_behavior?: Array<"reject" | "dismiss"> | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * This corresponds with the "Resurface banner" configuration option. | |
| * Controls when to show the consent banner again after the user has interacted with it. | |
| * Can include "reject", "dismiss", both, or be empty/null for default behavior (only resurface on cookie expiration or vendor changes). | |
| * @example | |
| * ```ts | |
| * ["reject"] // Resurface only on reject | |
| * ["dismiss"] // Resurface only on dismiss | |
| * ["reject", "dismiss"] // Resurface on both | |
| * null // Default behavior (no resurfacing) | |
| * ``` | |
| */ | |
| resurface_behavior?: Array<string>; | |
| /** | |
| * This corresponds with the "Resurface banner" configuration option. | |
| * Controls when to show the consent banner again after the user has interacted with it. | |
| * Can include "reject", "dismiss", both, or be empty/null for default behavior (only resurface on cookie expiration or vendor changes). | |
| * `@example` | |
| * |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/fides-js/src/docs/fides-experience-config.ts` around lines 90 - 102,
The resurface_behavior property is documented to accept null but its type is
currently Array<string>; update the type of resurface_behavior in
fides-experience-config (the resurface_behavior? property) to be nullable (e.g.,
Array<string> | null or string[] | null) so the TypeScript type matches the
documented/API shape, and adjust the JSDoc if needed to explicitly state it can
be null.
| // Resurface if configured for this consent method | ||
| if ( | ||
| cookie?.fides_meta.consentMethod && | ||
| experience.experience_config?.resurface_behavior?.includes( | ||
| cookie.fides_meta.consentMethod, | ||
| ) | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
resurface_behavior is unreachable for TCF overlays.
This branch never runs for ComponentType.TCF_OVERLAY because the function already returns in the TCF block at Lines 279-290. That means a TCF experience configured to resurface on reject/dismiss will still ignore this setting once the version hash matches.
Suggested fix
if (
experience.experience_config?.component === ComponentType.TCF_OVERLAY &&
!!cookie
) {
if (!!options && isConsentOverride(options)) {
return false;
}
+ if (
+ cookie.fides_meta.consentMethod &&
+ experience.experience_config?.resurface_behavior?.includes(
+ cookie.fides_meta.consentMethod,
+ )
+ ) {
+ return true;
+ }
if (experience.meta?.version_hash) {
return experience.meta.version_hash !== cookie.tcf_version_hash;
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Resurface if configured for this consent method | |
| if ( | |
| cookie?.fides_meta.consentMethod && | |
| experience.experience_config?.resurface_behavior?.includes( | |
| cookie.fides_meta.consentMethod, | |
| ) | |
| ) { | |
| return true; | |
| if ( | |
| experience.experience_config?.component === ComponentType.TCF_OVERLAY && | |
| !!cookie | |
| ) { | |
| if (!!options && isConsentOverride(options)) { | |
| return false; | |
| } | |
| if ( | |
| cookie.fides_meta.consentMethod && | |
| experience.experience_config?.resurface_behavior?.includes( | |
| cookie.fides_meta.consentMethod, | |
| ) | |
| ) { | |
| return true; | |
| } | |
| if (experience.meta?.version_hash) { | |
| return experience.meta.version_hash !== cookie.tcf_version_hash; | |
| } | |
| return true; | |
| } | |
| // Resurface if configured for this consent method | |
| if ( | |
| cookie?.fides_meta.consentMethod && | |
| experience.experience_config?.resurface_behavior?.includes( | |
| cookie.fides_meta.consentMethod, | |
| ) | |
| ) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/fides-js/src/lib/consent-utils.ts` around lines 314 - 321, The TCF
overlay early-return prevents honoring
experience.experience_config?.resurface_behavior for TCF overlays; update the
resurface logic in the consent-resurfacing function (the block that checks
ComponentType.TCF_OVERLAY and the block that checks
cookie?.fides_meta.consentMethod /
experience.experience_config?.resurface_behavior) so that resurface_behavior is
checked for TCF overlays before returning early, or incorporate the same
resurface_behavior check into the TCF overlay branch (use the same
cookie.fides_meta.consentMethod and
experience.experience_config?.resurface_behavior check and return true when
matched) ensuring ComponentType.TCF_OVERLAY experiences can resurface on
reject/dismiss.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/models/privacy_experience.py`:
- Around line 68-77: The EnumColumn for ResurfaceBehavior is persisting member
names instead of the defined string values; update the EnumColumn usage for
ResurfaceBehavior (where EnumColumn(ResurfaceBehavior, native_enum=False) is
declared) to include values_callable=lambda enum_cls: [member.value for member
in enum_cls] so the DB stores "reject"/"dismiss" rather than "REJECT"/"DISMISS";
apply the same change to the EnumColumn declaration for PrivacyNoticeRegion to
ensure it persists the enum .value strings rather than member names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8653f0bd-17da-43a9-826e-8f8458209bff
📒 Files selected for processing (1)
src/fides/api/models/privacy_experience.py
| class ResurfaceBehavior(Enum): | ||
| """ | ||
| Resurface behavior options - controls when to re-show the banner/modal. | ||
| Used to configure whether the experience resurfaces after rejection or dismissal. | ||
| """ | ||
|
|
||
| # Resurface the banner after user rejects | ||
| REJECT = "reject" | ||
| # Resurface the banner after user dismisses | ||
| DISMISS = "dismiss" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SQLAlchemy enum labels =="
python - <<'PY'
import sqlalchemy as sa
from enum import Enum
class ResurfaceBehavior(Enum):
REJECT = "reject"
DISMISS = "dismiss"
t = sa.Enum(ResurfaceBehavior, native_enum=False)
print(t.enums)
PY
echo
echo "== resurface_behavior usages =="
rg -n -C2 '\bresurface_behavior\b'
echo
echo "== enum mappings that already use values_callable in this file =="
rg -n -C2 'values_callable' src/fides/api/models/privacy_experience.pyRepository: ethyca/fides
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Read the privacy_experience.py file to examine the ResurfaceBehavior enum and resurface_behavior column
echo "=== Reading src/fides/api/models/privacy_experience.py ==="
wc -l src/fides/api/models/privacy_experience.py
echo -e "\n=== ResurfaceBehavior enum (around lines 68-77) ==="
sed -n '68,77p' src/fides/api/models/privacy_experience.py
echo -e "\n=== resurface_behavior column (around lines 124-129) ==="
sed -n '124,135p' src/fides/api/models/privacy_experience.py
echo -e "\n=== SupportedLanguage enum with values_callable (around lines 209-216) ==="
sed -n '200,230p' src/fides/api/models/privacy_experience.pyRepository: ethyca/fides
Length of output: 2179
🏁 Script executed:
#!/bin/bash
echo "=== Search for migrations mentioning resurface_behavior ==="
fd -e py migrations | head -20
echo -e "\n=== Look for migration files ==="
find . -type d -name migrations | head -5
echo -e "\n=== Search for test files mentioning ResurfaceBehavior or resurface_behavior ==="
rg -l 'ResurfaceBehavior|resurface_behavior' --type py tests/ 2>/dev/null | head -10
echo -e "\n=== Check SupportedLanguage usage to confirm values_callable behavior ==="
rg -n 'SupportedLanguage' src/fides/api/models/privacy_experience.py | head -20
echo -e "\n=== Search for any documentation or comments about enum storage behavior ==="
rg -n 'enum.*value|stored.*enum' src/fides/api/models/privacy_experience.pyRepository: ethyca/fides
Length of output: 1121
🏁 Script executed:
#!/bin/bash
echo "=== Migration file: 1ea164cee8bc ==="
cat src/fides/api/alembic/migrations/versions/1ea164cee8bc_fideslang_2_data_migrations.py | head -100
echo -e "\n=== Check enum on line 526 ==="
sed -n '520,535p' src/fides/api/models/privacy_experience.py
echo -e "\n=== Test file for privacy_experience (search for resurface_behavior) ==="
rg -n -B2 -A5 'resurface_behavior' tests/api/models/test_privacy_experience.py | head -60Repository: ethyca/fides
Length of output: 6673
🏁 Script executed:
#!/bin/bash
echo "=== Find the actual migration that creates resurface_behavior column ==="
find src/fides/api/alembic/migrations -name "*.py" -exec grep -l "resurface_behavior" {} \;
echo -e "\n=== Read the migration file that adds resurface_behavior ==="
# Look for the most recent migration mentioning resurface_behavior
migration_file=$(find src/fides/api/alembic/migrations -name "*.py" -exec grep -l "resurface_behavior" {} \; | sort | tail -1)
echo "File: $migration_file"
if [ -f "$migration_file" ]; then
cat "$migration_file" | head -200
fi
echo -e "\n=== Search for API schemas that might reference resurface_behavior ==="
rg -n 'resurface_behavior' src/fides/api/schemas/ -B2 -A2Repository: ethyca/fides
Length of output: 1948
🏁 Script executed:
#!/bin/bash
echo "=== Search for API serialization or validation of resurface_behavior ==="
rg -n 'resurface_behavior' src/fides/api --type py -B2 -A2 | head -80
echo -e "\n=== Check if there's any validation or transformation logic ==="
rg -n 'ResurfaceBehavior' src/fides/api --type py | grep -v test | head -20
echo -e "\n=== Look for direct string comparisons with resurface_behavior ==="
rg -n '"reject"|"dismiss"' src/fides/api --type py | head -20Repository: ethyca/fides
Length of output: 5621
🏁 Script executed:
#!/bin/bash
echo "=== Verify SQLAlchemy behavior: Enum with/without values_callable ==="
rg -n 'EnumColumn.*native_enum=False' src/fides/api/models/privacy_experience.py -A3
echo -e "\n=== Check if ResurfaceBehavior appears in any API response schemas ==="
find src/fides/api/schemas -name "*.py" -exec grep -l "resurface\|ResurfaceBehavior" {} \;
echo -e "\n=== Check how columns are serialized in API responses ==="
rg -n 'resurface_behavior' src/fides/api/schemas --type py -B2 -A2Repository: ethyca/fides
Length of output: 551
🏁 Script executed:
#!/bin/bash
echo "=== Check PrivacyNoticeRegion enum definition ==="
rg -n 'class PrivacyNoticeRegion' src/fides/api/models/privacy_experience.py -A15
echo -e "\n=== Confirm: is there any place that would read resurface_behavior as raw string from DB ==="
rg -n 'ARRAY.*String' src/fides/api/alembic/migrations/versions/xx_2026_01_13_1934_ea20059aee77_add_resurface_behavior_to_experience_.pyRepository: ethyca/fides
Length of output: 313
Use values_callable to persist enum values, not member names.
On line 125, EnumColumn(ResurfaceBehavior, native_enum=False) will store member names (REJECT/DISMISS) instead of the enum values (reject/dismiss) defined on the class. This diverges from the schema expected by the migration and from the pattern established by SupportedLanguage elsewhere in this file (lines 211–219). Add values_callable=lambda enum_cls: [member.value for member in enum_cls] to persist the correct values.
Proposed fix
resurface_behavior = Column(
- ARRAY(EnumColumn(ResurfaceBehavior, native_enum=False)),
+ ARRAY(
+ EnumColumn(
+ ResurfaceBehavior,
+ native_enum=False,
+ values_callable=lambda enum_cls: [member.value for member in enum_cls],
+ )
+ ),
nullable=True,
server_default="{}",
default=list,
)Also applies to: 189 (PrivacyNoticeRegion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/models/privacy_experience.py` around lines 68 - 77, The
EnumColumn for ResurfaceBehavior is persisting member names instead of the
defined string values; update the EnumColumn usage for ResurfaceBehavior (where
EnumColumn(ResurfaceBehavior, native_enum=False) is declared) to include
values_callable=lambda enum_cls: [member.value for member in enum_cls] so the DB
stores "reject"/"dismiss" rather than "REJECT"/"DISMISS"; apply the same change
to the EnumColumn declaration for PrivacyNoticeRegion to ensure it persists the
enum .value strings rather than member names.
|
for the base class, nullable has to be True because it's shared with the history table. |
|
@greptileai review |
| const isDisabled = | ||
| option.value === ResurfaceBehavior.DISMISS && | ||
| !values.dismissable; | ||
| return ( | ||
| <Checkbox | ||
| key={option.value} | ||
| value={option.value} | ||
| isDisabled={isDisabled} | ||
| > | ||
| <Box> | ||
| <Text fontSize="sm" fontWeight="medium"> | ||
| {option.label} | ||
| </Text> | ||
| <Text fontSize="xs" color="gray.600"> | ||
| {option.description} | ||
| </Text> | ||
| </Box> | ||
| </Checkbox> | ||
| ); |
There was a problem hiding this comment.
Stale "dismiss" value when dismissable is toggled off
When dismissable is set to false, the "Dismiss" checkbox in the resurface_behavior group becomes disabled — but the value is not cleared from values.resurface_behavior. This means:
- If a user previously checked "Resurface on Dismiss", then toggles
dismissableoff, the checkbox appears checked-and-grayed (confusing UX). - On form submission,
resurface_behaviorstill includes"dismiss"even thoughdismissableisfalse— storing a logically inconsistent configuration in the database.
The form should clear "dismiss" from resurface_behavior when dismissable is set to false. One approach: in the onChange handler for the dismissable switch (or in a useEffect watching values.dismissable), remove ResurfaceBehavior.DISMISS from the array if it is present:
// When dismissable is turned off, clear "dismiss" from resurface_behavior
useEffect(() => {
if (!values.dismissable && values.resurface_behavior?.includes(ResurfaceBehavior.DISMISS)) {
setFieldValue(
"resurface_behavior",
values.resurface_behavior.filter((v) => v !== ResurfaceBehavior.DISMISS) || null,
);
}
}, [values.dismissable]);
Ticket ENG-2338
Description Of Changes
This PR serves to implement a configurable banner resurfacing feature that allows privacy experiences to automatically re-display consent banners after users reject or dismiss them.
Code Changes
resurface_behaviorARRAY column toprivacyexperienceconfigtable with migrationResurfaceBehaviorenum withREJECTandDISMISSoptionsTODO
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit
New Features
Documentation
Tests
Chores