Split ThreadFollower into separate models for posts and threads #1920
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cellio
left a comment
cellio
left a comment
There was a problem hiding this comment.
There's an issue with the migration that @Oaphi will describe. Meanwhile, I've tested these changes (after locally making the migration fix) and behavior looks good -- the correct tables get updated on follow/unfollow on both posts and threads, and notifications get delivered as expected.
|
I tested |
Oaphi
left a comment
Oaphi
left a comment
There was a problem hiding this comment.
LGTM to me after the latest changes (tested, manual tests included), however, I am deferring to someone else to provide a final review given that I've made a couple of commits to the branch.
|
LGTM but want @ArtOfCode- to review. Also, hold merge until after we deploy; we have some other small things ready to ship, so let's get those out and then merge this just in case there are any issues that would require rollback. (I don't think there are, but this isn't user-facing so it doesn't need to be deployed right away.) |
|
|
||
| t.timestamps | ||
| end | ||
| add_index :new_thread_followers, [:user_id, :post_id], if_not_exists: true |
There was a problem hiding this comment.
I would add indexes here for user_id and post_id separately as well. We're likely to be querying for thread followers by post or by user as well as by both.
There was a problem hiding this comment.
Revision after discussion in Discord: let's add an additional index for :post_id - the composite index will work for searches on :user_id.
There was a problem hiding this comment.
Thanks for explaining that the composite index works differently in MySQL. I've now added the index on post_id (and the corresponding removal for rollback).
I'm avoiding mergingI'm waiting until potential problems in the https://discord.com/channels/634104110131445811/668890340199235613/1452154459223228518 |
Potentially irreversible change
This code change must not be merged without running the migration, because the code refers to a table that does not yet exist. The movement of data makes this change irreversible (at least for Rails' automatic rollback), and the fact that we use MySQL means that if the migration fails part way through it will be left in an intermediate state (for example, with a table created but no data moved). Separate
upanddownmethods have been added to the migration, designed to handle rerunning or undoing the migration after an only partial success. However, please ensure backups are up to date before running this migration.The ThreadFollower model was created to keep track of users following comment threads. Later it had a post_id field added to keep track of users following new threads on posts. This pull request splits out this later addition into a separate model NewThreadFollower.
Following discussion in Discord the migration file now covers all of:
new_thread_followerstable .post_idfromthread_followersto the newnew_thread_followerstable.post_idcolumn from thethread_followerstable.I'm making this change so that #1548 can add a large number of rows (one per post) to the
new_thread_followerstable, rather than adding to the current combined tablethread_followers. Waiting until after that large number of rows is added before splitting the table would require a much larger data movement between tables later (unless it is decided to never split the table, which I'm uncomfortable with since there's no reason for it to serve two independent purposes).I'm not experienced in migrations so if there are obvious things that don't need to be mentioned in review, please mention them anyway.