-
Notifications
You must be signed in to change notification settings - Fork 72
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
[Backend] Further Deduplicate PrivacyExperience and PrivacyExperienceConfig #3470
Conversation
…ce and PrivacyExperienceConfig. - Remove common fields between the models like PrivacyExperience.disabled and PrivacyExperience.experience_config_history_id - these are at risk of getting out of sync - Now that we've removed most of the fields from PrivacyExperience and it is largely just a linking table, I don't think we need to version this. Remove PrivacyExperience.version, and the entire PrivacyExperienceHistory table. - PrivacyPreferenceHistory was referencing the PrivacyExperienceHistory id, but now that this doesn't exist, have it reference the PrivacyExperience id instead. - When filtering PrivacyExperiences on disabled, actually filter the corresponding field on the PrivacyExperienceConfig resource.
Passing run #2447 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3470 +/- ##
==========================================
- Coverage 87.14% 87.12% -0.02%
==========================================
Files 312 312
Lines 18787 18757 -30
Branches 2389 2389
==========================================
- Hits 16371 16342 -29
+ Misses 1992 1991 -1
Partials 424 424
☔ View full report in Codecov by Sentry. |
…d add test for untested path.
src/fides/api/ctl/migrations/versions/317e6197c76a_reduce_experience_and_config_duplication.py
Show resolved
Hide resolved
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.
this looks great @pattisdr, nothing stood out to me as as a concern or in need of adjustment. i'm very happy to see things going in this direction, i think this has simplified the db models as well as the application logic significantly, and i don't think we lose any meaningful functionality as far as i can tell. nice job getting this in place before we roll it out 👍
src/fides/api/ctl/migrations/versions/317e6197c76a_reduce_experience_and_config_duplication.py
Show resolved
Hide resolved
appreciate the quick review @adamsachs 🙏 |
Closes #3465
❗ Contains data migration; check downrev before merge
Code Changes
Continue to deduplicate info between PrivacyExperience and PrivacyExperienceConfig.
Steps to Confirm
nox -s dev -- shell
version
anddisabled
are still present in the embeddedexperience_config
object)id
of one of the notices embedded in that experienceprivacy_experience_id
privacy_notice_history_id
andprivacy_experience_id
as well as theexperience_config_history_id
which was lifted from the experience and added here for record keepingPre-Merge Checklist
CHANGELOG.md
Description Of Changes
Further side effects of #3387
Now that we've relaxed a lot of the constraints around privacy experiences we no longer need as many fields on the PrivacyExperience table. There used to be necessary duplication between PrivacyExperience and PrivacyExperienceConfig but now this isn't required and we're just at risk of the tables getting out of sync. This surfaced when testing "creating experiences out of the box - as an area that had gotten out of sync). h/t to @adamsachs for also raising the importance of what tables were storing which information here: #3387 (comment)
Primarily I deduplicate all the info between the two tables except for "component" and stop tracking history of privacy experiences now that there's barely anything on this table.
Saving privacy preferences should now pass on the "experience id" instead of the "experience history id" (because it doesn't exist)