Adding property ID to privacy preference history #4886
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #7816 ↗︎
Details:
Review all test suite changes for PR #4886 ↗︎ |
||||||||||||||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4886 +/- ##
==========================================
- Coverage 86.75% 86.75% -0.01%
==========================================
Files 347 347
Lines 20965 20966 +1
Branches 2741 2741
==========================================
Hits 18189 18189
Misses 2297 2297
- Partials 479 480 +1 ☔ View full report in Codecov by Sentry. |
| @@ -388,6 +388,8 @@ def __tablename__(self) -> str: | |||
| MutableDict.as_mutable(JSONB) | |||
| ) # Dict of TCF attributes saved, for a TCF notice | |||
|
|
|||
| property_id = Column(String, index=True, nullable=True) | |||
There was a problem hiding this comment.
hm, this is the same as plus_property.id right? Just trying to think through if this should be a FK. We are enforcing other resources like the privacy notice, privacy notice history, etc. be proper FK's.
Cons are that this is a large table for some customers, adding a FK is tricky. It could be added with something like a "NOT VALID" at first and then issuing a separate query to validate the constraint separately.
It might not be worth that though, a string could be fine too, just wanted to surface
There was a problem hiding this comment.
Yes! Same thing, that was my intention but I forgot to add the foreign key constraint 😵💫. I set it to be nullable since it's not guaranteed that the associated privacy experience will be retrieved by property ID, which is the main bit of info we're tracking with this new addition. I also set ondelete="SET NULL" since we want to be able to delete properties freely and still keep these historical records around.
I didn't know about "NOT VALID" in Postgres so thanks for sharing! How would that apply here? From my understanding it allows us to delay the constraint validation and create the constraint more quickly. Would this optimization still be needed if every row has a null property_id?
There was a problem hiding this comment.
discussed in person, we'll revert back to a string to preserve the history in case the property is deleted in the future.
| op.create_index( | ||
| op.f("ix_privacypreferencehistory_property_id"), | ||
| "privacypreferencehistory", | ||
| ["property_id"], | ||
| unique=False, | ||
| ) |
There was a problem hiding this comment.
Comment on ticket: https://ethyca.atlassian.net/browse/PROD-2037?focusedCommentId=40988
| property_id = Column( | ||
| String, | ||
| index=True, | ||
| nullable=True, | ||
| ) |
There was a problem hiding this comment.
Can you add a quick model-level test on this field? There's an existing file tests/ops/models/test_privacy_preference.py
There was a problem hiding this comment.
Also, we could define this field on the ConsentReportingMixinV2 in one place I believe, it's shared between PrivacyPreferenceHistory and ServedNoticeHistory
galvana
deleted the
PROD-2037-store-property-ID-alongside-consent-preference
branch
Closes PROD-2037
Code Changes
property_idtoPrivacyPreferenceHistorymodelSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md