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
Change notifications unique indexes #2525 #3012
Change notifications unique indexes #2525 #3012
Conversation
|
Really good work @lightalloy ! I guess it needs to rebase or regenerate the Since we can't use foreign keys here, unique indexes are definitely of help! |
|
@rhymes thanks for reminding! I'll dot it. |
e62eab6
to
faa385b
Compare
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.
Code looks great! Going to reiterate your point before merging:
Before merging, all the notifications that have duplicate fields %i[user_id organization_id notifiable_id notifiable_type action] must be deleted.
faa385b
to
ab40bcc
Compare
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.
Approving now that concurrent migrations are in place 👍
What type of PR is this? (check all applicable)
Description
user_id,organization_id,actionfieldsPostgres allows to create records which contain the same fields if some of them contain null values, so I
user_idandorganization_idbecause notifications has eitheruser_idororganization_idactionis null and when it's not nullBefore merging, all the notifications that have duplicate fields
%i[user_id organization_id notifiable_id notifiable_type action]must be deletedRelated Tickets & Documents
#2525