-
Notifications
You must be signed in to change notification settings - Fork 84
[ENG-1290] location convenience fields #7024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…il.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…nce-fields-location
adamsachs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good to me! just a few of nits/ideas, but nothing blocking!
|
|
||
| # If location has parent groups, the first one is typically the country | ||
| # (e.g., us_ca belongs to [us]) | ||
| # For countries themselves (e.g., fr belongs to [eea]), location_country stays None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: doesn't this comment line make a bit more sense above the next block, since it has to do with populating location_country?
| "location_country": None, | ||
| "location_groups": [], | ||
| "location_regulations": [], | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you didn't want to extract these a bit more formally, e.g. as an enum or perhaps as a typeddict, or even just some constant vars?
basically, my feeling (without understanding the overall code structure super well, so take this with a grain of salt) is that this could be a bit DRYer alongside the same field names declared in L80 of privacy_request_data.py. obviously 3 field names isn't too intractable, but just formalizing that structure a bit can help to promote consistency and improve readability.
from a quick glance, i suppose that comment could hold true for many of the other field classes. perhaps you've already given it some thought.
anyway, not a blocker by any means, curious to hear what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating to use the convenience field schema values.
| location_convenience_field_names = [ | ||
| "location_country", | ||
| "location_groups", | ||
| "location_regulations", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to tie this to the above comment - this is the other area where i think it could be helpful to be referencing some shared code, rather than redefining these literals!
| # Normalize location to match locations.yml format (lowercase with underscores) | ||
| # ISO 3166: "US-CA" -> locations.yml: "us_ca" | ||
| location_normalized = location.lower().replace("-", "_") | ||
| location_data = locations_by_id.get(location_normalized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, this may be useful as a utility in the locations module - just generally, it's not great that we've got different conventions coming in on the FE than we store on the BE (nothing you've caused), but i think it could help to have that conversion standardized a bit on the BE? i haven't looked at this area of the codebase in a long time, so i could be missing something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a get location function that normalizes as well to src/fides/api/models/location_regulation_selections.py and updated the the conveniences to use it.
…nce-fields-location
…yca/fides into ENG-1290-convenience-fields-location
Co-authored-by: Jade Wibbels <jade@ethyca.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Ticket ENG-1290
Depends on #6984 to be merged before this one. It is testable though 😄
Description Of Changes
🎯 As a user, I want to specify conditions that must be true to create manual tasks, which check the request data, so that I can create tasks conditionally based on data in the request.
This PR adds some location based convenience fields. Fides uses a location hierarchy that is stored in a yaml. This means we can use the same data to improve the location customer experience with convenience columns.
Examples:
Code Changes
src/fides/api/task/conditional_dependencies/privacy_request/convenience_fields.pysrc/fides/api/task/conditional_dependencies/privacy_request/privacy_request_data.pysrc/fides/api/task/conditional_dependencies/privacy_request/schemas.pytests/api/task/manual/test_manual_task_conditional_evaluation.pySteps to Confirm
For a full list of available conditions you can hit the
GET /api/v1/plus/connection/{connection_key}/manual-task/dependency-conditions/privacy-request-fieldsendpoint. (available in https://github.com/ethyca/fidesplus/pull/2814/)Example - This includes my existing data set condition and adds two privacy request conditions:
The new conditions will show up on the integration once added.

Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works