qpixel icon indicating copy to clipboard operation
qpixel copied to clipboard

Split ThreadFollower into separate models for posts and threads

Open trichoplax opened this issue 3 months ago • 3 comments

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 up and down methods 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:

  • Create the new_thread_followers table .
  • Move all rows containing a post_id from thread_followers to the new new_thread_followers table.
  • Remove the now unused post_id column from the thread_followers table.

I'm making this change so that #1548 can add a large number of rows (one per post) to the new_thread_followers table, rather than adding to the current combined table thread_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.

trichoplax avatar Dec 03 '25 21:12 trichoplax

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 80.42%. Comparing base (44f1d22) to head (6d8e9a2).

Additional details and impacted files
Components Coverage Δ
controllers 74.69% <100.00%> (+0.03%) :arrow_up:
helpers 85.39% <ø> (+0.48%) :arrow_up:
jobs 79.24% <ø> (ø)
models 92.91% <100.00%> (+2.90%) :arrow_up:
tasks 61.11% <ø> (ø)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Dec 04 '25 00:12 codecov[bot]

I tested down and up after the latest changes and it seemed to work, but also, my dev environment got a little messy from the earlier changes, so somebody else should provide final approval.

cellio avatar Dec 08 '25 20:12 cellio

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.)

cellio avatar Dec 10 '25 17:12 cellio

I'm avoiding merging

I'm waiting until potential problems in the develop branch are resolved / ruled out:

https://discord.com/channels/634104110131445811/668890340199235613/1452154459223228518

trichoplax avatar Dec 21 '25 08:12 trichoplax