-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
@@ -714,6 +714,7 @@ class ProvidedIdentity(Base): # pylint: disable=R0904 | |||
), | |||
nullable=True, | |||
) # Type bytea in the db | |||
consent = relationship("Consent", back_populates="provided_identity", uselist=False) |
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.
I have uselist
as False
because I am assuming consent would only need to be given one time. Is that a correct assumption?
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.
The main thing that's enforcing the 1:1 is your unique constraint on Consent.provided_identity_id, this uselist by itself doesn't prevent there from being multiple consents for the same privacy request id. The uselist=False just loads this as a scalar. But yes, in my opinion the unique constraint makes sense to me.
Starting review.. |
@@ -714,6 +714,7 @@ class ProvidedIdentity(Base): # pylint: disable=R0904 | |||
), | |||
nullable=True, | |||
) # Type bytea in the db | |||
consent = relationship("Consent", back_populates="provided_identity", uselist=False) |
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.
The main thing that's enforcing the 1:1 is your unique constraint on Consent.provided_identity_id, this uselist by itself doesn't prevent there from being multiple consents for the same privacy request id. The uselist=False just loads this as a scalar. But yes, in my opinion the unique constraint makes sense to me.
src/fidesops/ops/migrations/versions/a0e6feb5bdc8_add_consent.py
Outdated
Show resolved
Hide resolved
@pattisdr I updated the models to allow multiple consents and added a test, but still need to update the database diagram. |
Database diagram is updated now also so I think it is ready for another review. |
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.
good work I just have a couple comments about the unit test and the downgrade but feel free to push back
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.
nice work here
Thanks @pattisdr! |
Co-authored-by: Paul Sanders <[email protected]>
Purpose
❗ Contains migrations.
Adds a table to store consent preferences for data subjects.
Changes
Checklist
CHANGELOG.md
fileCHANGELOG.md
file is being appended toUnreleased
section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1160