Conversation
outdooracorn
left a comment
There was a problem hiding this comment.
-
The title isn't quite right. This PR isn't updating a database migration. As mentioned in the previous PR, you shouldn't change migrations once they have been applied. This PR is updating the database schema, which requires creating a new migration to manage the required changes to the database. Suggestion:
Improve terms of use database schemas. -
The description isn't formatted correctly. The
Bug:needs to be a git trailer and should live at the end of the commit message. -
The description doesn't add anything that can't be easily deduced from looking at the code. As said in #1028 (review) (emphasis added):
Please add a PR description detailing the changes being made and the rationale for them.
You might want to look back at the original code review as to why we want to make these improvements, or ask a colleague for help if you are struggling to find the right words.
outdooracorn
left a comment
There was a problem hiding this comment.
- Ensures that if a Terms of Use version string ever changes, all acceptance entries automatically update to the new value, keeping references consistent.
While this is technically true, and I think it is good that you have included cascadeOnUpdate() and restrictOnDelete for the foreign key constraint in tou_acceptances, I wouldn't count that as the main reason.
For me, the main reason for having the foreign key constraint is so that only entries in tou_versions.version can be used in tou_acceptances.tou_version. Without this foreign key constraint, the database would accept any string of 10 characters or less, regardless of whether it exists in the tou_versions table.
versionis unique in thetou_versionstable, so make that the primary key and drop theidcolumn.tou_versions.versioncan be used intou_acceptances.tou_versionBug: T407722