Adding backwards compatibility for name identity #4791
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
| .filter( | ||
| ([key, value]) => | ||
| key === "name" || | ||
| key === "phone" || | ||
| key === "email" || | ||
| (typeof value === "object" && value.label) | ||
| ) |
There was a problem hiding this comment.
Only keeping the known identities (name, phone, email) or custom identities with a label, this is to ignore someone just doing
"loyality_id: "required"
it needs to be labeled
"loyalty_id": {"label" "Loyalty ID"}
There was a problem hiding this comment.
The repeated list of keys here would benefit from some kind of constant declaration like:
const DEFAULT_IDENTITY_INPUTS_MAP = {
"name": undefined,
"email": "email",
"phone": "phone_number",
};
...which could more easily do comparisons like Object.keys(DEFAULT_IDENTITY_INPUTS_MAP).includes("name") etc
| if (key === "phone") { | ||
| // eslint-disable-next-line no-param-reassign | ||
| key = "phone_number"; | ||
| } |
There was a problem hiding this comment.
The key is phone in config.json but it needs to be converted to phone_number to be posted
There was a problem hiding this comment.
Oof. That's messy. The IDENTITY_INPUTS_MAP suggestion I had above was to try to help make this slightly less magical, but it's still a confusing DX
| // we have to support name as an identity_input for legacy purposes | ||
| // but we ignore it since it's not unique enough to be treated as an identity | ||
| .filter(([key]) => key !== "name") |
There was a problem hiding this comment.
The name field will appear in the form but be ignored from the privacy request payload
There was a problem hiding this comment.
👍
OK, this'll work for our purposes. It's pretty confusing behaviour to explain to someone new to the code, but it's also confusing as is, so this is an improvement!
tests/ctl/api/test_seed.py
Outdated
| .filter( | ||
| ([key, value]) => | ||
| key !== "email" && | ||
| key !== "phone" && | ||
| key !== "name" && | ||
| typeof value !== "string" | ||
| ) |
There was a problem hiding this comment.
Filter out the default identities or identity inputs with just string values (such as "required" or "optional") from the dynamic form validation
| .filter( | ||
| ([key, item]) => | ||
| key !== "email" && | ||
| key !== "phone" && | ||
| key !== "name" && | ||
| typeof item !== "string" | ||
| ) |
There was a problem hiding this comment.
Same filtering to render the form fields
There was a problem hiding this comment.
nit: You'll find this kind of rendering logic easier to manage if you pre-filter this above, like
const defaultIdentityInputs = Object.entries(identityInputs).filter(...)
const customIdentityInputs = Object.entries(identityInputs).filter(...)
And then you can use those variables directly with less fuss 😄
| .filter( | ||
| ([key, value]) => | ||
| key === "name" || | ||
| key === "phone" || | ||
| key === "email" || | ||
| (typeof value === "object" && value.label) | ||
| ) |
There was a problem hiding this comment.
The repeated list of keys here would benefit from some kind of constant declaration like:
const DEFAULT_IDENTITY_INPUTS_MAP = {
"name": undefined,
"email": "email",
"phone": "phone_number",
};
...which could more easily do comparisons like Object.keys(DEFAULT_IDENTITY_INPUTS_MAP).includes("name") etc
| // we have to support name as an identity_input for legacy purposes | ||
| // but we ignore it since it's not unique enough to be treated as an identity | ||
| .filter(([key]) => key !== "name") |
There was a problem hiding this comment.
👍
OK, this'll work for our purposes. It's pretty confusing behaviour to explain to someone new to the code, but it's also confusing as is, so this is an improvement!
| if (key === "phone") { | ||
| // eslint-disable-next-line no-param-reassign | ||
| key = "phone_number"; | ||
| } |
There was a problem hiding this comment.
Oof. That's messy. The IDENTITY_INPUTS_MAP suggestion I had above was to try to help make this slightly less magical, but it's still a confusing DX
| .filter( | ||
| ([key, item]) => | ||
| key !== "email" && | ||
| key !== "phone" && | ||
| key !== "name" && | ||
| typeof item !== "string" | ||
| ) |
There was a problem hiding this comment.
nit: You'll find this kind of rendering logic easier to manage if you pre-filter this above, like
const defaultIdentityInputs = Object.entries(identityInputs).filter(...)
const customIdentityInputs = Object.entries(identityInputs).filter(...)
And then you can use those variables directly with less fuss 😄
Passing run #7243 ↗︎
Details:
Review all test suite changes for PR #4791 ↗︎ |
||||||||||||||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4791 +/- ##
==========================================
+ Coverage 86.28% 86.61% +0.32%
==========================================
Files 339 339
Lines 20090 20090
Branches 2586 2586
==========================================
+ Hits 17334 17400 +66
+ Misses 2290 2217 -73
- Partials 466 473 +7 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Adam Sachs <[email protected]>
Description Of Changes
Part of the work I did when adding custom identities was to demote "name" as an identity since it's not unique enough to uniquely identify a subject. This PR adds support to render the name field (if provided in the Privacy Center's
config.json) but ignore it as part of the privacy request payload.I'm also fixing a test with the demo seed data, I'm including it here to minimize the time spent juggling two separate PRs.
Code Changes
PrivacyRequestForm.tsxcomponenttest_seed.pytestsSteps to Confirm
nox -s devto start the Fides servercd clients/privacy-center && turbo run devnamein theidentity_inputsof the Privacy Center'sconfig.jsonand still be able to submit a requestPre-Merge Checklist
CHANGELOG.md