ENG-1923 Use override_mode in privacy notice sandbox #6980
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| placeholder="No override" | ||
| allowClear | ||
| options={options} | ||
| aria-labelledby="override-mode-select-label" |
There was a problem hiding this comment.
I'm not sure why but if I didn't add aria-labelledby I got an jsx-a11y error about components needing labels, even when having the <label> tag with htmlFor
| const [overrideMode, setOverrideMode] = useState<OverrideMode | null>(null); | ||
| const [checkedKeys, setCheckedKeys] = useState<Key[]>([]); | ||
| const [fetchedCheckedKeys, setFetchedCheckedKeys] = useState<Key[]>([]); | ||
| const [explicitlyChangedKeys, setExplicitlyChangedKeys] = useState<Key[]>([]); |
There was a problem hiding this comment.
this new state is needed to track the keys the user has explicitly checked/unchecked, separate from checkedKeys which includes cascading logic (e.g when override_mode is descendants, if I opt in to the parent notice checked keys will include both parent and children, but for the API call I want just the explicitly opted in parent to be sent)
Greptile Summary
Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
clients/admin-ui/src/features/poc/privacy-notices-sandbox/PrivacyNoticeSandboxRealData.tsx, line 66-103 (link)style:
childToParentMapis computed but no longer used - it's not passed toSavePreferencesSectionand the only other usage is inmergePreferencesWithExisting. Consider removing it from the memo return value or documenting why it's kept
7 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| // Use the explicitly changed keys (the ones the user actually clicked) | ||
| // Filter to only include keys that are actually in the changed set | ||
| const keysToSend = explicitlyChangedKeys.filter((key) => | ||
| changedKeys.includes(String(key)), | ||
| ); |
There was a problem hiding this comment.
logic: if user manually edits checked state without clicking (e.g., by programmatically calling setCheckedKeys), explicitlyChangedKeys would be empty and no preferences would be sent
| // Use the explicitly changed keys (the ones the user actually clicked) | |
| // Filter to only include keys that are actually in the changed set | |
| const keysToSend = explicitlyChangedKeys.filter((key) => | |
| changedKeys.includes(String(key)), | |
| ); | |
| // Use the explicitly changed keys (the ones the user actually clicked) | |
| // Filter to only include keys that are actually in the changed set | |
| const keysToSend = explicitlyChangedKeys.length > 0 | |
| ? explicitlyChangedKeys.filter((key) => | |
| changedKeys.includes(String(key)), | |
| ) | |
| : changedKeys; |
There was a problem hiding this comment.
🤔 does this even make sense? how would the user prorammatically call setCheckedKeys?
There was a problem hiding this comment.
You raise a good point! Looking at this code, the concern about programmatic calls to setCheckedKeys might not be as realistic as I initially thought.
In this component, onCheckedKeysChange (which would be setCheckedKeys) is only called from:
- The
PrivacyNoticesTreecomponent when users interact with checkboxes - The
onFetchedKeysChangecall after a successful save (line 153)
The second case is internal to this component and would still have explicitlyChangedKeys populated from the user's actions before saving.
For a truly programmatic call from outside this component to bypass the explicit tracking, someone would need to call the onCheckedKeysChange prop directly without going through the tree component's click handlers. While technically possible, it's not a likely scenario in normal usage.
However, there is still a potential edge case: if the preferences are reset or synchronized from an external source, explicitlyChangedKeys could indeed be empty while changedKeys has values. In that case, falling back to changedKeys would be the correct behavior.
So the suggestion is still valid as defensive programming, but you're right to question whether it addresses a realistic scenario. The fallback ensures the component works correctly even in edge cases where the explicit tracking gets out of sync.
gilluminate
left a comment
gilluminate
left a comment
There was a problem hiding this comment.
LGTM! I'll keep digging on the jsx-a11y issue, we may just need to define the custom select component in that config.
Ticket ENG-1923
Description Of Changes
In ENG-1923 we changed the
override_childrenquery param in the consent v3 API tooverride_mode(with valuesdescendants,ancestorsandall) . This PR updates the privacy notice sandbox to use this new query param instead ofoverride_children.Steps to Confirm
FIDESPLUS__EXPERIMENTAL__CONSENT_V3_ENABLED=trueEmail Marketingopt out notice with some children notices; make sure the notices are enabled<admin_ui_url>/sandbox/privacy-noticesand play around with the newoverride modedropdownPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works