-
Notifications
You must be signed in to change notification settings - Fork 73
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
Rename Privacy Notice Properties #5259
Rename Privacy Notice Properties #5259
Conversation
…regions" to something that doesn't overlap the fields on the PrivacyNotice schemas in Fidesplus. This way these properties won't automatically be called when trying to serialize a list of Privacy Notices via the API. - Make the new properties more efficient so they can still be used when serializing a single Privacy Notice.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #9810
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5259/merge
|
Run status |
Passed #9810
|
Run duration | 00m 37s |
Commit |
d3e48037b2 ℹ️: Merge 914b5f2f49370e512766fb22e70c6cfc1ae50601 into 9744bf9fe65fbe5dd034d3371a24...
|
Committer | Dawn Pattison |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
…onfig secrets, and keep privacy notice response configured_regions and systems_applicable to using the aliases -
@galvana I regenerated types as a last step and broke something here -- trying to see what went wrong |
…cessarily supply on PrivacyNoticeResponseWithRegions to improve performance.
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.
Thanks for updating the front-end types 🙏
.group_by(PrivacyExperience.region) | ||
.order_by(PrivacyExperience.region.asc()) | ||
|
||
# Raw sql for performance |
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.
⚡
fides Run #9844
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #9844
|
Run duration | 00m 38s |
Commit |
8350397d80: Rename Privacy Notice Properties (#5259)
|
Committer | Dawn Pattison |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
Closes #PROD-2708
Description Of Changes
Rename some properties on PrivacyNotice models that have the same name as fields exposed in the API. We want to intentionally choose when these properties are used in the API instead of having them be used automatically. Further, refactor these properties to be slightly more performant when they are called directly.
Code Changes
PrivacyNotice.systems_applicable
property renamed tocalculated_systems_applicable
. Refactored to be slightly more performant, and will typically be used now for serializing a single notice, not multiple notices.PrivacyNotice.configured_regions
property renamed toconfigured_regions_for_notice
and refactored to be slightly more performant by using raw sql. This will generally just be used for serializing a single privacy notice - when serializing bulk notices, Plus will typically do the regions calculation separately and share among multiple notices.Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
main
downgrade()
migration is correct and works